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

[release/1.7] Support encoded characters in hosts table key #10072

Closed

Conversation

brandond
Copy link
Contributor

@brandond brandond commented Apr 12, 2024

Fixes #10055

Allows escaping invalid table keys, for example the braces required to specify an IPv6 literal URL:

[host."https://\u005Bfd7c:53a5:aef5::242:ac11:7\u005D/v2"]
  capabilities = ["pull", "resolve"]
  skip_verify = true

This is about the most minimal fix I can come up with. Other options would probably require adding a new field to override the hostname, similar to OverridePath but for the hostname. go-toml is super helpful and does not expose any functions for decoding core types, so we use the json decoder for that since it also supports toml's \uXXXX format.

Honestly this datastructure should have been a table list with a server field for each entry, rather than trying to get clever and put the server URL in the table key. Using a list would have also avoided all the weirdness with having to re-parse the tree to get the key order from line numbers in the file.

[[host]]
  server = "https://[fd7c:53a5:aef5::242:ac11:7]/v2"
  capabilities = ["pull", "resolve"]
  skip_verify = true

@k8s-ci-robot
Copy link

Hi @brandond. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
@samuelkarp
Copy link
Member

We don't generally accept non-cherrypick PRs to release branches. I'm going to close this; can you reopen against main for review, and we can decide whether to cherrypick into 1.7 from there?

@brandond
Copy link
Contributor Author

brandond commented Apr 12, 2024

We'll be carrying this on the k3s side regardless since we are not ready to move to 2.x yet, and do need to support IPv6 mirror endpoints

Would you prefer to see a minimal fix like this, or something that addresses the larger issue of it being inappropriate to use URLs as toml table keys?

@samuelkarp
Copy link
Member

We'll be carrying this on the k3s side regardless since we are not ready to move to 2.x yet, and do need to support IPv6 mirror endpoints

Understood. Just from an upstream perspective, we don't typically accept net-new code to release branches unless there's a very good reason; the typical path is for new code to go in main and then be cherrypicked across if it's acceptable.

Would you prefer to see a minimal fix like this, or something that addresses the larger issue of it being inappropriate to use URLs as toml table keys?

Let's discuss that on the issue, but you could also open a PR for the minimal fix against main just to have the code ready to go.

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.

None yet

3 participants