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

Support the platform specific authentication of krane in "auth get" command #1413

Merged
merged 7 commits into from Jul 20, 2022

Conversation

ingwonsong
Copy link
Contributor

This PR enables to work "auth get" command with the platform specific keychains given in krane tool.

@imjasonh
Copy link
Collaborator

imjasonh commented Jul 20, 2022

This is really interesting! So the idea is that with this change, krane auth get 12345.dkr.ecr.us-east-1.amazonaws.com will print out the auth token, possibly as derived from a compiled-in cred helper like ecr-login etc.

I think we'll want a simple end-to-end test of this behavior, for GHCR and ECR at least.

For ECR, that means calling krane auth get ${{ env.AWS_ACCOUNT }}.dkr.ecr.${{ env.AWS_REGION }}.amazonaws.com and checking that it doesn't fail, and that it prints something -- though we should be careful not to print that out to logs.

Could you add that to this PR?

@codecov-commenter
Copy link

codecov-commenter commented Jul 20, 2022

Codecov Report

Merging #1413 (cce8f74) into main (f79ec21) will decrease coverage by 0.00%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##             main    #1413      +/-   ##
==========================================
- Coverage   73.38%   73.38%   -0.01%     
==========================================
  Files         115      115              
  Lines        8747     8750       +3     
==========================================
+ Hits         6419     6421       +2     
- Misses       1687     1688       +1     
  Partials      641      641              
Impacted Files Coverage Δ
cmd/gcrane/main.go 10.34% <0.00%> (-0.37%) ⬇️
pkg/crane/options.go 79.16% <100.00%> (+0.90%) ⬆️

@ingwonsong
Copy link
Contributor Author

@imjasonh Sure. I have just added the tests.
In addition, I also added small extension.

Previously, crane and krane get a registry address through stdio only, like:

$ echo "example.registry.com" | krane auth get

But, I added to another way to specify the registry address as an argument as follows:

$ krane auth get example.registry.com

cmd/crane/cmd/auth.go Outdated Show resolved Hide resolved
cmd/gcrane/main.go Outdated Show resolved Hide resolved
@jonjohnsonjr
Copy link
Collaborator

Thanks for this change, this is awesome.

If I had the time, I would set up workload identity so that we could test gcrane auth get and krane auth get against GCP, but I'm fine without it 😅

Looks like you just need to regenerate the docs for the presubmit.

@jonjohnsonjr jonjohnsonjr merged commit 5749ee6 into google:main Jul 20, 2022
@ingwonsong ingwonsong deleted the krane-login branch August 5, 2022 16:22
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.

None yet

4 participants