Check for the permissions instead of using a CLI flag (#2787)

* Check for the permissions instead of using a CLI flag. #2588 Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix bundle Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix log message Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Add E2E tests and fix #2833 Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix tests Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Apply changes requested in code review Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix changelog Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix yaml Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Apply changes requested in code review Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix k8sattributes processor Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix test Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Apply changes requested in code review Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Apply changes requested in code review Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Remove checked error Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Revert change Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix workflow Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix E2E test Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix bundle Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Apply changes requested in code review Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix docs Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fixes #2862 Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix E2E tests Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Apply changes requested in code review Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Remove kustomization Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Update bundle Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix typo Signed-off-by: Israel Blancas <iblancasa@gmail.com> --------- Signed-off-by: Israel Blancas <iblancasa@gmail.com>
open-telemetry · May 6, 2024 · 0e96e1f · 0e96e1f
1 parent d204609
commit 0e96e1f
Show file tree

Hide file tree

Showing 46 changed files with 989 additions and 127 deletions.
diff --git a/.chloggen/2833-fix-detector-resourcedetectionprocessor.yaml b/.chloggen/2833-fix-detector-resourcedetectionprocessor.yaml
@@ -0,0 +1,16 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix
+
+# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action)
+component: collector
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: "Use the k8snode detector instead of kubernetes for the automatic RBAC creation for the resourcedetector"
+
+# One or more tracking issues related to the change
+issues: [2833]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext:
diff --git a/.chloggen/2862-fix-clusterrolebinding-names.yaml b/.chloggen/2862-fix-clusterrolebinding-names.yaml
@@ -0,0 +1,16 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix
+
+# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action)
+component: collector
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: "When two Collectors are created with the same name but different namespaces, the ClusterRoleBinding created by the first will be overriden by the second one."
+
+# One or more tracking issues related to the change
+issues: [2862]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext:
diff --git a/.chloggen/replace-create-rbac-permissions-by-checking-the-sa-permissions.yaml b/.chloggen/replace-create-rbac-permissions-by-checking-the-sa-permissions.yaml
@@ -0,0 +1,16 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: enhancement
+
+# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action)
+component: operator
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Automatically enable RBAC creation if operator SA can create clusterroles and bindings. --create-rbac-permissions flag is noop and deprecated now.
+
+# One or more tracking issues related to the change
+issues: [2588]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext:
diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml
@@ -24,6 +24,7 @@ jobs:
          - "1.29"
        group:
          - e2e
+         - e2e-automatic-rbac
          - e2e-autoscale
          - e2e-instrumentation
          - e2e-opampbridge
@@ -40,7 +41,8 @@ jobs:
            setup: "add-multi-instrumentation-params prepare-e2e"
          - group: e2e-metadata-filters
            setup: "add-operator-arg OPERATOR_ARG='--annotations-filter=*filter.out --labels=*filter.out' prepare-e2e"
-
+         - group: e2e-automatic-rbac
+           setup: "add-rbac-permissions-to-operator prepare-e2e"
     steps:
     - name: Check out code into the Go module directory
       uses: actions/checkout@v4

diff --git a/.gitignore b/.gitignore
@@ -38,6 +38,7 @@ config/manager/kustomization.yaml
 # test resources
 kubeconfig
 tests/_build/
+config/rbac/extra-permissions-operator/
 
 # autoinstrumentation artifacts
 build

diff --git a/Makefile b/Makefile
@@ -157,9 +157,15 @@ add-multi-instrumentation-params:
 add-image-opampbridge:
 	@$(MAKE) add-operator-arg OPERATOR_ARG=--operator-opamp-bridge-image=$(OPERATOROPAMPBRIDGE_IMG)
 
-.PHONY: enable-operator-featuregates
-enable-operator-featuregates: OPERATOR_ARG = --feature-gates=$(FEATUREGATES)
-enable-operator-featuregates: add-operator-arg
+.PHONY: add-rbac-permissions-to-operator
+add-rbac-permissions-to-operator: manifests kustomize
+	# Kustomize only allows patches in the folder where the kustomization is located
+	# This folder is ignored by .gitignore
+	cp -r tests/e2e-automatic-rbac/extra-permissions-operator/ config/rbac/extra-permissions-operator
+	cd config/rbac && $(KUSTOMIZE) edit add patch --kind ClusterRole --name manager-role --path extra-permissions-operator/namespaces.yaml
+	cd config/rbac && $(KUSTOMIZE) edit add patch --kind ClusterRole --name manager-role --path extra-permissions-operator/nodes.yaml
+	cd config/rbac && $(KUSTOMIZE) edit add patch --kind ClusterRole --name manager-role --path extra-permissions-operator/rbac.yaml
+	cd config/rbac && $(KUSTOMIZE) edit add patch --kind ClusterRole --name manager-role --path extra-permissions-operator/replicaset.yaml
 
 # Deploy controller in the current Kubernetes context, configured in ~/.kube/config
 .PHONY: deploy
@@ -217,6 +223,11 @@ generate: controller-gen
 e2e: chainsaw
 	$(CHAINSAW) test --test-dir ./tests/e2e
 
+# end-to-end-test for testing automatic RBAC creation
+.PHONY: e2e-automatic-rbac
+e2e-automatic-rbac: chainsaw
+	$(CHAINSAW) test --test-dir ./tests/e2e-automatic-rbac
+
 # end-to-end-test for testing autoscale
 .PHONY: e2e-autoscale
 e2e-autoscale: chainsaw
@@ -272,9 +283,6 @@ e2e-upgrade: undeploy chainsaw
 .PHONY: prepare-e2e
 prepare-e2e: chainsaw set-image-controller add-image-targetallocator add-image-opampbridge container container-target-allocator container-operator-opamp-bridge start-kind cert-manager install-metrics-server install-targetallocator-prometheus-crds load-image-all deploy
 
-.PHONY: prepare-e2e-with-featuregates
-prepare-e2e-with-featuregates: chainsaw enable-operator-featuregates prepare-e2e
-
 .PHONY: scorecard-tests
 scorecard-tests: operator-sdk
 	$(OPERATOR_SDK) scorecard -w=5m bundle || (echo "scorecard test failed" && exit 1)
@@ -320,10 +328,6 @@ endif
 install-metrics-server:
 	./hack/install-metrics-server.sh
 
-.PHONY: install-prometheus-operator
-install-prometheus-operator:
-	./hack/install-prometheus-operator.sh
-
 # This only installs the CRDs Target Allocator supports
 .PHONY: install-targetallocator-prometheus-crds
 install-targetallocator-prometheus-crds:

diff --git a/apis/v1beta1/collector_webhook.go b/apis/v1beta1/collector_webhook.go
@@ -20,7 +20,6 @@ import (
 	"strings"
 
 	"github.com/go-logr/logr"
-	authorizationv1 "k8s.io/api/authorization/v1"
 	autoscalingv2 "k8s.io/api/autoscaling/v2"
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -359,7 +358,7 @@ func (c CollectorWebhook) validateTargetAllocatorConfig(ctx context.Context, r *
 		if subjectAccessReviews, err := c.reviewer.CheckPolicyRules(ctx, r.GetNamespace(), r.Spec.TargetAllocator.ServiceAccount, targetAllocatorCRPolicyRules...); err != nil {
 			return nil, fmt.Errorf("unable to check rbac rules %w", err)
 		} else if allowed, deniedReviews := rbac.AllSubjectAccessReviewsAllowed(subjectAccessReviews); !allowed {
-			return warningsGroupedByResource(deniedReviews), nil
+			return rbac.WarningsGroupedByResource(deniedReviews), nil
 		}
 	}
 
@@ -407,28 +406,6 @@ func checkAutoscalerSpec(autoscaler *AutoscalerSpec) error {
 	return nil
 }
 
-// warningsGroupedByResource is a helper to take the missing permissions and format them as warnings.
-func warningsGroupedByResource(reviews []*authorizationv1.SubjectAccessReview) []string {
-	fullResourceToVerbs := make(map[string][]string)
-	for _, review := range reviews {
-		if review.Spec.ResourceAttributes != nil {
-			key := fmt.Sprintf("%s/%s", review.Spec.ResourceAttributes.Group, review.Spec.ResourceAttributes.Resource)
-			if len(review.Spec.ResourceAttributes.Group) == 0 {
-				key = review.Spec.ResourceAttributes.Resource
-			}
-			fullResourceToVerbs[key] = append(fullResourceToVerbs[key], review.Spec.ResourceAttributes.Verb)
-		} else if review.Spec.NonResourceAttributes != nil {
-			key := fmt.Sprintf("nonResourceURL: %s", review.Spec.NonResourceAttributes.Path)
-			fullResourceToVerbs[key] = append(fullResourceToVerbs[key], review.Spec.NonResourceAttributes.Verb)
-		}
-	}
-	var warnings []string
-	for fullResource, verbs := range fullResourceToVerbs {
-		warnings = append(warnings, fmt.Sprintf("missing the following rules for %s: [%s]", fullResource, strings.Join(verbs, ",")))
-	}
-	return warnings
-}
-
 func SetupCollectorWebhook(mgr ctrl.Manager, cfg config.Config, reviewer *rbac.Reviewer) error {
 	cvw := &CollectorWebhook{
 		reviewer: reviewer,

diff --git a/bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml b/bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml
@@ -99,7 +99,7 @@ metadata:
     categories: Logging & Tracing,Monitoring
     certified: "false"
     containerImage: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator
-    createdAt: "2024-04-30T12:37:39Z"
+    createdAt: "2024-05-03T15:21:44Z"
     description: Provides the OpenTelemetry components, including the Collector
     operators.operatorframework.io/builder: operator-sdk-v1.29.0
     operators.operatorframework.io/project_layout: go.kubebuilder.io/v3
@@ -499,6 +499,11 @@ spec:
                 - --zap-log-level=info
                 - --zap-time-encoding=rfc3339nano
                 - --enable-nginx-instrumentation=true
+                env:
+                - name: SERVICE_ACCOUNT_NAME
+                  valueFrom:
+                    fieldRef:
+                      fieldPath: spec.serviceAccountName
                 image: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator:0.99.0
                 livenessProbe:
                   httpGet:

diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml
@@ -51,5 +51,10 @@ spec:
           requests:
             cpu: 100m
             memory: 64Mi
+        env:
+          - name: SERVICE_ACCOUNT_NAME
+            valueFrom:
+              fieldRef:
+                fieldPath: spec.serviceAccountName
       serviceAccountName: controller-manager
       terminationGracePeriodSeconds: 10
diff --git a/controllers/opampbridge_controller_test.go b/controllers/opampbridge_controller_test.go
@@ -37,6 +37,7 @@ import (
 	"github.com/open-telemetry/opentelemetry-operator/controllers"
 	"github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift"
 	"github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus"
+	"github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac"
 	"github.com/open-telemetry/opentelemetry-operator/internal/config"
 )
 
@@ -48,6 +49,9 @@ var opampBridgeMockAutoDetector = &mockAutoDetect{
 	PrometheusCRsAvailabilityFunc: func() (prometheus.Availability, error) {
 		return prometheus.Available, nil
 	},
+	RBACPermissionsFunc: func(ctx context.Context) (rbac.Availability, error) {
+		return rbac.Available, nil
+	},
 }
 
 func TestNewObjectsOnReconciliation_OpAMPBridge(t *testing.T) {

diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go
@@ -39,6 +39,7 @@ import (
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1beta1"
 	"github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift"
 	"github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus"
+	"github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac"
 	"github.com/open-telemetry/opentelemetry-operator/internal/config"
 	"github.com/open-telemetry/opentelemetry-operator/internal/manifests"
 	"github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector"
@@ -239,7 +240,7 @@ func (r *OpenTelemetryCollectorReconciler) SetupWithManager(mgr ctrl.Manager) er
 		Owns(&autoscalingv2.HorizontalPodAutoscaler{}).
 		Owns(&policyV1.PodDisruptionBudget{})
 
-	if r.config.CreateRBACPermissions() {
+	if r.config.CreateRBACPermissions() == rbac.Available {
 		builder.Owns(&rbacv1.ClusterRoleBinding{})
 		builder.Owns(&rbacv1.ClusterRole{})
 	}

diff --git a/controllers/suite_test.go b/controllers/suite_test.go
@@ -56,6 +56,7 @@ import (
 	"github.com/open-telemetry/opentelemetry-operator/internal/autodetect"
 	"github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift"
 	"github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus"
+	autoRBAC "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac"
 	"github.com/open-telemetry/opentelemetry-operator/internal/config"
 	"github.com/open-telemetry/opentelemetry-operator/internal/manifests"
 	"github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/testdata"
@@ -95,6 +96,7 @@ var _ autodetect.AutoDetect = (*mockAutoDetect)(nil)
 type mockAutoDetect struct {
 	OpenShiftRoutesAvailabilityFunc func() (openshift.RoutesAvailability, error)
 	PrometheusCRsAvailabilityFunc   func() (prometheus.Availability, error)
+	RBACPermissionsFunc             func(ctx context.Context) (autoRBAC.Availability, error)
 }
 
 func (m *mockAutoDetect) PrometheusCRsAvailability() (prometheus.Availability, error) {
@@ -111,6 +113,13 @@ func (m *mockAutoDetect) OpenShiftRoutesAvailability() (openshift.RoutesAvailabi
 	return openshift.RoutesNotAvailable, nil
 }
 
+func (m *mockAutoDetect) RBACPermissions(ctx context.Context) (autoRBAC.Availability, error) {
+	if m.RBACPermissionsFunc != nil {
+		return m.RBACPermissionsFunc(ctx)
+	}
+	return autoRBAC.NotAvailable, nil
+}
+
 func TestMain(m *testing.M) {
 	ctx, cancel = context.WithCancel(context.TODO())
 	defer cancel()

diff --git a/internal/autodetect/main.go b/internal/autodetect/main.go
@@ -16,11 +16,16 @@
 package autodetect
 
 import (
+	"context"
+	"fmt"
+
 	"k8s.io/client-go/discovery"
 	"k8s.io/client-go/rest"
 
 	"github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift"
 	"github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus"
+	autoRBAC "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac"
+	"github.com/open-telemetry/opentelemetry-operator/internal/rbac"
 )
 
 var _ AutoDetect = (*autoDetect)(nil)
@@ -29,14 +34,16 @@ var _ AutoDetect = (*autoDetect)(nil)
 type AutoDetect interface {
 	OpenShiftRoutesAvailability() (openshift.RoutesAvailability, error)
 	PrometheusCRsAvailability() (prometheus.Availability, error)
+	RBACPermissions(ctx context.Context) (autoRBAC.Availability, error)
 }
 
 type autoDetect struct {
-	dcl discovery.DiscoveryInterface
+	dcl      discovery.DiscoveryInterface
+	reviewer *rbac.Reviewer
 }
 
 // New creates a new auto-detection worker, using the given client when talking to the current cluster.
-func New(restConfig *rest.Config) (AutoDetect, error) {
+func New(restConfig *rest.Config, reviewer *rbac.Reviewer) (AutoDetect, error) {
 	dcl, err := discovery.NewDiscoveryClientForConfig(restConfig)
 	if err != nil {
 		// it's pretty much impossible to get into this problem, as most of the
@@ -46,7 +53,8 @@ func New(restConfig *rest.Config) (AutoDetect, error) {
 	}
 
 	return &autoDetect{
-		dcl: dcl,
+		dcl:      dcl,
+		reviewer: reviewer,
 	}, nil
 }
 
@@ -83,3 +91,15 @@ func (a *autoDetect) OpenShiftRoutesAvailability() (openshift.RoutesAvailability
 
 	return openshift.RoutesNotAvailable, nil
 }
+
+func (a *autoDetect) RBACPermissions(ctx context.Context) (autoRBAC.Availability, error) {
+	w, err := autoRBAC.CheckRBACPermissions(ctx, a.reviewer)
+	if err != nil {
+		return autoRBAC.NotAvailable, err
+	}
+	if w != nil {
+		return autoRBAC.NotAvailable, fmt.Errorf("missing permissions: %s", w)
+	}
+
+	return autoRBAC.Available, nil
+}