Skip to content
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

vSphere: add token auth support for tags client #75515

Merged
merged 3 commits into from Mar 26, 2019

Conversation

@dougm
Copy link
Member

commented Mar 20, 2019

What type of PR is this?

/kind bug

What this PR does / why we need it:

Support vSphere SAML token auth when using Zones.

Which issue(s) this PR fixes:

Fixes #75511

Special notes for your reviewer:

SAML auth support for the vCenter rest API endpoint came to govmomi a bit after Zone support came to vSphere Cloud Provider.

Does this PR introduce a user-facing change?:

Support vSphere SAML token auth when using Zones
@dougm

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2019

/sig vmware

@k8s-ci-robot k8s-ci-robot added sig/vmware and removed needs-sig labels Mar 20, 2019

@dougm

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2019

@k8s-ci-robot k8s-ci-robot requested review from deads2k and lavalamp Mar 20, 2019

@dougm dougm changed the title Vsphere token auth vSphere: add token auth support for tags client Mar 20, 2019

@dougm dougm force-pushed the dougm:vsphere-token-auth branch from c3840b4 to 3c44407 Mar 20, 2019

@dougm

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2019

/test pull-kubernetes-e2e-gce-100-performance

@neolit123
Copy link
Member

left a comment

added minor, optional nits.

return err
}

if signer == nil {
klog.V(3).Infof("SessionManager.Login with username '%s'", connection.Username)

This comment has been minimized.

Copy link
@neolit123

neolit123 Mar 21, 2019

Member

optionally ... username: %s

This comment has been minimized.

Copy link
@dougm

dougm Mar 21, 2019

Author Member

Do you mean remove the quotes? It can be useful to quote values from config to see any leading/trailing whitespace. But using %q is probably better for that.

This comment has been minimized.

Copy link
@neolit123

neolit123 Mar 21, 2019

Member

yes, i guess %q is the alternative to : %s, if whitespace is to be tracked.

This comment has been minimized.

Copy link
@dougm

dougm Mar 25, 2019

Author Member

@neolit123 ok changed to %q

return m.Login(ctx, neturl.UserPassword(connection.Username, connection.Password))
}

klog.V(3).Infof("SessionManager.LoginByToken with certificate '%s'", connection.Username)

This comment has been minimized.

Copy link
@neolit123

neolit123 Mar 21, 2019

Member

SessionManager.LoginByToken for username: %s?

@dvonthenen

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

Same for the comment/feedback on https://github.com/kubernetes/kubernetes/pull/75515/files#diff-28cf498e949ce852692c39ebc3d17818R142, does the value of Username contain the cert info? or the username?

If it contains the cert info, it's fine as is otherwise just a change to from certificate->Username. Other than that, this PR is good!

@dougm

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2019

@dvonthenen @neolit123 Username/Password fields can hold cert/key, this existing comment still applies:

// TODO: Add separate fields for certificate and private-key.
// For now we can leave the config structs and validation as-is and
// decide to use LoginByToken if the username value is PEM encoded.

@dvonthenen

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 21, 2019

@dougm dougm force-pushed the dougm:vsphere-token-auth branch from 3c44407 to 83b35a8 Mar 25, 2019

@dougm

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2019

/assign @cblecker
/assign @dims

@cblecker @dims can you review the godeps update?

dougm added some commits Mar 20, 2019

vSphere: add token auth support for tags client
SAML auth support for the vCenter rest API endpoint came to govmomi
a bit after Zone support came to vSphere Cloud Provider.

Fixes #75511

@dougm dougm force-pushed the dougm:vsphere-token-auth branch from 83b35a8 to 85907f6 Mar 25, 2019

@dougm

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2019

/test pull-kubernetes-e2e-gce

@cblecker

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

/approve
for Godeps/

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, dougm

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dvonthenen

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 26, 2019

@k8s-ci-robot k8s-ci-robot merged commit 9c973c6 into kubernetes:master Mar 26, 2019

17 checks passed

cla/linuxfoundation dougm authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

k8s-ci-robot added a commit that referenced this pull request Apr 5, 2019

Merge pull request #75742 from rhockenbury/automated-cherry-pick-of-#…
…75515-upstream-release-1.14

Automated cherry pick of #75515: godeps: update vmware/govmomi to v0.20 release
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.