Skip to content

Fix URL encode resource's id before calling API endpoints#10292

Merged
traefiker merged 1 commit intotraefik:v2.11from
andsarr:fix/get_router_by_name
Jan 25, 2024
Merged

Fix URL encode resource's id before calling API endpoints#10292
traefiker merged 1 commit intotraefik:v2.11from
andsarr:fix/get_router_by_name

Conversation

@andsarr
Copy link
Copy Markdown
Contributor

@andsarr andsarr commented Dec 10, 2023

What does this PR do?

This PR fix the 404 error when a router's name contains the character /.

From the UI, when the user clicks in the table, the router's name is encoded before calling the API. In the backend, the URL path parameter is decoded before the look up.

Motivation

Fixes 10177

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

Should I also decode the URL path parameter for the other API endpoints?

@kevinpollet
Copy link
Copy Markdown
Member

Hello @andsarr, and thank you for your contribution.

We have tested the changes, and everything is working well. We would appreciate it if you could provide fixes for other endpoints (services, middlewares) and all UDP and TCP resources.

Could you please update the pull request to include these changes?

@andsarr
Copy link
Copy Markdown
Contributor Author

andsarr commented Dec 19, 2023

@kevinpollet sure! I'll do it!

@andsarr andsarr force-pushed the fix/get_router_by_name branch 3 times, most recently from 1fcdb0a to fcd6c0c Compare December 20, 2023 20:34
@andsarr
Copy link
Copy Markdown
Contributor Author

andsarr commented Dec 20, 2023

@kevinpollet now it is ready. Since I am using a middleware for decoding the URL path parameters, I didn't need to change too many files in the backend.

@andsarr
Copy link
Copy Markdown
Contributor Author

andsarr commented Dec 21, 2023

I don't understand why the unit tests fails in GitHub. If I execute make test-unit in my PC, all the tests are green. I also tried to executing the failing test independently and it was successful.

The weirdest part is that if I run IN_DOCKER= make test-unit then the Open Telemetry test is the one that fails. I'll keep digging.

@ldez
Copy link
Copy Markdown
Contributor

ldez commented Dec 21, 2023

It's just a flaky test, we relaunched the check.

@rtribotte
Copy link
Copy Markdown
Member

Hello @andsarr,

Thanks for the recent adjustments!

The middleware could have been an elegant solution. Still, since it requires modification of the Vars map in the request context, I would prefer the previous approach to transform the parameter in each handler. Also, it would not hide that we do such a transformation.

The trace log seems unnecessary as it does not provide much information, and in the Traefik code base, we do not use this level. For these reasons, it should be removed.

@rtribotte
Copy link
Copy Markdown
Member

Also, since this PR is a bug fix, can you please rebase it on the v2.10 branch?

@rtribotte rtribotte modified the milestones: 3.0, 2.11 Dec 26, 2023
@andsarr andsarr force-pushed the fix/get_router_by_name branch from fcd6c0c to 0a79643 Compare December 27, 2023 12:42
@andsarr andsarr changed the base branch from v3.0 to v2.10 December 27, 2023 12:59
@andsarr andsarr force-pushed the fix/get_router_by_name branch from bf8f577 to 5013bbc Compare December 27, 2023 13:34
@andsarr
Copy link
Copy Markdown
Contributor Author

andsarr commented Dec 27, 2023

@rtribotte I have implemented all the requested changes. I have run the tests locally and they all passed. I think I am having the same issue as here #10292 (comment)

@mmatur mmatur changed the base branch from v2.10 to v2.11 January 2, 2024 14:00
@andsarr
Copy link
Copy Markdown
Contributor Author

andsarr commented Jan 4, 2024

Hi all!
Is there something else needed to be done from my side?

@mmatur
Copy link
Copy Markdown
Member

mmatur commented Jan 4, 2024

Hi @andsarr,

Nothing to do on your side, maintainers need to review your PR and it will be available in the next Traefik v2.11 version

Copy link
Copy Markdown
Member

@rtribotte rtribotte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

@rtribotte rtribotte removed their assignment Jan 19, 2024
@kevinpollet kevinpollet changed the title fix: URL encode router's id before calling the API endpoint fix: URL encode resource's id before calling API endpoints Jan 22, 2024
@andsarr andsarr force-pushed the fix/get_router_by_name branch 3 times, most recently from 0aea21d to 780d377 Compare January 24, 2024 21:00
Co-authored-by: Kevin Pollet <pollet.kevin@gmail.com>
@andsarr andsarr force-pushed the fix/get_router_by_name branch from 780d377 to 0333e54 Compare January 24, 2024 21:15
@andsarr
Copy link
Copy Markdown
Contributor Author

andsarr commented Jan 24, 2024

@kevinpollet I have addressed all the comments.

Copy link
Copy Markdown
Member

@kevinpollet kevinpollet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

Copy link
Copy Markdown
Contributor

@lbenguigui lbenguigui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@traefiker traefiker merged commit 49f04f2 into traefik:v2.11 Jan 25, 2024
@andsarr andsarr deleted the fix/get_router_by_name branch January 25, 2024 10:36
@mmatur mmatur changed the title fix: URL encode resource's id before calling API endpoints Fix URL encode resource's id before calling API endpoints Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants