Merge pull request #13621 from enj/enj/f/sa_oauth_events

Automatic merge from submit-queue

Add events to SA as OAuth client flow

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

```$ oc describe sa/proxy                              
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

pkg/oauth/apiserver/apiserver.go

@@ -8,6 +8,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/serializer"
 	"k8s.io/apiserver/pkg/registry/rest"
 	genericapiserver "k8s.io/apiserver/pkg/server"
+	corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
 	restclient "k8s.io/client-go/rest"
 	coreclient "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/core/internalversion"
 
@@ -121,8 +122,19 @@ func (c *OAuthAPIServerConfig) newV1RESTStorage() (map[string]rest.Storage, erro
 	if err != nil {
 		return nil, err
 	}
+	coreV1Client, err := corev1.NewForConfig(c.CoreAPIServerClientConfig)
+	if err != nil {
+		return nil, err
+	}
 
-	combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(coreClient, coreClient, routeClient, oauthClient.OAuthClients(), saAccountGrantMethod)
+	combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(
+		coreClient,
+		coreClient,
+		coreV1Client.Events(""),
+		routeClient,
+		oauthClient.OAuthClients(),
+		saAccountGrantMethod,
+	)
 	authorizeTokenStorage, err := authorizetokenetcd.NewREST(c.GenericConfig.RESTOptionsGetter, combinedOAuthClientGetter)
 	if err != nil {
 		return nil, fmt.Errorf("error building REST storage: %v", err)

pkg/oauth/apiserver/auth.go

@@ -79,7 +79,14 @@ func (c *OAuthServerConfig) WithOAuth(handler http.Handler) (http.Handler, error
 	// pass through all other requests
 	mux.Handle("/", handler)
 
-	combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(c.KubeClient.Core(), c.KubeClient.Core(), c.RouteClient.Route(), c.OAuthClientClient, oauthapi.GrantHandlerType(c.Options.GrantConfig.ServiceAccountMethod))
+	combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(
+		c.KubeClient.Core(),
+		c.KubeClient.Core(),
+		c.EventsClient,
+		c.RouteClient.Route(),
+		c.OAuthClientClient,
+		oauthapi.GrantHandlerType(c.Options.GrantConfig.ServiceAccountMethod),
+	)
 
 	errorPageHandler, err := c.getErrorHandler()
 	if err != nil {

pkg/oauth/apiserver/oauth_apiserver.go

@@ -14,6 +14,7 @@ import (
 	apirequest "k8s.io/apiserver/pkg/endpoints/request"
 	genericapiserver "k8s.io/apiserver/pkg/server"
 	genericfilters "k8s.io/apiserver/pkg/server/filters"
+	corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/client-go/rest"
 	kapi "k8s.io/kubernetes/pkg/api"
 	kclientset "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
@@ -40,6 +41,9 @@ type OAuthServerConfig struct {
 	// KubeClient is kubeclient with enough permission for the auth API
 	KubeClient kclientset.Interface
 
+	// EventsClient is for creating user events
+	EventsClient corev1.EventInterface
+
 	// RouteClient provides a client for OpenShift routes API.
 	RouteClient routeclient.Interface
 
@@ -80,11 +84,16 @@ func NewOAuthServerConfig(oauthConfig configapi.OAuthConfig, userClientConfig *r
 	if err != nil {
 		return nil, err
 	}
+	eventsClient, err := corev1.NewForConfig(userClientConfig)
+	if err != nil {
+		return nil, err
+	}
 
 	ret := &OAuthServerConfig{
 		GenericConfig:                  genericConfig,
 		Options:                        oauthConfig,
 		SessionAuth:                    sessionAuth,
+		EventsClient:                   eventsClient.Events(""),
 		IdentityClient:                 userClient.Identities(),
 		UserClient:                     userClient.Users(),
 		UserIdentityMappingClient:      userClient.UserIdentityMappings(),

pkg/serviceaccounts/oauthclient/oauthclientregistry.go

@@ -10,7 +10,12 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
+	"k8s.io/apimachinery/pkg/util/sets"
 	apiserverserviceaccount "k8s.io/apiserver/pkg/authentication/serviceaccount"
+	corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
+	clientv1 "k8s.io/client-go/pkg/api/v1"
+	"k8s.io/client-go/tools/record"
 	kapi "k8s.io/kubernetes/pkg/api"
 	kcoreclient "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/core/internalversion"
 	"k8s.io/kubernetes/pkg/serviceaccount"
@@ -20,7 +25,6 @@ import (
 	"github.com/openshift/origin/pkg/oauth/registry/oauthclient"
 	routeapi "github.com/openshift/origin/pkg/route/apis/route"
 	routeclient "github.com/openshift/origin/pkg/route/generated/internalclientset/typed/route/internalversion"
-	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 const (
@@ -46,8 +50,8 @@ var modelPrefixes = []string{
 // namesToObjMapperFunc is linked to a given GroupKind.
 // Based on the namespace and names provided, it builds a map of resource name to redirect URIs.
 // The redirect URIs represent the default values as specified by the resource.
-// These values can be overridden by user specified data.
-type namesToObjMapperFunc func(namespace string, names sets.String) map[string]redirectURIList
+// These values can be overridden by user specified data. Errors returned are informative and non-fatal.
+type namesToObjMapperFunc func(namespace string, names sets.String) (map[string]redirectURIList, []error)
 
 var emptyGroupKind = schema.GroupKind{} // Used with static redirect URIs
 var routeGroupKind = routeapi.SchemeGroupVersion.WithKind(routeKind).GroupKind()
@@ -57,9 +61,10 @@ var legacyRouteGroupKind = routeapi.LegacySchemeGroupVersion.WithKind(routeKind)
 // var ingressGroupKind = routeapi.SchemeGroupVersion.WithKind(IngressKind).GroupKind()
 
 type saOAuthClientAdapter struct {
-	saClient     kcoreclient.ServiceAccountsGetter
-	secretClient kcoreclient.SecretsGetter
-	routeClient  routeclient.RoutesGetter
+	saClient      kcoreclient.ServiceAccountsGetter
+	secretClient  kcoreclient.SecretsGetter
+	eventRecorder record.EventRecorder
+	routeClient   routeclient.RoutesGetter
 	// TODO add ingress support
 	//ingressClient ??
 
@@ -188,22 +193,27 @@ var _ oauthclient.Getter = &saOAuthClientAdapter{}
 func NewServiceAccountOAuthClientGetter(
 	saClient kcoreclient.ServiceAccountsGetter,
 	secretClient kcoreclient.SecretsGetter,
+	eventClient corev1.EventInterface,
 	routeClient routeclient.RoutesGetter,
 	delegate oauthclient.Getter,
 	grantMethod oauthapi.GrantHandlerType,
 ) oauthclient.Getter {
-
+	eventBroadcaster := record.NewBroadcaster()
+	eventBroadcaster.StartRecordingToSink(&corev1.EventSinkImpl{Interface: eventClient})
+	recorder := eventBroadcaster.NewRecorder(kapi.Scheme, clientv1.EventSource{Component: "service-account-oauth-client-getter"})
 	return &saOAuthClientAdapter{
-		saClient:     saClient,
-		secretClient: secretClient,
-		routeClient:  routeClient,
-		delegate:     delegate,
-		grantMethod:  grantMethod,
-		decoder:      kapi.Codecs.UniversalDecoder(),
+		saClient:      saClient,
+		secretClient:  secretClient,
+		eventRecorder: recorder,
+		routeClient:   routeClient,
+		delegate:      delegate,
+		grantMethod:   grantMethod,
+		decoder:       kapi.Codecs.UniversalDecoder(),
 	}
 }
 
 func (a *saOAuthClientAdapter) Get(name string, options metav1.GetOptions) (*oauthapi.OAuthClient, error) {
+	var err error
 	saNamespace, saName, err := apiserverserviceaccount.SplitUsername(name)
 	if err != nil {
 		return a.delegate.Get(name, options)
@@ -214,25 +224,48 @@ func (a *saOAuthClientAdapter) Get(name string, options metav1.GetOptions) (*oau
 		return nil, err
 	}
 
+	var saErrors []error
+	var failReason string
+	// Create a warning event combining the collected annotation errors upon failure.
+	defer func() {
+		if err != nil && len(saErrors) > 0 && len(failReason) > 0 {
+			a.eventRecorder.Event(sa, kapi.EventTypeWarning, failReason, utilerrors.NewAggregate(saErrors).Error())
+		}
+	}()
+
 	redirectURIs := []string{}
-	if modelsMap := parseModelsMap(sa.Annotations, a.decoder); len(modelsMap) > 0 {
-		if uris := a.extractRedirectURIs(modelsMap, saNamespace); len(uris) > 0 {
+	modelsMap, errs := parseModelsMap(sa.Annotations, a.decoder)
+	if len(errs) > 0 {
+		saErrors = append(saErrors, errs...)
+	}
+
+	if len(modelsMap) > 0 {
+		uris, extractErrors := a.extractRedirectURIs(modelsMap, saNamespace)
+		if len(uris) > 0 {
 			redirectURIs = append(redirectURIs, uris.extractValidRedirectURIStrings()...)
 		}
+		if len(extractErrors) > 0 {
+			saErrors = append(saErrors, extractErrors...)
+		}
 	}
 	if len(redirectURIs) == 0 {
-		return nil, fmt.Errorf(
-			"%v has no redirectURIs; set %v<some-value>=<redirect> or create a dynamic URI using %v<some-value>=<reference>",
+		err = fmt.Errorf("%v has no redirectURIs; set %v<some-value>=<redirect> or create a dynamic URI using %v<some-value>=<reference>",
 			name, OAuthRedirectModelAnnotationURIPrefix, OAuthRedirectModelAnnotationReferencePrefix,
 		)
+		failReason = "NoSAOAuthRedirectURIs"
+		saErrors = append(saErrors, err)
+		return nil, err
 	}
 
 	tokens, err := a.getServiceAccountTokens(sa)
 	if err != nil {
 		return nil, err
 	}
 	if len(tokens) == 0 {
-		return nil, fmt.Errorf("%v has no tokens", name)
+		err = fmt.Errorf("%v has no tokens", name)
+		failReason = "NoSAOAuthTokens"
+		saErrors = append(saErrors, err)
+		return nil, err
 	}
 
 	saWantsChallenges, _ := strconv.ParseBool(sa.Annotations[OAuthWantChallengesAnnotationPrefix])
@@ -255,9 +288,10 @@ func (a *saOAuthClientAdapter) Get(name string, options metav1.GetOptions) (*oau
 
 // 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 {
+// and serves no functional purpose other than making testing easier. Errors returned are informative and non-fatal.
+func parseModelsMap(annotations map[string]string, decoder runtime.Decoder) (map[string]model, []error) {
 	models := map[string]model{}
+	parseErrors := []error{}
 	for key, value := range annotations {
 		prefix, name, ok := parseModelPrefixName(key)
 		if !ok {
@@ -268,16 +302,20 @@ func parseModelsMap(annotations map[string]string, decoder runtime.Decoder) map[
 		case OAuthRedirectModelAnnotationURIPrefix:
 			if u, err := url.Parse(value); err == nil {
 				m.updateFromURI(u)
+			} else {
+				parseErrors = append(parseErrors, err)
 			}
 		case OAuthRedirectModelAnnotationReferencePrefix:
 			r := &oauthapi.OAuthRedirectReference{}
 			if err := runtime.DecodeInto(decoder, []byte(value), r); err == nil {
 				m.updateFromReference(&r.Reference)
+			} else {
+				parseErrors = append(parseErrors, err)
 			}
 		}
 		models[name] = m
 	}
-	return models
+	return models, parseErrors
 }
 
 // parseModelPrefixName determines if the given key is a model prefix.
@@ -292,9 +330,10 @@ func parseModelPrefixName(key string) (string, string, bool) {
 }
 
 // extractRedirectURIs builds redirect URIs using the given models and namespace.
-// The returned redirect URIs may contain duplicates and invalid entries.
-func (a *saOAuthClientAdapter) extractRedirectURIs(modelsMap map[string]model, namespace string) redirectURIList {
+// The returned redirect URIs may contain duplicates and invalid entries. Errors returned are informative and non-fatal.
+func (a *saOAuthClientAdapter) extractRedirectURIs(modelsMap map[string]model, namespace string) (redirectURIList, []error) {
 	var data redirectURIList
+	routeErrors := []error{}
 	groupKindModelListMapper := map[schema.GroupKind]modelList{} // map of GroupKind to all models belonging to it
 	groupKindModelToURI := map[schema.GroupKind]namesToObjMapperFunc{
 		routeGroupKind: a.redirectURIsFromRoutes,
@@ -318,27 +357,37 @@ func (a *saOAuthClientAdapter) extractRedirectURIs(modelsMap map[string]model, n
 
 	for gk, models := range groupKindModelListMapper {
 		if names := models.getNames(); names.Len() > 0 {
-			if objMapper := groupKindModelToURI[gk](namespace, names); len(objMapper) > 0 {
+			objMapper, errs := groupKindModelToURI[gk](namespace, names)
+			if len(objMapper) > 0 {
 				data = append(data, models.getRedirectURIs(objMapper)...)
 			}
+			if len(errs) > 0 {
+				routeErrors = append(routeErrors, errs...)
+			}
 		}
 	}
 
-	return data
+	return data, routeErrors
 }
 
 // redirectURIsFromRoutes is the namesToObjMapperFunc specific to Routes.
 // Returns a map of route name to redirect URIs that contain the default data as specified by the route's ingresses.
-func (a *saOAuthClientAdapter) redirectURIsFromRoutes(namespace string, osRouteNames sets.String) map[string]redirectURIList {
+// Errors returned are informative and non-fatal.
+func (a *saOAuthClientAdapter) redirectURIsFromRoutes(namespace string, osRouteNames sets.String) (map[string]redirectURIList, []error) {
 	var routes []routeapi.Route
+	routeErrors := []error{}
 	routeInterface := a.routeClient.Routes(namespace)
 	if osRouteNames.Len() > 1 {
 		if r, err := routeInterface.List(metav1.ListOptions{}); err == nil {
 			routes = r.Items
+		} else {
+			routeErrors = append(routeErrors, err)
 		}
 	} else {
 		if r, err := routeInterface.Get(osRouteNames.List()[0], metav1.GetOptions{}); err == nil {
 			routes = append(routes, *r)
+		} else {
+			routeErrors = append(routeErrors, err)
 		}
 	}
 	routeMap := map[string]redirectURIList{}
@@ -347,7 +396,7 @@ func (a *saOAuthClientAdapter) redirectURIsFromRoutes(namespace string, osRouteN
 			routeMap[route.Name] = redirectURIsFromRoute(&route)
 		}
 	}
-	return routeMap
+	return routeMap, routeErrors
 }
 
 // redirectURIsFromRoute returns a list of redirect URIs that contain the default data as specified by the given route's ingresses.