Skip to content

Login refactor (small breakout): hostsstore resolution cleanup#3410

Merged
AkihiroSuda merged 1 commit intocontainerd:mainfrom
apostasie:dev-login-hosts-file-default-port
Sep 24, 2024
Merged

Login refactor (small breakout): hostsstore resolution cleanup#3410
AkihiroSuda merged 1 commit intocontainerd:mainfrom
apostasie:dev-login-hosts-file-default-port

Conversation

@apostasie
Copy link
Copy Markdown
Contributor

@apostasie apostasie commented Sep 4, 2024

Fix #3047

Commit message for details.

This is not testable yet (IIRC, because of the way we currently resolve authentication), but I will add tests for this specific condition later on in another upcoming "login" PR.

As described in containerd#3047, hosts.toml file lookup will treat
https://foo:443 and https://foo differently, possibly leading to divergent behaviors *for the same registry*.
This PR makes it so that a registry URL using the default https port (443) will ALSO lookup files
stored in a "portless" directory.

Finally, as dockerconfigresolver.go will soon go under significant changes, the hosts.toml resolution functions have been also
isolated in their separate, own file.

Signed-off-by: apostasie <spam_blackhole@farcloser.world>
@apostasie apostasie force-pushed the dev-login-hosts-file-default-port branch from a49487d to 1a4a5a9 Compare September 4, 2024 20:29
@apostasie
Copy link
Copy Markdown
Contributor Author

CI failure appears to be a network fluke.

@AkihiroSuda AkihiroSuda added this to the v2.0.0 milestone Sep 10, 2024
@apostasie
Copy link
Copy Markdown
Contributor Author

Gentle ping on this.
Although there is no rush - as login rewrite is still pending the design document to be reviewed - would be nice to get this in.

Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit 2db8052 into containerd:main Sep 24, 2024
@nakamorichi
Copy link
Copy Markdown

nakamorichi commented Sep 25, 2024

@apostasie @AkihiroSuda This may have broken repository authentication (currently not able to confirm due to lack of time).

When doing nerdctl login, the repo address in ~/.docker/config.json ends up with port, and at least authentication to ECR fails. When removing the port manually, ECR auth works. Could be also ECR issue, though. I started experiencing the issue recently, but can't pinpoint exact time or particular change as I upgraded various components in my build environment recently.

EDIT: I'm using v2.0.0-rc.2, but maybe this change is not yet merged there? In that case, the issue is related to something else (perhaps some issue mentioned in #3072 ).

@apostasie
Copy link
Copy Markdown
Contributor Author

@nakamorichi will look asap tomorrow.

@apostasie
Copy link
Copy Markdown
Contributor Author

@nakamorichi confirming this change here has not made it to any rc.
I would still like to look into your problem.
Can you open an issue with details on how to reproduce?

thanks

@apostasie
Copy link
Copy Markdown
Contributor Author

apostasie commented Sep 25, 2024

@nakamorichi
Using the latest main, things work for me with the following:

aws ecr-public get-login-password --region us-east-1 | nerdctl login --username AWS --password-stdin public.ecr.aws

(works as well with private repositories)

If you are still facing issues, as suggested, you should open a new ticket, with details on what exactly you are doing / how to reproduce.
Thanks.

@nakamorichi
Copy link
Copy Markdown

@apostasie Understood. I'll see if I can find a way to reproduce it. Btw, I'm using lima on macOS Sequoia, but I'm not sure how it could have impact in this case.

@apostasie
Copy link
Copy Markdown
Contributor Author

@apostasie Understood. I'll see if I can find a way to reproduce it. Btw, I'm using lima on macOS Sequoia, but I'm not sure how it could have impact in this case.

Am on Lima too (and whatever is macOS latest).
I do not think it would - very curious about your problem though - lmk if you investigate more)

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.

hosts.toml file resolution does not seem to account for default port being ommitted

3 participants