diff --git a/.chloggen/3380-ta-serviceaccount-check.yaml b/.chloggen/3380-ta-serviceaccount-check.yaml new file mode 100755 index 0000000000..4bcfc4e0df --- /dev/null +++ b/.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: diff --git a/apis/v1alpha1/targetallocator_webhook.go b/apis/v1alpha1/targetallocator_webhook.go index bed76f29a4..1a3687dd65 100644 --- a/apis/v1alpha1/targetallocator_webhook.go +++ b/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 } diff --git a/apis/v1alpha1/targetallocator_webhook_test.go b/apis/v1alpha1/targetallocator_webhook_test.go index aedbb62c82..5e665368a2 100644 --- a/apis/v1alpha1/targetallocator_webhook_test.go +++ b/apis/v1alpha1/targetallocator_webhook_test.go @@ -224,6 +224,10 @@ 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, @@ -231,18 +235,18 @@ func TestTargetAllocatorValidatingWebhook(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: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]", }, }, { diff --git a/apis/v1beta1/collector_webhook.go b/apis/v1beta1/collector_webhook.go index 68b572b8c1..7c66d388c5 100644 --- a/apis/v1beta1/collector_webhook.go +++ b/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 } diff --git a/apis/v1beta1/collector_webhook_test.go b/apis/v1beta1/collector_webhook_test.go index 72ab56aab0..abdad0a8c6 100644 --- a/apis/v1beta1/collector_webhook_test.go +++ b/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]", }, }, { diff --git a/apis/v1beta1/targetallocator_rbac.go b/apis/v1beta1/targetallocator_rbac.go index 4fb48832e6..2ef66b4541 100644 --- a/apis/v1beta1/targetallocator_rbac.go +++ b/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 { diff --git a/internal/rbac/format.go b/internal/rbac/format.go index 784bcc39c2..da0a4ca1c2 100644 --- a/internal/rbac/format.go +++ b/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 } diff --git a/internal/rbac/format_test.go b/internal/rbac/format_test.go index 82f97d25fe..8c08464c40 100644 --- a/internal/rbac/format_test.go +++ b/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]"}, }, }