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 Cloud Provider: add SAML token authentication support #63824

Merged
merged 3 commits into from May 17, 2018

Conversation

@dougm
Copy link
Member

dougm commented May 14, 2018

What this PR does / why we need it:

The vSphere cloud provider currently supports username+password based authentication, this PR adds an option to use token based authentication.

Which issue(s) this PR fixes:

Fixes #63209

Special notes for your reviewer:

For now the config structs and validation are left as-is and
the LoginByToken method is used if the username value is PEM encoded.
In this case of username field configured with the public key, the password
field is expected to be configured with the private key.

In a follow-up PR we can look at collapsing the auth related fields into
a common struct to avoid duplication of field merging and validation.
And then add separate fields for the public and private keys.

Release note:

vSphere Cloud Provider: add SAML token authentication support
@dougm

This comment has been minimized.

Copy link
Member Author

dougm commented May 14, 2018

/sig vmware

/assign @abrarshivani @rohitjogvmw

@dougm dougm force-pushed the dougm:vsphere-token-auth branch from 2517c81 to c0e87f5 May 14, 2018

@anfernee

This comment has been minimized.

Copy link
Member

anfernee commented May 15, 2018

/ok-to-test

dougm added some commits May 14, 2018

vSphere Cloud Provider: add SAML token authentication support
For now the config structs and validation are left as-is and
the LoginByToken method is used if the username value is PEM encoded.
In this case of username field configured with the public key, the password
field is expected to be configured with the private key.

In a follow-up PR we can look at collapsing the auth related fields into
a common struct to avoid duplication of field merging and validation.
And then add separate fields for the public and private keys.

Fixes #63209

@dougm dougm force-pushed the dougm:vsphere-token-auth branch from c0e87f5 to 0791fca May 15, 2018

@dougm

This comment has been minimized.

Copy link
Member Author

dougm commented May 15, 2018

/test pull-kubernetes-integration

@k8s-ci-robot k8s-ci-robot added the lgtm label May 17, 2018

@abrarshivani
Copy link
Member

abrarshivani left a comment

/lgtm

func (connection *VSphereConnection) login(ctx context.Context, client *vim25.Client) error {
m := session.NewManager(client)

// TODO: Add separate fields for certificate and private-key.

This comment has been minimized.

@abrarshivani

abrarshivani May 17, 2018

Member

Is this issue tracked somewhere?

This comment has been minimized.

@dougm

dougm May 17, 2018

Author Member

Not yet, I'll create a new issue.

@dougm

This comment has been minimized.

Copy link
Member Author

dougm commented May 17, 2018

/assign @cblecker @thockin @sttts

@ dep-approvers can you review the Godeps updates?

@cblecker

This comment has been minimized.

Copy link
Member

cblecker commented May 17, 2018

There's some new imports, but they're all under the same repo/dep, and all Apache 2.0 licensed.

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 17, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abrarshivani, 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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 17, 2018

Automatic merge from submit-queue (batch tested with PRs 63886, 63857, 63824). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit a481f4b into kubernetes:master May 17, 2018

18 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation dougm authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
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-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented Jul 6, 2018

This looks like it has data races:

=== RUN   TestVSphereLoginByToken
E0705 17:13:06.921738    9479 connection.go:134] Logout failed: Post https://127.0.0.1:36902/sdk: dial tcp 127.0.0.1:36902: connect: connection refused
==================
WARNING: DATA RACE
Write at 0x00c4200fbbc0 by goroutine 48:
  github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/cloudprovider/providers/vsphere/vclib.(*VSphereConnection).Connect()
      /go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/cloudprovider/providers/vsphere/vclib/connection.go:59 +0x3ff
  github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/cloudprovider/providers/vsphere.TestVSphereLoginByToken()
      /go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/cloudprovider/providers/vsphere/vsphere_test.go:246 +0x335
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:777 +0x16d

