Skip to content

✨ Add ServiceAccount validation to ClusterExtension reconciliation #2488

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
perdasilva wants to merge 1 commit into
base: main
Choose a base branch
from
Open

✨ Add ServiceAccount validation to ClusterExtension reconciliation #2488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions cmd/operator-controller/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,8 +625,12 @@ func (c *boxcutterReconcilerConfigurator) Configure(ceReconciler *controllers.Cl
ActionClientGetter: acg,
RevisionGenerator: rg,
}
tokenGetter := authentication.NewTokenGetter(coreClient, authentication.WithExpirationDuration(1*time.Hour))
ceReconciler.ReconcileSteps = []controllers.ReconcileStepFunc{
controllers.HandleFinalizers(c.finalizers),
controllers.ValidateClusterExtension(
controllers.ServiceAccountValidator(tokenGetter),
),
controllers.MigrateStorage(storageMigrator),
controllers.RetrieveRevisionStates(revisionStatesGetter),
controllers.ResolveBundle(c.resolver, c.mgr.GetClient()),
Expand Down Expand Up @@ -656,20 +660,14 @@ func (c *boxcutterReconcilerConfigurator) Configure(ceReconciler *controllers.Cl
return fmt.Errorf("unable to add tracking cache to manager: %v", err)
}

cerCoreClient, err := corev1client.NewForConfig(c.mgr.GetConfig())
if err != nil {
return fmt.Errorf("unable to create client for ClusterExtensionRevision controller: %w", err)
}
cerTokenGetter := authentication.NewTokenGetter(cerCoreClient, authentication.WithExpirationDuration(1*time.Hour))

revisionEngineFactory, err := controllers.NewDefaultRevisionEngineFactory(
c.mgr.GetScheme(),
trackingCache,
discoveryClient,
c.mgr.GetRESTMapper(),
fieldOwnerPrefix,
c.mgr.GetConfig(),
cerTokenGetter,
tokenGetter,
)
if err != nil {
return fmt.Errorf("unable to create revision engine factory: %w", err)
Expand Down Expand Up @@ -747,6 +745,9 @@ func (c *helmReconcilerConfigurator) Configure(ceReconciler *controllers.Cluster
revisionStatesGetter := &controllers.HelmRevisionStatesGetter{ActionClientGetter: acg}
ceReconciler.ReconcileSteps = []controllers.ReconcileStepFunc{
controllers.HandleFinalizers(c.finalizers),
controllers.ValidateClusterExtension(
controllers.ServiceAccountValidator(tokenGetter),
),
controllers.RetrieveRevisionStates(revisionStatesGetter),
controllers.ResolveBundle(c.resolver, c.mgr.GetClient()),
controllers.UnpackBundle(c.imagePuller, c.imageCache),
Expand Down
240 changes: 196 additions & 44 deletions internal/operator-controller/controllers/clusterextension_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"testing"
"testing/fstest"
"time"

bsemver "github.com/blang/semver/v4"
"github.com/google/go-cmp/cmp"
Expand All @@ -15,11 +16,16 @@ import (
"helm.sh/helm/v3/pkg/chart"
"helm.sh/helm/v3/pkg/release"
"helm.sh/helm/v3/pkg/storage/driver"
authenticationv1 "k8s.io/api/authentication/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/equality"
apimeta "k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/rand"
"k8s.io/client-go/kubernetes/fake"
clientgotesting "k8s.io/client-go/testing"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer"
Expand Down Expand Up @@ -767,60 +773,177 @@ func TestClusterExtensionBoxcutterApplierFailsDoesNotLeakDeprecationErrors(t *te
require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{}))
}

func TestClusterExtensionServiceAccountNotFound(t *testing.T) {
cl, reconciler := newClientAndReconciler(t, func(d *deps) {
d.RevisionStatesGetter = &MockRevisionStatesGetter{
Err: &authentication.ServiceAccountNotFoundError{
ServiceAccountName: "missing-sa",
ServiceAccountNamespace: "default",
}}
})

ctx := context.Background()
extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))}

t.Log("Given a cluster extension with a missing service account")
clusterExtension := &ocv1.ClusterExtension{
ObjectMeta: metav1.ObjectMeta{Name: extKey.Name},
Spec: ocv1.ClusterExtensionSpec{
Source: ocv1.SourceConfig{
SourceType: "Catalog",
Catalog: &ocv1.CatalogFilter{
PackageName: "test-package",
func TestValidateClusterExtension(t *testing.T) {
tests := []struct {
name string
validators []controllers.ClusterExtensionValidator
expectError bool
errorMessageIncludes string
}{
{
name: "all validators pass",
validators: []controllers.ClusterExtensionValidator{
// Validator that always passes
func(_ context.Context, _ *ocv1.ClusterExtension) error {
return nil
},
},
Namespace: "default",
ServiceAccount: ocv1.ServiceAccountReference{
Name: "missing-sa",
expectError: false,
},
{
name: "validator fails - sets Progressing condition",
validators: []controllers.ClusterExtensionValidator{
func(_ context.Context, _ *ocv1.ClusterExtension) error {
return errors.New("generic validation error")
},
},
expectError: true,
errorMessageIncludes: "generic validation error",
},
{
name: "multiple validators - collects all failures",
validators: []controllers.ClusterExtensionValidator{
// First validator fails
func(_ context.Context, _ *ocv1.ClusterExtension) error {
return errors.New("first validator failed")
},
// Second validator also fails
func(_ context.Context, _ *ocv1.ClusterExtension) error {
return errors.New("second validator failed")
},
// Third validator fails too
func(_ context.Context, _ *ocv1.ClusterExtension) error {
return errors.New("third validator failed")
},
},
expectError: true,
errorMessageIncludes: "first validator failed\nsecond validator failed\nthird validator failed",
},
{
name: "multiple validators - all pass",
validators: []controllers.ClusterExtensionValidator{
func(_ context.Context, _ *ocv1.ClusterExtension) error {
return nil
},
func(_ context.Context, _ *ocv1.ClusterExtension) error {
return nil
},
func(_ context.Context, _ *ocv1.ClusterExtension) error {
return nil
},
},
expectError: false,
},
{
name: "multiple validators - some pass, some fail",
validators: []controllers.ClusterExtensionValidator{
// First validator passes
func(_ context.Context, _ *ocv1.ClusterExtension) error {
return nil
},
// Second validator fails
func(_ context.Context, _ *ocv1.ClusterExtension) error {
return errors.New("validation error 1")
},
// Third validator passes
func(_ context.Context, _ *ocv1.ClusterExtension) error {
return nil
},
// Fourth validator fails
func(_ context.Context, _ *ocv1.ClusterExtension) error {
return errors.New("validation error 2")
},
},
expectError: true,
errorMessageIncludes: "validation error 1\nvalidation error 2",
},
{
name: "service account not found",
validators: []controllers.ClusterExtensionValidator{
serviceAccountValidatorWithFakeClient(&corev1.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{
Name: "missing-sa",
Namespace: "test-ns",
},
}),
},
expectError: true,
errorMessageIncludes: `service account "test-sa" not found in namespace "test-ns"`,
},
{
name: "service account found",
validators: []controllers.ClusterExtensionValidator{
serviceAccountValidatorWithFakeClient(&corev1.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{
Name: "test-sa",
Namespace: "test-ns",
},
}),
},
expectError: false,
},
}

require.NoError(t, cl.Create(ctx, clusterExtension))
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ctx := context.Background()

t.Log("When reconciling the cluster extension")
res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey})
cl, reconciler := newClientAndReconciler(t, func(d *deps) {
d.RevisionStatesGetter = &MockRevisionStatesGetter{
RevisionStates: &controllers.RevisionStates{},
}
d.Validators = tt.validators
})

require.Equal(t, ctrl.Result{}, res)
require.Error(t, err)
var saErr *authentication.ServiceAccountNotFoundError
require.ErrorAs(t, err, &saErr)
t.Log("By fetching updated cluster extension after reconcile")
require.NoError(t, cl.Get(ctx, extKey, clusterExtension))
extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))}

