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

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

Merged
merged 1 commit into from Aug 7, 2017

Conversation

@zhouhaibing089
Contributor

zhouhaibing089 commented Jan 9, 2017

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] #25536, I can carry this on when it is fine to go.
  2. [made by @kfox1111] #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

This comment has been minimized.

Contributor

k8s-ci-robot commented Jan 9, 2017

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

This comment has been minimized.

k8s-reviewable commented Jan 9, 2017

This change is Reviewable

@anguslees

This comment has been minimized.

Member

anguslees commented Jan 14, 2017

@k8s-bot ok to test
/lgtm

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Jan 14, 2017

@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

This comment has been minimized.

Member

anguslees commented Jan 14, 2017

@idvoretskyi sig-openstack area tag please

@liggitt liggitt assigned liggitt and unassigned davidopp Jan 14, 2017

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

This comment has been minimized.

@liggitt

liggitt Jan 14, 2017

Member

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

This comment has been minimized.

@zhouhaibing089

zhouhaibing089 Jan 14, 2017

Contributor

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

// 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)

This comment has been minimized.

@liggitt

liggitt Jan 14, 2017

Member

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

This comment has been minimized.

@zhouhaibing089

zhouhaibing089 Jan 14, 2017

Contributor

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

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

This comment has been minimized.

@liggitt

liggitt Jan 14, 2017

Member

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.

This comment has been minimized.

@liggitt

liggitt Jan 14, 2017

Member

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

This comment has been minimized.

@zhouhaibing089

zhouhaibing089 Jan 14, 2017

Contributor

@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.

This comment has been minimized.

@ericchiang

ericchiang Jan 14, 2017

Member

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.

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

This comment has been minimized.

@liggitt

liggitt Jan 14, 2017

Member

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

This comment has been minimized.

@liggitt

liggitt Jan 14, 2017

Member

Well, maybe not in a test

@liggitt

This comment has been minimized.

Member

liggitt commented Jan 14, 2017

@wjun

This comment has been minimized.

wjun commented Jan 14, 2017

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.

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

This comment has been minimized.

@ericchiang

ericchiang Jan 14, 2017

Member

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

This comment has been minimized.

@zhouhaibing089

zhouhaibing089 Jan 15, 2017

Contributor

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

// 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)

This comment has been minimized.

@ericchiang

ericchiang Jan 14, 2017

Member

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?

This comment has been minimized.

@zhouhaibing089

zhouhaibing089 Jan 15, 2017

Contributor

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:

  1. call keystone token api, but it requires admin role(at least in our openstack deployment).
  2. 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

This comment has been minimized.

Contributor

zhouhaibing089 commented Jan 15, 2017

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

@zhouhaibing089

This comment has been minimized.

Contributor

zhouhaibing089 commented Aug 3, 2017

@sttts @liggitt is this PR in good shape now and can we get this in?

@sttts

This comment has been minimized.

Contributor

sttts commented Aug 3, 2017

The plumbing looks fine. Do you have anybody reviewing the actual plugin logic in staging/src/k8s.io/client-go/plugin/pkg/client/auth/openstack/openstack.go ? @liggitt ?

@zhouhaibing089

This comment has been minimized.

Contributor

zhouhaibing089 commented Aug 3, 2017

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

@sttts

This comment has been minimized.

Contributor

sttts commented Aug 3, 2017

Alright.

/lgtm
/approve no-issue

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Aug 3, 2017

@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

This comment has been minimized.

Contributor

sttts commented Aug 3, 2017

/retest

@zhouhaibing089

This comment has been minimized.

Contributor

zhouhaibing089 commented Aug 4, 2017

updated BUILD file.

@dims

This comment has been minimized.

Member

dims commented Aug 6, 2017

/ok-to-test

@dims

This comment has been minimized.

Member

dims commented Aug 6, 2017

/lgtm

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Aug 6, 2017

[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:

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

@k8s-merge-robot k8s-merge-robot removed the lgtm label Aug 6, 2017

@zhouhaibing089

This comment has been minimized.

Contributor

zhouhaibing089 commented Aug 6, 2017

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

@dims

This comment has been minimized.

Member

dims commented Aug 6, 2017

/retest

1 similar comment
@sttts

This comment has been minimized.

Contributor

sttts commented Aug 7, 2017

/retest

@sttts sttts added the lgtm label Aug 7, 2017

@fejta-bot

This comment has been minimized.

fejta-bot commented Aug 7, 2017

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

Review the full test history for this PR.

1 similar comment
@fejta-bot

This comment has been minimized.

fejta-bot commented Aug 7, 2017

/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-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Aug 7, 2017

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

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

10 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation zhouhaibing089 authorized
Details
pull-kubernetes-bazel Job succeeded.
Details
pull-kubernetes-e2e-gce-etcd3 Jenkins job succeeded.
Details
pull-kubernetes-e2e-kops-aws Jenkins job succeeded.
Details
pull-kubernetes-federation-e2e-gce Jenkins job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Jenkins job succeeded.
Details
pull-kubernetes-node-e2e Jenkins job succeeded.
Details
pull-kubernetes-unit Jenkins job succeeded.
Details
pull-kubernetes-verify Jenkins job succeeded.
Details

dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018

Merge pull request kubernetes#39587 from zhouhaibing089/openstack-aut…
…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
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment