Add new operator capability to check if it has access to do an operation (open-telemetry#2467)

* rbac pr testing

* makefile convenience

* Add test

* add chlog

* Add a comment

* update to allow for policy rule checking

* change package

* lint fail

* better formatting

* don't use leading slash for empty group

* add more detail for comment

Author: jaronoff97

.chloggen/mandate-rbac.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. operator, target allocator, github action)
+component: operator
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: enables the operator to create subject access reviews for different required permissions.
+
+# One or more tracking issues related to the change
+issues: [2426]
+
+# (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:

Makefile

@@ -97,6 +97,11 @@ all: manager
 .PHONY: ci
 ci: test
 
+# helper to get your gomod up-to-date
+.PHONY: gomod
+gomod:
+	go mod tidy && go mod vendor && (cd cmd/otel-allocator && go mod tidy) && (cd cmd/operator-opamp-bridge && go mod tidy)
+
 # Run tests
 # setup-envtest uses KUBEBUILDER_ASSETS which points to a directory with binaries (api-server, etcd and kubectl)
 .PHONY: test

apis/v1alpha1/collector_webhook.go

@@ -17,9 +17,12 @@ 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"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/validation"
@@ -28,12 +31,41 @@ import (
 
 	"github.com/open-telemetry/opentelemetry-operator/internal/config"
 	ta "github.com/open-telemetry/opentelemetry-operator/internal/manifests/targetallocator/adapters"
+	"github.com/open-telemetry/opentelemetry-operator/internal/rbac"
 	"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
 )
 
 var (
 	_ admission.CustomValidator = &CollectorWebhook{}
 	_ admission.CustomDefaulter = &CollectorWebhook{}
+
+	// targetAllocatorCRPolicyRules are the policy rules required for the CR functionality.
+	targetAllocatorCRPolicyRules = []*rbacv1.PolicyRule{
+		{
+			APIGroups: []string{"monitoring.coreos.com"},
+			Resources: []string{"servicemonitors", "podmonitors"},
+			Verbs:     []string{"*"},
+		}, {
+			APIGroups: []string{""},
+			Resources: []string{"nodes", "nodes/metrics", "services", "endpoints", "pods"},
+			Verbs:     []string{"get", "list", "watch"},
+		}, {
+			APIGroups: []string{""},
+			Resources: []string{"configmaps"},
+			Verbs:     []string{"get"},
+		}, {
+			APIGroups: []string{"discovery.k8s.io"},
+			Resources: []string{"endpointslices"},
+			Verbs:     []string{"get", "list", "watch"},
+		}, {
+			APIGroups: []string{"networking.k8s.io"},
+			Resources: []string{"ingresses"},
+			Verbs:     []string{"get", "list", "watch"},
+		}, {
+			NonResourceURLs: []string{"/metrics"},
+			Verbs:           []string{"get"},
+		},
+	}
 )
 
 // +kubebuilder:webhook:path=/mutate-opentelemetry-io-v1alpha1-opentelemetrycollector,mutating=true,failurePolicy=fail,groups=opentelemetry.io,resources=opentelemetrycollectors,verbs=create;update,versions=v1alpha1,name=mopentelemetrycollector.kb.io,sideEffects=none,admissionReviewVersions=v1
