Add events to SA as OAuth client flow

Signed-off-by: Monis Khan <[email protected]>

Author: enj

pkg/cmd/server/origin/auth.go

@@ -91,7 +91,15 @@ func (c *AuthConfig) WithOAuth(handler http.Handler) (http.Handler, error) {
 		return nil, err
 	}
 	clientRegistry := clientregistry.NewRegistry(clientStorage)
-	combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(c.KubeClient.Core(), c.KubeClient.Core(), c.OpenShiftClient, clientRegistry, oauthapi.GrantHandlerType(c.Options.GrantConfig.ServiceAccountMethod))
+
+	combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(
+		c.KubeClient.Core(),
+		c.KubeClient.Core(),
+		c.KubeClient.Core(),
+		c.OpenShiftClient,
+		clientRegistry,
+		oauthapi.GrantHandlerType(c.Options.GrantConfig.ServiceAccountMethod),
+	)
 
 	accessTokenStorage, err := accesstokenetcd.NewREST(c.RESTOptionsGetter, combinedOAuthClientGetter, c.EtcdBackends...)
 	if err != nil {

pkg/cmd/server/origin/master.go

@@ -801,7 +801,16 @@ func (c *MasterConfig) GetRestStorage() map[unversioned.GroupVersion]map[string]
 	}
 
 	osClient, kubeClient := c.OAuthServerClients()
-	combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(kubeClient.Core(), kubeClient.Core(), osClient, clientRegistry, saAccountGrantMethod)
+
+	combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(
+		kubeClient.Core(),
+		kubeClient.Core(),
+		kubeClient.Core(),
+		osClient,
+		clientRegistry,
+		saAccountGrantMethod,
+	)
+
 	authorizeTokenStorage, err := authorizetokenetcd.NewREST(c.RESTOptionsGetter, combinedOAuthClientGetter)
 	checkStorageErr(err)
 	accessTokenStorage, err := accesstokenetcd.NewREST(c.RESTOptionsGetter, combinedOAuthClientGetter)

pkg/serviceaccounts/oauthclient/oauthclientregistry.go

