Permission check fixed for the serviceaccount of the target allocator (…

…open-telemetry#3391)

* Permission check fixed for the serviceaccount of the target allocator

* serviceaccount name included in warning message and unit tests are adjusted

.chloggen/3380-ta-serviceaccount-check.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: target allocator
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: "Permission check fixed for the serviceaccount of the target allocator"
+
+# One or more tracking issues related to the change
+issues: [3380]
+
+# (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/targetallocator_webhook.go

@@ -26,6 +26,7 @@ import (
 
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1beta1"
 	"github.com/open-telemetry/opentelemetry-operator/internal/config"
+	"github.com/open-telemetry/opentelemetry-operator/internal/naming"
 	"github.com/open-telemetry/opentelemetry-operator/internal/rbac"
 )
 
@@ -119,7 +120,11 @@ func (w TargetAllocatorWebhook) validate(ctx context.Context, ta *TargetAllocato
 
 	// if the prometheusCR is enabled, it needs a suite of permissions to function
 	if ta.Spec.PrometheusCR.Enabled {
-		warnings, err := v1beta1.CheckTargetAllocatorPrometheusCRPolicyRules(ctx, w.reviewer, ta.Spec.ServiceAccount, ta.GetNamespace())
+		saname := ta.Spec.ServiceAccount
+		if len(ta.Spec.ServiceAccount) == 0 {
+			saname = naming.TargetAllocatorServiceAccount(ta.Name)
+		}
+		warnings, err := v1beta1.CheckTargetAllocatorPrometheusCRPolicyRules(ctx, w.reviewer, ta.GetNamespace(), saname)
 		if err != nil || len(warnings) > 0 {
 			return warnings, err
 		}

apis/v1alpha1/targetallocator_webhook_test.go

@@ -224,25 +224,29 @@ func TestTargetAllocatorValidatingWebhook(t *testing.T) {
 			name:          "prom CR admissions warning",
 			shouldFailSar: true, // force failure
 			targetallocator: TargetAllocator{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-ta",
+					Namespace: "test-ns",
+				},
 				Spec: TargetAllocatorSpec{
 					PrometheusCR: v1beta1.TargetAllocatorPrometheusCR{
 						Enabled: true,
 					},
 				},
 			},
 			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 namespaces: [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]",
+				"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - monitoring.coreos.com/servicemonitors: [*]",
+				"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - monitoring.coreos.com/podmonitors: [*]",
+				"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - nodes/metrics: [get,list,watch]",
+				"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - services: [get,list,watch]",
+				"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - endpoints: [get,list,watch]",
+				"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - namespaces: [get,list,watch]",
+				"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - networking.k8s.io/ingresses: [get,list,watch]",
+				"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - nodes: [get,list,watch]",
+				"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - pods: [get,list,watch]",
+				"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - configmaps: [get]",
+				"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - discovery.k8s.io/endpointslices: [get,list,watch]",
+				"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - nonResourceURL: /metrics: [get]",
 			},
 		},
 		{

apis/v1beta1/collector_webhook.go

@@ -29,6 +29,7 @@ import (
 	"github.com/open-telemetry/opentelemetry-operator/internal/config"
 	"github.com/open-telemetry/opentelemetry-operator/internal/fips"
 	ta "github.com/open-telemetry/opentelemetry-operator/internal/manifests/targetallocator/adapters"
+	"github.com/open-telemetry/opentelemetry-operator/internal/naming"
 	"github.com/open-telemetry/opentelemetry-operator/internal/rbac"
 	"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
 )
@@ -341,8 +342,12 @@ func (c CollectorWebhook) validateTargetAllocatorConfig(ctx context.Context, r *
 	}
 	// if the prometheusCR is enabled, it needs a suite of permissions to function
 	if r.Spec.TargetAllocator.PrometheusCR.Enabled {
+		saname := r.Spec.TargetAllocator.ServiceAccount
+		if len(r.Spec.TargetAllocator.ServiceAccount) == 0 {
+			saname = naming.TargetAllocatorServiceAccount(r.Name)
+		}
 		warnings, err := CheckTargetAllocatorPrometheusCRPolicyRules(
-			ctx, c.reviewer, r.Spec.TargetAllocator.ServiceAccount, r.GetNamespace())
+			ctx, c.reviewer, r.GetNamespace(), saname)
 		if err != nil || len(warnings) > 0 {
 			return warnings, err
 		}

apis/v1beta1/collector_webhook_test.go

@@ -651,6 +651,10 @@ func TestOTELColValidatingWebhook(t *testing.T) {
 			name:          "prom CR admissions warning",
 			shouldFailSar: true, // force failure
 			otelcol: v1beta1.OpenTelemetryCollector{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "adm-warning",
+					Namespace: "test-ns",
+				},
 				Spec: v1beta1.OpenTelemetryCollectorSpec{
 					Mode: v1beta1.ModeStatefulSet,
 					OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{
@@ -693,18 +697,18 @@ func TestOTELColValidatingWebhook(t *testing.T) {
 				},
 			},
 			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 namespaces: [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]",
+				"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - monitoring.coreos.com/servicemonitors: [*]",
+				"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - monitoring.coreos.com/podmonitors: [*]",
+				"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - nodes/metrics: [get,list,watch]",
+				"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - services: [get,list,watch]",
+				"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - endpoints: [get,list,watch]",
+				"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - namespaces: [get,list,watch]",
+				"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - networking.k8s.io/ingresses: [get,list,watch]",
+				"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - nodes: [get,list,watch]",
+				"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - pods: [get,list,watch]",
+				"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - configmaps: [get]",
+				"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - discovery.k8s.io/endpointslices: [get,list,watch]",
+				"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - nonResourceURL: /metrics: [get]",
 			},
 		},
 		{

apis/v1beta1/targetallocator_rbac.go

@@ -61,8 +61,8 @@ func CheckTargetAllocatorPrometheusCRPolicyRules(
 	serviceAccountName string) (warnings []string, err error) {
 	subjectAccessReviews, err := reviewer.CheckPolicyRules(
 		ctx,
-		namespace,
 		serviceAccountName,
+		namespace,
 		targetAllocatorCRPolicyRules...,
 	)
 	if err != nil {

internal/rbac/format.go

@@ -23,22 +23,27 @@ import (
 
 // 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)
+	userFullResourceToVerbs := make(map[string]map[string][]string)
 	for _, review := range reviews {
+		if _, ok := userFullResourceToVerbs[review.Spec.User]; !ok {
+			userFullResourceToVerbs[review.Spec.User] = make(map[string][]string)
+		}
 		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)
+			userFullResourceToVerbs[review.Spec.User][key] = append(userFullResourceToVerbs[review.Spec.User][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)
+			userFullResourceToVerbs[review.Spec.User][key] = append(userFullResourceToVerbs[review.Spec.User][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, ",")))
+	for user, fullResourceToVerbs := range userFullResourceToVerbs {
+		for fullResource, verbs := range fullResourceToVerbs {
+			warnings = append(warnings, fmt.Sprintf("missing the following rules for %s - %s: [%s]", user, fullResource, strings.Join(verbs, ",")))
+		}
 	}
 	return warnings
 }

internal/rbac/format_test.go

@@ -37,6 +37,7 @@ func TestWarningsGroupedByResource(t *testing.T) {
 			reviews: []*v1.SubjectAccessReview{
 				{
 					Spec: v1.SubjectAccessReviewSpec{
+						User: "system:serviceaccount:test-ns:test-targetallocator",
 						ResourceAttributes: &v1.ResourceAttributes{
 							Verb:     "get",
 							Group:    "",
@@ -45,13 +46,14 @@ func TestWarningsGroupedByResource(t *testing.T) {
 					},
 				},
 			},
-			expected: []string{"missing the following rules for namespaces: [get]"},
+			expected: []string{"missing the following rules for system:serviceaccount:test-ns:test-targetallocator - namespaces: [get]"},
 		},
 		{
 			desc: "One access review with non resource attributes",
 			reviews: []*v1.SubjectAccessReview{
 				{
 					Spec: v1.SubjectAccessReviewSpec{
+						User: "system:serviceaccount:test-ns:test-targetallocator",
 						ResourceAttributes: &v1.ResourceAttributes{
 							Verb:     "watch",
 							Group:    "apps",
@@ -60,7 +62,7 @@ func TestWarningsGroupedByResource(t *testing.T) {
 					},
 				},
 			},
-			expected: []string{"missing the following rules for apps/replicasets: [watch]"},
+			expected: []string{"missing the following rules for system:serviceaccount:test-ns:test-targetallocator - apps/replicasets: [watch]"},
 		},
 	}