Refresh SA auth token in signaturediscovery client before fetching container image signatures #449
+115
−22
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yawangwang
force-pushed
the
fix-vm-sa-credential-expire
branch
from
9904833 to
ed0f812
Compare
yawangwang
changed the title
yawangwang
force-pushed
the
fix-vm-sa-credential-expire
branch
3 times, most recently
from
efb0f2d to
bc27300
Compare
jkl73
reviewed
May 10, 2024
| ) | ||
|
|
||
| const signatureTagSuffix = "sig" | ||
|
|
||
| type ( | ||
| oauth2TokenFetcher func(context.Context) (oauth2.Token, error) |
There was a problem hiding this comment.
Instead of having the logic in the signature client, can we move it to auth.go package? So the resolver can handle the token fetching itself
launcher/auth/auth.go
Outdated
| @@ -1,4 +1,5 @@ | |||
| package launcher | |||
| // Package auth contains functionalities to authenticate docker repo. | |||
| package auth | |||
There was a problem hiding this comment.
Like having a new Resolver in this package that takes in mds client instead of the token
yawangwang
force-pushed
the
fix-vm-sa-credential-expire
branch
from
bc27300 to
cbc5822
Compare
jkl73
approved these changes
May 14, 2024
alexmwu
approved these changes
May 16, 2024
launcher/auth/auth.go
Outdated
| @@ -1,8 +1,10 @@ | |||
| package launcher | |||
| // Package auth contains functionalities to authenticate docker repo. | |||
There was a problem hiding this comment.
This would be better named something like registryauth.
There was a problem hiding this comment.
Please comment this as a breaking change in the PR description
| OriginalImageDesc v1.Descriptor | ||
| RemoteOpts []containerd.RemoteOpt | ||
| refreshResolver remoteResolverFetcher |
There was a problem hiding this comment.
remoteResolverFetcher or refreshedResolverFetcher to match the name below.
yawangwang
force-pushed
the
fix-vm-sa-credential-expire
branch
from
cbc5822 to
5ce689e
Compare
yawangwang
force-pushed
the
fix-vm-sa-credential-expire
branch
from
5ce689e to
ad7225e
Compare
|
/gcbrun |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently we only fetch the VM SA once, for the purpose of downloading workload image from Artifact registry.
But it doesn't work well for long running workloads because the VM SA token will expire after 1 hr, and thus lead to 401 errors when the signaturediscovery client tries to fetch image signatures periodically.
We'll need to refresh the resolver for signaturediscovery client to authenticate with the target docker repo before fetching container signatures.
Breaking changes:
auth.goto from packagelaunchera to a new sub-packagegithub.com/google/go-tpm-tools/launcher/registryauthso that auth utilities can be used cross packages. Add a new method that takes in a metadata server client and returns a refreshed remote resolver.