-
Notifications
You must be signed in to change notification settings - Fork 39.3k
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
Make secret volume plugin use secret manager #40208
Make secret volume plugin use secret manager #40208
Conversation
Jenkins GKE smoke e2e failed for commit b3e7292. Full PR test history. cc @wojtek-t The magic incantation to run this job again is Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Jenkins GCI GKE smoke e2e failed for commit b3e7292. Full PR test history. cc @wojtek-t The magic incantation to run this job again is Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@@ -620,3 +620,7 @@ func (adc *attachDetachController) GetHostIP() (net.IP, error) { | |||
func (adc *attachDetachController) GetNodeAllocatable() (v1.ResourceList, error) { | |||
return v1.ResourceList{}, nil | |||
} | |||
|
|||
func (adc *attachDetachController) GetSecretFunc() func(namespace, name string) (*v1.Secret, error) { | |||
return nil |
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.
returns don't match signature
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.
done
@@ -80,3 +80,7 @@ func (ctrl *PersistentVolumeController) GetHostIP() (net.IP, error) { | |||
func (ctrl *PersistentVolumeController) GetNodeAllocatable() (v1.ResourceList, error) { | |||
return v1.ResourceList{}, nil | |||
} | |||
|
|||
func (adc *PersistentVolumeController) GetSecretFunc() func(namespace, name string) (*v1.Secret, error) { | |||
return nil |
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.
fix returns
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.
why wouldn't this use ctrl.kubeClient.Secrets(namespace).Get(name)?
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.
done
@@ -226,6 +224,11 @@ func getSecretNames(pod *v1.Pod) sets.String { | |||
} | |||
} |
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.
also need to iterate over container EnvFrom
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.
What is using EnvFrom ? From what I found in the code, volume secret plugin is not using them...
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.
#40195 is open to use it
if b.getSecret != nil { | ||
secret, err = b.getSecret(b.pod.Namespace, b.source.SecretName) | ||
} else { | ||
kubeClient := b.plugin.host.GetKubeClient() |
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.
why are we keeping this path?
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.
Agreed. Why can't getSecret
function replace this? I mentioned the same thing in the previous PR (#39558) too...
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 looked into this PR and I didn't see comment about it. But yeah, we can remove it.
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.
done
b3e7292
to
e0784f2
Compare
Volume host is ugly but that's not your fault. We'll go in a clean it out at some point. |
/lgtm |
Automatic merge from submit-queue (batch tested with PRs 40205, 40208) |
@saad-ali - thanks a lot! |
Ref #19188
@gmarek