Merge pull request #1381 from openshift/wire-serviceaccountissuer-v2

OCPBUGS-2198: AUTH-309: Wire support for trusted service account issuers

Author: mfojtik

Diff for: go.mod

@@ -12,8 +12,8 @@ require (
 	github.com/google/go-cmp v0.5.6
 	github.com/imdario/mergo v0.3.8
 	github.com/miekg/dns v1.1.25
-	github.com/openshift/api v0.0.0-20220831183848-09c070622e2c
-	github.com/openshift/build-machinery-go v0.0.0-20220720161851-9b4f0386f6b0
+	github.com/openshift/api v0.0.0-20221010120431-ec3e44909d5c
+	github.com/openshift/build-machinery-go v0.0.0-20220913142420-e25cf57ea46d
 	github.com/openshift/client-go v0.0.0-20220831193253-4950ae70c8ea
 	github.com/openshift/library-go v0.0.0-20221004015544-d6c64d0262cc
 	github.com/pkg/profile v1.5.0 // indirect

Diff for: go.sum

@@ -417,10 +417,10 @@ github.com/onsi/ginkgo/v2 v2.1.4 h1:GNapqRSid3zijZ9H77KrgVG4/8KqiyRsxcSxe+7ApXY=
 github.com/onsi/gomega v0.0.0-20170829124025-dcabb60a477c/go.mod h1:C1qb7wdrVGGVU+Z6iS04AVkA3Q65CEZX59MT0QO5uiA=
 github.com/onsi/gomega v1.7.0/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1CpauHY=
 github.com/onsi/gomega v1.19.0 h1:4ieX6qQjPP/BfC3mpsAtIGGlxTWPeA3Inl/7DtXw1tw=
-github.com/openshift/api v0.0.0-20220831183848-09c070622e2c h1:7Ar64e7somcImBRqyOLJINqirHeNwB9QdBR2AUNrnPw=
-github.com/openshift/api v0.0.0-20220831183848-09c070622e2c/go.mod h1:9JWn+H7X8wEPPc9D63krigXl8r3F1Mt6/lC98brUyhQ=
-github.com/openshift/build-machinery-go v0.0.0-20220720161851-9b4f0386f6b0 h1:ucmy9vO5mJToDExXpfBZ80XOufC9g919NsYFvO6D6CY=
-github.com/openshift/build-machinery-go v0.0.0-20220720161851-9b4f0386f6b0/go.mod h1:b1BuldmJlbA/xYtdZvKi+7j5YGB44qJUJDZ9zwiNCfE=
+github.com/openshift/api v0.0.0-20221010120431-ec3e44909d5c h1:DCQmLFr7V7yrnx1+OTJBKA4HyEs4QQ4sLXEMCKop4x8=
+github.com/openshift/api v0.0.0-20221010120431-ec3e44909d5c/go.mod h1:JRz+ZvTqu9u7t6suhhPTacbFl5K65Y6rJbNM7HjWA3g=
+github.com/openshift/build-machinery-go v0.0.0-20220913142420-e25cf57ea46d h1:RR4ah7FfaPR1WePizm0jlrsbmPu91xQZnAsVVreQV1k=
+github.com/openshift/build-machinery-go v0.0.0-20220913142420-e25cf57ea46d/go.mod h1:b1BuldmJlbA/xYtdZvKi+7j5YGB44qJUJDZ9zwiNCfE=
 github.com/openshift/client-go v0.0.0-20220831193253-4950ae70c8ea h1:7JbjIzWt3Q75ErY1PAZ+gCA+bErI6HSlpffHFmMMzqM=
 github.com/openshift/client-go v0.0.0-20220831193253-4950ae70c8ea/go.mod h1:+J8DqZC60acCdpYkwVy/KH4cudgWiFZRNOBeghCzdGA=
 github.com/openshift/library-go v0.0.0-20221004015544-d6c64d0262cc h1:bmOGYOuO1pV6mSxIBwf9Q76pCV/18utG0DYlh4NJPeU=

Diff for: pkg/operator/configobservation/auth/auth_serviceaccountissuer.go

@@ -2,7 +2,10 @@ package auth
 
 import (
 	"fmt"
+	operatorv1 "github.com/openshift/api/operator/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"net/url"
+	"sort"
 	"strings"
 
 	"k8s.io/klog/v2"
@@ -33,31 +36,28 @@ func ObserveServiceAccountIssuer(
 ) (map[string]interface{}, []error) {
 
 	listers := genericListers.(configobservation.Listers)
-	ret, errs := observedConfig(existingConfig, listers.AuthConfigLister.Get, listers.InfrastructureLister().Get, recorder)
+	ret, errs := observedConfig(existingConfig, listers.KubeAPIServerOperatorLister().Get, listers.InfrastructureLister().Get, recorder)
 	return configobserver.Pruned(ret, serviceAccountIssuerPath, audiencesPath, jwksURIPath), errs
 }
 
 // observedConfig returns an unstructured fragment of KubeAPIServerConfig that may
 // include an override of the default service account issuer if one was set in the
 // Authentication resource.
-func observedConfig(
-	existingConfig map[string]interface{},
-	getAuthConfig func(string) (*configv1.Authentication, error),
-	getInfrastructureConfig func(string) (*configv1.Infrastructure, error),
-	recorder events.Recorder,
-) (map[string]interface{}, []error) {
+func observedConfig(existingConfig map[string]interface{},
+	getOperator func(name string) (*operatorv1.KubeAPIServer, error),
+	getInfrastructureConfig func(string) (*configv1.Infrastructure, error), recorder events.Recorder) (map[string]interface{}, []error) {
 
 	errs := []error{}
 	var issuerChanged bool
-	var existingIssuer, newIssuer string
+	var existingActiveIssuer, newActiveIssuer string
 	// when the issuer will change, indicate that by setting `issuerChanged` to true
 	// to emit the informative event
 	defer func() {
 		if issuerChanged {
 			recorder.Eventf(
 				"ObserveServiceAccountIssuer",
 				"ServiceAccount issuer changed from %v to %v",
-				existingIssuer, newIssuer,
+				existingActiveIssuer, newActiveIssuer,
 			)
 		}
 	}()
@@ -68,34 +68,38 @@ func observedConfig(
 	}
 
 	if len(existingIssuers) > 0 {
-		existingIssuer = existingIssuers[0]
+		existingActiveIssuer = existingIssuers[0]
 	}
 
-	authConfig, err := getAuthConfig("cluster")
+	operator, err := getOperator("cluster")
 	if apierrors.IsNotFound(err) {
-		klog.Warningf("authentications.config.openshift.io/cluster: not found")
-		// No issuer if the auth config is missing
-		authConfig = &configv1.Authentication{}
+		klog.Warningf("kubeapiserver.operators.openshift.io/cluster: not found")
+		operator = &operatorv1.KubeAPIServer{}
 	} else if err != nil {
 		return existingConfig, append(errs, err)
 	}
 
-	newIssuer = authConfig.Spec.ServiceAccountIssuer
-	if err := checkIssuer(newIssuer); err != nil {
+	newActiveIssuer = getActiveServiceAccountIssuer(operator.Status.ServiceAccountIssuers)
+	if err := checkIssuer(newActiveIssuer); err != nil {
 		return existingConfig, append(errs, err)
 	}
 
-	if len(newIssuer) != 0 {
-		issuerChanged = existingIssuer != newIssuer
+	if len(newActiveIssuer) > 0 {
+		currentTrustedServiceAccountIssuers := getTrustedServiceAccountIssuers(operator.Status.ServiceAccountIssuers)
+		issuerChanged = issuersChanged(
+			existingIssuers,
+			append([]string{newActiveIssuer}, currentTrustedServiceAccountIssuers...)...,
+		)
+
+		value := []interface{}{newActiveIssuer}
+		for _, i := range currentTrustedServiceAccountIssuers {
+			value = append(value, i)
+		}
 		// configure the issuer if set by the user and is a valid issuer
 		return map[string]interface{}{
 			"apiServerArguments": map[string]interface{}{
-				"service-account-issuer": []interface{}{
-					newIssuer,
-				},
-				"api-audiences": []interface{}{
-					newIssuer,
-				},
+				"service-account-issuer": value,
+				"api-audiences":          value,
 			},
 		}, errs
 	}
@@ -113,7 +117,7 @@ func observedConfig(
 		return existingConfig, append(errs, fmt.Errorf("APIServerInternalURL missing from infrastructure/cluster"))
 	}
 
-	issuerChanged = existingIssuer != newIssuer
+	issuerChanged = existingActiveIssuer != newActiveIssuer
 	return map[string]interface{}{
 		"apiServerArguments": map[string]interface{}{
 			"service-account-jwks-uri": []interface{}{
@@ -123,6 +127,34 @@ func observedConfig(
 	}, errs
 }
 
+// issuersChanged compares the command line flags used for KAS and the operator status service account issuers.
+// if these two sets are different, we have a change and we need to update the KAS.
+func issuersChanged(kasIssuers []string, trustedOperatorIssuers ...string) bool {
+	sort.Strings(kasIssuers)
+	operatorAllIssuers := trustedOperatorIssuers
+	sort.Strings(operatorAllIssuers)
+	return !sets.NewString(kasIssuers...).Equal(sets.NewString(operatorAllIssuers...))
+}
+
+func getTrustedServiceAccountIssuers(issuers []operatorv1.ServiceAccountIssuerStatus) []string {
+	result := []string{}
+	for i := range issuers {
+		if issuers[i].ExpirationTime != nil {
+			result = append(result, issuers[i].Name)
+		}
+	}
+	return result
+}
+
+func getActiveServiceAccountIssuer(issuers []operatorv1.ServiceAccountIssuerStatus) string {
+	for i := range issuers {
+		if issuers[i].ExpirationTime == nil {
+			return issuers[i].Name
+		}
+	}
+	return ""
+}
+
 // checkIssuer validates the issuer in the same way that it will be validated by
 // kube-apiserver
 func checkIssuer(issuer string) error {

Diff for: pkg/operator/configobservation/auth/auth_serviceaccountissuer_test.go

@@ -3,7 +3,9 @@ package auth
 import (
 	"encoding/json"
 	"fmt"
+	operatorv1 "github.com/openshift/api/operator/v1"
 	"testing"
+	"time"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/stretchr/testify/require"
@@ -23,13 +25,15 @@ func TestObservedConfig(t *testing.T) {
 	expectedErrInfra := fmt.Errorf("bar")
 
 	for _, tc := range []struct {
-		name           string
-		issuer         string
-		existingIssuer string
-		authError      error
-		infraError     error
-		expectedIssuer string
-		expectedChange bool
+		name                   string
+		issuer                 string
+		trustedIssuers         []string
+		existingIssuer         string
+		authError              error
+		infraError             error
+		expectedIssuer         string
+		expectedTrustedIssuers []string
+		expectedChange         bool
 	}{
 		{
 			name:           "no issuer, no previous issuer",
@@ -57,6 +61,14 @@ func TestObservedConfig(t *testing.T) {
 			issuer:         "https://example.com",
 			expectedIssuer: "https://example.com",
 		},
+		{
+			name:                   "issuer set, previous issuer and trusted issuers same",
+			existingIssuer:         "https://example.com",
+			issuer:                 "https://example.com",
+			trustedIssuers:         []string{"https://trusted.example.com"},
+			expectedIssuer:         "https://example.com",
+			expectedTrustedIssuers: []string{"https://trusted.example.com"},
+		},
 		{
 			name:           "issuer set, previous issuer different",
 			existingIssuer: "https://example.com",
@@ -83,9 +95,9 @@ func TestObservedConfig(t *testing.T) {
 			testRecorder := events.NewInMemoryRecorder("SAIssuerTest")
 
 			newConfig, errs := observedConfig(
-				unstructuredAPIConfigForIssuer(t, tc.existingIssuer),
-				func(_ string) (*configv1.Authentication, error) {
-					return authConfigForIssuer(tc.issuer), tc.authError
+				unstructuredAPIConfigForIssuer(t, tc.existingIssuer, tc.trustedIssuers),
+				func(_ string) (*operatorv1.KubeAPIServer, error) {
+					return kasStatusForIssuer(tc.issuer, tc.trustedIssuers...), tc.authError
 				},
 				func(_ string) (*configv1.Infrastructure, error) {
 					return &configv1.Infrastructure{
@@ -101,7 +113,7 @@ func TestObservedConfig(t *testing.T) {
 			if tc.authError == nil && tc.infraError == nil {
 				require.Len(t, errs, 0)
 			}
-			expectedConfig = apiConfigForIssuer(tc.expectedIssuer)
+			expectedConfig = apiConfigForIssuer(tc.expectedIssuer, tc.expectedTrustedIssuers)
 
 			// Check that errors are passed through
 			if tc.authError != nil {
@@ -130,22 +142,34 @@ func TestObservedConfig(t *testing.T) {
 	}
 }
 
-func authConfigForIssuer(issuer string) *configv1.Authentication {
-	return &configv1.Authentication{
-		Spec: configv1.AuthenticationSpec{
-			ServiceAccountIssuer: issuer,
+func kasStatusForIssuer(active string, trustedIssuers ...string) *operatorv1.KubeAPIServer {
+	if len(active) == 0 {
+		return &operatorv1.KubeAPIServer{
+			Status: operatorv1.KubeAPIServerStatus{},
+		}
+	}
+	status := []operatorv1.ServiceAccountIssuerStatus{
+		{
+			Name: active,
+		},
+	}
+	for i := range trustedIssuers {
+		status = append(status, operatorv1.ServiceAccountIssuerStatus{
+			Name:           trustedIssuers[i],
+			ExpirationTime: &metav1.Time{Time: time.Now().Add(12 * time.Hour)},
+		})
+	}
+	return &operatorv1.KubeAPIServer{
+		Status: operatorv1.KubeAPIServerStatus{
+			ServiceAccountIssuers: status,
 		},
 	}
 }
 
-func apiConfigForIssuer(issuer string) *kubecontrolplanev1.KubeAPIServerConfig {
+func apiConfigForIssuer(issuer string, trustedIssuers []string) *kubecontrolplanev1.KubeAPIServerConfig {
 	args := map[string]kubecontrolplanev1.Arguments{
-		"service-account-issuer": {
-			issuer,
-		},
-		"api-audiences": {
-			issuer,
-		},
+		"service-account-issuer": append([]string{issuer}, trustedIssuers...),
+		"api-audiences":          append([]string{issuer}, trustedIssuers...),
 	}
 	if len(issuer) == 0 {
 		delete(args, "service-account-issuer")
@@ -164,8 +188,8 @@ func apiConfigForIssuer(issuer string) *kubecontrolplanev1.KubeAPIServerConfig {
 // unstructuredAPIConfigForIssuer round-trips through the golang type
 // to ensure the input to the function under test will match what will
 // be received at runtime.
-func unstructuredAPIConfigForIssuer(t *testing.T, issuer string) map[string]interface{} {
-	config := apiConfigForIssuer(issuer)
+func unstructuredAPIConfigForIssuer(t *testing.T, issuer string, trustedIssuers []string) map[string]interface{} {
+	config := apiConfigForIssuer(issuer, trustedIssuers)
 	// Unmarshaling to unstructured requires explicitly setting kind
 	config.TypeMeta = metav1.TypeMeta{
 		Kind: "KubeAPIServerConfig",

Diff for: pkg/operator/configobservation/configobservercontroller/observe_config_controller.go

@@ -1,6 +1,7 @@
 package configobservercontroller
 
 import (
+	operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/tools/cache"
 
@@ -38,6 +39,7 @@ func NewConfigObserver(
 	operatorClient v1helpers.StaticPodOperatorClient,
 	kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces,
 	configInformer configinformers.SharedInformerFactory,
+	operatorInformer operatorv1informers.SharedInformerFactory,
 	resourceSyncer resourcesynccontroller.ResourceSyncer,
 	eventRecorder events.Recorder,
 ) *ConfigObserver {
@@ -68,6 +70,7 @@ func NewConfigObserver(
 		configInformer.Config().V1().Nodes().Informer(),
 		configInformer.Config().V1().Proxies().Informer(),
 		configInformer.Config().V1().Schedulers().Informer(),
+		operatorInformer.Operator().V1().KubeAPIServers().Informer(),
 	}
 	for _, ns := range interestingNamespaces {
 		infomers = append(infomers, kubeInformersForNamespaces.InformersFor(ns).Core().V1().ConfigMaps().Informer())
@@ -92,6 +95,8 @@ func NewConfigObserver(
 				ConfigSecretLister_: kubeInformersForNamespaces.InformersFor(operatorclient.GlobalUserSpecifiedConfigNamespace).Core().V1().Secrets().Lister(),
 				ConfigmapLister_:    kubeInformersForNamespaces.ConfigMapLister(),
 
+				KubeAPIServerOperatorLister_: operatorInformer.Operator().V1().KubeAPIServers().Lister(),
+
 				ResourceSync: resourceSyncer,
 				PreRunCachesSynced: append(preRunCacheSynced,
 					operatorClient.Informer().HasSynced,

Diff for: pkg/operator/configobservation/interfaces.go

@@ -5,6 +5,7 @@ import (
 	"k8s.io/client-go/tools/cache"
 
 	configlistersv1 "github.com/openshift/client-go/config/listers/config/v1"
+	operatorlistersv1 "github.com/openshift/client-go/operator/listers/operator/v1"
 	"github.com/openshift/library-go/pkg/operator/configobserver/cloudprovider"
 	libgoetcd "github.com/openshift/library-go/pkg/operator/configobserver/etcd"
 	"github.com/openshift/library-go/pkg/operator/resourcesynccontroller"
@@ -24,6 +25,8 @@ type Listers struct {
 	ProxyLister_          configlistersv1.ProxyLister
 	SchedulerLister       configlistersv1.SchedulerLister
 
+	KubeAPIServerOperatorLister_ operatorlistersv1.KubeAPIServerLister
+
 	ConfigmapLister_    corelistersv1.ConfigMapLister
 	SecretLister_       corelistersv1.SecretLister
 	ConfigSecretLister_ corelistersv1.SecretLister
@@ -36,6 +39,10 @@ func (l Listers) APIServerLister() configlistersv1.APIServerLister {
 	return l.APIServerLister_
 }
 
+func (l Listers) KubeAPIServerOperatorLister() operatorlistersv1.KubeAPIServerLister {
+	return l.KubeAPIServerOperatorLister_
+}
+
 func (l Listers) FeatureGateLister() configlistersv1.FeatureGateLister {
 	return l.FeatureGateLister_
 }