Bad conditional in vSphereLogin function #36169
Conversation
With this conditional being == instead of !=, a login would never actually be attempted by this provider, and disk attachments would fail.
|
Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot ok to test" on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands will still work. Regular contributors should join the org to skip this step. If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository. |
|
I'm pretty sure HPE has a signed CLA with the Linux Foundation? |
k8s-github-robot
added
size/XS
|
@k8s-bot ok to test |
|
There, Linux Foundation CLA signed |
k8s-github-robot
added
the
do-not-merge
apelisse
removed
the
do-not-merge
|
What else is needed to get this PR through? I'm not sure how anyone is using Kubernetes on vSphere right now :) |
|
@k8s-bot approvers |
| @@ -360,7 +360,7 @@ func vSphereLogin(vs *VSphere, ctx context.Context) error { | |||
| m := session.NewManager(vs.client.Client) | |||
| // retrieve client's current session | |||
| u, err := m.UserSession(ctx) | |||
| if err == nil && u == nil { | |||
| if err == nil && u != nil { | |||
There was a problem hiding this comment.
@robdaemon Thanks for doing this. This will create user session even if it fails to obtain the existing one. So, I would suggest you to do like following,
. . .
if err != nil {
glog.Errorf("Error while obtaining user session. err: %q", err)
return err
}
if u != nil {
return nil
}
. . .
This will handle the error. You can check conditions here.
There was a problem hiding this comment.
Okay, I've made the changes you request here.
Check for error conditions from the vSphere API and return the err if one occurs. The vSphere API does not return an err for unauthenticated users, it just returns a nil user object.
|
LGTM |
k8s-github-robot
added
release-note
|
@k8s-bot ok to test |
|
@mikedanese Can this be merged in once the tests pass? |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue |
With this conditional being == instead of !=, a login would never actually be attempted by this provider, and disk attachments would fail with a NotAuthenticated error from vSphere.
This change is