Prevent asking pull scope for cross-repo mounting #604
Conversation
Codecov Report
@@ Coverage Diff @@
## master #604 +/- ##
=======================================
Coverage 72.85% 72.85%
=======================================
Files 102 102
Lines 4487 4487
=======================================
Hits 3269 3269
Misses 807 807
Partials 411 411
Continue to review full report at Codecov.
|
pkg/v1/remote/write.go
Outdated
| if ml.Reference.Context() != ref.Context() { | ||
| // we will add push scope for ref.Context() after the loop. | ||
| // for now we ask pull scope for references of the same registry | ||
| if ml.Reference.Context().Registry == ref.Context().Registry { |
There was a problem hiding this comment.
I think we want to keep both the old check and the new check, otherwise you could end up with scopes like:
repository:test-kaniko/test-kaniko:push,pull&repository:test-kaniko/test-kaniko:pull
... which might confuse a registry.
There was a problem hiding this comment.
@jonjohnsonjr Ok. I also added a test to validate this behaviour.
In the mean time, I find (but I am not an expert) the two lines confusing:
scopeSet[ml.Reference.Context().Scope(transport.PullScope)] = struct{}{}
...
scopes = append(scopes, ref.Scope(transport.PushScope))
Why don't we do
scopeSet[ml.Reference.Context().Scope(transport.PullScope)] = struct{}{}
scopes = append(scopes, ref.Context().Scope(transport.PushScope))
...
(or without `Context()` in both instruction?
There was a problem hiding this comment.
Why don't we do
Good question!
(or without
Context()in both instruction?
I'd prefer this. I'm not sure why we call Context on the MountableLayer -- I'm guessing the author was thinking "I need read scopes for this layer's repository, so I'll call Context", but I believe there's no different. You could just call ml.Reference.Scope(transport.PullScope) instead, which I think is cleaner.
There was a problem hiding this comment.
Ok I've re pushed this change
This bumps the github.com/google/go-containerregistry dependency so that a fix for supporting app and run images on from different registries is pulled in. In particular, google/go-containerregistry#604 is the fix. Previously, when the app and run image are on different registries, the app image registry would be asked for the pull scope of the run image which will fail since it's not present.
This bumps the github.com/google/go-containerregistry dependency so that a fix for supporting app and run images on from different registries is pulled in. In particular, google/go-containerregistry#604 is the fix. Previously, when the app and run image are on different registries, the app image registry would be asked for the pull scope of the run image which will fail since it's not present. Signed-off-by: Nan Zhong <nan@notanumber.io>
This bumps the github.com/google/go-containerregistry dependency so that a fix for supporting app and run images on from different registries is pulled in. In particular, google/go-containerregistry#604 is the fix. Previously, when the app and run image are on different registries, the app image registry would be asked for the pull scope of the run image which will fail since it's not present. Signed-off-by: Nan Zhong <nan@notanumber.io>
@jonjohnsonjr As promised, I implemented the fix we discussed. I also added a test to enforce the implementation.
See #600