Add pkg/authn/{amazon,azure,google} #1231

imjasonh · 2022-01-06T01:10:14Z

pkg/authn/amazon is a wrapper around docker-credential-ecr-login's logic, using authn.NewKeychainFromHelper.
pkg/authn/azure is a wrapper around docker-credential-acr-env's logic, using authn.NewKeychainFromHelper.
pkg/authn/google is the auth-related functions from pkg/v1/google
pkg/v1/google.{authstuff} is aliased to pkg/authn/google and marked as Deprecated -- in a future release, we can remove it and make pkg/authn/google a separate Go module.
update pkg/authn/README.md to document these new wrappers.

Other minor stuff:

make sure we build/test all these new sub-Go-modules in hack/presubmit.sh
bump sub-Go-modules' nominal deps on the main Go module, mainly to appease dep-checking tools that don't understand the replace => ../../ stuff

Notes for future-me: when we remove the deprecated pkg/v1/google.{authstuff} aliases, we can make pkg/authn/google its own module, and probably pkg/gcrane and cmd/gcrane to avoid polluting the main module with Google-specific stuff. Or... just decide it's not worth it and keep Google stuff in the main module.

- update pkg/authn/README - include new modules in hack/presubmit.sh

codecov-commenter · 2022-01-06T01:16:52Z

Codecov Report

Merging #1231 (aef4708) into main (d9bfbcb) will not change coverage.
The diff coverage is 65.00%.

@@           Coverage Diff           @@
##             main    #1231   +/-   ##
=======================================
  Coverage   73.80%   73.80%           
=======================================
  Files         111      111           
  Lines        8191     8191           
=======================================
  Hits         6045     6045           
  Misses       1549     1549           
  Partials      597      597

