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.
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
test/e2e_node: add kubelet credential provider tests #108651
test/e2e_node: add kubelet credential provider tests #108651
Changes from all commits
f440a69
758d78a
ddeb1e1
fe55bf1
bfed342
3bd37e6
a4b7959
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
how much different is it from the real one? What is blocking from using a real one? My worry as with any fake that we may end up chasing issues that would result from the infra and the real provider updates.
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.
There are quite a few differences, the actual implementation is here: https://github.com/kubernetes/cloud-provider-gcp/tree/master/cmd/auth-provider-gcp
Having a local copy just for testing is really to avoid dependencies we would have with k8s.io/cloud-provider-gcp. We're trying to remove go module dependencies to cloud provider implementations eventually so it would be counter intuitive to import k8s.io/cloud-provider-gcp in order to test the credential provider mechanism. Luckily the GCP provider doesn't require vendoring a cloud SDK so a local copy just for node e2e testing is actually do-able.
This is fair and I think the full e2e test suite (not node e2e) should use the actual provider configured via prow jobs. This is something @adisky was working on. The goal of this specific PR is to provide test coverage for the kubelet plugin mechanism itself, it just so happens that we run node e2es on GCE nodes and all the authenticated images are on gcr.io which necessitates this.
I think we're trying to find a balance here of doing the bare minimum to only validate the core Kubernetes parts, i.e. the kubelet code executing the plugins and not the plugins themselves, which is difficult because we heavily rely on GCP infrastructure for our 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.
ok, it is reasonable then. If there is an opportunity to shrink code even further, it will be good.
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.
with this kubernetes/k8s.io#3411 we may need to include
registry.k8s.io
. Not sure if private registry will also be moved there.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.
ack -- I don't think any of the authenticated images use that but we should switch over when we do
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.
since it's for tests only, should it be just
*
?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.
Hmm that would mean we run the auth flow for all images, not just the ones the plugin can handle right?
Btw, the match images are taken from here
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 don't know of any places where we don't use gcr.io for private images yet, but in the future it would be unexpected to run this plugin when the image is from another source
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 was mostly thinking from the perspective of not overcomplicating code as test knows which images will be pulled.
Practically
registry.k8s.io
will be just a proxy to gcr.io when running in GCE. Speculating here... it is interesting, if this proxy will return302
with the gcr.io address, will credential provider be used to pull from that redirected source?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.
Hmm that's a good point -- if registry.k8s.io is just an alias for gcr.io domain we might need to adjust the plugin config to handle it