@@ -42,9 +74,10 @@ var (
 // +kubebuilder:object:generate=false
 
 type CollectorWebhook struct {
-	logger logr.Logger
-	cfg    config.Config
-	scheme *runtime.Scheme
+	logger   logr.Logger
+	cfg      config.Config
+	scheme   *runtime.Scheme
+	reviewer *rbac.Reviewer
 }
 
 func (c CollectorWebhook) Default(ctx context.Context, obj runtime.Object) error {
@@ -60,23 +93,23 @@ func (c CollectorWebhook) ValidateCreate(ctx context.Context, obj runtime.Object
 	if !ok {
 		return nil, fmt.Errorf("expected an OpenTelemetryCollector, received %T", obj)
 	}
-	return c.validate(otelcol)
+	return c.validate(ctx, otelcol)
 }
 
 func (c CollectorWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
 	otelcol, ok := newObj.(*OpenTelemetryCollector)
 	if !ok {
 		return nil, fmt.Errorf("expected an OpenTelemetryCollector, received %T", newObj)
 	}
-	return c.validate(otelcol)
+	return c.validate(ctx, otelcol)
 }
 
 func (c CollectorWebhook) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
 	otelcol, ok := obj.(*OpenTelemetryCollector)
 	if !ok || otelcol == nil {
 		return nil, fmt.Errorf("expected an OpenTelemetryCollector, received %T", obj)
 	}
-	return c.validate(otelcol)
+	return c.validate(ctx, otelcol)
 }
 
 func (c CollectorWebhook) defaulter(r *OpenTelemetryCollector) error {
@@ -169,7 +202,7 @@ func (c CollectorWebhook) defaulter(r *OpenTelemetryCollector) error {
 	return nil
 }
 
-func (c CollectorWebhook) validate(r *OpenTelemetryCollector) (admission.Warnings, error) {
+func (c CollectorWebhook) validate(ctx context.Context, r *OpenTelemetryCollector) (admission.Warnings, error) {
 	warnings := admission.Warnings{}
 	// validate volumeClaimTemplates
 	if r.Spec.Mode != ModeStatefulSet && len(r.Spec.VolumeClaimTemplates) > 0 {
@@ -214,6 +247,14 @@ func (c CollectorWebhook) validate(r *OpenTelemetryCollector) (admission.Warning
 		if err != nil {
 			return warnings, fmt.Errorf("the OpenTelemetry Spec Prometheus configuration is incorrect, %w", err)
 		}
+		// if the prometheusCR is enabled, it needs a suite of permissions to function
+		if r.Spec.TargetAllocator.PrometheusCR.Enabled {
+			if subjectAccessReviews, err := c.reviewer.CheckPolicyRules(ctx, r.GetNamespace(), r.Spec.TargetAllocator.ServiceAccount, targetAllocatorCRPolicyRules...); err != nil {
+				return warnings, fmt.Errorf("unable to check rbac rules %w", err)
+			} else if allowed, deniedReviews := rbac.AllSubjectAccessReviewsAllowed(subjectAccessReviews); !allowed {
+				warnings = append(warnings, warningsGroupedByResource(deniedReviews)...)
+			}
+		}
 	}
 
 	// validator port config
@@ -360,11 +401,34 @@ func checkAutoscalerSpec(autoscaler *AutoscalerSpec) error {
 	return nil
 }
 
-func SetupCollectorWebhook(mgr ctrl.Manager, cfg config.Config) error {
+// 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{
-		logger: mgr.GetLogger().WithValues("handler", "CollectorWebhook"),
-		scheme: mgr.GetScheme(),
-		cfg:    cfg,
+		reviewer: reviewer,
+		logger:   mgr.GetLogger().WithValues("handler", "CollectorWebhook"),
+		scheme:   mgr.GetScheme(),
+		cfg:      cfg,
 	}
 	return ctrl.NewWebhookManagedBy(mgr).
 		For(&OpenTelemetryCollector{}).

apis/v1alpha1/collector_webhook_test.go

@@ -23,15 +23,19 @@ import (
 	"github.com/go-logr/logr"
 	"github.com/stretchr/testify/assert"
 	appsv1 "k8s.io/api/apps/v1"
+	authv1 "k8s.io/api/authorization/v1"
 	autoscalingv2 "k8s.io/api/autoscaling/v2"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/client-go/kubernetes/fake"
 	"k8s.io/client-go/kubernetes/scheme"
+	kubeTesting "k8s.io/client-go/testing"
 
 	"github.com/open-telemetry/opentelemetry-operator/internal/config"
+	"github.com/open-telemetry/opentelemetry-operator/internal/rbac"
 )
 
 var (
@@ -438,6 +442,7 @@ func TestOTELColValidatingWebhook(t *testing.T) {
 		otelcol          OpenTelemetryCollector
 		expectedErr      string
 		expectedWarnings []string
+		shouldFailSar    bool
 	}{
 		{
 			name:    "valid empty spec",
@@ -469,6 +474,131 @@ func TestOTELColValidatingWebhook(t *testing.T) {
     protocols:
       thrift_http:
         endpoint: 0.0.0.0:15268
+`,
+					Ports: []v1.ServicePort{
+						{
+							Name: "port1",
+							Port: 5555,
+						},
+						{
+							Name:     "port2",
+							Port:     5554,
+							Protocol: v1.ProtocolUDP,
+						},
+					},
+					Autoscaler: &AutoscalerSpec{
+						Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{
+							ScaleDown: &autoscalingv2.HPAScalingRules{
+								StabilizationWindowSeconds: &three,
+							},
+							ScaleUp: &autoscalingv2.HPAScalingRules{
+								StabilizationWindowSeconds: &five,
+							},
+						},
+						TargetCPUUtilization: &five,
+					},
+				},
+			},
+			expectedWarnings: []string{
+				"MaxReplicas is deprecated",
+				"MinReplicas is deprecated",
+			},
+		},
+		{
+			name:          "prom CR admissions warning",
+			shouldFailSar: true, // force failure
+			otelcol: OpenTelemetryCollector{
+				Spec: OpenTelemetryCollectorSpec{
+					Mode:            ModeStatefulSet,
+					MinReplicas:     &one,
+					Replicas:        &three,
+					MaxReplicas:     &five,
+					UpgradeStrategy: "adhoc",
+					TargetAllocator: OpenTelemetryTargetAllocator{
+						Enabled:      true,
+						PrometheusCR: OpenTelemetryTargetAllocatorPrometheusCR{Enabled: true},
+					},
+					Config: `receivers:
+  examplereceiver:
+    endpoint: "0.0.0.0:12345"
+  examplereceiver/settings:
+    endpoint: "0.0.0.0:12346"
+  prometheus:
+    config:
+      scrape_configs:
+        - job_name: otel-collector
+          scrape_interval: 10s
+  jaeger/custom:
+    protocols:
+      thrift_http:
+        endpoint: 0.0.0.0:15268
+`,
+					Ports: []v1.ServicePort{
+						{
+							Name: "port1",
+							Port: 5555,
+						},
+						{
+							Name:     "port2",
+							Port:     5554,
+							Protocol: v1.ProtocolUDP,
+						},
+					},
+					Autoscaler: &AutoscalerSpec{
+						Behavior: &autoscalingv2.HorizontalPodAutoscalerBehavior{
+							ScaleDown: &autoscalingv2.HPAScalingRules{
+								StabilizationWindowSeconds: &three,
+							},
+							ScaleUp: &autoscalingv2.HPAScalingRules{
+								StabilizationWindowSeconds: &five,
+							},
+						},
+						TargetCPUUtilization: &five,
+					},
+				},
+			},
+			expectedWarnings: []string{
+				"missing the following rules for monitoring.coreos.com/servicemonitors: [*]",
+				"missing the following rules for monitoring.coreos.com/podmonitors: [*]",
+				"missing the following rules for nodes/metrics: [get,list,watch]",
+				"missing the following rules for services: [get,list,watch]",
+				"missing the following rules for endpoints: [get,list,watch]",
+				"missing the following rules for networking.k8s.io/ingresses: [get,list,watch]",
+				"missing the following rules for nodes: [get,list,watch]",
+				"missing the following rules for pods: [get,list,watch]",
+				"missing the following rules for configmaps: [get]",
+				"missing the following rules for discovery.k8s.io/endpointslices: [get,list,watch]",
+				"missing the following rules for nonResourceURL: /metrics: [get]",
+				"MaxReplicas is deprecated",
+				"MinReplicas is deprecated",
+			},
+		},
+		{
+			name:          "prom CR no admissions warning",
+			shouldFailSar: false, // force SAR okay
+			otelcol: OpenTelemetryCollector{
+				Spec: OpenTelemetryCollectorSpec{
+					Mode:            ModeStatefulSet,
+					Replicas:        &three,
+					UpgradeStrategy: "adhoc",
+					TargetAllocator: OpenTelemetryTargetAllocator{
+						Enabled:      true,
+						PrometheusCR: OpenTelemetryTargetAllocatorPrometheusCR{Enabled: true},
+					},
+					Config: `receivers:
+  examplereceiver:
+    endpoint: "0.0.0.0:12345"
+  examplereceiver/settings:
+    endpoint: "0.0.0.0:12346"
+  prometheus:
+    config:
+      scrape_configs:
+        - job_name: otel-collector
+          scrape_interval: 10s
+  jaeger/custom:
+    protocols:
+      thrift_http:
+        endpoint: 0.0.0.0:15268
 `,
 					Ports: []v1.ServicePort{
 						{
@@ -923,19 +1053,37 @@ func TestOTELColValidatingWebhook(t *testing.T) {
 					config.WithCollectorImage("collector:v0.0.0"),
 					config.WithTargetAllocatorImage("ta:v0.0.0"),
 				),
+				reviewer: getReviewer(test.shouldFailSar),
 			}
 			ctx := context.Background()
 			warnings, err := cvw.ValidateCreate(ctx, &test.otelcol)
 			if test.expectedErr == "" {
 				assert.NoError(t, err)
-				return
-			}
-			if len(test.expectedWarnings) == 0 {
-				assert.Empty(t, warnings, test.expectedWarnings)
 			} else {
-				assert.ElementsMatch(t, warnings, test.expectedWarnings)
+				assert.ErrorContains(t, err, test.expectedErr)
 			}
-			assert.ErrorContains(t, err, test.expectedErr)
+			assert.Equal(t, len(test.expectedWarnings), len(warnings))
+			assert.ElementsMatch(t, warnings, test.expectedWarnings)
 		})
 	}
 }
+
+func getReviewer(shouldFailSAR bool) *rbac.Reviewer {
+	c := fake.NewSimpleClientset()
+	c.PrependReactor("create", "subjectaccessreviews", func(action kubeTesting.Action) (handled bool, ret runtime.Object, err error) {
+		// check our expectation here
+		if !action.Matches("create", "subjectaccessreviews") {
+			return false, nil, fmt.Errorf("must be a create for a SAR")
+		}
+		sar, ok := action.(kubeTesting.CreateAction).GetObject().DeepCopyObject().(*authv1.SubjectAccessReview)
+		if !ok || sar == nil {
+			return false, nil, fmt.Errorf("bad object")
+		}
+		sar.Status = authv1.SubjectAccessReviewStatus{
+			Allowed: !shouldFailSAR,
+			Denied:  shouldFailSAR,
+		}
+		return true, sar, nil
+	})
+	return rbac.NewReviewer(c)
+}