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

Login refactor (small breakout): add support for explicitly setting --insecure-registry=false #3411

Closed

Conversation

apostasie
Copy link
Contributor

This is the ground work for fixing #3046.

See commit message for details.

TL;DR:
docker login --insecure-registry false localhost should NOT treat localhost as insecure.

@apostasie apostasie changed the title Add support for explicitly setting --insecure-registry=false Login refactor (small breakout): add support for explicitly setting --insecure-registry=false Sep 4, 2024
@@ -39,6 +39,8 @@ type Config struct {
HostsDir []string `toml:"hosts_dir"`
Experimental bool `toml:"experimental"`
HostGatewayIP string `toml:"host_gateway_ip"`

ExplicitInsecureRegistry bool `toml:"-"`
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this. The struct is dedicated for TOML properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was thinking about that.

We could use a different struct instead (or just an extra param) - but then we would have to modify the signature of a lot of functions in the path of pull, push, login.

Let me know what you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Alternative proposal (overloading globalconfig) with the latest push.
LMK your thoughts.

As described in containerd#3046 it is not currently possible to communicate
with localhost using TLS, as insecure-registry is forcefully set for localhost-ish addresses.
This PR does introduce support to differenciate between the default value of --insecure-registry when it is NOT
specified (which is false), and the fact that the user has explicitly passed the flag as `--insecure-registry=false`.

Being able to differenciate these two cases will enable us to treat differently:
`docker login localhost` (usual behavior, insecure set to true)
and
`docker login --insecure-regitry=false localhost` (new behavior, NOT setting insecure to true)

Implementation will follow in a subsequent PR that will rewrite login.go.

Signed-off-by: apostasie <[email protected]>
@AkihiroSuda
Copy link
Member

Implementation will follow in a subsequent PR that will rewrite login.go.

The struct looks good, but I expect the implementation to be accompanied in the same PR

@apostasie
Copy link
Contributor Author

Implementation will follow in a subsequent PR that will rewrite login.go.

The struct looks good, but I expect the implementation to be accompanied in the same PR

The implementation does require rather large modifications to dockerconfigresolver and login code which are difficult to breakdown.

What about I close this PR then - and just land this as part of the (larger) PR to come?

I will try my best to have as many separate commits as possible to ease review.

@apostasie apostasie closed this Sep 5, 2024
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.

2 participants