Login & authentication rework, part 1: credentials management and URL parsing #3293

apostasie · 2024-08-11T20:47:27Z

This is a breakout of #3249 and first part of fixing our credentials management / login issues (see 3249 for details about issues, including #3072 which is the reason a significant rewrite is warranted IMHO).

The focus is this PR is solely:

providing a CredentialsStore abstraction that takes care of any magic / legacy identifiers
handling all registry url parsing & scheme / url monkeying in one place (RegistryURL)

This does:

fix Login to docker.io failed. (expected acArg to be "docker.io", got "registry-1.docker.io") #3245 and a number of other oddities (now enshrined in tests)
significantly increase testing (+~1000 lines is just tests), both unit testing for CredentialsStore and RegistryURL parsing, and integration testing, which should hopefully give us a much firmer understanding of what is supposed to happen when
reduce code duplication
greatly simplify handling of registry urls
allows fixing further login issues as a follow-up, on better grounds
allows for future implementation of namespaced hosts login

What this does NOT:

does not try to solve [Design discussion] docker credentials and hosts.toml #3265 (that will come later)
does not do anything for hosts.toml resolution (ditto)
does not add Harbor testing (ditto)

apostasie · 2024-08-12T02:28:45Z

@fahedouch @AkihiroSuda PTAL at your convenience

apostasie · 2024-08-12T02:29:11Z

cmd/nerdctl/login_linux_test.go

 )

-func safeRandomString(n int) string {


Moved to test_registry

pkg/imgutil/dockerconfigresolver/credentialsstore.go

pkg/imgutil/dockerconfigresolver/credentialsstore_test.go

apostasie · 2024-08-12T17:12:45Z

Conflict is likely trivial. Will rebase after review, in case there are any comments or concerns to address.

apostasie · 2024-08-15T18:09:35Z

@AkihiroSuda we have regular network related failures on Ubuntu 20.04 / v1.6.

Symptom being no route to host:

time="2024-08-15T17:33:19Z" level=error msg="failed to call tryLoginWithRegHost" error="failed to call rh.Authorizer.Authorize: failed to fetch oauth token: Post \"[http://10.4.0.1:5000/auth\](http://10.4.0.1:5000/auth/)": dial tcp 10.4.0.1:5000: connect: no route to host" i=0
        time="2024-08-15T17:33:19Z" level=fatal msg="failed to call rh.Authorizer.Authorize: failed to fetch oauth token: Post \"[http://10.4.0.1:5000/auth\](http://10.4.0.1:5000/auth/)": dial tcp 10.4.0.1:5000: connect: no route to host"

We could retry on login for that condition - but I am more interested in fixing the root issue.

Do you think this is something in rootlesskit or bypass4netns?

AkihiroSuda · 2024-08-15T18:12:46Z

Do you think this is something in rootlesskit or bypass4netns?

No, this seems rootful
https://github.com/containerd/nerdctl/actions/runs/10407628383/job/28823319574?pr=3293

apostasie · 2024-08-15T18:16:42Z

Do you think this is something in rootlesskit or bypass4netns?

No, this seems rootful https://github.com/containerd/nerdctl/actions/runs/10407628383/job/28823319574?pr=3293

Yes, you are right.

So, I guess when under pressure (starting / stopping many containers), networking is breaking down or is slow to converge?

pkg/imgutil/dockerconfigresolver/credentialsstore_test.go

pkg/testutil/testregistry/testregistry_linux.go

djdongjin

Left some early comments. Still have some changes left to be reviewed. thanks

cmd/nerdctl/logout.go

pkg/testutil/testregistry/testregistry_linux.go

cmd/nerdctl/login_linux_test.go

djdongjin · 2024-08-27T17:48:18Z

pkg/imgutil/dockerconfigresolver/credentialsstore.go

+	"github.com/containerd/log"
+)
+
+type Credentials = types.AuthConfig


Suggest change to type AuthConfig = types.AuthConfig. If we want to have a type alias we should keep the name the same.

Ok.
I guess I do not like AuthConfig as a name, but that's fine, let's keep it the same.

Actually, do you mind if we keep Credentials?
AuthConfig is really a bad name... If anything, it does evoke https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/7/html/system-level_authentication_guide/authconfig-install#authconfig-install instead of what it really is... just credentials...

