Move FG check to main

Signed-off-by: Per Goncalves da Silva <[email protected]>

Author: Per Goncalves da Silva

cmd/operator-controller/main.go

@@ -304,7 +304,10 @@ func run() error {
 		return err
 	}
 	tokenGetter := authentication.NewTokenGetter(coreClient, authentication.WithExpirationDuration(1*time.Hour))
-	clientRestConfigMapper := action.ClusterExtensionUserRestConfigMapper(tokenGetter)
+	clientRestConfigMapper := action.ServiceAccountRestConfigMapper(tokenGetter)
+	if features.OperatorControllerFeatureGate.Enabled(features.SyntheticPermissions) {
+		clientRestConfigMapper = action.SyntheticUserRestConfigMapper(clientRestConfigMapper)
+	}
 
 	cfgGetter, err := helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper(),
 		helmclient.StorageDriverMapper(action.ChunkedStorageDriverMapper(coreClient, mgr.GetAPIReader(), cfg.systemNamespace)),

internal/operator-controller/action/restconfig.go

@@ -2,6 +2,7 @@ package action
 
 import (
 	"context"
+	"fmt"
 	"net/http"
 
 	"k8s.io/apimachinery/pkg/types"
@@ -11,38 +12,22 @@ import (
 
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/authentication"
-	"github.com/operator-framework/operator-controller/internal/operator-controller/features"
 )
 
 const syntheticServiceAccountName = "olm.synthetic-user"
 
-type clusterExtensionRestConfigMapper struct {
-	saRestConfigMapper        func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error)
-	synthUserRestConfigMapper func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error)
-}
-
-func (m *clusterExtensionRestConfigMapper) mapper() func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) {
-	synthAuthFeatureEnabled := features.OperatorControllerFeatureGate.Enabled(features.SyntheticPermissions)
+// SyntheticUserRestConfigMapper returns an AuthConfigMapper that that impersonates synthetic users and groups for Object o.
+// o is expected to be a ClusterExtension. If the service account defined in o is different from 'olm.synthetic-user', the
+// defaultAuthMapper will be used
+func SyntheticUserRestConfigMapper(defaultAuthMapper func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error)) func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) {
 	return func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) {
-		cExt := o.(*ocv1.ClusterExtension)
-		if synthAuthFeatureEnabled && cExt.Spec.ServiceAccount.Name == syntheticServiceAccountName {
-			return m.synthUserRestConfigMapper(ctx, o, c)
+		cExt, err := validate(o, c)
+		if err != nil {
+			return nil, err
+		}
+		if cExt.Spec.ServiceAccount.Name != syntheticServiceAccountName {
+			return defaultAuthMapper(ctx, cExt, c)
 		}
-		return m.saRestConfigMapper(ctx, o, c)
-	}
-}
-
-func ClusterExtensionUserRestConfigMapper(tokenGetter *authentication.TokenGetter) func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) {
-	m := &clusterExtensionRestConfigMapper{
-		saRestConfigMapper:        serviceAccountRestConfigMapper(tokenGetter),
-		synthUserRestConfigMapper: syntheticUserRestConfigMapper(),
-	}
-	return m.mapper()
-}
-
-func syntheticUserRestConfigMapper() func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) {
-	return func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) {
-		cExt := o.(*ocv1.ClusterExtension)
 		cc := rest.CopyConfig(c)
 		cc.Wrap(func(rt http.RoundTripper) http.RoundTripper {
 			return transport.NewImpersonatingRoundTripper(authentication.SyntheticImpersonationConfig(*cExt), rt)
@@ -51,21 +36,39 @@ func syntheticUserRestConfigMapper() func(ctx context.Context, o client.Object,
 	}
 }
 
-func serviceAccountRestConfigMapper(tokenGetter *authentication.TokenGetter) func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) {
+// ServiceAccountRestConfigMapper returns an AuthConfigMapper scoped to the service account defined in o, which is expected to
+// be a ClusterExtension
+func ServiceAccountRestConfigMapper(tokenGetter *authentication.TokenGetter) func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) {
 	return func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) {
-		cExt := o.(*ocv1.ClusterExtension)
-		saKey := types.NamespacedName{
-			Name:      cExt.Spec.ServiceAccount.Name,
-			Namespace: cExt.Spec.Namespace,
+		cExt, err := validate(o, c)
+		if err != nil {
+			return nil, err
 		}
 		saConfig := rest.AnonymousClientConfig(c)
 		saConfig.Wrap(func(rt http.RoundTripper) http.RoundTripper {
 			return &authentication.TokenInjectingRoundTripper{
 				Tripper:     rt,
 				TokenGetter: tokenGetter,
-				Key:         saKey,
+				Key: types.NamespacedName{
+					Name:      cExt.Spec.ServiceAccount.Name,
+					Namespace: cExt.Spec.Namespace,
+				},
 			}
 		})
 		return saConfig, nil
 	}
 }
+
+func validate(o client.Object, c *rest.Config) (*ocv1.ClusterExtension, error) {
+	if c == nil {
+		return nil, fmt.Errorf("rest config is nil")
+	}
+	if o == nil {
+		return nil, fmt.Errorf("object is nil")
+	}
+	cExt, ok := o.(*ocv1.ClusterExtension)
+	if !ok {
+		return nil, fmt.Errorf("object is not a ClusterExtension")
+	}
+	return cExt, nil
+}

internal/operator-controller/action/restconfig_test.go

@@ -1,92 +1,177 @@
-package action
+package action_test
 
 import (
 	"context"
+	"errors"
+	"net/http"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/rest"
-	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
-	"github.com/operator-framework/operator-controller/internal/operator-controller/features"
+	"github.com/operator-framework/operator-controller/internal/operator-controller/action"
+	"github.com/operator-framework/operator-controller/internal/operator-controller/authentication"
 )
 
-const (
-	saAccountWrapper = "service account wrapper"
-	synthUserWrapper = "synthetic user wrapper"
-)
-
-func fakeRestConfigWrapper() clusterExtensionRestConfigMapper {
-	// The rest config's host field is artificially used to differentiate between the wrappers
-	return clusterExtensionRestConfigMapper{
-		saRestConfigMapper: func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) {
-			return &rest.Config{
-				Host: saAccountWrapper,
-			}, nil
-		},
-		synthUserRestConfigMapper: func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) {
-			return &rest.Config{
-				Host: synthUserWrapper,
-			}, nil
+func Test_ServiceAccountRestConfigMapper(t *testing.T) {
+	for _, tc := range []struct {
+		description   string
+		obj           client.Object
+		cfg           *rest.Config
+		expectedError error
+	}{
+		{
+			description:   "return error if object is nil",
+			cfg:           &rest.Config{},
+			expectedError: errors.New("object is nil"),
+		}, {
+			description:   "return error if cfg is nil",
+			obj:           &ocv1.ClusterExtension{},
+			expectedError: errors.New("rest config is nil"),
+		}, {
+			description:   "return error if object is not a ClusterExtension",
+			obj:           &corev1.Secret{},
+			cfg:           &rest.Config{},
+			expectedError: errors.New("object is not a ClusterExtension"),
+		}, {
+			description: "succeeds if object is not a ClusterExtension",
+			obj: &ocv1.ClusterExtension{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "my-clusterextension",
+				},
+				Spec: ocv1.ClusterExtensionSpec{
+					ServiceAccount: ocv1.ServiceAccountReference{
+						Name: "my-service-account",
+					},
+					Namespace: "my-namespace",
+				},
+			},
+			cfg: &rest.Config{},
 		},
+	} {
+		t.Run(tc.description, func(t *testing.T) {
+			tokenGetter := &authentication.TokenGetter{}
+			saMapper := action.ServiceAccountRestConfigMapper(tokenGetter)
+			actualCfg, err := saMapper(context.Background(), tc.obj, tc.cfg)
+			if tc.expectedError != nil {
+				require.Nil(t, actualCfg)
+				require.EqualError(t, err, tc.expectedError.Error())
+			} else {
+				require.NoError(t, err)
+				transport, err := rest.TransportFor(actualCfg)
+				require.NoError(t, err)
+				require.NotNil(t, transport)
+				tokenInjectionRoundTripper, ok := transport.(*authentication.TokenInjectingRoundTripper)
+				require.True(t, ok)
+				require.Equal(t, tokenGetter, tokenInjectionRoundTripper.TokenGetter)
+				require.Equal(t, types.NamespacedName{Name: "my-service-account", Namespace: "my-namespace"}, tokenInjectionRoundTripper.Key)
+			}
+		})
 	}
 }
 
-func TestMapper_SyntheticPermissionsEnabled(t *testing.T) {
-	featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.SyntheticPermissions, true)
-
+func Test_SyntheticUserRestConfigMapper_Fails(t *testing.T) {
 	for _, tc := range []struct {
-		description        string
-		serviceAccountName string
-		expectedMapper     string
-		fgEnabled          bool
+		description   string
+		obj           client.Object
+		cfg           *rest.Config
+		expectedError error
 	}{
 		{
-			description:        "user service account wrapper if extension service account is _not_ called olm.synthetic-user",
-			serviceAccountName: "not.olm.synthetic-user",
-			expectedMapper:     saAccountWrapper,
-			fgEnabled:          true,
+			description:   "return error if object is nil",
+			cfg:           &rest.Config{},
+			expectedError: errors.New("object is nil"),
 		}, {
-			description:        "user synthetic user wrapper is extension service account is called olm.synthetic-user",
-			serviceAccountName: "olm.synthetic-user",
-			expectedMapper:     synthUserWrapper,
-			fgEnabled:          true,
+			description:   "return error if cfg is nil",
+			obj:           &ocv1.ClusterExtension{},
+			expectedError: errors.New("rest config is nil"),
+		}, {
+			description:   "return error if object is not a ClusterExtension",
+			obj:           &corev1.Secret{},
+			cfg:           &rest.Config{},
+			expectedError: errors.New("object is not a ClusterExtension"),
 		},
 	} {
 		t.Run(tc.description, func(t *testing.T) {
-			m := fakeRestConfigWrapper()
-			mapper := m.mapper()
-			ext := &ocv1.ClusterExtension{
-				Spec: ocv1.ClusterExtensionSpec{
-					ServiceAccount: ocv1.ServiceAccountReference{
-						Name: tc.serviceAccountName,
-					},
-				},
+			tokenGetter := &authentication.TokenGetter{}
+			saMapper := action.ServiceAccountRestConfigMapper(tokenGetter)
+			actualCfg, err := saMapper(context.Background(), tc.obj, tc.cfg)
+			if tc.expectedError != nil {
+				require.Nil(t, actualCfg)
+				require.EqualError(t, err, tc.expectedError.Error())
+			} else {
+				require.NoError(t, err)
+				transport, err := rest.TransportFor(actualCfg)
+				require.NoError(t, err)
+				require.NotNil(t, transport)
+				tokenInjectionRoundTripper, ok := transport.(*authentication.TokenInjectingRoundTripper)
+				require.True(t, ok)
+				require.Equal(t, tokenGetter, tokenInjectionRoundTripper.TokenGetter)
+				require.Equal(t, types.NamespacedName{Name: "my-service-account", Namespace: "my-namespace"}, tokenInjectionRoundTripper.Key)
 			}
-			cfg, err := mapper(context.Background(), ext, &rest.Config{})
-			require.NoError(t, err)
-
-			// The rest config's host field is artificially used to differentiate between the wrappers
-			require.Equal(t, tc.expectedMapper, cfg.Host)
 		})
 	}
 }
+func Test_SyntheticUserRestConfigMapper_UsesDefaultConfigMapper(t *testing.T) {
+	isDefaultRequestMapperUsed := false
+	defaultServiceMapper := func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) {
+		isDefaultRequestMapperUsed = true
+		return c, nil
+	}
+	syntheticAuthServiceMapper := action.SyntheticUserRestConfigMapper(defaultServiceMapper)
+	obj := &ocv1.ClusterExtension{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "my-clusterextension",
+		},
+		Spec: ocv1.ClusterExtensionSpec{
+			ServiceAccount: ocv1.ServiceAccountReference{
+				Name: "my-service-account",
+			},
+			Namespace: "my-namespace",
+		},
+	}
+	actualCfg, err := syntheticAuthServiceMapper(context.Background(), obj, &rest.Config{})
+	require.NoError(t, err)
+	require.NotNil(t, actualCfg)
+	require.True(t, isDefaultRequestMapperUsed)
+}
 