Previous read at 0x00c4200fbbc0 by goroutine 5:
  github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/cloudprovider/providers/vsphere.logout()
      /go/src/github.com/openshift/origin/vendor/k8s.io/kubernetes/pkg/cloudprovider/providers/vsphere/vsphere.go:496 +0x13b

Goroutine 48 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:824 +0x564
  testing.runTests.func1()
      /usr/local/go/src/testing/testing.go:1063 +0xa4
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:777 +0x16d
  testing.runTests()
      /usr/local/go/src/testing/testing.go:1061 +0x4e1
  testing.(*M).Run()
      /usr/local/go/src/testing/testing.go:978 +0x2cd
  main.main()
      _testmain.go:112 +0x324

Goroutine 5 (running) created at:
  runtime.createfing()
      /usr/local/go/src/runtime/mfinal.go:156 +0x61
  os.NewFile()
      /usr/local/go/src/os/file_unix.go:79 +0x55
  os.init()
      /usr/local/go/src/os/file.go:58 +0x418
  main.init()
      <autogenerated>:1 +0x96
==================
--- FAIL: TestVSphereLoginByToken (0.31s)
	testing.go:730: race detected during execution of test
@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented Jul 6, 2018

It looks like the SetFinalizer(logout) is what is causing the race - finalizers are called from other threads, so you need synchronization on the connection object in order to prevent this race.

smarterclayton added a commit to smarterclayton/origin that referenced this pull request Jul 6, 2018

UPSTREAM: <drop>: vSphere test has race conditions, disable
Using `SetFinalizer(logout)` in vsphere code is not safe (since the
connection object has no mutex and finalizers run in other threads).

Disabling the test until resolution of kubernetes/kubernetes#63824 (comment)

openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Jul 6, 2018

UPSTREAM: <drop>: vSphere test has race conditions, disable
Using `SetFinalizer(logout)` in vsphere code is not safe (since the
connection object has no mutex and finalizers run in other threads).

Disabling the test until resolution of kubernetes#63824 (comment)

Origin-commit: 65640428ea2f23e5d2f6bf9d7190801bbc7ffb2e
@dougm

This comment has been minimized.

Copy link
Member Author

dougm commented Jul 6, 2018

@smarterclayton PR with fix #65913

liggitt added a commit to liggitt/kubernetes that referenced this pull request Jul 18, 2018

UPSTREAM: <drop>: vSphere test has race conditions, disable
Using `SetFinalizer(logout)` in vsphere code is not safe (since the
connection object has no mutex and finalizers run in other threads).

Disabling the test until resolution of kubernetes#63824 (comment)

Origin-commit: 65640428ea2f23e5d2f6bf9d7190801bbc7ffb2e

liggitt added a commit to liggitt/kubernetes that referenced this pull request Jul 18, 2018

UPSTREAM: <drop>: vSphere test has race conditions, disable
Using `SetFinalizer(logout)` in vsphere code is not safe (since the
connection object has no mutex and finalizers run in other threads).

Disabling the test until resolution of kubernetes#63824 (comment)

Origin-commit: 65640428ea2f23e5d2f6bf9d7190801bbc7ffb2e

liggitt added a commit to liggitt/kubernetes that referenced this pull request Jul 18, 2018

UPSTREAM: <drop>: vSphere test has race conditions, disable
Using `SetFinalizer(logout)` in vsphere code is not safe (since the
connection object has no mutex and finalizers run in other threads).

Disabling the test until resolution of kubernetes#63824 (comment)

Origin-commit: 65640428ea2f23e5d2f6bf9d7190801bbc7ffb2e

liggitt added a commit to openshift/kubernetes that referenced this pull request Jul 23, 2018

UPSTREAM: <drop>: vSphere test has race conditions, disable
Using `SetFinalizer(logout)` in vsphere code is not safe (since the
connection object has no mutex and finalizers run in other threads).

Disabling the test until resolution of kubernetes#63824 (comment)

Origin-commit: 65640428ea2f23e5d2f6bf9d7190801bbc7ffb2e
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.