It also "makes sense" that the CredentialsStore would store Credentials.

pkg/imgutil/dockerconfigresolver/credentialsstore.go

djdongjin · 2024-08-27T17:54:30Z

pkg/imgutil/dockerconfigresolver/credentialsstore.go

+type isFileStore interface {
+	IsFileStore() bool
+	GetFilename() string
+}


interface should be moved to the begining of the file, or at least before the implementation.

It is private and only used internally in this file - there is no implementation, as it is used solely to cast docker credentials store.

In that context, do you feel it should still be moved up? (no strong opinion on my side)

djdongjin · 2024-08-27T17:57:45Z

pkg/imgutil/dockerconfigresolver/defaults.go

+var (
+	ErrUnableToInstantiate = errors.New("unable to instantiate docker credentials store")
+	ErrUnableToErase       = errors.New("unable to erase credentials")
+	ErrUnableToStore       = errors.New("unable to store credentials")
+	ErrUnableToRetrieve    = errors.New("unable to retrieve credentials")
+)
+
+// Errors returned by `Parse`
+var (
+	ErrUnparsableURL     = errors.New("unparsable registry URL")
+	ErrUnsupportedScheme = errors.New("unsupported scheme in registry URL")
+)


nit: are they used widely in nerdctl or by downstream? if not I prefer just returning a errors.New(...) directly when needed instead of predefining them.

nit: are they used widely in nerdctl or by downstream?

Not yet.

The reason I like them is that they allow downstream code to actually test (errors.Is(err, ErrXYZ)) the error (instead of matching strings, which I find abhorrent...), and allows providing more refined user feedback based on these (or decide to ignore certain errors in certain circumstances).

This of course applies to testing as well (as a nice benefit, where we can actually verify what error is being returned in integration testing without having to duplicate strings again).

LMK if you feel strongly about these and of course open to amending.

