Check for the permissions instead of using a CLI flag. #2588

Signed-off-by: Israel Blancas <[email protected]>

Author: iblancasa

.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: Check the permissions for the SA running the OpenTelemetry Operator instead of using the create-rbac-permissions flag.
+
+# 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:

apis/v1alpha1/collector_webhook.go

@@ -17,10 +17,8 @@ package v1alpha1
 import (
 	"context"
 	"fmt"
-	"strings"
 
 	"github.com/go-logr/logr"
-	v1 "k8s.io/api/authorization/v1"
 	autoscalingv2 "k8s.io/api/autoscaling/v2"
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -378,7 +376,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
 		}
 	}
 
@@ -426,28 +424,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 []*v1.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,

config/manager/manager.yaml

@@ -52,5 +52,10 @@ spec:
           requests:
             cpu: 100m
             memory: 64Mi
+        env:
+          - name: SERVICE_ACCOUNT_NAME
+            valueFrom:
+              fieldRef:
+                fieldPath: spec.serviceAccountName
       serviceAccountName: controller-manager
       terminationGracePeriodSeconds: 10

internal/autodetect/operator.go

@@ -0,0 +1,37 @@
+// Copyright The OpenTelemetry Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package autodetect
+
+import (
+	"fmt"
+	"os"
+)
+
+func GetOperatorNamespace() (string, error) {
+	nsBytes, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace")
+	if err != nil {
+		return "", err
+	}
+	return string(nsBytes), nil
+}
+
+func GetOperatorServiceAccount() (string, error) {
+	saEnvVar := "SERVICE_ACCOUNT_NAME"
+	sa := os.Getenv(saEnvVar)
+	if sa == "" {
+		return sa, fmt.Errorf("%s env variable not found", saEnvVar)
+	}
+	return sa, nil
+}

internal/rbac/format.go

@@ -0,0 +1,44 @@
+// Copyright The OpenTelemetry Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package rbac
+
+import (
+	"fmt"
+	"strings"
+
+	v1 "k8s.io/api/authorization/v1"
+)
+
+// WarningsGroupedByResource is a helper to take the missing permissions and format them as warnings.
+func WarningsGroupedByResource(reviews []*v1.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
+}

main.go

