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

udpa: clarify URI fragment encoding of directives. #30

Merged
merged 4 commits into from
Jun 29, 2020

Conversation

htuch
Copy link
Collaborator

@htuch htuch commented Jun 25, 2020

After spending some time looking at RFC3986 and chatting with Louis, it
seems we can support multiple directives in the fragment, but we need to
be very precise on how they are percent encoded.

This patch proposes that we move from having multiple fragments (which
are not allowed in RFC compliant URIs) to a single fragment with
comma-separated percent encoded directives. Comma was selected as it is
an available character in RFC3986 sub-delims, intuitively indicates
separation, and is less likely to appear in URIs (forcing percent
encoding) than other alternatives, e.g. '&' which would appear in most
URIs with context parameters.

Percent encoded directives in URIs should remain fairly readable, for
example udpa://foo#entry=something,alt=udpa://bar/baz&x=y requires no
percent encoding.

Signed-off-by: Harvey Tuch htuch@google.com

After spending some time looking at RFC3986 and chatting with Louis, it
seems we can support multiple directives in the fragment, but we need to
be very precise on how they are percent encoded.

This patch proposes that we move from having multiple fragments (which
are not allowed in RFC compliant URIs) to a single fragment with
comma-separated percent encoded directives. Comma was selected as it is
an available character in RFC3986 sub-delims, intuitively indicates
separation, and is less likely to appear in URIs (forcing percent
encoding) than other alternatives, e.g. '&' which would appear in most
URIs with context parameters.

Percent encoded directives in URIs should remain fairly readable, for
example udpa://foo#entry=something,alt=udpa://bar/baz&x=y requires no
percent encoding.

Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch
Copy link
Collaborator Author

htuch commented Jun 25, 2020

@markdroth @louiscryan for review

Signed-off-by: Harvey Tuch <htuch@google.com>
Copy link
Collaborator

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

I can live with this if it's the consensus, but I'd still prefer to use a repeated UdpaResourceLocator instead of having an alt directive. To me, the fact that we need so much machinery around encoding this in a URI is a pretty strong indicator that while we can make this work, that doesn't mean we should.

//
// When encoding to URIs, directives take form:
//
// <directive name>=<string representation of directive value>.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest removing the trailing . here, just to make it clear that that's not expected to be present in the format.

@htuch
Copy link
Collaborator Author

htuch commented Jun 29, 2020

@markdroth if we did move #alt out in favor of an explicit fallback list, I think I'd still want to maintain this encoding machinery for other potential embedded URIs. The rational here is that we might one day want to do some kind of template composition and reference a base URI. I realize we're most definitely not going to do this in the current proposal, but leaving this flexibility in the addressing scheme seems to make sense, even if this is only used by intermediaries like Istio xDS proxy, etc.

htuch added 2 commits June 28, 2020 20:56
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch htuch merged commit efcf912 into cncf:master Jun 29, 2020
@htuch htuch deleted the resource-locators branch June 29, 2020 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants