plugin/pkg/client/auth: add openstack auth provider #39587

zhouhaibing089 · 2017-01-09T02:01:39Z

This is an implementation of auth provider for OpenStack world, just like python-openstackclient, we read the environment variables of a list OS_*, and client will cache a token to interact with each components, we can do the same here, the client side can cache a token locally at the first time, and rotate automatically when it expires.

This requires an implementation of token authenticator at server side, refer:

[made by me] [WIP] - keystone token authentication #25536, I can carry this on when it is fine to go.
[made by @kfox1111] OpenStack Keystone Token Authentication #25391

The reason why I want to add this is due to the client-side nature, it will be confusing to implement it downstream, we would like to add this support here, and customers can get kubectl like they usually do(brew install kubernetes-cli), and it will just work.

When this is done, we can deprecate the password keystone authenticator as the following reasons:

as mentioned at some other places, the domain is another parameters which should be provided.
in case the user supplies apikey and secrets, we might want to fill the UserInfo with the real name which is not implemented for now.

cc @erictune @liggitt

add openstack auth provider

k8s-ci-robot · 2017-01-09T02:01:40Z

Hi @zhouhaibing089. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should 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 by org members will still work. Regular contributors should join the org to skip this step.

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.

k8s-reviewable · 2017-01-09T02:01:47Z

This change is

anguslees · 2017-01-14T05:29:17Z

@k8s-bot ok to test
/lgtm

k8s-ci-robot · 2017-01-14T05:29:17Z

@anguslees: you can't LGTM a PR unless you are an assignee.

In response to this comment:

@k8s-bot ok to test
/lgtm

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.

anguslees · 2017-01-14T05:33:11Z

@idvoretskyi sig-openstack area tag please

liggitt · 2017-01-14T14:41:34Z

plugin/pkg/client/auth/openstack/openstack.go

+
+// refreshToken token by authenticate with openstack again
+func refreshToken() (string, error) {
+	options, err := openstack.AuthOptionsFromEnv()


Is an openstack token ever obtained from then env directly? cc @kfox1111

@liggitt no it is not, it needs to call keystone with these env vars, and then, a token will be returned.

liggitt · 2017-01-14T14:44:10Z

plugin/pkg/client/auth/openstack/openstack.go

+
+// RoundTrip adds the bearer token into the request, and rotate it automatically
+func (t *tokenRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
+	req.Header.Set("Authorization", "Bearer "+t.token)


It looks like the first time, the token will always be blank. Is that expected?

well, you catch it! it still works though(the auto renew). I will add a call once the round tripper is initialized.

liggitt · 2017-01-14T14:46:56Z

plugin/pkg/client/auth/openstack/openstack.go

+	}
+	t.failed = true
+	// will need to refresh the token, and redo the request
+	if t.token, err = t.getToken(); err != nil {


Is the same auth provider reused if a single CLI instance makes multiple API calls? I think @ericchiang just made some changes to the OIDC auth provider to avoid multiple expensive reconstructions in that case. Would be worth logging the number of times a token is requested and making sure it only happens once in the normal case.

I'm also not seeing where the retrieved token is persisted.

@liggitt each call via cli(kubectl), it will be reconstructed, this is true. a better way to handle this is to cache(truly) the token into the kubeconfig, if that do makes it better.

I think @ericchiang just made some changes to the OIDC auth provider to avoid multiple expensive reconstructions in that case.

To reiterate the auth provider is going to be constantly re-initialized within the span on a single kubeclt call. We remedied that by having a global cache of clients and did a lookup on initialization. If initialization is expensive, you'll need to cache the results.

liggitt · 2017-01-14T14:48:11Z

plugin/pkg/client/auth/openstack/openstack_test.go

+func RandStringBytes(n int) string {
+	b := make([]byte, n)
+	for i := range b {
+		b[i] = LetterBytes[rand.Intn(len(LetterBytes))]


You need your own instance of math.Rand seeded with something actually random

Well, maybe not in a test

liggitt · 2017-01-14T14:50:43Z

cc @kubernetes/sig-openstack-misc

wjun · 2017-01-14T15:20:34Z

As long as any approach cannot keep end users unaware of openstack specific information besides domain name, username, password which other systems also have, we cannot remove the current username/password authn which hide openstack specific information from end users perfectly. k8s users do not need to know openstack's information in some cases.

ericchiang · 2017-01-14T18:38:25Z

plugin/pkg/client/auth/openstack/openstack.go

+	return &tokenRoundTripper{RoundTripper: rt, getToken: refreshToken}
+}
+
+func (c *openstackAuthProvider) Login() error { return nil }


Maybe we should just remove this from the interface? Nothing implements it and there's no way to trigger it.

well, I think it is not tied with this pr, but I am wiling to check and remove it.

ericchiang · 2017-01-14T18:42:05Z

plugin/pkg/client/auth/openstack/openstack.go

+// RoundTrip adds the bearer token into the request, and rotate it automatically
+func (t *tokenRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
+	req.Header.Set("Authorization", "Bearer "+t.token)
+	resp, err := t.RoundTripper.RoundTrip(req)


We removed similar logic from the OIDC plugin. I don't think http.RoundTrippers should be retrying requests. Is there a way to determine if a token is invalid without getting back a StatusUnauthorized?

Is there a way to determine if a token is invalid without getting back a StatusUnauthorized?

As far as I know, there is no easy way:

call keystone token api, but it requires admin role(at least in our openstack deployment).

local validation, the PKI token can be validated locally by: 1) download the keystone certificate, 2) decode the token. but it only handles PKI tokens, also if it is totally local, the revocation will not be aware of.

zhouhaibing089 · 2017-01-15T08:06:52Z

Well, I have no idea on why the Jenkins verification failed, I've already fixed the bazel things as I can see..

zhouhaibing089 · 2017-08-03T07:46:20Z

@sttts I think @liggitt and @ericchiang and folks in sig-openstack already give it a review.

sttts · 2017-08-03T07:52:58Z

Alright.

/lgtm
/approve no-issue

k8s-ci-robot · 2017-08-03T09:22:37Z

@zhouhaibing089: you can't request testing unless you are a kubernetes member.

In response to this:

/retest

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.

sttts · 2017-08-03T09:26:45Z

/retest

zhouhaibing089 · 2017-08-04T02:47:36Z

updated BUILD file.

dims · 2017-08-06T02:09:18Z

/ok-to-test

dims · 2017-08-06T02:10:14Z

/lgtm

k8s-github-robot · 2017-08-06T02:10:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anguslees, dims, sttts, zhouhaibing089

Associated issue requirement bypassed by: sttts

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~staging/src/k8s.io/client-go/OWNERS~~ [sttts]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

zhouhaibing089 · 2017-08-06T04:25:03Z

just run ./hack/update-staging-godeps.sh..

dims · 2017-08-06T23:43:36Z

/retest

sttts · 2017-08-07T07:18:52Z

/retest

fejta-bot · 2017-08-07T10:22:29Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-08-07T14:55:27Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-github-robot · 2017-08-07T17:58:56Z

Automatic merge from submit-queue (batch tested with PRs 50087, 39587, 50042, 50241, 49914)

@kfox1111

…h-provider Automatic merge from submit-queue (batch tested with PRs 50087, 39587, 50042, 50241, 49914) plugin/pkg/client/auth: add openstack auth provider This is an implementation of auth provider for OpenStack world, just like python-openstackclient, we read the environment variables of a list `OS_*`, and client will cache a token to interact with each components, we can do the same here, the client side can cache a token locally at the first time, and rotate automatically when it expires. This requires an implementation of token authenticator at server side, refer: 1. [made by me] kubernetes#25536, I can carry this on when it is fine to go. 2. [made by @kfox1111] kubernetes#25391 The reason why I want to add this is due to the `client-side` nature, it will be confusing to implement it downstream, we would like to add this support here, and customers can get `kubectl` like they usually do(`brew install kubernetes-cli`), and it will just work. When this is done, we can deprecate the password keystone authenticator as the following reasons: 1. as mentioned at some other places, the `domain` is another parameters which should be provided. 2. in case the user supplies `apikey` and `secrets`, we might want to fill the `UserInfo` with the real name which is not implemented for now. cc @erictune @liggitt ``` add openstack auth provider ```

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 9, 2017

k8s-github-robot assigned davidopp Jan 9, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jan 9, 2017

zhouhaibing089 force-pushed the openstack-auth-provider branch 2 times, most recently from 29e35aa to 2b0b1fc Compare January 9, 2017 02:51

zhouhaibing089 mentioned this pull request Jan 11, 2017

OpenStack Keystone Token Authentication #25391

Closed

liggitt added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Jan 13, 2017

liggitt mentioned this pull request Jan 13, 2017

Add authentication on keystone v3 #37669

Closed

liggitt assigned liggitt and unassigned davidopp Jan 14, 2017

liggitt reviewed Jan 14, 2017

View reviewed changes

liggitt added the area/provider/openstack Issues or PRs related to openstack provider label Jan 14, 2017

ericchiang reviewed Jan 14, 2017

View reviewed changes

zhouhaibing089 force-pushed the openstack-auth-provider branch 4 times, most recently from 0c95f4f to fcb5b2b Compare January 15, 2017 07:36

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 3, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2017

zhouhaibing089 force-pushed the openstack-auth-provider branch from ea2acaa to 5a4f638 Compare August 4, 2017 02:46

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 4, 2017

zhouhaibing089 force-pushed the openstack-auth-provider branch from 5a4f638 to f1e98c9 Compare August 5, 2017 14:38

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2017

k8s-ci-robot assigned dims Aug 6, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 6, 2017

plugin/pkg/client/auth: add openstack auth provider

a0cebcb

zhouhaibing089 force-pushed the openstack-auth-provider branch from f1e98c9 to a0cebcb Compare August 6, 2017 04:24

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 6, 2017

sttts added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2017

k8s-github-robot merged commit 59b8fa3 into kubernetes:master Aug 7, 2017

liggitt mentioned this pull request Feb 8, 2018

Remove experimental keystone authenticator #59492

Merged

plugin/pkg/client/auth: add openstack auth provider #39587

plugin/pkg/client/auth: add openstack auth provider #39587

Conversation

zhouhaibing089 commented Jan 9, 2017 • edited

k8s-ci-robot commented Jan 9, 2017

k8s-reviewable commented Jan 9, 2017

anguslees commented Jan 14, 2017

k8s-ci-robot commented Jan 14, 2017

anguslees commented Jan 14, 2017

liggitt Jan 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Jan 14, 2017

wjun commented Jan 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouhaibing089 commented Jan 15, 2017

zhouhaibing089 commented Aug 3, 2017

sttts commented Aug 3, 2017

k8s-ci-robot commented Aug 3, 2017

sttts commented Aug 3, 2017

zhouhaibing089 commented Aug 4, 2017

dims commented Aug 6, 2017

dims commented Aug 6, 2017

k8s-github-robot commented Aug 6, 2017

zhouhaibing089 commented Aug 6, 2017

dims commented Aug 6, 2017

sttts commented Aug 7, 2017

fejta-bot commented Aug 7, 2017

fejta-bot commented Aug 7, 2017

k8s-github-robot commented Aug 7, 2017

zhouhaibing089 commented Jan 9, 2017 •

edited

liggitt Jan 14, 2017 •

edited