Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add handling for trailing slash in the KESTRA_URL configuration to avoid missing or double slash #6041

Open
wrussell1999 opened this issue Nov 21, 2024 · 6 comments · May be fixed by #6373
Open
Assignees
Labels
area/backend Needs backend code changes bug Something isn't working

Comments

@wrussell1999
Copy link
Member

Describe the issue

When I executed a flow using the API, I got a response with a URL to Kestra that contained two slashes when only one was needed.

Flow:

id: hello_world
namespace: company.team

inputs:
  - id: greeting
    type: STRING
    defaults: hey

tasks:
  - id: hello
    type: io.kestra.plugin.core.log.Log
    message: "{{ inputs.greeting }}"

API Request made in the terminal:

curl -X POST http://localhost:8084/api/v1/executions/company.team/hello_world

Response given:

{
    "id": "6gTABm82mx5li7tHkgqyNU",
    "namespace": "company.team",
    "flowId": "hello_world",
    "flowRevision": 10,
    "inputs": {
        "greeting": "hey"
    },
    "labels": [
        {
            "key": "system.correlationId",
            "value": "6gTABm82mx5li7tHkgqyNU"
        }
    ],
    "state": {
        "current": "CREATED",
        "histories": [
            {
                "state": "CREATED",
                "date": "2024-11-21T16:35:36.815338221Z"
            }
        ],
        "duration": 0.012298625,
        "startDate": "2024-11-21T16:35:36.815338221Z"
    },
    "originalId": "6gTABm82mx5li7tHkgqyNU",
    "deleted": false,
    "metadata": {
        "attemptNumber": 1,
        "originalCreatedDate": "2024-11-21T16:35:36.815378387Z"
    },
    "url": "http://localhost:8084//ui/executions/company.team/hello_world/6gTABm82mx5li7tHkgqyNU"
}

Specifically the bottom line url has two / after the server URL. It should be:
"url": "http://localhost:8084/ui/executions/company.team/hello_world/6gTABm82mx5li7tHkgqyNU"

Should be a quick fix, and doesn't stop it working - just a minor bug.

Environment

  • Kestra Version: develop OSS 78f5ff2
@wrussell1999 wrussell1999 added bug Something isn't working area/backend Needs backend code changes labels Nov 21, 2024
@wrussell1999 wrussell1999 changed the title API response includes double slashes API response includes double slash Nov 21, 2024
@github-project-automation github-project-automation bot moved this to Backlog in Issues Nov 21, 2024
@benjoEK1337
Copy link
Contributor

hey. The code implementation takes the URL specified in the configuration files (application-{env}.yml or any other way to provide the value) and concats the "/ui" + the rest of the path to that URL. This could be fixed in the code by removing the slash if specified or to tell users to specify the URL without the slash at the end.

Here is the code creating the final URL:

private URI executionUrl(Execution execution) { return URI.create(kestraUrl.orElse("") + "/ui" + (execution.getTenantId() != null ? "/" + execution.getTenantId(): "") + "/executions/" + execution.getNamespace() + "/" + execution.getFlowId() + "/" + execution.getId() ); }

where kestraUrl is fetched from configuration:

@Value("${kestra.url}") private Optional<String> kestraUrl;

So, did you maybe specify in your application-local.yml or anywhere else the environment variable kestra.url as: "http://localhost:8084/"?

@loicmathieu what do you think?

@anna-geller anna-geller changed the title API response includes double slash Add handling for trailing slash in the KESTRA_URL configuration to avoid missing or double slash Nov 25, 2024
@benjoEK1337
Copy link
Contributor

Hey @anna-geller,

Is there an internal decision to handle this case? If so, I’d be happy to take care of it.

Also, @wrussell1999 could you confirm whether the / was added to the configuration somewhere?

@anna-geller
Copy link
Member

anna-geller commented Nov 26, 2024

Hi @benjoEK1337, we would love to see your contribution. There are no internal discussions about this — in the end, we can't guarantee that the user adds a trailing slash in their configuration file so the code needs to:

  1. check if the the KESTRA_URL is set
  2. if yes, then we need to check if it ends with / and we need to append a trailing slash to the API response if the slash is missing at the end
  3. if no, no url should be returned in the payload

WDYT? does it sound like a good plan to you? 🚀

@benjoEK1337
Copy link
Contributor

I think that the expected behavior for the user is not to add the trailing slash. The current implementation doesn't expect the trailing slash, so if the user adds a trailing slash, the returned URL contains two trailing slashes. So, my suggestion for implementation is to check if the user added a trailing slash, and then remove it in the code.

Regarding the case when KESTRA_URL is not set, the current logic is handling it this way:

  1. check if the KESTRA_URL is set
  2. If yes, append to the KESTRA_URL these: "/ui" plus the rest of the path containing tenant ID (if it exists), namespace, flow ID, and ID. For example - "url": "http://localhost:8084//ui/executions/company.team/hello_world/6gTABm82mx5li7tHkgqyNU"
  3. If no, return the URL in the format "url": "/ui/executions/company.team/hello_world/6gTABm82mx5li7tHkgqyNU"

So if no KESTRA URL is set, it doesn't include the KESTRA_URL in the returned URL.

So should we change to not return the URL when the KESTRA_URL is not set, as you suggested, or keep this part of the logic as it is?

@anna-geller
Copy link
Member

The current behavior seems fine. Looking forward to your PR! Thanks for looking into this

@benjoEK1337
Copy link
Contributor

Hey @anna-geller, I created the PR. Could you please assign me to this task and add a reviewer to the PR? Thanks 🚀

@MilosPaunovic MilosPaunovic moved this from Backlog to In review in Issues Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend Needs backend code changes bug Something isn't working
Projects
Status: In review
Development

Successfully merging a pull request may close this issue.

3 participants