Skip to content

Commit 8f77e39

Browse files
davidhajadavidhaja
committed
serviceaccount name included in warning message and unit tests are adjusted
1 parent 4e374f0 commit 8f77e39

File tree

5 files changed

+52
-32
lines changed

5 files changed

+52
-32
lines changed

‎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.GetNamespace(), ta.Spec.ServiceAccount)
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_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
{

‎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)