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
fix: UI can now get clusters with slashes in name (#9812) #9813
fix: UI can now get clusters with slashes in name (#9812) #9813
Conversation
Codecov ReportBase: 45.60% // Head: 45.63% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #9813 +/- ##
==========================================
+ Coverage 45.60% 45.63% +0.02%
==========================================
Files 239 239
Lines 28973 28978 +5
==========================================
+ Hits 13214 13224 +10
+ Misses 13940 13933 -7
- Partials 1819 1821 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
e22dc27
to
a70a9b1
Compare
What was the full URL sent to the API server? My fix just urlencodes the cluster name before sending it to the API, and the API urldecodes the cluster name, so if you looked up like |
URL encode works well with cluster name API. Changes LGTM! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG
a70a9b1
to
1f0ebd8
Compare
1f0ebd8
to
68498eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
server/cluster/cluster.go
Outdated
@@ -144,7 +145,11 @@ func (s *Server) getCluster(ctx context.Context, q *cluster.ClusterQuery) (*appv | |||
q.Server = "" | |||
q.Name = "" | |||
if q.Id.Type == "name" { | |||
q.Name = q.Id.Value | |||
name, err := url.QueryUnescape(q.Id.Value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a breaking API change for all clusters for which url.QueryUnescape(name) != name
. For example, if my cluster were named (for some weird reason) in%40cluster
, I'd now get a 403 when requesting that cluster unless I did so with the UI.
Could we introduce a new name_escaped
to solve the bug and still support other use cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I changed the UI to submit with type name_escaped
instead and adjusted the backend side of things to handle that appropriately while still preserving the original behavior. I also duplicated the test to check that clusters where something in their name could be interpreted as escaped will pass through unmodified.
Fixes argoproj#9812 If a cluster name has a slash in it, the API would not be able to fetch that cluster and would display "in-cluster (undefined)" for that application. This fixes that issue by URI-encoding the cluster name on the UI side and URI-decoding the cluster name on the API side. Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com>
68498eb
to
60bdccf
Compare
Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com>
I built another internal image with the updated PR and deployed it. Everything seems good, continues to handle cluster names properly that do and don't have / in them. @crenshaw-dev could I get your reconsideration? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it. Thanks for your patience, @erhudy!
* fix: #9812 UI can now get clusters with slashes in name Fixes #9812 If a cluster name has a slash in it, the API would not be able to fetch that cluster and would display "in-cluster (undefined)" for that application. This fixes that issue by URI-encoding the cluster name on the UI side and URI-decoding the cluster name on the API side. Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com> * Retrigger CI pipeline Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com> Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com>
* fix: #9812 UI can now get clusters with slashes in name Fixes #9812 If a cluster name has a slash in it, the API would not be able to fetch that cluster and would display "in-cluster (undefined)" for that application. This fixes that issue by URI-encoding the cluster name on the UI side and URI-decoding the cluster name on the API side. Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com> * Retrigger CI pipeline Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com> Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com>
* fix: #9812 UI can now get clusters with slashes in name Fixes #9812 If a cluster name has a slash in it, the API would not be able to fetch that cluster and would display "in-cluster (undefined)" for that application. This fixes that issue by URI-encoding the cluster name on the UI side and URI-decoding the cluster name on the API side. Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com> * Retrigger CI pipeline Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com> Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com>
Cherry-picked onto release-2.5 for 2.5.3, release-2.4 for 2.4.18, and release-2.3 for 2.3.12. |
…goproj#9813) * fix: argoproj#9812 UI can now get clusters with slashes in name Fixes argoproj#9812 If a cluster name has a slash in it, the API would not be able to fetch that cluster and would display "in-cluster (undefined)" for that application. This fixes that issue by URI-encoding the cluster name on the UI side and URI-decoding the cluster name on the API side. Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com> * Retrigger CI pipeline Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com> Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com>
…goproj#9813) * fix: argoproj#9812 UI can now get clusters with slashes in name Fixes argoproj#9812 If a cluster name has a slash in it, the API would not be able to fetch that cluster and would display "in-cluster (undefined)" for that application. This fixes that issue by URI-encoding the cluster name on the UI side and URI-decoding the cluster name on the API side. Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com> * Retrigger CI pipeline Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com> Signed-off-by: Edmund Rhudy <erhudy@users.noreply.github.com> Signed-off-by: emirot <emirot.nolan@gmail.com>
Fixes #9812
If a cluster name has a slash in it, the API would not be able
to fetch that cluster and would display "in-cluster (undefined)"
for that application. This fixes that issue by URI-encoding
the cluster name on the UI side and URI-decoding the cluster name
on the API side.
Signed-off-by: Edmund Rhudy erhudy@users.noreply.github.com
Note on DCO:
If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.
Checklist: