Add API events for SA OAuth failures

Collect errors during validation of a service account's OAuth
configuration, logging them to a warning event upon a fatal error. The
event is limited to 1 per minute, to reduce the amount of etcd writes
resulting from incrementing the duplicate event counter.

Signed-off-by: Matt Rogers <[email protected]>

Author: Matt Rogers

pkg/cmd/server/origin/oauth_apiserver_adapter.go

@@ -58,6 +58,7 @@ func NewOAuthServerConfigFromMasterConfig(masterConfig *MasterConfig) (*oauthapi
 	// TODO pass a privileged client config through during construction.  It is NOT a loopback client.
 	oauthServerConfig.OpenShiftClient = masterConfig.PrivilegedLoopbackOpenShiftClient
 	oauthServerConfig.KubeClient = masterConfig.PrivilegedLoopbackKubernetesClientsetInternal
+	oauthServerConfig.KubeExternalClient = masterConfig.PrivilegedLoopbackKubernetesClientsetExternal
 
 	// Build the list of valid redirect_uri prefixes for a login using the openshift-web-console client to redirect to
 	if !options.DisabledFeatures.Has(configapi.FeatureWebConsole) {

pkg/cmd/server/origin/storage.go

@@ -10,6 +10,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	apirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/rest"
+	corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
 	restclient "k8s.io/client-go/rest"
 	"k8s.io/client-go/util/flowcontrol"
 	kapi "k8s.io/kubernetes/pkg/api"
@@ -339,7 +340,15 @@ func (c OpenshiftAPIConfig) GetRestStorage() (map[schema.GroupVersion]map[string
 		saAccountGrantMethod = oauthapi.GrantHandlerType(c.ServiceAccountMethod)
 	}
 
-	combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(c.KubeClientInternal.Core(), c.KubeClientInternal.Core(), c.DeprecatedOpenshiftClient, clientRegistry, saAccountGrantMethod)
+	combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(
+		c.KubeClientInternal.Core(),
+		c.KubeClientInternal.Core(),
+		corev1.New(c.KubeClientExternal.Core().RESTClient()).Events(""),
+		c.DeprecatedOpenshiftClient,
+		clientRegistry,
+		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

@@ -24,6 +24,7 @@ import (
 	x509request "k8s.io/apiserver/pkg/authentication/request/x509"
 	kuser "k8s.io/apiserver/pkg/authentication/user"
 	apirequest "k8s.io/apiserver/pkg/endpoints/request"
+	corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/kubernetes/pkg/client/retry"
 
 	"github.com/openshift/origin/pkg/auth/authenticator/challenger/passwordchallenger"
@@ -92,7 +93,15 @@ func (c *OAuthServerConfig) 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(),
+		// TODO: simplify this construction
+		corev1.New(c.KubeExternalClient.Core().RESTClient()).Events(""),
+		c.OpenShiftClient,
+		clientRegistry,
+		oauthapi.GrantHandlerType(c.Options.GrantConfig.ServiceAccountMethod),
+	)
 
 	accessTokenStorage, err := accesstokenetcd.NewREST(c.RESTOptionsGetter, combinedOAuthClientGetter, c.EtcdBackends...)
 	if err != nil {

pkg/oauth/apiserver/oauth_apiserver.go

@@ -15,6 +15,7 @@ import (
 	"k8s.io/apiserver/pkg/storage"
 	"k8s.io/client-go/rest"
 	kapi "k8s.io/kubernetes/pkg/api"
+	kclientsetexternal "k8s.io/kubernetes/pkg/client/clientset_generated/clientset"
 	kclientset "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
 
 	"github.com/openshift/origin/pkg/auth/server/session"
@@ -36,6 +37,9 @@ type OAuthServerConfig struct {
 	// KubeClient is kubeclient with enough permission for the auth API
 	KubeClient kclientset.Interface
 
+	// KubeExternalClient is for creating user events
+	KubeExternalClient kclientsetexternal.Interface
+
 	// OpenShiftClient is osclient with enough permission for the auth API
 	OpenShiftClient osclient.Interface
 

pkg/serviceaccounts/oauthclient/oauthclientregistry.go

@@ -10,8 +10,12 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/util/sets"
 	apiserverserviceaccount "k8s.io/apiserver/pkg/authentication/serviceaccount"
 	apirequest "k8s.io/apiserver/pkg/endpoints/request"
+	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"
@@ -21,7 +25,6 @@ import (
 	oauthapi "github.com/openshift/origin/pkg/oauth/apis/oauth"
 	"github.com/openshift/origin/pkg/oauth/registry/oauthclient"
 	routeapi "github.com/openshift/origin/pkg/route/apis/route"
-	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 const (
@@ -47,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()
@@ -58,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  osclient.RoutesNamespacer
+	saClient      kcoreclient.ServiceAccountsGetter
+	secretClient  kcoreclient.SecretsGetter
+	eventRecorder record.EventRecorder
+	routeClient   osclient.RoutesNamespacer
 	// TODO add ingress support
 	//ingressClient ??
 
@@ -186,11 +190,36 @@ 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 corev1.EventInterface, routeClient osclient.RoutesNamespacer, delegate oauthclient.Getter, grantMethod oauthapi.GrantHandlerType) oauthclient.Getter {
+	opts := record.NewDefaultEventCorrelatorOptions()
+	// Limit the OAuth events to 1 new event per minute.
+	opts.SpamBurst = 1
+	opts.SpamQPS = 1. / 60.
+
+	eventBroadcaster := record.NewBroadcaster()
+	eventBroadcaster.StartRecordingToSinkWithOptions(&corev1.EventSinkImpl{Interface: eventClient}, opts)
+	recorder := eventBroadcaster.NewRecorder(kapi.Scheme, clientv1.EventSource{Component: "service-account-oauth-client-getter"})
+	return &saOAuthClientAdapter{
+		saClient:      saClient,
+		secretClient:  secretClient,
+		eventRecorder: recorder,
+		routeClient:   routeClient,
+		delegate:      delegate,
+		grantMethod:   grantMethod,
+		decoder:       kapi.Codecs.UniversalDecoder(),
+	}
+}
+
+func joinErrors(errors []error) string {
+	msg := []string{}
+	for _, e := range errors {
+		msg = append(msg, e.Error())
+	}
+	return strings.Join(msg, ",")
 }
 
 func (a *saOAuthClientAdapter) GetClient(ctx apirequest.Context, name string, options *metav1.GetOptions) (*oauthapi.OAuthClient, error) {
+	var err error
 	saNamespace, saName, err := apiserverserviceaccount.SplitUsername(name)
 	if err != nil {
 		return a.delegate.GetClient(ctx, name, options)
@@ -201,25 +230,44 @@ func (a *saOAuthClientAdapter) GetClient(ctx apirequest.Context, name string, op
 		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.Eventf(sa, kapi.EventTypeWarning, failReason, "%s", joinErrors(saErrors))
+		}
+	}()
+
 	redirectURIs := []string{}
-	if modelsMap := parseModelsMap(sa.Annotations, a.decoder); len(modelsMap) > 0 {
-		if uris := a.extractRedirectURIs(modelsMap, saNamespace); len(uris) > 0 {
+	modelsMap, saErrors := parseModelsMap(sa.Annotations, a.decoder)
+	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])
@@ -242,9 +290,10 @@ func (a *saOAuthClientAdapter) GetClient(ctx apirequest.Context, name string, op
 
 // 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 {
@@ -255,16 +304,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.
@@ -279,9 +332,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,
@@ -305,27 +359,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{}
@@ -334,7 +398,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.