Skip to content

Commit ab3b938

Browse files
author
Israel Blancas
committed
Apply changes requested in code review
Signed-off-by: Israel Blancas <[email protected]>
1 parent c722ee4 commit ab3b938

File tree

4 files changed

+361
-12
lines changed

4 files changed

+361
-12
lines changed

‎internal/manifests/collector/collector.go

+10-11
Original file line numberDiff line numberDiff line change
@@ -84,18 +84,10 @@ func Build(params manifests.Params) ([]client.Object, error) {
8484
}
8585
}
8686

87-
if params.ErrorAsWarning && params.Config.CreateRBACPermissions() == rbac.NotAvailable && params.Reviewer != nil {
88-
saName := params.OtelCol.Spec.ServiceAccount
89-
if saName == "" {
90-
sa, err := manifests.Factory(ServiceAccount)(params)
91-
if err != nil {
92-
return nil, err
93-
}
94-
saName = sa.GetName()
95-
}
96-
warnings, err := CheckRbacRules(params, saName)
87+
if needsCheckSaPermissions(params) {
88+
warnings, err := CheckRbacRules(params, params.OtelCol.Spec.ServiceAccount)
9789
if err != nil {
98-
return nil, fmt.Errorf("error checking RBAC rules for serviceAccount %s: %w", saName, err)
90+
return nil, fmt.Errorf("error checking RBAC rules for serviceAccount %s: %w", params.OtelCol.Spec.ServiceAccount, err)
9991
}
10092

10193
var w []error
@@ -115,3 +107,10 @@ func Build(params manifests.Params) ([]client.Object, error) {
115107
}
116108
return resourceManifests, nil
117109
}
110+
111+
func needsCheckSaPermissions(params manifests.Params) bool {
112+
return params.ErrorAsWarning &&
113+
params.Config.CreateRBACPermissions() == rbac.NotAvailable &&
114+
params.Reviewer != nil &&
115+
params.OtelCol.Spec.ServiceAccount != ""
116+
}

‎internal/manifests/collector/collector_test.go

+343
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,343 @@
1+
// Copyright The OpenTelemetry Authors
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package collector
16+
17+
import (
18+
"context"
19+
"fmt"
20+
"testing"
21+
22+
"github.com/go-logr/logr"
23+
"github.com/stretchr/testify/assert"
24+
"github.com/stretchr/testify/require"
25+
otelColFeatureGate "go.opentelemetry.io/collector/featuregate"
26+
v1 "k8s.io/api/authorization/v1"
27+
rbacv1 "k8s.io/api/rbac/v1"
28+
29+
"github.com/open-telemetry/opentelemetry-operator/apis/v1beta1"
30+
"github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus"
31+
autoRbac "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac"
32+
"github.com/open-telemetry/opentelemetry-operator/internal/config"
33+
"github.com/open-telemetry/opentelemetry-operator/internal/manifests"
34+
irbac "github.com/open-telemetry/opentelemetry-operator/internal/rbac"
35+
"github.com/open-telemetry/opentelemetry-operator/pkg/featuregate"
36+
)
37+
38+
func TestNeedsCheckSaPermissions(t *testing.T) {
39+
tests := []struct {
40+
name string
41+
params manifests.Params
42+
expected bool
43+
}{
44+
{
45+
name: "should return true when all conditions are met",
46+
params: manifests.Params{
47+
ErrorAsWarning: true,
48+
Config: config.New(config.WithRBACPermissions(autoRbac.NotAvailable)),
49+
Reviewer: &mockReviewer{},
50+
OtelCol: v1beta1.OpenTelemetryCollector{
51+
Spec: v1beta1.OpenTelemetryCollectorSpec{
52+
OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{
53+
ServiceAccount: "test-sa",
54+
},
55+
},
56+
},
57+
},
58+
expected: true,
59+
},
60+
{
61+
name: "should return false when ErrorAsWarning is false",
62+
params: manifests.Params{
63+
ErrorAsWarning: false,
64+
Config: config.New(config.WithRBACPermissions(autoRbac.NotAvailable)),
65+
Reviewer: &mockReviewer{},
66+
OtelCol: v1beta1.OpenTelemetryCollector{
67+
Spec: v1beta1.OpenTelemetryCollectorSpec{
68+
OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{
69+
ServiceAccount: "test-sa",
70+
},
71+
},
72+
},
73+
},
74+
expected: false,
75+
},
76+
{
77+
name: "should return false when RBAC is available",
78+
params: manifests.Params{
79+
ErrorAsWarning: true,
80+
Config: config.New(config.WithRBACPermissions(autoRbac.Available)),
81+
Reviewer: &mockReviewer{},
82+
OtelCol: v1beta1.OpenTelemetryCollector{
83+
Spec: v1beta1.OpenTelemetryCollectorSpec{
84+
OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{
85+
ServiceAccount: "test-sa",
86+
},
87+
},
88+
},
89+
},
90+
expected: false,
91+
},
92+
{
93+
name: "should return false when Reviewer is nil",
94+
params: manifests.Params{
95+
ErrorAsWarning: true,
96+
Config: config.New(config.WithRBACPermissions(autoRbac.NotAvailable)),
97+
Reviewer: nil,
98+
OtelCol: v1beta1.OpenTelemetryCollector{
99+
Spec: v1beta1.OpenTelemetryCollectorSpec{
100+
OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{
101+
ServiceAccount: "test-sa",
102+
},
103+
},
104+
},
105+
},
106+
expected: false,
107+
},
108+
{
109+
name: "should return false when ServiceAccount is empty",
110+
params: manifests.Params{
111+
ErrorAsWarning: true,
112+
Config: config.New(config.WithRBACPermissions(autoRbac.NotAvailable)),
113+
Reviewer: &mockReviewer{},
114+
OtelCol: v1beta1.OpenTelemetryCollector{
115+
Spec: v1beta1.OpenTelemetryCollectorSpec{
116+
OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{
117+
ServiceAccount: "",
118+
},
119+
},
120+
},
121+
},
122+
expected: false,
123+
},
124+
}
125+
126+
for _, tt := range tests {
127+
t.Run(tt.name, func(t *testing.T) {
128+
result := needsCheckSaPermissions(tt.params)
129+
assert.Equal(t, tt.expected, result)
130+
})
131+
}
132+
}
133+
134+
type mockReviewer struct{}
135+
136+
var _ irbac.SAReviewer = &mockReviewer{}
137+
138+
func (m *mockReviewer) CheckPolicyRules(ctx context.Context, serviceAccount, serviceAccountNamespace string, rules ...*rbacv1.PolicyRule) ([]*v1.SubjectAccessReview, error) {
139+
return nil, fmt.Errorf("error checking policy rules")
140+
}
141+
142+
func (m *mockReviewer) CanAccess(ctx context.Context, serviceAccount, serviceAccountNamespace string, res *v1.ResourceAttributes, nonResourceAttributes *v1.NonResourceAttributes) (*v1.SubjectAccessReview, error) {
143+
return nil, nil
144+
}
145+
146+
func TestBuild(t *testing.T) {
147+
logger := logr.Discard()
148+
tests := []struct {
149+
name string
150+
params manifests.Params
151+
expectedObjects int
152+
wantErr bool
153+
featureGate *otelColFeatureGate.Gate
154+
}{
155+
{
156+
name: "deployment mode builds expected manifests",
157+
params: manifests.Params{
158+
Log: logger,
159+
OtelCol: v1beta1.OpenTelemetryCollector{
160+
Spec: v1beta1.OpenTelemetryCollectorSpec{
161+
Mode: v1beta1.ModeDeployment,
162+
},
163+
},
164+
Config: config.New(),
165+
},
166+
expectedObjects: 5,
167+
wantErr: false,
168+
},
169+
{
170+
name: "statefulset mode builds expected manifests",
171+
params: manifests.Params{
172+
Log: logger,
173+
OtelCol: v1beta1.OpenTelemetryCollector{
174+
Spec: v1beta1.OpenTelemetryCollectorSpec{
175+
Mode: v1beta1.ModeStatefulSet,
176+
},
177+
},
178+
Config: config.New(),
179+
},
180+
expectedObjects: 5,
181+
wantErr: false,
182+
},
183+
{
184+
name: "sidecar mode skips deployment manifests",
185+
params: manifests.Params{
186+
Log: logger,
187+
OtelCol: v1beta1.OpenTelemetryCollector{
188+
Spec: v1beta1.OpenTelemetryCollectorSpec{
189+
Mode: v1beta1.ModeSidecar,
190+
},
191+
},
192+
Config: config.New(),
193+
},
194+
expectedObjects: 3,
195+
wantErr: false,
196+
},
197+
{
198+
name: "rbac available adds cluster role manifests",
199+
params: manifests.Params{
200+
Log: logger,
201+
OtelCol: v1beta1.OpenTelemetryCollector{
202+
Spec: v1beta1.OpenTelemetryCollectorSpec{
203+
Mode: v1beta1.ModeDeployment,
204+
Config: v1beta1.Config{
205+
Processors: &v1beta1.AnyConfig{
206+
Object: map[string]any{
207+
"k8sattributes": map[string]any{},
208+
},
209+
},
210+
Service: v1beta1.Service{
211+
Pipelines: map[string]*v1beta1.Pipeline{
212+
"traces": {
213+
Processors: []string{"k8sattributes"},
214+
},
215+
},
216+
},
217+
},
218+
},
219+
},
220+
Config: config.New(config.WithRBACPermissions(autoRbac.Available)),
221+
},
222+
expectedObjects: 7,
223+
wantErr: false,
224+
},
225+
{
226+
name: "metrics enabled adds monitoring service monitor",
227+
params: manifests.Params{
228+
Log: logger,
229+
OtelCol: v1beta1.OpenTelemetryCollector{
230+
Spec: v1beta1.OpenTelemetryCollectorSpec{
231+
Mode: v1beta1.ModeDeployment,
232+
Observability: v1beta1.ObservabilitySpec{
233+
Metrics: v1beta1.MetricsConfigSpec{
234+
EnableMetrics: true,
235+
},
236+
},
237+
},
238+
},
239+
Config: config.New(config.WithPrometheusCRAvailability(prometheus.Available)),
240+
},
241+
expectedObjects: 6,
242+
wantErr: false,
243+
featureGate: featuregate.PrometheusOperatorIsAvailable,
244+
},
245+
{
246+
name: "metrics enabled adds service monitors",
247+
params: manifests.Params{
248+
Log: logger,
249+
OtelCol: v1beta1.OpenTelemetryCollector{
250+
Spec: v1beta1.OpenTelemetryCollectorSpec{
251+
Mode: v1beta1.ModeDeployment,
252+
Observability: v1beta1.ObservabilitySpec{
253+
Metrics: v1beta1.MetricsConfigSpec{
254+
EnableMetrics: true,
255+
},
256+
},
257+
Config: v1beta1.Config{
258+
Exporters: v1beta1.AnyConfig{
259+
Object: map[string]any{
260+
"prometheus": map[string]any{
261+
"endpoint": "1.2.3.4:1234",
262+
},
263+
},
264+
},
265+
Service: v1beta1.Service{
266+
Pipelines: map[string]*v1beta1.Pipeline{
267+
"metrics": {
268+
Exporters: []string{"prometheus"},
269+
},
270+
},
271+
},
272+
},
273+
},
274+
},
275+
Config: config.New(config.WithPrometheusCRAvailability(prometheus.Available)),
276+
},
277+
expectedObjects: 9,
278+
wantErr: false,
279+
featureGate: featuregate.PrometheusOperatorIsAvailable,
280+
},
281+
{
282+
name: "check sa permissions",
283+
params: manifests.Params{
284+
ErrorAsWarning: true,
285+
Reviewer: &mockReviewer{},
286+
Log: logger,
287+
OtelCol: v1beta1.OpenTelemetryCollector{
288+
Spec: v1beta1.OpenTelemetryCollectorSpec{
289+
OpenTelemetryCommonFields: v1beta1.OpenTelemetryCommonFields{
290+
ServiceAccount: "test-sa",
291+
},
292+
Mode: v1beta1.ModeDeployment,
293+
Observability: v1beta1.ObservabilitySpec{
294+
Metrics: v1beta1.MetricsConfigSpec{
295+
EnableMetrics: true,
296+
},
297+
},
298+
Config: v1beta1.Config{
299+
Processors: &v1beta1.AnyConfig{
300+
Object: map[string]any{
301+
"k8sattributes": map[string]any{},
302+
},
303+
},
304+
Service: v1beta1.Service{
305+
Pipelines: map[string]*v1beta1.Pipeline{
306+
"metrics": {
307+
Processors: []string{"k8sattributes"},
308+
},
309+
},
310+
},
311+
},
312+
},
313+
},
314+
Config: config.New(config.WithPrometheusCRAvailability(prometheus.Available)),
315+
},
316+
expectedObjects: 9,
317+
wantErr: true,
318+
featureGate: featuregate.PrometheusOperatorIsAvailable,
319+
},
320+
}
321+
322+
for _, tt := range tests {
323+
t.Run(tt.name, func(t *testing.T) {
324+
if tt.featureGate != nil {
325+
err := otelColFeatureGate.GlobalRegistry().Set(tt.featureGate.ID(), true)
326+
require.NoError(t, err)
327+
defer func() {
328+
err := otelColFeatureGate.GlobalRegistry().Set(tt.featureGate.ID(), false)
329+
require.NoError(t, err)
330+
}()
331+
}
332+
333+
objects, err := Build(tt.params)
334+
if tt.wantErr {
335+
require.Error(t, err)
336+
return
337+
}
338+
339+
require.NoError(t, err)
340+
assert.Len(t, objects, tt.expectedObjects)
341+
})
342+
}
343+
}

‎internal/manifests/params.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,6 @@ type Params struct {
3636
TargetAllocator *v1alpha1.TargetAllocator
3737
OpAMPBridge v1alpha1.OpAMPBridge
3838
Config config.Config
39-
Reviewer *rbac.Reviewer
39+
Reviewer rbac.SAReviewer
4040
ErrorAsWarning bool
4141
}

‎internal/rbac/access.go

+7
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,13 @@ const (
2929
serviceAccountFmtStr = "system:serviceaccount:%s:%s"
3030
)
3131

32+
type SAReviewer interface {
33+
CheckPolicyRules(ctx context.Context, serviceAccount, serviceAccountNamespace string, rules ...*rbacv1.PolicyRule) ([]*v1.SubjectAccessReview, error)
34+
CanAccess(ctx context.Context, serviceAccount, serviceAccountNamespace string, res *v1.ResourceAttributes, nonResourceAttributes *v1.NonResourceAttributes) (*v1.SubjectAccessReview, error)
35+
}
36+
37+
var _ SAReviewer = &Reviewer{}
38+
3239
type Reviewer struct {
3340
client kubernetes.Interface
3441
}

0 commit comments

Comments
 (0)