@@ -8,17 +8,18 @@ import (
 	"strings"
 
 	kapi "k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/api/unversioned"
 	kcoreclient "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/core/internalversion"
+	"k8s.io/kubernetes/pkg/client/record"
 	"k8s.io/kubernetes/pkg/runtime"
 	"k8s.io/kubernetes/pkg/serviceaccount"
+	"k8s.io/kubernetes/pkg/util/sets"
 
 	scopeauthorizer "github.com/openshift/origin/pkg/authorization/authorizer/scope"
 	osclient "github.com/openshift/origin/pkg/client"
 	oauthapi "github.com/openshift/origin/pkg/oauth/api"
 	"github.com/openshift/origin/pkg/oauth/registry/oauthclient"
 	routeapi "github.com/openshift/origin/pkg/route/api"
-	"k8s.io/kubernetes/pkg/api/unversioned"
-	"k8s.io/kubernetes/pkg/util/sets"
 )
 
 const (
@@ -57,6 +58,7 @@ var legacyRouteGroupKind = routeapi.LegacySchemeGroupVersion.WithKind(routeKind)
 type saOAuthClientAdapter struct {
 	saClient     kcoreclient.ServiceAccountsGetter
 	secretClient kcoreclient.SecretsGetter
+	eventsClient kcoreclient.EventsGetter
 	routeClient  osclient.RoutesNamespacer
 	// TODO add ingress support
 	//ingressClient ??
@@ -183,8 +185,8 @@ func (uri *redirectURI) merge(m *model) {
 
 var _ oauthclient.Getter = &saOAuthClientAdapter{}
 
-func NewServiceAccountOAuthClientGetter(saClient kcoreclient.ServiceAccountsGetter, secretClient kcoreclient.SecretsGetter, routeClient osclient.RoutesNamespacer, delegate oauthclient.Getter, grantMethod oauthapi.GrantHandlerType) oauthclient.Getter {
-	return &saOAuthClientAdapter{saClient: saClient, secretClient: secretClient, routeClient: routeClient, delegate: delegate, grantMethod: grantMethod, decoder: kapi.Codecs.UniversalDecoder()}
+func NewServiceAccountOAuthClientGetter(saClient kcoreclient.ServiceAccountsGetter, secretClient kcoreclient.SecretsGetter, eventClient kcoreclient.EventsGetter, routeClient osclient.RoutesNamespacer, delegate oauthclient.Getter, grantMethod oauthapi.GrantHandlerType) oauthclient.Getter {
+	return &saOAuthClientAdapter{saClient: saClient, secretClient: secretClient, eventsClient: eventClient, routeClient: routeClient, delegate: delegate, grantMethod: grantMethod, decoder: kapi.Codecs.UniversalDecoder()}
 }
 
 func (a *saOAuthClientAdapter) GetClient(ctx kapi.Context, name string) (*oauthapi.OAuthClient, error) {
@@ -198,13 +200,16 @@ func (a *saOAuthClientAdapter) GetClient(ctx kapi.Context, name string) (*oautha
 		return nil, err
 	}
 
+	recorder := a.getEventRecorder(saNamespace)
+
 	redirectURIs := []string{}
-	if modelsMap := parseModelsMap(sa.Annotations, a.decoder); len(modelsMap) > 0 {
+	if modelsMap := parseModelsMap(sa, a.decoder, recorder); len(modelsMap) > 0 {
 		if uris := a.extractRedirectURIs(modelsMap, saNamespace); len(uris) > 0 {
 			redirectURIs = append(redirectURIs, uris.extractValidRedirectURIStrings()...)
 		}
 	}
 	if len(redirectURIs) == 0 {
+		recorder.Event(sa, kapi.EventTypeWarning, "OAuthNoRedirectURIs", "Has no redirectURIs")
 		return nil, fmt.Errorf(
 			"%v has no redirectURIs; set %v<some-value>=<redirect> or create a dynamic URI using %v<some-value>=<reference>",
 			name, OAuthRedirectModelAnnotationURIPrefix, OAuthRedirectModelAnnotationReferencePrefix,
@@ -234,30 +239,48 @@ func (a *saOAuthClientAdapter) GetClient(ctx kapi.Context, name string) (*oautha
 		RedirectURIs: sets.NewString(redirectURIs...).List(),
 		GrantMethod:  a.grantMethod,
 	}
+
+	// TODO: is this safe to tell - could leak Route info?
+	recorder.Eventf(sa, kapi.EventTypeNormal, "OAuthAllRedirectURIs", "Has the following redirectURIs: %v", saClient.RedirectURIs)
+
 	return saClient, nil
 }
 
+// TODO this is super naive and inefficient
+func (a *saOAuthClientAdapter) getEventRecorder(namespace string) record.EventRecorder {
+	eventBroadcaster := record.NewBroadcaster()
+	eventBroadcaster.StartRecordingToSink(&kcoreclient.EventSinkImpl{Interface: a.eventsClient.Events(namespace)})
+	return eventBroadcaster.NewRecorder(kapi.EventSource{Component: "service-account-oauth-client-getter"})
+}
+
 // parseModelsMap builds a map of model name to model using a service account's annotations.
 // The model name is only used for building the map (it ties together the uri and reference annotations)
 // and serves no functional purpose other than making testing easier.
-func parseModelsMap(annotations map[string]string, decoder runtime.Decoder) map[string]model {
+func parseModelsMap(sa *kapi.ServiceAccount, decoder runtime.Decoder, recorder record.EventRecorder) map[string]model {
 	models := map[string]model{}
-	for key, value := range annotations {
+	for key, value := range sa.Annotations {
 		prefix, name, ok := parseModelPrefixName(key)
 		if !ok {
+			recorder.Eventf(sa, kapi.EventTypeNormal, "OAuthAnnotationSkipped", "Annotation key does not match an OAuth prefix: %s=%s", key, value)
 			continue
 		}
 		m := models[name]
 		switch prefix {
 		case OAuthRedirectModelAnnotationURIPrefix:
 			if u, err := url.Parse(value); err == nil {
 				m.updateFromURI(u)
+			} else {
+				recorder.Eventf(sa, kapi.EventTypeWarning, "OAuthAnnotationSkipped", "Annotation value is not a valid URL: %s=%s", key, value)
 			}
 		case OAuthRedirectModelAnnotationReferencePrefix:
 			r := &oauthapi.OAuthRedirectReference{}
 			if err := runtime.DecodeInto(decoder, []byte(value), r); err == nil {
 				m.updateFromReference(&r.Reference)
+			} else {
+				recorder.Eventf(sa, kapi.EventTypeWarning, "OAuthAnnotationSkipped", "Annotation value is not a valid OAuthRedirectReference: %s=%s", key, value)
 			}
+		default:
+			panic("unreacable")
 		}
 		models[name] = m
 	}

pkg/serviceaccounts/oauthclient/oauthclientregistry_test.go

@@ -8,6 +8,7 @@ import (
 	kapi "k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/unversioned"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake"
+	"k8s.io/kubernetes/pkg/client/record"
 	"k8s.io/kubernetes/pkg/client/testing/core"
 	"k8s.io/kubernetes/pkg/runtime"
 	"k8s.io/kubernetes/pkg/types"
@@ -542,7 +543,7 @@ func TestGetClient(t *testing.T) {
 
 	for _, tc := range testCases {
 		delegate := &fakeDelegate{}
-		getter := NewServiceAccountOAuthClientGetter(tc.kubeClient.Core(), tc.kubeClient.Core(), tc.osClient, delegate, oauthapi.GrantHandlerPrompt)
+		getter := NewServiceAccountOAuthClientGetter(tc.kubeClient.Core(), tc.kubeClient.Core(), tc.kubeClient.Core(), tc.osClient, delegate, oauthapi.GrantHandlerPrompt)
 		client, err := getter.GetClient(kapi.NewContext(), tc.clientName)
 		switch {
 		case len(tc.expectedErr) == 0 && err == nil:
@@ -811,8 +812,10 @@ func TestParseModelsMap(t *testing.T) {
 			},
 		},
 	} {
-		if !reflect.DeepEqual(test.expected, parseModelsMap(test.annotations, decoder)) {
-			t.Errorf("%s: expected %#v, got %#v", test.name, test.expected, parseModelsMap(test.annotations, decoder))
+		recorder := record.NewFakeRecorder(1000)
+		sa := &kapi.ServiceAccount{ObjectMeta: kapi.ObjectMeta{Annotations: test.annotations}}
+		if !reflect.DeepEqual(test.expected, parseModelsMap(sa, decoder, recorder)) {
+			t.Errorf("%s: expected %#v, got %#v", test.name, test.expected, parseModelsMap(sa, decoder, recorder))
 		}
 	}
 }