-
Notifications
You must be signed in to change notification settings - Fork 40k
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
Return error if get NodeStageSecret and NodePublishSecret failed #61096
Conversation
/assign @vladimirvivien |
/assign @jsafrane |
/ok-to-test |
/approve |
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.
Some nits, otherwise LGTM
pkg/volume/csi/csi_util.go
Outdated
@@ -23,16 +23,16 @@ import ( | |||
"k8s.io/client-go/kubernetes" | |||
) | |||
|
|||
func getCredentialsFromSecret(k8s kubernetes.Interface, secretRef *api.SecretReference) map[string]string { | |||
func getCredentialsFromSecret(k8s kubernetes.Interface, secretRef *api.SecretReference) (map[string]string, error) { | |||
credentials := map[string]string{} | |||
secret, err := k8s.CoreV1().Secrets(secretRef.Namespace).Get(secretRef.Name, meta.GetOptions{}) | |||
if err != nil { | |||
glog.Warningf("failed to find the secret %s in the namespace %s with error: %v\n", secretRef.Name, secretRef.Namespace, err) |
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.
Change Warningf
to Errorf
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.
Fixed.
pkg/volume/csi/csi_attacher.go
Outdated
if csiSource.NodeStageSecretRef != nil { | ||
nodeStageSecrets, err = getCredentialsFromSecret(c.k8s, csiSource.NodeStageSecretRef) | ||
if err != nil { | ||
return fmt.Errorf("get secret %s/%s failed: %v", |
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.
nit: "fetching NodeStageSecretRef failed for secret %s/%s failed: %v
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.
Thanks for reminding, already fixed.
@@ -153,6 +153,15 @@ func (c *csiMountMgr) SetUpAt(dir string, fsGroup *int64) error { | |||
|
|||
attribs := csiSource.VolumeAttributes | |||
|
|||
nodePublishSecrets := map[string]string{} | |||
if csiSource.NodePublishSecretRef != nil { | |||
nodePublishSecrets, err = getCredentialsFromSecret(c.k8s, csiSource.NodePublishSecretRef) |
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.
nit: "fetching NodePublishSecretRef failed for secret %s/%s failed: %v
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.
Same as above.
/uncc |
Thanks!! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jsafrane, mlmhl, saad-ali The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test all Tests are more than 96 hours old. Re-running tests. |
Please cherry pick back to 1.10 branch once this is merged. |
Automatic merge from submit-queue (batch tested with PRs 61096, 61955, 61542, 60597). If you want to cherry-pick this change to another branch, please follow the instructions here. |
@mlmhl Please cherry pick back to 1.10 branch. |
@saad-ali Thanks for reminding, the cherry pick PR has been submitted. |
…origin-release-1.10 Automatic merge from submit-queue. Automated cherry pick of #61096: return error if get NodeStageSecret and NodePublishSecret failed Cherry pick of #61096 on release-1.10. #61096: Return error if get NodeStageSecret and NodePublishSecret failed **Release note**: ```release-note Return error if get NodeStageSecret and NodePublishSecret failed in CSI volume plugin ```
What this PR does / why we need it:
Currently, if got NodeStageSecret or NodePublishSecret failed, we just log the error and assume that there is no credential. I think we should report the error as if user specified these secret, they expect to apply some credentials.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #61052
Release note:
/sig storage