use oauth REST endpoints instead of raw etcd #15968

deads2k · 2017-08-24T20:20:10Z

@mfojtik this is needed to remove direct etcd access which bypasses any admission checks and auditing and prevents the future separation of processes.

deads2k · 2017-08-24T20:22:25Z

pkg/oauth/apiserver/auth.go

+	return baseMux, nil
+}
+
+func (c *OAuthServerConfig) RegisterTokenRequestEndpoint(context genericapiserver.PostStartHookContext) error {


This bit is nasty. The server won't report healthy until it succeeds though. We need to decide where this should logically live, but I think this is the part where we actually depend on it.

huh... adding a handler to the mux post-startup seems weirder than adding a handler and having it respond with errors until populated with the necessary clients post-startup

huh... adding a handler to the mux post-startup seems weirder than adding a handler and having it respond with errors until populated with the necessary clients post-startup

If you prefer. How about I cut the links in this pull and twiddle it after. This is the big money and rest is arguing over spoils.

liggitt · 2017-08-24T20:30:08Z

pkg/cmd/server/origin/master.go

-	return oauthServer.GenericAPIServer.PrepareRun().GenericAPIServer.Handler.FullHandlerChain, nil
+	return oauthServer.GenericAPIServer.PrepareRun().GenericAPIServer.Handler.FullHandlerChain,
+		map[string]apiserver.PostStartHookFunc{
+			"oauth.openshift.io-RegisterTokenRequestEndpoint": config.RegisterTokenRequestEndpoint,


I wasn't expecting handler registration in post-start hooks... what requires this to be async?

I wasn't expecting handler registration in post-start hooks... what requires this to be async?

The creation of clients needs to be a post-start hook, the handler could give failures until they exist.

liggitt · 2017-08-24T20:44:35Z

pkg/cmd/server/origin/master_config.go

@@ -696,7 +699,7 @@ func newAuthenticator(config configapi.MasterConfig, restOptionsGetter restoptio

 	// OAuth token
 	if config.OAuthConfig != nil {
-		oauthTokenAuthenticator, err := getEtcdTokenAuthenticator(restOptionsGetter, userGetter, groupMapper)
+		oauthTokenAuthenticator, err := getEtcdTokenAuthenticator(accessTokenGetter, userGetter, groupMapper)


should we be able to drop this at this point?

should we be able to drop this at this point?

I wasn't anticipating dropping anything. I'll look back and see.

deads2k · 2017-08-26T00:27:14Z

Comments addressed

deads2k · 2017-08-28T14:13:22Z

/retest

mfojtik · 2017-08-28T14:32:39Z

pkg/auth/server/grant/grant_test.go

+					auth := action.(clienttesting.UpdateAction).GetObject().(*oapi.OAuthClientAuthorization)
+					if !reflect.DeepEqual(testCase.ExpectUpdatedAuthScopes, auth.Scopes) {
+						t.Errorf("%s: expected updated scopes %v, got %v", k, testCase.ExpectUpdatedAuthScopes, auth.Scopes)
+					}


bit: break ?

mfojtik · 2017-08-28T14:32:50Z

pkg/auth/server/grant/grant_test.go

+					auth := action.(clienttesting.CreateAction).GetObject().(*oapi.OAuthClientAuthorization)
+					if !reflect.DeepEqual(testCase.ExpectCreatedAuthScopes, auth.Scopes) {
+						t.Errorf("%s: expected created scopes %v, got %v", k, testCase.ExpectCreatedAuthScopes, auth.Scopes)
+					}


nit: break?

mfojtik · 2017-08-28T14:33:20Z

pkg/cmd/server/origin/master.go

-	return oauthServer.GenericAPIServer.PrepareRun().GenericAPIServer.Handler.FullHandlerChain, nil
+	return oauthServer.GenericAPIServer.PrepareRun().GenericAPIServer.Handler.FullHandlerChain,
+		map[string]apiserver.PostStartHookFunc{
+			"oauth.openshift.io-EnsureBootstrapOAuthClients": config.EnsureBootstrapOAuthClients,


shouldn't this be a const?

shouldn't this be a const?

I don't think so. We aren't really expecting these to be stable or referred to.

mfojtik · 2017-08-28T14:39:52Z

pkg/oauth/apiserver/auth.go

 	osOAuthClientConfig.RedirectUrl = c.Options.MasterPublicURL + path.Join(oauthutil.OpenShiftOAuthAPIPrefix, tokenrequest.DisplayTokenEndpoint)

 	osOAuthClient, _ := osincli.NewClient(osOAuthClientConfig)
 	if len(*c.Options.MasterCA) > 0 {
 		rootCAs, err := cmdutil.CertPoolFromFile(*c.Options.MasterCA)
 		if err != nil {
-			glog.Fatal(err)
+			return nil, err


@deads2k i hope this error is properly captured in upper level :)

@deads2k i hope this error is properly captured in upper level :)

it is

mfojtik · 2017-08-28T14:42:07Z

pkg/oauth/apiserver/oauth_apiserver.go

@@ -185,3 +194,42 @@ func (c *OAuthServerConfig) buildHandlerChainForOAuth(startingHandler http.Handl
 	handler = genericfilters.WithPanicRecovery(handler)
 	return handler
 }
+
+// TODO, this moves to the `apiserver.go` when we have it for this group
+// TODO TODO, this actually looks a lot like a controller or an add-on manager style thing.  Seems like we'd want to do this outside


metatodo? :)

also lacks normal godoc

mfojtik · 2017-08-28T14:42:48Z

pkg/oauth/registry/oauthclientauthorization/strategy.go

@@ -137,3 +137,7 @@ func Matcher(label labels.Selector, field fields.Selector) kstorage.SelectionPre
 func SelectableFields(obj *oauthapi.OAuthClientAuthorization) fields.Set {
 	return oauthapi.OAuthClientAuthorizationToSelectableFields(obj)
 }
+
+func ClientAuthorizationName(userName, clientName string) string {


mfojtik · 2017-08-28T14:44:08Z

test/integration/master_routes_test.go

@@ -99,6 +99,7 @@ var expectedIndex = []string{
 	// "/healthz/poststarthook/extensions/third-party-resources",  // Do not enable this controller, we do not support it
 	"/healthz/poststarthook/generic-apiserver-start-informers",
 	"/healthz/poststarthook/kube-apiserver-autoregistration",
+	"/healthz/poststarthook/oauth.openshift.io-EnsureBootstrapOAuthClients",


why is this camelcase while the rest is lowecase?

why is this camelcase while the rest is lowecase?

we're working through what it should look like.

mfojtik · 2017-08-28T14:44:29Z

test/integration/oauthstorage_test.go

-	clientStorage, err := clientetcd.NewREST(optsGetter)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
+		t.Fatal(err)


now we won't know this was unexpected :-)

mfojtik · 2017-08-28T14:45:14Z

just couple small nits, but the intent and how this was done LGTM

/approve no-issue

openshift-merge-robot · 2017-08-28T14:45:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, mfojtik

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

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

~~pkg/auth/OWNERS~~ [deads2k,mfojtik]
~~pkg/cmd/OWNERS~~ [deads2k,mfojtik]
~~pkg/oauth/OWNERS~~ [deads2k,mfojtik]
~~pkg/serviceaccounts/OWNERS~~ [deads2k,mfojtik]
~~test/integration/OWNERS~~ [deads2k,mfojtik]

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

deads2k · 2017-08-28T15:36:53Z

comments addressed. applying label.

openshift-merge-robot · 2017-08-28T18:42:56Z

Automatic merge from submit-queue

openshift-merge-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 24, 2017

openshift-merge-robot assigned fabianofranz and liggitt Aug 24, 2017

openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-api-review labels Aug 24, 2017

deads2k commented Aug 24, 2017

View reviewed changes

liggitt reviewed Aug 24, 2017

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2017

deads2k force-pushed the oauth-03-use-client branch from 7160376 to 6ad2935 Compare August 25, 2017 17:55

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

deads2k force-pushed the oauth-03-use-client branch 2 times, most recently from 25878d0 to 5583ecb Compare August 25, 2017 19:31

deads2k added the api-approved label Aug 25, 2017

mfojtik added the retest-not-required label Aug 28, 2017

deads2k force-pushed the oauth-03-use-client branch from 5583ecb to bc1998f Compare August 28, 2017 12:15

openshift-merge-robot removed the needs-api-review label Aug 28, 2017

deads2k removed the api-approved label Aug 28, 2017

mfojtik reviewed Aug 28, 2017

View reviewed changes

use oauth client instead of registry

df6eccb

deads2k force-pushed the oauth-03-use-client branch from bc1998f to df6eccb Compare August 28, 2017 15:36

deads2k added the lgtm Indicates that a PR is ready to be merged. label Aug 28, 2017

openshift-merge-robot merged commit 26b5a8c into openshift:master Aug 28, 2017

deads2k deleted the oauth-03-use-client branch January 24, 2018 14:32

use oauth REST endpoints instead of raw etcd #15968

use oauth REST endpoints instead of raw etcd #15968

Uh oh!

Conversation

deads2k commented Aug 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Aug 26, 2017

Uh oh!

deads2k commented Aug 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfojtik commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-merge-robot commented Aug 28, 2017

Uh oh!

deads2k commented Aug 28, 2017

Uh oh!

openshift-merge-robot commented Aug 28, 2017

Uh oh!

Uh oh!

mfojtik commented Aug 28, 2017 •

edited

Loading