-func TestMapper_SyntheticPermissionsDisabled(t *testing.T) {
-	m := fakeRestConfigWrapper()
-	mapper := m.mapper()
-	ext := &ocv1.ClusterExtension{
+func Test_SyntheticUserRestConfigMapper_UsesSyntheticAuthMapper(t *testing.T) {
+	syntheticAuthServiceMapper := action.SyntheticUserRestConfigMapper(func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) {
+		return c, nil
+	})
+	obj := &ocv1.ClusterExtension{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "my-clusterextension",
+		},
 		Spec: ocv1.ClusterExtensionSpec{
 			ServiceAccount: ocv1.ServiceAccountReference{
 				Name: "olm.synthetic-user",
 			},
+			Namespace: "my-namespace",
 		},
 	}
-	cfg, err := mapper(context.Background(), ext, &rest.Config{})
+	actualCfg, err := syntheticAuthServiceMapper(context.Background(), obj, &rest.Config{})
 	require.NoError(t, err)
+	require.NotNil(t, actualCfg)
+
+	// test that the impersonation headers are appropriately injected into the request
+	// by wrapping a fake round tripper around the returned configurations transport
+	// nolint:bodyclose
+	_, _ = actualCfg.WrapTransport(fakeRoundTripper(func(req *http.Request) (*http.Response, error) {
+		require.Equal(t, "olm:clusterextension:my-clusterextension", req.Header.Get("Impersonate-User"))
+		require.Equal(t, "olm:clusterextensions", req.Header.Get("Impersonate-Group"))
+		return &http.Response{}, nil
+	})).RoundTrip(&http.Request{})
+}
+
+type fakeRoundTripper func(req *http.Request) (*http.Response, error)
 
-	// The rest config's host field is artificially used to differentiate between the wrappers
-	require.Equal(t, saAccountWrapper, cfg.Host)
+func (f fakeRoundTripper) RoundTrip(request *http.Request) (*http.Response, error) {
+	return f(request)
 }