Skip to content

[release/1.7] Fix issue with empty host tree in hosts.toml#10028

Merged
mxpv merged 1 commit into
containerd:release/1.7from
brandond:fix-hosts-toml
May 11, 2026
Merged

[release/1.7] Fix issue with empty host tree in hosts.toml#10028
mxpv merged 1 commit into
containerd:release/1.7from
brandond:fix-hosts-toml

Conversation

@brandond

@brandond brandond commented Apr 1, 2024

Copy link
Copy Markdown
Contributor

Fixes: #10027

Allow hosts.toml to contain only root-level fields without an explicit [host] section

@k8s-ci-robot

Copy link
Copy Markdown

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.

Details

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.

@dmcgowan

dmcgowan commented Apr 2, 2024

Copy link
Copy Markdown
Member

Seems the issue here would be better fixed by just changing getSortedhosts to return an empty list in that case. What is root.Get("host") returning when you provide it the [host] config? It seems that code should handle root.Get("host") returning nil, but is that what it sees.

@brandond

brandond commented Apr 2, 2024

Copy link
Copy Markdown
Contributor Author

Seems the issue here would be better fixed by just changing getSortedhosts to return an empty list in that case.

Sure, that'd work too. It didn't seem like there was much point in calling getSortedHosts at all if there are no HostConfigs in the first place, but what you're suggesting is probably a slightly less involved change.

What is root.Get("host") returning when you provide it the [host] config?

I don't know, I didn't poke at the toml parser return; I suspect it just returns an empty tree so that list := append([]string{}, iter.Keys()...) just ends up creating an empty list, which is what gets returned.

@dmcgowan dmcgowan changed the title Fix issue with empty host tree in hosts.toml [release/1.7] Fix issue with empty host tree in hosts.toml Apr 5, 2024
@dmcgowan

Copy link
Copy Markdown
Member

Note this fix is not needed in main as we have bumped the toml library major version and the new implementation does not have this issue.

@thaJeztah

Copy link
Copy Markdown
Member

Is this fix needed in 1.6, or not affected?

@cpuguy83

Copy link
Copy Markdown
Member

Can you squash commits and describe the issue in the commit?

@dims dims left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@dims

dims commented Jul 10, 2024

Copy link
Copy Markdown
Member

/ok-to-test

@djdongjin

Copy link
Copy Markdown
Member

Hi @brandond can you squash the commit? Then the PR should be ready to be merged?

Is this fix needed in 1.6, or not affected?

I think it's needed in 1.6, I remember seeing the same issue on 1.6

Comment thread remotes/docker/config/hosts.go
@AkihiroSuda

Copy link
Copy Markdown
Member

Can we merge this?

@estesp

estesp commented Jul 16, 2025

Copy link
Copy Markdown
Member

I think we were hoping for the submitter (@brandond) to squash the commits and add a description of the fix to the commit message. Maybe we should carry or does anyone have a strong opinion on the squash/comment? We've asked several times

@brandond

Copy link
Copy Markdown
Contributor Author

I can rebase and squash, sure.

Allows parsing a hosts.toml file that has no `[host]` or
`[host."https://registry.example.com"]` entries.

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>
@brandond

Copy link
Copy Markdown
Contributor Author

Done.

@estesp

estesp commented Jul 17, 2025

Copy link
Copy Markdown
Member

Done.

Thanks!

@github-project-automation github-project-automation Bot moved this from Needs Triage to Review In Progress in Pull Request Review Jul 17, 2025
@samuelkarp

Copy link
Copy Markdown
Member

@brandond Is this still relevant to you? I'm coming in to look at old PRs and this one seems to have been approved, but never got merged for some reason. We've recently transitioned 1.7 to focus primarily on GKE's use cases (since @chrishenzie and I are extending maintenance on the branch beyond the rest of the maintainers) and this is not really relevant to GKE, but I'd feel bad closing this since it's been approved and stuck for nearly a year...

@mxpv mxpv merged commit 33d9e24 into containerd:release/1.7 May 11, 2026
133 of 136 checks passed
@github-project-automation github-project-automation Bot moved this from Review In Progress to Done in Pull Request Review May 11, 2026
@samuelkarp samuelkarp added the area/distribution Image Distribution label May 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.