Skip to content

Commit 967f9c1

Browse files
author
Israel Blancas
authored
Merge branch 'main' into 3370
2 parents 117114e + afc0cae commit 967f9c1

8 files changed

+75
-34
lines changed

‎.chloggen/3380-ta-serviceaccount-check.yaml

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
2+
change_type: bug_fix
3+
4+
# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action)
5+
component: target allocator
6+
7+
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
8+
note: "Permission check fixed for the serviceaccount of the target allocator"
9+
10+
# One or more tracking issues related to the change
11+
issues: [3380]
12+
13+
# (Optional) One or more lines of additional information to render under the primary note.
14+
# These lines will be padded with 2 spaces and then inserted directly into the document.
15+
# Use pipe (|) for multiline entries.
16+
subtext:

‎apis/v1alpha1/targetallocator_webhook.go

+6-1
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import (
2626

2727
"github.com/open-telemetry/opentelemetry-operator/apis/v1beta1"
2828
"github.com/open-telemetry/opentelemetry-operator/internal/config"
29+
"github.com/open-telemetry/opentelemetry-operator/internal/naming"
2930
"github.com/open-telemetry/opentelemetry-operator/internal/rbac"
3031
)
3132

@@ -119,7 +120,11 @@ func (w TargetAllocatorWebhook) validate(ctx context.Context, ta *TargetAllocato
119120

120121
// if the prometheusCR is enabled, it needs a suite of permissions to function
121122
if ta.Spec.PrometheusCR.Enabled {
122-
warnings, err := v1beta1.CheckTargetAllocatorPrometheusCRPolicyRules(ctx, w.reviewer, ta.Spec.ServiceAccount, ta.GetNamespace())
123+
saname := ta.Spec.ServiceAccount
124+
if len(ta.Spec.ServiceAccount) == 0 {
125+
saname = naming.TargetAllocatorServiceAccount(ta.Name)
126+
}
127+
warnings, err := v1beta1.CheckTargetAllocatorPrometheusCRPolicyRules(ctx, w.reviewer, ta.GetNamespace(), saname)
123128
if err != nil || len(warnings) > 0 {
124129
return warnings, err
125130
}

‎apis/v1alpha1/targetallocator_webhook_test.go

+16-12
Original file line numberDiff line numberDiff line change
@@ -224,25 +224,29 @@ func TestTargetAllocatorValidatingWebhook(t *testing.T) {
224224
name: "prom CR admissions warning",
225225
shouldFailSar: true, // force failure
226226
targetallocator: TargetAllocator{
227+
ObjectMeta: metav1.ObjectMeta{
228+
Name: "test-ta",
229+
Namespace: "test-ns",
230+
},
227231
Spec: TargetAllocatorSpec{
228232
PrometheusCR: v1beta1.TargetAllocatorPrometheusCR{
229233
Enabled: true,
230234
},
231235
},
232236
},
233237
expectedWarnings: []string{
234-
"missing the following rules for monitoring.coreos.com/servicemonitors: [*]",
235-
"missing the following rules for monitoring.coreos.com/podmonitors: [*]",
236-
"missing the following rules for nodes/metrics: [get,list,watch]",
237-
"missing the following rules for services: [get,list,watch]",
238-
"missing the following rules for endpoints: [get,list,watch]",
239-
"missing the following rules for namespaces: [get,list,watch]",
240-
"missing the following rules for networking.k8s.io/ingresses: [get,list,watch]",
241-
"missing the following rules for nodes: [get,list,watch]",
242-
"missing the following rules for pods: [get,list,watch]",
243-
"missing the following rules for configmaps: [get]",
244-
"missing the following rules for discovery.k8s.io/endpointslices: [get,list,watch]",
245-
"missing the following rules for nonResourceURL: /metrics: [get]",
238+
"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - monitoring.coreos.com/servicemonitors: [*]",
239+
"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - monitoring.coreos.com/podmonitors: [*]",
240+
"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - nodes/metrics: [get,list,watch]",
241+
"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - services: [get,list,watch]",
242+
"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - endpoints: [get,list,watch]",
243+
"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - namespaces: [get,list,watch]",
244+
"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - networking.k8s.io/ingresses: [get,list,watch]",
245+
"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - nodes: [get,list,watch]",
246+
"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - pods: [get,list,watch]",
247+
"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - configmaps: [get]",
248+
"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - discovery.k8s.io/endpointslices: [get,list,watch]",
249+
"missing the following rules for system:serviceaccount:test-ns:test-ta-targetallocator - nonResourceURL: /metrics: [get]",
246250
},
247251
},
248252
{

‎apis/v1beta1/collector_webhook.go

+6-1
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import (
2929
"github.com/open-telemetry/opentelemetry-operator/internal/config"
3030
"github.com/open-telemetry/opentelemetry-operator/internal/fips"
3131
ta "github.com/open-telemetry/opentelemetry-operator/internal/manifests/targetallocator/adapters"
32+
"github.com/open-telemetry/opentelemetry-operator/internal/naming"
3233
"github.com/open-telemetry/opentelemetry-operator/internal/rbac"
3334
"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
3435
)
@@ -341,8 +342,12 @@ func (c CollectorWebhook) validateTargetAllocatorConfig(ctx context.Context, r *
341342
}
342343
// if the prometheusCR is enabled, it needs a suite of permissions to function
343344
if r.Spec.TargetAllocator.PrometheusCR.Enabled {
345+
saname := r.Spec.TargetAllocator.ServiceAccount
346+
if len(r.Spec.TargetAllocator.ServiceAccount) == 0 {
347+
saname = naming.TargetAllocatorServiceAccount(r.Name)
348+
}
344349
warnings, err := CheckTargetAllocatorPrometheusCRPolicyRules(
345-
ctx, c.reviewer, r.Spec.TargetAllocator.ServiceAccount, r.GetNamespace())
350+
ctx, c.reviewer, r.GetNamespace(), saname)
346351
if err != nil || len(warnings) > 0 {
347352
return warnings, err
348353
}

‎apis/v1beta1/collector_webhook_test.go

+16-12
Original file line numberDiff line numberDiff line change
@@ -651,6 +651,10 @@ func TestOTELColValidatingWebhook(t *testing.T) {
651651
name: "prom CR admissions warning",
652652
shouldFailSar: true, // force failure
653653
otelcol: v1beta1.OpenTelemetryCollector{
654+
ObjectMeta: metav1.ObjectMeta{
655+
Name: "adm-warning",
656+
Namespace: "test-ns",
657+
},
654658
Spec: v1beta1.OpenTelemetryCollectorSpec{
655659
Mode: v1beta1.ModeStatefulSet,
656660
OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{
@@ -693,18 +697,18 @@ func TestOTELColValidatingWebhook(t *testing.T) {
693697
},
694698
},
695699
expectedWarnings: []string{
696-
"missing the following rules for monitoring.coreos.com/servicemonitors: [*]",
697-
"missing the following rules for monitoring.coreos.com/podmonitors: [*]",
698-
"missing the following rules for nodes/metrics: [get,list,watch]",
699-
"missing the following rules for services: [get,list,watch]",
700-
"missing the following rules for endpoints: [get,list,watch]",
701-
"missing the following rules for namespaces: [get,list,watch]",
702-
"missing the following rules for networking.k8s.io/ingresses: [get,list,watch]",
703-
"missing the following rules for nodes: [get,list,watch]",
704-
"missing the following rules for pods: [get,list,watch]",
705-
"missing the following rules for configmaps: [get]",
706-
"missing the following rules for discovery.k8s.io/endpointslices: [get,list,watch]",
707-
"missing the following rules for nonResourceURL: /metrics: [get]",
700+
"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - monitoring.coreos.com/servicemonitors: [*]",
701+
"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - monitoring.coreos.com/podmonitors: [*]",
702+
"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - nodes/metrics: [get,list,watch]",
703+
"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - services: [get,list,watch]",
704+
"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - endpoints: [get,list,watch]",
705+
"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - namespaces: [get,list,watch]",
706+
"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - networking.k8s.io/ingresses: [get,list,watch]",
707+
"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - nodes: [get,list,watch]",
708+
"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - pods: [get,list,watch]",
709+
"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - configmaps: [get]",
710+
"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - discovery.k8s.io/endpointslices: [get,list,watch]",
711+
"missing the following rules for system:serviceaccount:test-ns:adm-warning-targetallocator - nonResourceURL: /metrics: [get]",
708712
},
709713
},
710714
{

‎apis/v1beta1/targetallocator_rbac.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -61,8 +61,8 @@ func CheckTargetAllocatorPrometheusCRPolicyRules(
6161
serviceAccountName string) (warnings []string, err error) {
6262
subjectAccessReviews, err := reviewer.CheckPolicyRules(
6363
ctx,
64-
namespace,
6564
serviceAccountName,
65+
namespace,
6666
targetAllocatorCRPolicyRules...,
6767
)
6868
if err != nil {

‎internal/rbac/format.go

+10-5
Original file line numberDiff line numberDiff line change
@@ -23,22 +23,27 @@ import (
2323

2424
// WarningsGroupedByResource is a helper to take the missing permissions and format them as warnings.
2525
func WarningsGroupedByResource(reviews []*v1.SubjectAccessReview) []string {
26-
fullResourceToVerbs := make(map[string][]string)
26+
userFullResourceToVerbs := make(map[string]map[string][]string)
2727
for _, review := range reviews {
28+
if _, ok := userFullResourceToVerbs[review.Spec.User]; !ok {
29+
userFullResourceToVerbs[review.Spec.User] = make(map[string][]string)
30+
}
2831
if review.Spec.ResourceAttributes != nil {
2932
key := fmt.Sprintf("%s/%s", review.Spec.ResourceAttributes.Group, review.Spec.ResourceAttributes.Resource)
3033
if len(review.Spec.ResourceAttributes.Group) == 0 {
3134
key = review.Spec.ResourceAttributes.Resource
3235
}
33-
fullResourceToVerbs[key] = append(fullResourceToVerbs[key], review.Spec.ResourceAttributes.Verb)
36+
userFullResourceToVerbs[review.Spec.User][key] = append(userFullResourceToVerbs[review.Spec.User][key], review.Spec.ResourceAttributes.Verb)
3437
} else if review.Spec.NonResourceAttributes != nil {
3538
key := fmt.Sprintf("nonResourceURL: %s", review.Spec.NonResourceAttributes.Path)
36-
fullResourceToVerbs[key] = append(fullResourceToVerbs[key], review.Spec.NonResourceAttributes.Verb)
39+
userFullResourceToVerbs[review.Spec.User][key] = append(userFullResourceToVerbs[review.Spec.User][key], review.Spec.NonResourceAttributes.Verb)
3740
}
3841
}
3942
var warnings []string
40-
for fullResource, verbs := range fullResourceToVerbs {
41-
warnings = append(warnings, fmt.Sprintf("missing the following rules for %s: [%s]", fullResource, strings.Join(verbs, ",")))
43+
for user, fullResourceToVerbs := range userFullResourceToVerbs {
44+
for fullResource, verbs := range fullResourceToVerbs {
45+
warnings = append(warnings, fmt.Sprintf("missing the following rules for %s - %s: [%s]", user, fullResource, strings.Join(verbs, ",")))
46+
}
4247
}
4348
return warnings
4449
}

‎internal/rbac/format_test.go

+4-2
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ func TestWarningsGroupedByResource(t *testing.T) {
3737
reviews: []*v1.SubjectAccessReview{
3838
{
3939
Spec: v1.SubjectAccessReviewSpec{
40+
User: "system:serviceaccount:test-ns:test-targetallocator",
4041
ResourceAttributes: &v1.ResourceAttributes{
4142
Verb: "get",
4243
Group: "",
@@ -45,13 +46,14 @@ func TestWarningsGroupedByResource(t *testing.T) {
4546
},
4647
},
4748
},
48-
expected: []string{"missing the following rules for namespaces: [get]"},
49+
expected: []string{"missing the following rules for system:serviceaccount:test-ns:test-targetallocator - namespaces: [get]"},
4950
},
5051
{
5152
desc: "One access review with non resource attributes",
5253
reviews: []*v1.SubjectAccessReview{
5354
{
5455
Spec: v1.SubjectAccessReviewSpec{
56+
User: "system:serviceaccount:test-ns:test-targetallocator",
5557
ResourceAttributes: &v1.ResourceAttributes{
5658
Verb: "watch",
5759
Group: "apps",
@@ -60,7 +62,7 @@ func TestWarningsGroupedByResource(t *testing.T) {
6062
},
6163
},
6264
},
63-
expected: []string{"missing the following rules for apps/replicasets: [watch]"},
65+
expected: []string{"missing the following rules for system:serviceaccount:test-ns:test-targetallocator - apps/replicasets: [watch]"},
6466
},
6567
}
6668

0 commit comments

Comments
 (0)