Impacted Files	Coverage Δ
pkg/gcrane/copy.go	`84.89% <ø> (ø)`
pkg/authn/google/auth.go	`60.00% <60.00%> (ø)`
pkg/authn/google/keychain.go	`85.00% <85.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9bfbcb...aef4708. Read the comment docs.

mattmoor

Honestly, I find the simplicity of this fantastic. My least favorite part is the google bit, which isn't this PR's fault.

pkg/authn/google/auth.go

mattmoor · 2022-01-06T02:41:15Z

pkg/authn/google/auth.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package google


This is so big and fat compared to the others :(

I suppose putting this in its own module is a tall order given that gcrane is a bit sprawled between pkg/ and cmd/ 🤔 blah

Yeah, that's more or less where I'm at too. I'd love to have it in its own module, but it's going to be a bit of work to get it there. Worth it? Maybe. Worth doing immediately? Probably not. 🤷

pkg/authn/google/keychain.go

jonjohnsonjr · 2022-01-06T17:22:57Z

I'm -1 because I don't want this repo to be a dumping ground for keychain implementations. If we need one, docker-credential-magic seems like a better fit.

jonjohnsonjr

I think #1227 is sufficient for this to be easy to use. It doesn't seem worth the maintenance burden to make these one-liners slightly shorter.

mattmoor · 2022-01-06T17:31:46Z

Given that the ultimate goal of this effort (AIUI, @imjasonh correct me) is to improve the state of k8schain (which does live here) by replacing Vincent's personal soft fork repo with two vendor supported repos (very easily with better funding than this repo), I'm at a loss for why we wouldn't take this change.

The contentious parts are (very impressively ✨ ) a single-line and sequestered in their own modules.

mattmoor · 2022-01-06T17:34:06Z

@jonjohnsonjr would it be meaningfully different if these one-liners existed in a revamped k8schain package? I don't really think so, but if that's what it takes to kill our dependency on github.com/vdemeester/k8s-pkg-credentialprovider, so be it.

imjasonh · 2022-01-06T17:47:07Z

Given that the ultimate goal of this effort (AIUI, @imjasonh correct me) is to improve the state of k8schain (which does live here) by replacing Vincent's personal soft fork repo with two vendor supported repos (very easily with better funding than this repo), I'm at a loss for why we wouldn't take this change.

Agree, that's the goal.

I think Jon's point (which I see) is that having us maintain pkg/authn/amazon as a whole module, when it's just a one-line wrapper around external code, seems burdensome. It could be better to just document that one line (which we do today at HEAD), and use that in k8schain, which we should deprecate.

The contentious parts are (very impressively ✨ ) a single-line and sequestered in their own modules.

"A single line justifying a whole module" is I think not the unambiguous win you think it is 🙃

But I'll take ✨ s wherever I can get them 🤩

@jonjohnsonjr would it be meaningfully different if these one-liners existed in a revamped k8schain package? I don't really think so, but if that's what it takes to kill our dependency on github.com/vdemeester/k8s-pkg-credentialprovider, so be it.

Yes, maybe, I think. The question is why should users depend on our single-line adapter module instead of going straight from pkg/authn -> ecr-login, which is more transparent about the separation of responsibilities. Maybe if pkg/authn/amazon was more "our code" like pkg/authn/google is, but ugh I don't want to maintain any more of those.

I'm not sure if you're proposing k8schain.AmazonKeychain to be the home of the wrapper (I don't think you are?), but if you are, I'd be -1 on it, since I eventually want k8schain to disappear entirely, and adding things to it makes it harder.

imjasonh · 2022-01-06T17:48:28Z

btw, here's the next step in the evolution (IF we proceed along this route): imjasonh/go-containerregistry@auth-auth-auth...imjasonh:authn-kubernetes -- it splits out pkg/authn/kubernetes (still wrapping Vincent's fork), and has k8schain wrap all the authn.Keychains. It should be functionally equivalent, with a smaller and less tangled dependency graph.

edit: rebased that change onto main and opened as #1234

mattmoor · 2022-01-06T18:05:14Z

I'm not sure if you're proposing k8schain.AmazonKeychain

I'm not, I'm more or less proposing moving the one-liners to your next PR, though I'm puzzled why that isn't killing Vincent's fork entirely, but need to look closer.

mattmoor · 2022-01-06T18:06:38Z

... at the end of the day, I have zero interest in amazon.Keychain (or google.Keychain!) I am only interested in the omni Keychain 😉

imjasonh · 2022-01-06T18:18:30Z

... at the end of the day, I have zero interest in amazon.Keychain (or google.Keychain!) I am only interested in the omni Keychain 😉

Boy do I have good news for you.

imjasonh · 2022-01-07T18:03:44Z

I think we've settled on a happy medium in #1234 that doesn't make us maintain one-liner modules.

Dropping this PR.

imjasonh added 4 commits January 5, 2022 19:36

Add pkg/authn/{amazon,azure}

25d5e55

Move pkg/v1/google -> pkg/authn/google

9bcf51e

- update pkg/authn/README - include new modules in hack/presubmit.sh

update go.mods, cover krane in presubmit

6320848

use pkg/authn/google.Keychain in gcrane

02d6491

imjasonh requested review from mattmoor and jonjohnsonjr January 6, 2022 01:10

imjasonh added 2 commits January 5, 2022 20:10

Deprecate pkg/v1/google.Keychain

27a378d

fix pkg/gcrane

aef4708

imjasonh mentioned this pull request Jan 6, 2022

Add cred helper wrappers for ECR and ACR docker-credential-magic/docker-credential-magic#41

Closed

mattmoor approved these changes Jan 6, 2022

View reviewed changes

This was referenced Jan 6, 2022

Long delays on first taskrun caused by aws-sdk-go tektoncd/pipeline#4087

Closed

Create release tags for k8schain #1233

Closed

jonjohnsonjr requested changes Jan 6, 2022

View reviewed changes

imjasonh closed this Jan 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pkg/authn/{amazon,azure,google} #1231

Add pkg/authn/{amazon,azure,google} #1231

imjasonh commented Jan 6, 2022 •

edited

Loading

codecov-commenter commented Jan 6, 2022 •

edited

Loading

mattmoor left a comment

mattmoor Jan 6, 2022

mattmoor Jan 6, 2022

imjasonh Jan 6, 2022 •

edited

Loading

jonjohnsonjr commented Jan 6, 2022

jonjohnsonjr left a comment

mattmoor commented Jan 6, 2022

mattmoor commented Jan 6, 2022

imjasonh commented Jan 6, 2022

imjasonh commented Jan 6, 2022 •

edited

Loading

mattmoor commented Jan 6, 2022

mattmoor commented Jan 6, 2022

imjasonh commented Jan 6, 2022

imjasonh commented Jan 7, 2022

Add pkg/authn/{amazon,azure,google} #1231

Add pkg/authn/{amazon,azure,google} #1231

Conversation

imjasonh commented Jan 6, 2022 • edited Loading

codecov-commenter commented Jan 6, 2022 • edited Loading

Codecov Report

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor Jan 6, 2022

Choose a reason for hiding this comment

mattmoor Jan 6, 2022

Choose a reason for hiding this comment

imjasonh Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

jonjohnsonjr commented Jan 6, 2022

jonjohnsonjr left a comment

Choose a reason for hiding this comment

mattmoor commented Jan 6, 2022

mattmoor commented Jan 6, 2022

imjasonh commented Jan 6, 2022

imjasonh commented Jan 6, 2022 • edited Loading

mattmoor commented Jan 6, 2022

mattmoor commented Jan 6, 2022

imjasonh commented Jan 6, 2022

imjasonh commented Jan 7, 2022

imjasonh commented Jan 6, 2022 •

edited

Loading

codecov-commenter commented Jan 6, 2022 •

edited

Loading

imjasonh Jan 6, 2022 •

edited

Loading

imjasonh commented Jan 6, 2022 •

edited

Loading