Add events to SA as OAuth client flow by enj · Pull Request #13621 · openshift/origin

enj · 2017-04-04T16:10:35Z

Add warning events for service account OAuth configuration errors to aid in troubleshooting. Display these events when describing service accounts.

Name:           proxy                                                 
Namespace:      myproject                                                      
Labels:         <none>                                                         
Annotations:    serviceaccounts.openshift.io/oauth-bogus-annotation.primary={"kind":"OAuthRedirectReference","apiVersion":"v1","reference":{"kind":"Route","name":"
proxy"}}                                                                                                                                                      
                                                                               
Image pull secrets:     proxy-dockercfg-5wzxb                  
                                                                                                                                                              
Mountable secrets:      proxy-token-p8fd7                                                                   
                        proxy-dockercfg-5wzxb                                                                    
                                                                                                                              
Tokens:                 proxy-token-frfsj                                                                                
                        proxy-token-p8fd7                                                              
                                                                                                          
Events:                                                                                                   
  FirstSeen     LastSeen        Count   From                                    SubObjectPath   Type            Reason                  Message               
  ---------     --------        -----   ----                                    -------------   --------        ------                  -------
  10s           10s             1       service-account-oauth-client-getter                     Warning         NoSAOAuthRedirectURIs   system:serviceaccount:
myproject:proxy has no redirectURIs; set serviceaccounts.openshift.io/oauth-redirecturi.<some-value>=<redirect> or create a dynamic URI using serviceaccounts.
openshift.io/oauth-redirectreference.<some-value>=<reference>

https://trello.com/c/oRkv75dy/15-5-add-api-events-for-using-sa-as-oauth-clients

openshift-bot · 2017-04-04T18:46:09Z

Evaluated for origin test up to c8c6237

openshift-bot · 2017-04-04T19:56:42Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/560/) (Base Commit: f0670df)

smarterclayton · 2017-04-12T05:14:00Z

Is this using events as validation?

smarterclayton · 2017-04-12T05:15:34Z

pkg/serviceaccounts/oauthclient/oauthclientregistry.go

+
+	// TODO: is this safe to tell - could leak Route info?
+	a.recorder.Eventf(sa, kapi.EventTypeNormal, "OAuthAllRedirectURIs", "Has the following redirectURIs: %v", saClient.RedirectURIs)
+


You're sending one event per oauth login to the master? That's... probably going to eat into our write budget.

enj · 2017-04-12T20:57:52Z

@smarterclayton

Is this using events as validation?

Sort of? We cannot do validation on SA annotations. This lets the user know if something looks off.

You're sending one event per oauth login to the master? That's... probably going to eat into our write budget.

Based on some offline discussions with Jordan, I plan to change this to send a single warning event when something looks incorrect. That event would contain a sorted list of problems noticed. Thus if the user goes through the same error flow multiple times, the same event should just have its counter incremented. No events will be generated when everything is working as expected.

openshift-bot · 2017-07-06T13:31:14Z

Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again

deads2k · 2017-07-31T12:59:57Z

/assign liggitt

Thus if the user goes through the same error flow multiple times, the same event should just have its counter incremented. No events will be generated when everything is working as expected.

incrementing a counter is still an etcd write.

smarterclayton · 2017-07-31T13:47:42Z

We need some mechanism for sure. But it has to be no more than O(0) writes measured over 5s-1m. On Jul 31, 2017, at 9:00 AM, David Eads <notifications@github.com> wrote: /assign liggitt Thus if the user goes through the same error flow multiple times, the same event should just have its counter incremented. No events will be generated when everything is working as expected. incrementing a counter is still an etcd write. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#13621 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_px3GUYLyKmkPvL64jKpx_Dftm3O7ks5sTc_RgaJpZM4MzFl0> .

spadgett · 2017-08-18T18:16:59Z

@benjaminapetersen Might be best to stop trying to humanize these and display the reason as-is

enj · 2017-08-24T17:36:49Z

@liggitt what do you expect to happen when non-fatal errors are encountered in a GetClient flow? I am mostly envisioning a scenario where a user has annotation for redirect uris X and Y, but only Y is "broken." Thus GetClient will succeed and the user could encounter an OAuth error if Y is used. I feel like we have forcibly limited the events from spamming enough that it should be OK to always create an event for any errors encountered (and rely on the event rate limiting to prevent us from ever doing more than one event per minute).

mrogers950 · 2017-09-19T19:25:00Z

flake #16144

mrogers950 · 2017-09-19T20:54:14Z

/retest

mrogers950 · 2017-09-20T15:31:02Z

I've rebased and excluded the event limit configuration patch (in lieu of further developments upstream) - should be all set here. @enj @smarterclayton PTAL

enj · 2017-09-25T19:55:27Z

pkg/oauth/apiserver/apiserver.go

+	combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(
+		coreClient,
+		coreClient,
+		// TODO: fix this construction


I would expect coreClient.Events("") or if that does not work then:

coreV1Client, err := corev1.NewForConfig(c.CoreAPIServerClientConfig) + coreV1Client.Events("")

enj · 2017-09-25T19:55:35Z

pkg/oauth/apiserver/auth.go

+	combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(
+		c.KubeClient.Core(),
+		c.KubeClient.Core(),
+		// TODO: simplify this construction


Same comments.

enj · 2017-09-25T20:01:00Z

pkg/cmd/server/origin/oauth_apiserver_adapter.go

 	// TODO pass a privileged client config through during construction.  It is NOT a loopback client.
 	oauthServerConfig.RouteClient = routeClient
 	oauthServerConfig.KubeClient = masterConfig.PrivilegedLoopbackKubernetesClientsetInternal
+	oauthServerConfig.KubeExternalClient = masterConfig.PrivilegedLoopbackKubernetesClientsetExternal


You may not need this if you can reuse coreClient

See https://github.com/openshift/origin/pull/13621/files#r141110811

enj · 2017-09-25T20:02:27Z

pkg/oauth/apiserver/apiserver.go


-	combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(coreClient, coreClient, routeClient, oauthClient.OAuthClients(), saAccountGrantMethod)
+	combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(
+		coreClient,


Call the sub methods on this to get the specific interfaces you need.

This did not seem possible with coreClient, are you suggesting a change in NewServiceAccountOAuthClientGetter() argument types as well?

enj · 2017-09-25T20:03:58Z

pkg/oauth/apiserver/oauth_apiserver.go

 	KubeClient kclientset.Interface

+	// KubeExternalClient is for creating user events
+	KubeExternalClient kclientsetexternal.Interface


Lets make sure we really need this per above comments.

enj · 2017-09-25T20:32:40Z

pkg/serviceaccounts/oauthclient/oauthclientregistry.go

+	// Create a warning event combining the collected annotation errors upon failure.
+	defer func() {
+		if err != nil && len(saErrors) > 0 && len(failReason) > 0 {
+			a.eventRecorder.Eventf(sa, kapi.EventTypeWarning, failReason, "%s", joinErrors(saErrors))


Use Event since you do not need the format.

enj · 2017-09-25T21:18:37Z

pkg/serviceaccounts/oauthclient/oauthclientregistry_test.go

-	}

+		if len(tc.expectedEventMsg) > 0 {
+			ev := <-fakerecorder.Events


I think you want a select statement with a default to prevent this from ever dead locking.

enj · 2017-09-25T21:19:30Z

pkg/serviceaccounts/oauthclient/oauthclientregistry_test.go

 	} {
-		if !reflect.DeepEqual(test.expected, parseModelsMap(test.annotations, decoder)) {
-			t.Errorf("%s: expected %#v, got %#v", test.name, test.expected, parseModelsMap(test.annotations, decoder))
+		models, _ := parseModelsMap(test.annotations, decoder)


Don't ignore the errors.

enj · 2017-09-25T21:20:09Z

pkg/serviceaccounts/oauthclient/oauthclientregistry_test.go

 	} {
 		a := buildRouteClient(test.routes)
-		actual := test.models.getRedirectURIs(a.redirectURIsFromRoutes(test.namespace, test.models.getNames()))
+		uris, _ := a.redirectURIsFromRoutes(test.namespace, test.models.getNames())


Don't ignore the errors.

enj · 2017-09-25T21:20:18Z

pkg/serviceaccounts/oauthclient/oauthclientregistry_test.go

 		a := buildRouteClient(test.routes)
-		if !reflect.DeepEqual(test.expected, a.redirectURIsFromRoutes(test.namespace, test.names)) {
-			t.Errorf("%s: expected %#v, got %#v", test.name, test.expected, a.redirectURIsFromRoutes(test.namespace, test.names))
+		uris, _ := a.redirectURIsFromRoutes(test.namespace, test.names)


Don't ignore the errors.

enj · 2017-09-26T16:14:42Z

pkg/oauth/apiserver/auth.go

+		c.KubeClient.Core(),
+		c.KubeClient.Core(),
+		// TODO: simplify this construction
+		corev1.New(c.KubeExternalClient.Core().RESTClient()).Events(""),


enj · 2017-09-26T16:30:24Z

pkg/oauth/apiserver/oauth_apiserver.go


+	// KubeExternalClient is for creating user events
+	KubeExternalClient kclientsetexternal.Interface
+


Add an events client directly in this config and build it in NewOAuthServerConfig

Collect errors during validation of a service account's OAuth configuration, logging them to a warning event upon a fatal error. Signed-off-by: Matt Rogers <mrogers@redhat.com>

Signed-off-by: Matt Rogers <mrogers@redhat.com>

mrogers950 · 2017-09-26T18:49:45Z

/lgtm

enj · 2017-09-26T18:50:58Z

/approve

I am happy with this (it's not my PR anymore).

openshift-merge-robot · 2017-09-26T18:51:01Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: enj, mrogers950
We suggest the following additional approver: deads2k

Assign the PR to them by writing /assign @deads2k in a comment when ready.

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

Details

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

~~pkg/oauth/OWNERS~~ [enj]
~~pkg/serviceaccounts/OWNERS~~ [enj]
vendor/k8s.io/kubernetes/pkg/printers/OWNERS

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

enj · 2017-09-26T21:25:41Z

/retest

enj · 2017-09-26T21:28:30Z

xref #16568

Still need issue for the core client events problem.

openshift-merge-robot · 2017-09-26T22:33:31Z

Automatic merge from submit-queue

smarterclayton reviewed Apr 12, 2017

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2017

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 19, 2017

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2017

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 13, 2017

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 28, 2017

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 6, 2017

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 6, 2017

openshift-merge-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 24, 2017

openshift-merge-robot assigned smarterclayton and deads2k Jul 25, 2017

openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 28, 2017

openshift-ci-robot assigned liggitt Jul 31, 2017

mrogers950 force-pushed the enj/f/sa_oauth_events branch from c8c6237 to 659945d Compare August 1, 2017 15:46

openshift-merge-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 1, 2017

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

mrogers950 force-pushed the enj/f/sa_oauth_events branch from 6f7a3fe to c159076 Compare September 12, 2017 13:56

mrogers950 force-pushed the enj/f/sa_oauth_events branch from c159076 to 24fc801 Compare September 19, 2017 15:31

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

mrogers950 force-pushed the enj/f/sa_oauth_events branch from 24fc801 to f65d877 Compare September 19, 2017 18:04

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

mrogers950 force-pushed the enj/f/sa_oauth_events branch from f65d877 to 88f5dd7 Compare September 25, 2017 19:18

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

enj commented Sep 25, 2017

View reviewed changes

mrogers950 force-pushed the enj/f/sa_oauth_events branch from 88f5dd7 to 1ee9812 Compare September 26, 2017 15:35

enj commented Sep 26, 2017

View reviewed changes

Matt Rogers added 2 commits September 26, 2017 14:24

Add API events for SA OAuth failures

25b1ed1

Collect errors during validation of a service account's OAuth configuration, logging them to a warning event upon a fatal error. Signed-off-by: Matt Rogers <mrogers@redhat.com>

UPSTREAM: 51035: Show events when describing service accounts

dfe104f

Signed-off-by: Matt Rogers <mrogers@redhat.com>

mrogers950 force-pushed the enj/f/sa_oauth_events branch from 1ee9812 to dfe104f Compare September 26, 2017 18:24

openshift-ci-robot assigned mrogers950 Sep 26, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 26, 2017

enj added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 26, 2017

openshift-merge-robot merged commit fa1def6 into openshift:master Sep 26, 2017


		// TODO: is this safe to tell - could leak Route info?
		a.recorder.Eventf(sa, kapi.EventTypeNormal, "OAuthAllRedirectURIs", "Has the following redirectURIs: %v", saClient.RedirectURIs)


		// KubeExternalClient is for creating user events
		KubeExternalClient kclientsetexternal.Interface

Conversation

enj commented Apr 4, 2017 • edited by mrogers950 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-bot commented Apr 4, 2017

Uh oh!

openshift-bot commented Apr 4, 2017

Uh oh!

smarterclayton commented Apr 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enj commented Apr 12, 2017

Uh oh!

openshift-bot commented Jul 6, 2017

Uh oh!

deads2k commented Jul 31, 2017

Uh oh!

smarterclayton commented Jul 31, 2017 via email

Uh oh!

spadgett commented Aug 18, 2017

Uh oh!

enj commented Aug 24, 2017

Uh oh!

mrogers950 commented Sep 19, 2017

Uh oh!

mrogers950 commented Sep 19, 2017

Uh oh!

mrogers950 commented Sep 20, 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!

mrogers950 commented Sep 26, 2017

Uh oh!

enj commented Sep 26, 2017

Uh oh!

openshift-merge-robot commented Sep 26, 2017

Uh oh!

enj commented Sep 26, 2017

Uh oh!

enj commented Sep 26, 2017

Uh oh!

openshift-merge-robot commented Sep 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

enj commented Apr 4, 2017 •

edited by mrogers950

Loading