Overall motivation for me is that our error-ing is not great right now (see #3302 for example), and that is generally in the codebase.

We tend to just bubble-up system errors that are meaningless or even misleading for the user (#3197) - and at best, makes it hard to pinpoint for us when reading bug reports.

This is obviously a very large endeavor to change this situation overall, and it might take a few iterations to come up with a reasonable set of errors and informative enough wrapping, but I believe it is essential.

apostasie · 2024-08-27T18:39:27Z

Left some early comments. Still have some changes left to be reviewed. thanks

Thanks a lot @djdongjin
Really appreciate the review.
I answered you in the comments - addressed some with the latest push - provided clarification on others - so, let me know in there how you feel, and I will look for further comments from you when you will be done reviewing.

apostasie · 2024-08-27T19:30:33Z

Failure are unrelated:

ComposeUp random fail
networking fluke: dial tcp 10.4.0.1:5000: connect: no route to host" i=0
docker inability to pull an image (might be for the same reason as ^)

Could we poke the CI to restart these?

djdongjin

I probably need more time to review some of the changes, as it's a bit difficut to identify which are logic changes, and which are refactors/code relocation/etc.

Also some suggestions on reducing PR size:

Add UTs first in a separate PR or commit if UT itself has a lot of changes. If you're trying to fix something and the UT helps reproduce it, you can add and skip that UT (or just add it in a later PR).
Do refactor or move code around in a separate PR: it's difficult to identify a small change if it's mixed with a refactor.
You can use tools like https://github.com/modularml/stack-pr to make small PRs and make a big change incrementally.

apostasie · 2024-08-28T22:06:14Z

I probably need more time to review some of the changes, as it's a bit difficut to identify which are logic changes, and which are refactors/code relocation/etc.

That is fine.
I do appreciate my latest PRs are more sizable - and certainly understand reviewing these things is significant work - really grateful for that @djdongjin.

Also some suggestions on reducing PR size:

Add UTs first in a separate PR or commit if UT itself has a lot of changes. If you're trying to fix something and the UT helps reproduce it, you can add and skip that UT (or just add it in a later PR).

Do refactor or move code around in a separate PR: it's difficult to identify a small change if it's mixed with a refactor.

You can use tools like https://github.com/modularml/stack-pr to make small PRs and make a big change incrementally.

Will definitely look into stack-pr - great tips, thanks.

Let me see if I can massage my two other "big-ish" PRs into something more amenable to review.

Meanwhile, we can just stay focused on this one - and also #3362 would be nice to get if you have a minute <- it is a small one! Pinky swear! :-)

apostasie · 2024-08-28T22:20:51Z

Latest CI fail is the usual TestIPFSComposeUp - this one fails more often than it succeeds.

Unfortunately, I couldn't figure it out so far (#3305) - it is a massive PITA.

Anyone familiar with IPFS around?

AkihiroSuda · 2024-08-29T07:26:15Z

IPFS

cc @ktock

apostasie · 2024-08-30T22:23:36Z

@AkihiroSuda could you poke the CI for the flaky IPFS 20.04/1.6 run?

apostasie · 2024-09-02T17:23:17Z

Rebased. Pending green CI.

pkg/imgutil/dockerconfigresolver/registryurl.go

apostasie · 2024-09-03T03:15:36Z

Rebased.
Debug statements removed.
Pending CI.

AkihiroSuda

Thanks

Signed-off-by: apostasie <spam_blackhole@farcloser.world>

apostasie · 2024-09-03T15:55:13Z

Thanks

Last push just enable the commented out test (see answer in comment about Lima error)

djdongjin

LGTM, thanks

apostasie force-pushed the dev-login-1 branch 5 times, most recently from d0e699e to 0afef56 Compare August 12, 2024 02:01

apostasie changed the title ~~[IGNORE FOR NOW]~~ Login & authentication rework, part 1: credentials management and URL parsing Aug 12, 2024

apostasie commented Aug 12, 2024

View reviewed changes

cmd/nerdctl/login_linux_test.go Outdated

)

func safeRandomString(n int) string {

Copy link

Contributor Author

apostasie Aug 12, 2024

Choose a reason for hiding this comment

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

Moved to test_registry

apostasie commented Aug 12, 2024

View reviewed changes

pkg/imgutil/dockerconfigresolver/credentialsstore.go Outdated Show resolved Hide resolved

apostasie commented Aug 12, 2024

View reviewed changes

pkg/imgutil/dockerconfigresolver/credentialsstore_test.go Outdated Show resolved Hide resolved

apostasie marked this pull request as ready for review August 12, 2024 02:34

AkihiroSuda requested a review from fahedouch August 12, 2024 07:42

apostasie force-pushed the dev-login-1 branch 2 times, most recently from d50e9da to 864c5be Compare August 15, 2024 17:08

AkihiroSuda requested a review from a team August 15, 2024 17:09

apostasie force-pushed the dev-login-1 branch from 864c5be to 61923af Compare August 15, 2024 17:10

AkihiroSuda added the area/login authentification/ login label Aug 15, 2024

apostasie force-pushed the dev-login-1 branch from 61923af to bfbfa82 Compare August 17, 2024 15:33

AkihiroSuda reviewed Aug 17, 2024

View reviewed changes

pkg/imgutil/dockerconfigresolver/credentialsstore_test.go Show resolved Hide resolved

AkihiroSuda reviewed Aug 17, 2024

View reviewed changes

pkg/imgutil/dockerconfigresolver/credentialsstore_test.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Aug 17, 2024

View reviewed changes

pkg/testutil/testregistry/testregistry_linux.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Aug 17, 2024

View reviewed changes

pkg/testutil/testregistry/testregistry_linux.go Outdated Show resolved Hide resolved

AkihiroSuda requested a review from a team August 17, 2024 16:21

apostasie force-pushed the dev-login-1 branch 3 times, most recently from b0d4293 to 996f7c6 Compare August 20, 2024 05:01

AkihiroSuda requested review from a team and djdongjin August 22, 2024 15:21

djdongjin reviewed Aug 27, 2024

View reviewed changes

apostasie force-pushed the dev-login-1 branch from 3c7de2a to e2b7f49 Compare August 27, 2024 18:37

djdongjin reviewed Aug 28, 2024

View reviewed changes

apostasie mentioned this pull request Sep 1, 2024

Login & authentication rework, part 2: prompt cleanup #3392

Merged

apostasie force-pushed the dev-login-1 branch from e2b7f49 to f16657d Compare September 2, 2024 17:19

apostasie force-pushed the dev-login-1 branch from f16657d to 17b2ad0 Compare September 2, 2024 17:37

AkihiroSuda requested a review from djdongjin September 3, 2024 01:24

AkihiroSuda reviewed Sep 3, 2024

View reviewed changes

pkg/imgutil/dockerconfigresolver/registryurl.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Sep 3, 2024

View reviewed changes

pkg/imgutil/dockerconfigresolver/registryurl.go Outdated Show resolved Hide resolved

apostasie mentioned this pull request Sep 3, 2024

[ON HOLD] Disable TestIPFSComposeUp #3396

Closed

apostasie force-pushed the dev-login-1 branch from 17b2ad0 to 864b432 Compare September 3, 2024 03:13

AkihiroSuda approved these changes Sep 3, 2024

View reviewed changes

Auth code refactor: credstore and registry URL

04cc74b

Signed-off-by: apostasie <spam_blackhole@farcloser.world>

apostasie force-pushed the dev-login-1 branch from 864b432 to 04cc74b Compare September 3, 2024 15:54

djdongjin approved these changes Sep 3, 2024

View reviewed changes

djdongjin added this to the v2.0.0 milestone Sep 3, 2024

djdongjin merged commit 30c63cf into containerd:main Sep 3, 2024
22 checks passed

apostasie mentioned this pull request Sep 4, 2024

dockerConfigFile.GetCredentialsStore.Store faulty symlink resolution behavior #3413

Open

apostasie mentioned this pull request Oct 2, 2024

Fix Docker Hub push regression #3485

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Login & authentication rework, part 1: credentials management and URL parsing #3293

Login & authentication rework, part 1: credentials management and URL parsing #3293

apostasie commented Aug 11, 2024 •

edited

Loading

apostasie commented Aug 12, 2024

apostasie Aug 12, 2024

apostasie commented Aug 12, 2024

apostasie commented Aug 15, 2024

AkihiroSuda commented Aug 15, 2024

apostasie commented Aug 15, 2024

djdongjin left a comment

djdongjin Aug 27, 2024

apostasie Aug 27, 2024

apostasie Sep 1, 2024

djdongjin Aug 27, 2024

apostasie Aug 27, 2024

djdongjin Aug 27, 2024

apostasie Aug 27, 2024

apostasie Aug 27, 2024

apostasie commented Aug 27, 2024

apostasie commented Aug 27, 2024 •

edited

Loading

djdongjin left a comment

apostasie commented Aug 28, 2024

apostasie commented Aug 28, 2024 •

edited

Loading

AkihiroSuda commented Aug 29, 2024

apostasie commented Aug 30, 2024

apostasie commented Sep 2, 2024

apostasie commented Sep 3, 2024 •

edited

Loading

AkihiroSuda left a comment

apostasie commented Sep 3, 2024

djdongjin left a comment

Login & authentication rework, part 1: credentials management and URL parsing #3293

Login & authentication rework, part 1: credentials management and URL parsing #3293

Conversation

apostasie commented Aug 11, 2024 • edited Loading

apostasie commented Aug 12, 2024

Choose a reason for hiding this comment

apostasie commented Aug 12, 2024

apostasie commented Aug 15, 2024

AkihiroSuda commented Aug 15, 2024

apostasie commented Aug 15, 2024

djdongjin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apostasie commented Aug 27, 2024

apostasie commented Aug 27, 2024 • edited Loading

djdongjin left a comment

Choose a reason for hiding this comment

apostasie commented Aug 28, 2024

apostasie commented Aug 28, 2024 • edited Loading

AkihiroSuda commented Aug 29, 2024

apostasie commented Aug 30, 2024

apostasie commented Sep 2, 2024

apostasie commented Sep 3, 2024 • edited Loading

AkihiroSuda left a comment

Choose a reason for hiding this comment

apostasie commented Sep 3, 2024

djdongjin left a comment

Choose a reason for hiding this comment

apostasie commented Aug 11, 2024 •

edited

Loading

apostasie commented Aug 27, 2024 •

edited

Loading

apostasie commented Aug 28, 2024 •

edited

Loading

apostasie commented Sep 3, 2024 •

edited

Loading