-
Notifications
You must be signed in to change notification settings - Fork 528
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
Add pkg/authn/kubernetes #1234
Add pkg/authn/kubernetes #1234
Conversation
70e2d8c
to
6f79f33
Compare
Codecov Report
@@ Coverage Diff @@
## main #1234 +/- ##
=======================================
Coverage 73.77% 73.77%
=======================================
Files 111 111
Lines 8183 8183
=======================================
Hits 6037 6037
Misses 1549 1549
Partials 597 597 Continue to review full report at Codecov.
|
hack/presubmit.sh failed in CI:
This is because The pkg/authn/k8schain module is still |
pkg/authn/kubernetes/keychain.go
Outdated
return nil, err | ||
} | ||
for k, v := range cfg.Auths { | ||
m[k] = v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This (and line 119 below) means that later-specified secrets with matching repo/registry keys will overwrite previous ones. New
fetches explicitly listed imagePullSecrets
before searching for those attached to the SA, so those later ones, if there's a conflict, will win.
We have some options:
- don't overwrite -- first matching auth wins
- fetch implicit SA secrets before explicitly listed secrets
In any case, we probably need to spell it out better and guard it with tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to chime in here (I meant to before). If this matches K8s' behavior, then it SGTM. It's unfortunate we don't track that anymore, but it felt like that was only a matter of time anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it turns out the previous behavior was first-one-wins, which I verified with a test on main
that I ported to this PR.
pkg/authn/k8schain/k8schain.go
Outdated
// | ||
// Deprecated: Use pkg/authn/kubernetes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is probably worth separating the conversation about Deprecation from this PR, because I don't see the value in forcing all of the folks that are using k8schain
to drop it so that we can stop maintaining what is now a very small shim. We can always add these later if we can agree on a path forward for folks.
To me the main win here is cutting out the need for Vincent's fork, which is HUGE, and I totally appreciate all of the work (and arguing!) to get us to this point. When the world stops ending, I owe you some beverages 🍻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you're probably right. Done.
@@ -0,0 +1,47 @@ | |||
module github.com/google/go-containerregistry/pkg/authn/kubernetes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this just live in k8schain rather than having yet another submodule?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect (@imjasonh can correct) this was in an effort to deprecate k8schain
, which I think warrants more discussion. So if we push out deprecation for another convo, this SGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's useful to be able to have a K8s-secret-backed Keychain that doesn't also pull in all of k8schain's dependencies, even though they're much smaller and more well behaved now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@imjasonh just one question, because we remove the files that have //go:build !disable_gcp
, are we sure the behavior of the new thing we use instead (some part of go-containerregistry
) doesn't do what upstream k8s did (querying a url infinitely if it came to not answering) ?
I am mainly asking to not re-open a bug in tekton (and knative) on openshift 😝. Otherwise looks good and simple 😻
@@ -3,11 +3,16 @@ module github.com/google/go-containerregistry/pkg/authn/k8schain | |||
go 1.14 | |||
|
|||
require ( | |||
github.com/google/go-containerregistry v0.5.2-0.20210609162550-f0ce2270b3b4 | |||
github.com/vdemeester/k8s-pkg-credentialprovider v1.21.0-1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
Good question. First, based on my testing with the AWS keychain imported but not used, it doesn't exhibit the 10-minute mystery wait that was observed in Tekton (tektoncd/pipeline#4087). This isn't a guaranteed fix, because to be fair I never saw the mystery wait anyway, even before Tekton snipped it out. Maybe I wasn't testing the right way? 🤷 Secondly, a nice thing about Tekton could even jump through some hoops and retain the build tag logic, and have AWS disabled selectively at compile-time. We can discuss that in the PR where we bring this change into Tekton. |
Ah that make complete sense 😬 👍🏼 |
Another alternative would be to simply |
This takes the Kubernetes client-go parts from k8schain, removes the forked-upstream credentialprovider bits and rewrites them to include only exactly what we need. It also reimplements k8schain in terms of this new K8s helper, and of wrapped cred helpers using authn.NewKeychainFromHelper.
931cc7b
to
1352567
Compare
@imjasonh just a heads up, we have previously had issues with the hanging requests in @crossplane and recently had them resurface when folks are using proxies, so definitely happy to see this land. I have some community members running tests of consuming this implementation across a variety of environments (pull through caches, pulling using IRSA, etc.) in crossplane/crossplane#2841. Happy to exercise any other scenarios of interest and report back as helpful. Appreciate the efforts here :) |
@hasheddan that's great to hear! More testers on this will definitely be helpful. ❤️ |
This takes the Kubernetes client-go parts from k8schain, removes the
forked-upstream credentialprovider bits and rewrites them to include
only exactly what we need.
It also reimplements k8schain in terms of this new K8s helper, and of
wrapped cred helpers using authn.NewKeychainFromHelper.
cc @vdemeester