t.Log("By checking the status conditions")
installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled)
require.NotNil(t, installedCond)
require.Equal(t, metav1.ConditionUnknown, installedCond.Status)
require.Contains(t, installedCond.Message, fmt.Sprintf("service account %q not found in namespace %q: unable to authenticate with the Kubernetes cluster.",
"missing-sa", "default"))
clusterExtension := &ocv1.ClusterExtension{
ObjectMeta: metav1.ObjectMeta{Name: extKey.Name},
Spec: ocv1.ClusterExtensionSpec{
Source: ocv1.SourceConfig{
SourceType: "Catalog",
Catalog: &ocv1.CatalogFilter{
PackageName: "test-package",
},
},
Namespace: "test-ns",
ServiceAccount: ocv1.ServiceAccountReference{
Name: "test-sa",
},
},
}

progressingCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeProgressing)
require.NotNil(t, progressingCond)
require.Equal(t, metav1.ConditionTrue, progressingCond.Status)
require.Equal(t, ocv1.ReasonRetrying, progressingCond.Reason)
require.Contains(t, progressingCond.Message, "installation cannot proceed due to missing ServiceAccount")
require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{}))
require.NoError(t, cl.Create(ctx, clusterExtension))

t.Log("When reconciling the cluster extension")
res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey})
require.Equal(t, ctrl.Result{}, res)
if tt.expectError {
require.Error(t, err)
t.Log("By fetching updated cluster extension after reconcile")
require.NoError(t, cl.Get(ctx, extKey, clusterExtension))

t.Log("By checking the status conditions")
installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled)
require.NotNil(t, installedCond)
require.Equal(t, metav1.ConditionUnknown, installedCond.Status)
require.Contains(t, installedCond.Message, "installation cannot proceed due to the following validation error(s)")
require.Contains(t, installedCond.Message, tt.errorMessageIncludes)

progressingCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeProgressing)
require.NotNil(t, progressingCond)
require.Equal(t, metav1.ConditionTrue, progressingCond.Status)
require.Equal(t, ocv1.ReasonRetrying, progressingCond.Reason)
require.Contains(t, progressingCond.Message, "installation cannot proceed due to the following validation error(s)")
require.Contains(t, progressingCond.Message, tt.errorMessageIncludes)
require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{}))
} else {
require.NoError(t, err)
require.NoError(t, cl.Get(ctx, extKey, clusterExtension))
require.Empty(t, clusterExtension.Status.Conditions)
}
})
}
}

func TestClusterExtensionApplierFailsWithBundleInstalled(t *testing.T) {
Expand Down Expand Up @@ -2878,3 +3001,32 @@ func TestCheckCatalogsExist(t *testing.T) {
require.False(t, exists)
})
}

func serviceAccountValidatorWithFakeClient(serviceAccount *corev1.ServiceAccount) controllers.ClusterExtensionValidator {
if serviceAccount == nil {
return controllers.ServiceAccountValidator(authentication.NewTokenGetter(fake.NewClientset().CoreV1()))
}
fakeClientset := fake.NewClientset(serviceAccount)
fakeClientset.PrependReactor("create", "serviceaccounts", func(action clientgotesting.Action) (bool, runtime.Object, error) {
if action.GetSubresource() != "token" || action.GetNamespace() != serviceAccount.Namespace {
// Let other reactors handle standard SA creates
return false, nil, nil
}
createAction, ok := action.(clientgotesting.CreateActionImpl)
if !ok {
return false, nil, fmt.Errorf("unexpected action: expected CreateActionImpl but got %#v", action)
}
if createAction.GetNamespace() != serviceAccount.Namespace || createAction.Name != serviceAccount.Name {
return false, nil, nil
}
tr := &authenticationv1.TokenRequest{
Status: authenticationv1.TokenRequestStatus{
Token: "fake-jwt-token-string",
ExpirationTimestamp: metav1.NewTime(time.Now().Add(1 * time.Hour)),
},
}
return true, tr, nil
})
tokenGetter := authentication.NewTokenGetter(fakeClientset.CoreV1())
return controllers.ServiceAccountValidator(tokenGetter)
}
Loading
Loading