Fix go namespace slashes #886
Conversation
Signed-off-by: Nell Shamrell <[email protected]>
Signed-off-by: Nell Shamrell <[email protected]>
Signed-off-by: Nell Shamrell <[email protected]>
Signed-off-by: Nell Shamrell <[email protected]>
Signed-off-by: Nell Shamrell <[email protected]>
Signed-off-by: Nell Shamrell <[email protected]>
nellshamrell
force-pushed
the
fix-go-namespace-slashes
branch
from
9e8387f
to
cdddaba
Compare
Signed-off-by: Nell Shamrell <[email protected]>
routes/definitions.js
Outdated
| // Painful way of handling go namespaces with multiple slashes | ||
| // Unfortunately, it seems the best option without doing a massive | ||
| // rearchitecture of the entire coordinate system | ||
| if (request.params.type == "go" && request.params.provider == "golang") { | ||
| if (request.params.type == 'go' && request.params.provider == 'golang') { |
There was a problem hiding this comment.
Possibly some lint rule is not set up very well.
There was a problem hiding this comment.
Michael, do you mean that these should be triple equals to be the best JavaScript it can be? ===
(Agree if so)
There was a problem hiding this comment.
Good catch. the == should be ===. What I mean is the double quote can be caught by lint rules. I think == could also be caught in lint rules.
|
Feels a little brute force and we only support the go component with up to three slashes. Could we replace slash in the namespace with another char, like _ or whitespace, and then call encodeURIComponent()? @jeffwilcox for more inputs. |
|
@MichaelTsengLZ it is, indeed, brute force. I tried a few experiments with replacing the slashes with other characters. The problem is that there any character that can be used in a URL can be used in a go namespace (including underscores, hyphens, periods, etc.). We could consider something like whitespace, but I feel that would make the API more difficult to use (it's not intuitive to include whitespace in an API call). I do think we would run into trouble when the load balancer decoded the encoding before passing the url to the service. I feel this approach introduces the fewest compromises and we can add more "extra" arguments to support more slashes in go namespaces. |
routes/definitions.js
Outdated
| // Painful way of handling go namespaces with multiple slashes | ||
| // Unfortunately, it seems the best option without doing a massive | ||
| // rearchitecture of the entire coordinate system | ||
| if (request.params.type == "go" && request.params.provider == "golang") { | ||
| if (request.params.type == 'go' && request.params.provider == 'golang') { |
There was a problem hiding this comment.
Michael, do you mean that these should be triple equals to be the best JavaScript it can be? ===
(Agree if so)
|
As to the brute force and other aspect... sorry, I don't have enough background on ClearlyDefined to help here, but I trust Nell and the design she has in mind for the implementation. |
Whitespace will be encoded as %20 in the URL while slash will be encoded as %2F. Therefore, I don't think it will complicate the URL that much. But I agree this is also very hacky and not sure whether it will cause any issue when nginx parse it to really whitespace in URL and then pass it to node app. |
Signed-off-by: Nell Shamrell <[email protected]>
|
@MichaelTsengLZ I tested how the API behaves when it receives an API call with whitespace in it, it results in it not being able to find the route (similar to what was previously happening when the slashes were decoded). We would need to add in special handling for the whitespaces similar to what we are doing with slashes here: (The 404 not found error occurs when the express app cannot find a route that corresponds to the request) |
|
I have corrected the "==" to "===". That does, indeed, make it the best Javascript it can be :) |
Fix go namespace slashes
Background
This is a fix for #884
When go support was deployed to staging and production, I discovered that when someone requests a definition with url encoded slashes, such as:
The load balancer decodes the slashes before the request is passed to the service, which means the coordinates requested are:
Which results in a 404 not found, as there are more parameters in the url than the route expect.
Implementation
This fix is a bit brute force, but it does work. It allows for up to three additional parameters in the URL and, for go components only, strings the relevant arguments together, then parses the revision, name, and namespace out of that string.