@@ -29,6 +29,7 @@ import (
 	"github.com/spf13/pflag"
 	colfeaturegate "go.opentelemetry.io/collector/featuregate"
 	networkingv1 "k8s.io/api/networking/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
 	k8sruntime "k8s.io/apimachinery/pkg/runtime"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/client-go/kubernetes"
@@ -134,7 +135,7 @@ func main() {
 	pflag.BoolVar(&enableLeaderElection, "enable-leader-election", false,
 		"Enable leader election for controller manager. "+
 			"Enabling this will ensure there is only one active controller manager.")
-	pflag.BoolVar(&createRBACPermissions, "create-rbac-permissions", false, "Automatically create RBAC permissions needed by the processors")
+	pflag.BoolVar(&createRBACPermissions, "create-rbac-permissions", false, "Automatically create RBAC permissions needed by the processors (deprecated)")
 	pflag.BoolVar(&enableMultiInstrumentation, "enable-multi-instrumentation", false, "Controls whether the operator supports multi instrumentation")
 	pflag.BoolVar(&enableApacheHttpdInstrumentation, constants.FlagApacheHttpd, true, "Controls whether the operator supports Apache HTTPD auto-instrumentation")
 	pflag.BoolVar(&enableDotNetInstrumentation, constants.FlagDotNet, true, "Controls whether the operator supports dotnet auto-instrumentation")
@@ -188,54 +189,6 @@ func main() {
 
 	restConfig := ctrl.GetConfigOrDie()
 
-	// builds the operator's configuration
-	ad, err := autodetect.New(restConfig)
-	if err != nil {
-		setupLog.Error(err, "failed to setup auto-detect routine")
-		os.Exit(1)
-	}
-
-	cfg := config.New(
-		config.WithLogger(ctrl.Log.WithName("config")),
-		config.WithVersion(v),
-		config.WithCollectorImage(collectorImage),
-		config.WithCreateRBACPermissions(createRBACPermissions),
-		config.WithEnableMultiInstrumentation(enableMultiInstrumentation),
-		config.WithEnableApacheHttpdInstrumentation(enableApacheHttpdInstrumentation),
-		config.WithEnableDotNetInstrumentation(enableDotNetInstrumentation),
-		config.WithEnableNginxInstrumentation(enableNginxInstrumentation),
-		config.WithEnablePythonInstrumentation(enablePythonInstrumentation),
-		config.WithTargetAllocatorImage(targetAllocatorImage),
-		config.WithOperatorOpAMPBridgeImage(operatorOpAMPBridgeImage),
-		config.WithAutoInstrumentationJavaImage(autoInstrumentationJava),
-		config.WithAutoInstrumentationNodeJSImage(autoInstrumentationNodeJS),
-		config.WithAutoInstrumentationPythonImage(autoInstrumentationPython),
-		config.WithAutoInstrumentationDotNetImage(autoInstrumentationDotNet),
-		config.WithAutoInstrumentationGoImage(autoInstrumentationGo),
-		config.WithAutoInstrumentationApacheHttpdImage(autoInstrumentationApacheHttpd),
-		config.WithAutoInstrumentationNginxImage(autoInstrumentationNginx),
-		config.WithAutoDetect(ad),
-		config.WithLabelFilters(labelsFilter),
-		config.WithAnnotationFilters(annotationsFilter),
-	)
-	err = cfg.AutoDetect()
-	if err != nil {
-		setupLog.Error(err, "failed to autodetect config variables")
-	}
-	// Only add these to the scheme if they are available
-	if cfg.PrometheusCRAvailability() == prometheus.Available {
-		setupLog.Info("Prometheus CRDs are installed, adding to scheme.")
-		utilruntime.Must(monitoringv1.AddToScheme(scheme))
-	} else {
-		setupLog.Info("Prometheus CRDs are not installed, skipping adding to scheme.")
-	}
-	if cfg.OpenShiftRoutesAvailability() == openshift.RoutesAvailable {
-		setupLog.Info("Openshift CRDs are installed, adding to scheme.")
-		utilruntime.Must(routev1.Install(scheme))
-	} else {
-		setupLog.Info("Openshift CRDs are not installed, skipping adding to scheme.")
-	}
-
 	var namespaces map[string]cache.Config
 	watchNamespace, found := os.LookupEnv("WATCH_NAMESPACE")
 	if found {
@@ -284,17 +237,81 @@ func main() {
 		os.Exit(1)
 	}
 
+	clientset, clientErr := kubernetes.NewForConfig(mgr.GetConfig())
+	if err != nil {
+		setupLog.Error(clientErr, "failed to create kubernetes clientset")
+	}
+
+	reviewer := rbac.NewReviewer(clientset)
+	createRBACPermissions = true
 	ctx := ctrl.SetupSignalHandler()
-	err = addDependencies(ctx, mgr, cfg, v)
+	w, err := checkRbacPermissions(reviewer, ctx)
 	if err != nil {
-		setupLog.Error(err, "failed to add/run bootstrap dependencies to the controller manager")
+		createRBACPermissions = false
+		setupLog.Info("the operator has not permissions to create rbac resources", "error", err, "serviceAccount")
+	}
+	if w != nil {
+		createRBACPermissions = false
+		setupLog.Info("the operator has not permissions to create rbac resources", "permissions", w)
+	}
+
+	if createRBACPermissions {
+		setupLog.Info("the operator has permissions to create rbac resources")
+	}
+
+	// builds the operator's configuration
+	ad, err := autodetect.New(restConfig)
+	if err != nil {
+		setupLog.Error(err, "failed to setup auto-detect routine")
 		os.Exit(1)
 	}
-	clientset, clientErr := kubernetes.NewForConfig(mgr.GetConfig())
+
+	cfg := config.New(
+		config.WithLogger(ctrl.Log.WithName("config")),
+		config.WithVersion(v),
+		config.WithCollectorImage(collectorImage),
+		config.WithCreateRBACPermissions(createRBACPermissions),
+		config.WithEnableMultiInstrumentation(enableMultiInstrumentation),
+		config.WithEnableApacheHttpdInstrumentation(enableApacheHttpdInstrumentation),
+		config.WithEnableDotNetInstrumentation(enableDotNetInstrumentation),
+		config.WithEnableNginxInstrumentation(enableNginxInstrumentation),
+		config.WithEnablePythonInstrumentation(enablePythonInstrumentation),
+		config.WithTargetAllocatorImage(targetAllocatorImage),
+		config.WithOperatorOpAMPBridgeImage(operatorOpAMPBridgeImage),
+		config.WithAutoInstrumentationJavaImage(autoInstrumentationJava),
+		config.WithAutoInstrumentationNodeJSImage(autoInstrumentationNodeJS),
+		config.WithAutoInstrumentationPythonImage(autoInstrumentationPython),
+		config.WithAutoInstrumentationDotNetImage(autoInstrumentationDotNet),
+		config.WithAutoInstrumentationGoImage(autoInstrumentationGo),
+		config.WithAutoInstrumentationApacheHttpdImage(autoInstrumentationApacheHttpd),
+		config.WithAutoInstrumentationNginxImage(autoInstrumentationNginx),
+		config.WithAutoDetect(ad),
+		config.WithLabelFilters(labelsFilter),
+		config.WithAnnotationFilters(annotationsFilter),
+	)
+	err = cfg.AutoDetect()
 	if err != nil {
-		setupLog.Error(clientErr, "failed to create kubernetes clientset")
+		setupLog.Error(err, "failed to autodetect config variables")
+	}
+	// Only add these to the scheme if they are available
+	if cfg.PrometheusCRAvailability() == prometheus.Available {
+		setupLog.Info("Prometheus CRDs are installed, adding to scheme.")
+		utilruntime.Must(monitoringv1.AddToScheme(scheme))
+	} else {
+		setupLog.Info("Prometheus CRDs are not installed, skipping adding to scheme.")
+	}
+	if cfg.OpenShiftRoutesAvailability() == openshift.RoutesAvailable {
+		setupLog.Info("Openshift CRDs are installed, adding to scheme.")
+		utilruntime.Must(routev1.Install(scheme))
+	} else {
+		setupLog.Info("Openshift CRDs are not installed, skipping adding to scheme.")
+	}
+
+	err = addDependencies(ctx, mgr, cfg, v)
+	if err != nil {
+		setupLog.Error(err, "failed to add/run bootstrap dependencies to the controller manager")
+		os.Exit(1)
 	}
-	reviewer := rbac.NewReviewer(clientset)
 
 	if err = controllers.NewReconciler(controllers.Params{
 		Client:   mgr.GetClient(),
@@ -410,3 +427,32 @@ func tlsConfigSetting(cfg *tls.Config, tlsOpt tlsConfig) {
 	}
 	cfg.CipherSuites = cipherSuiteIDs
 }
+
+func checkRbacPermissions(reviewer *rbac.Reviewer, ctx context.Context) (admission.Warnings, error) {
+	notPossibleToCheck := "unable to check rbac rules:"
+
+	namespace, err := autodetect.GetOperatorNamespace()
+	if err != nil {
+		return nil, fmt.Errorf("%s: %w", notPossibleToCheck, err)
+	}
+
+	serviceAccount, err := autodetect.GetOperatorServiceAccount()
+	if err != nil {
+		return nil, fmt.Errorf("%s: %w", notPossibleToCheck, err)
+	}
+
+	rules := []*rbacv1.PolicyRule{
+		{
+			APIGroups: []string{"rbac.authorization.k8s.io"},
+			Resources: []string{"clusterrolebindings", "clusterroles"},
+			Verbs:     []string{"create", "delete", "get", "list", "patch", "update"},
+		},
+	}
+
+	if subjectAccessReviews, err := reviewer.CheckPolicyRules(ctx, serviceAccount, namespace, rules...); err != nil {
+		return nil, fmt.Errorf("%s: %w", notPossibleToCheck, err)
+	} else if allowed, deniedReviews := rbac.AllSubjectAccessReviewsAllowed(subjectAccessReviews); !allowed {
+		return rbac.WarningsGroupedByResource(deniedReviews), nil
+	}
+	return nil, nil
+}