From b521f0f664bdcdb9bb6616044e770fc4a088fa2c Mon Sep 17 00:00:00 2001 From: Luca Burgazzoli Date: Fri, 29 Nov 2024 10:47:07 +0100 Subject: [PATCH] Add special handling for Aggregatin ClusterRole Signed-off-by: Luca Burgazzoli --- .../actions/deploy/action_deploy.go | 13 +- .../actions/deploy/action_deploy_test.go | 141 ++++++++++++++++++ 2 files changed, 153 insertions(+), 1 deletion(-) diff --git a/pkg/controller/actions/deploy/action_deploy.go b/pkg/controller/actions/deploy/action_deploy.go index 2728098e8ff..93f77772a5c 100644 --- a/pkg/controller/actions/deploy/action_deploy.go +++ b/pkg/controller/actions/deploy/action_deploy.go @@ -327,7 +327,6 @@ func (a *Action) patch( if err := RemoveDeploymentsResources(obj); err != nil { return nil, fmt.Errorf("failed to apply allow list to Deployment %s/%s: %w", obj.GetNamespace(), obj.GetName(), err) } - default: // do nothing break @@ -392,6 +391,18 @@ func (a *Action) apply( if err := MergeDeployments(old, obj); err != nil { return nil, fmt.Errorf("failed to merge Deployment %s/%s: %w", obj.GetNamespace(), obj.GetName(), err) } + case gvk.ClusterRole: + // For ClusterRole, if AggregationRule is set, then the Rules are controller managed + // and direct changes to Rules will be stomped by the controller. This also happen if + // the rules are set to an empty slice or nil hence we are removing the rules field + // if the ClusterRole is set to be an aggregation role. + _, found, err := unstructured.NestedFieldNoCopy(obj.Object, "aggregationRule") + if err != nil { + return nil, err + } + if found { + unstructured.RemoveNestedField(obj.Object, "rules") + } default: // do nothing break diff --git a/pkg/controller/actions/deploy/action_deploy_test.go b/pkg/controller/actions/deploy/action_deploy_test.go index e441da7559c..fd971ea301a 100644 --- a/pkg/controller/actions/deploy/action_deploy_test.go +++ b/pkg/controller/actions/deploy/action_deploy_test.go @@ -2,29 +2,40 @@ package deploy_test import ( "context" + "path/filepath" "strconv" "strings" "testing" "github.com/blang/semver/v4" + "github.com/onsi/gomega/gstruct" "github.com/operator-framework/api/pkg/lib/version" "github.com/rs/xid" appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + apimachinery "k8s.io/apimachinery/pkg/types" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" componentsv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1" dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/deploy" + odhCli "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/client" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/utils/test/fakeclient" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/utils/test/matchers/jq" + "github.com/opendatahub-io/opendatahub-operator/v2/tests/envtestutil" . "github.com/onsi/gomega" ) @@ -239,3 +250,133 @@ func TestDeployNotOwnedCreate(t *testing.T) { jq.Match(`.spec.strategy.type == "%s"`, appsv1.RollingUpdateDeploymentStrategyType), )) } + +func TestDeployClusterRole(t *testing.T) { + g := NewWithT(t) + s := runtime.NewScheme() + + utilruntime.Must(corev1.AddToScheme(s)) + utilruntime.Must(appsv1.AddToScheme(s)) + utilruntime.Must(apiextensionsv1.AddToScheme(s)) + utilruntime.Must(componentsv1.AddToScheme(s)) + utilruntime.Must(rbacv1.AddToScheme(s)) + + projectDir, err := envtestutil.FindProjectRoot() + g.Expect(err).NotTo(HaveOccurred()) + + envTest := &envtest.Environment{ + CRDInstallOptions: envtest.CRDInstallOptions{ + Scheme: s, + Paths: []string{ + filepath.Join(projectDir, "config", "crd", "bases"), + }, + ErrorIfPathMissing: true, + CleanUpAfterUse: false, + }, + } + + t.Cleanup(func() { + _ = envTest.Stop() + }) + + cfg, err := envTest.Start() + g.Expect(err).NotTo(HaveOccurred()) + + envTestClient, err := client.New(cfg, client.Options{Scheme: s}) + g.Expect(err).NotTo(HaveOccurred()) + + cli, err := odhCli.NewFromConfig(cfg, envTestClient) + g.Expect(err).NotTo(HaveOccurred()) + + t.Run("aggregation", func(t *testing.T) { + ctx := context.Background() + name := xid.New().String() + + deployClusterRoles(t, ctx, cli, rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + Rules: []rbacv1.PolicyRule{{ + Verbs: []string{"*"}, + Resources: []string{"*"}, + APIGroups: []string{"*"}, + }}, + AggregationRule: &rbacv1.AggregationRule{ + ClusterRoleSelectors: []metav1.LabelSelector{{ + MatchLabels: map[string]string{"foo": "bar"}, + }}, + }, + }) + + out := rbacv1.ClusterRole{} + err = cli.Get(ctx, client.ObjectKey{Name: name}, &out) + g.Expect(err).NotTo(HaveOccurred()) + + g.Expect(out).To(gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{ + "Rules": BeEmpty(), + })) + }) + + t.Run("no aggregation", func(t *testing.T) { + ctx := context.Background() + name := xid.New().String() + + deployClusterRoles(t, ctx, cli, rbacv1.ClusterRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + Rules: []rbacv1.PolicyRule{{ + Verbs: []string{"*"}, + Resources: []string{"*"}, + APIGroups: []string{"*"}, + }}, + }) + + out := rbacv1.ClusterRole{} + err = cli.Get(ctx, client.ObjectKey{Name: name}, &out) + g.Expect(err).NotTo(HaveOccurred()) + + g.Expect(out).To(gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{ + "Rules": HaveLen(1), + })) + }) +} + +func deployClusterRoles(t *testing.T, ctx context.Context, cli *odhCli.Client, roles ...rbacv1.ClusterRole) { + t.Helper() + + g := NewWithT(t) + + rr := types.ReconciliationRequest{ + Client: cli, + DSCI: &dsciv1.DSCInitialization{Spec: dsciv1.DSCInitializationSpec{ + ApplicationsNamespace: xid.New().String(), + }}, + DSC: &dscv1.DataScienceCluster{}, + Instance: &componentsv1.Dashboard{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + UID: apimachinery.UID(xid.New().String()), + }, + }, + Release: cluster.Release{ + Name: cluster.OpenDataHub, + Version: version.OperatorVersion{Version: semver.Version{ + Major: 1, Minor: 2, Patch: 3, + }}}, + } + + for i := range roles { + g.Expect( + rr.AddResources(roles[i].DeepCopy()), + ).ShouldNot( + HaveOccurred(), + ) + } + + g.Expect( + deploy.NewAction()(ctx, &rr), + ).ShouldNot( + HaveOccurred(), + ) +}