From de651f3a329628cab30c10252af32028f2e6faaa Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Mon, 8 Apr 2024 15:22:06 +0200 Subject: [PATCH 01/26] Check for the permissions instead of using a CLI flag. #2588 Signed-off-by: Israel Blancas --- ...ssions-by-checking-the-sa-permissions.yaml | 16 ++ apis/v1alpha1/collector_webhook.go | 26 +-- config/manager/manager.yaml | 5 + internal/autodetect/operator.go | 37 +++++ internal/rbac/format.go | 44 +++++ main.go | 154 ++++++++++++------ 6 files changed, 203 insertions(+), 79 deletions(-) create mode 100755 .chloggen/replace-create-rbac-permissions-by-checking-the-sa-permissions.yaml create mode 100644 internal/autodetect/operator.go create mode 100644 internal/rbac/format.go diff --git a/.chloggen/replace-create-rbac-permissions-by-checking-the-sa-permissions.yaml b/.chloggen/replace-create-rbac-permissions-by-checking-the-sa-permissions.yaml new file mode 100755 index 0000000000..3cf4cf6f57 --- /dev/null +++ b/.chloggen/replace-create-rbac-permissions-by-checking-the-sa-permissions.yaml @@ -0,0 +1,16 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: enhancement + +# 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: operator + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Check the permissions for the SA running the OpenTelemetry Operator instead of using the create-rbac-permissions flag. + +# One or more tracking issues related to the change +issues: [2588] + +# (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: \ No newline at end of file diff --git a/apis/v1alpha1/collector_webhook.go b/apis/v1alpha1/collector_webhook.go index 97ee9cc3a4..20d434a5b4 100644 --- a/apis/v1alpha1/collector_webhook.go +++ b/apis/v1alpha1/collector_webhook.go @@ -17,10 +17,8 @@ package v1alpha1 import ( "context" "fmt" - "strings" "github.com/go-logr/logr" - v1 "k8s.io/api/authorization/v1" autoscalingv2 "k8s.io/api/autoscaling/v2" rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/runtime" @@ -378,7 +376,7 @@ func (c CollectorWebhook) validateTargetAllocatorConfig(ctx context.Context, r * if subjectAccessReviews, err := c.reviewer.CheckPolicyRules(ctx, r.GetNamespace(), r.Spec.TargetAllocator.ServiceAccount, targetAllocatorCRPolicyRules...); err != nil { return nil, fmt.Errorf("unable to check rbac rules %w", err) } else if allowed, deniedReviews := rbac.AllSubjectAccessReviewsAllowed(subjectAccessReviews); !allowed { - return warningsGroupedByResource(deniedReviews), nil + return rbac.WarningsGroupedByResource(deniedReviews), nil } } @@ -426,28 +424,6 @@ func checkAutoscalerSpec(autoscaler *AutoscalerSpec) error { return nil } -// 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) - for _, review := range reviews { - 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) - } else if review.Spec.NonResourceAttributes != nil { - key := fmt.Sprintf("nonResourceURL: %s", review.Spec.NonResourceAttributes.Path) - fullResourceToVerbs[key] = append(fullResourceToVerbs[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, ","))) - } - return warnings -} - func SetupCollectorWebhook(mgr ctrl.Manager, cfg config.Config, reviewer *rbac.Reviewer) error { cvw := &CollectorWebhook{ reviewer: reviewer, diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index 76eb71ac95..35c9072ffc 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -52,5 +52,10 @@ spec: requests: cpu: 100m memory: 64Mi + env: + - name: SERVICE_ACCOUNT_NAME + valueFrom: + fieldRef: + fieldPath: spec.serviceAccountName serviceAccountName: controller-manager terminationGracePeriodSeconds: 10 diff --git a/internal/autodetect/operator.go b/internal/autodetect/operator.go new file mode 100644 index 0000000000..13a4ea0a72 --- /dev/null +++ b/internal/autodetect/operator.go @@ -0,0 +1,37 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package autodetect + +import ( + "fmt" + "os" +) + +func GetOperatorNamespace() (string, error) { + nsBytes, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace") + if err != nil { + return "", err + } + return string(nsBytes), nil +} + +func GetOperatorServiceAccount() (string, error) { + saEnvVar := "SERVICE_ACCOUNT_NAME" + sa := os.Getenv(saEnvVar) + if sa == "" { + return sa, fmt.Errorf("%s env variable not found", saEnvVar) + } + return sa, nil +} diff --git a/internal/rbac/format.go b/internal/rbac/format.go new file mode 100644 index 0000000000..784bcc39c2 --- /dev/null +++ b/internal/rbac/format.go @@ -0,0 +1,44 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package rbac + +import ( + "fmt" + "strings" + + v1 "k8s.io/api/authorization/v1" +) + +// 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) + for _, review := range reviews { + 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) + } else if review.Spec.NonResourceAttributes != nil { + key := fmt.Sprintf("nonResourceURL: %s", review.Spec.NonResourceAttributes.Path) + fullResourceToVerbs[key] = append(fullResourceToVerbs[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, ","))) + } + return warnings +} diff --git a/main.go b/main.go index fca734ce3b..59f8cec490 100644 --- a/main.go +++ b/main.go @@ -29,6 +29,7 @@ import ( "github.com/spf13/pflag" colfeaturegate "go.opentelemetry.io/collector/featuregate" networkingv1 "k8s.io/api/networking/v1" + rbacv1 "k8s.io/api/rbac/v1" k8sruntime "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/client-go/kubernetes" @@ -134,7 +135,7 @@ func main() { pflag.BoolVar(&enableLeaderElection, "enable-leader-election", false, "Enable leader election for controller manager. "+ "Enabling this will ensure there is only one active controller manager.") - pflag.BoolVar(&createRBACPermissions, "create-rbac-permissions", false, "Automatically create RBAC permissions needed by the processors") + pflag.BoolVar(&createRBACPermissions, "create-rbac-permissions", false, "Automatically create RBAC permissions needed by the processors (deprecated)") pflag.BoolVar(&enableMultiInstrumentation, "enable-multi-instrumentation", false, "Controls whether the operator supports multi instrumentation") pflag.BoolVar(&enableApacheHttpdInstrumentation, constants.FlagApacheHttpd, true, "Controls whether the operator supports Apache HTTPD auto-instrumentation") pflag.BoolVar(&enableDotNetInstrumentation, constants.FlagDotNet, true, "Controls whether the operator supports dotnet auto-instrumentation") @@ -188,54 +189,6 @@ func main() { restConfig := ctrl.GetConfigOrDie() - // builds the operator's configuration - ad, err := autodetect.New(restConfig) - if err != nil { - setupLog.Error(err, "failed to setup auto-detect routine") - os.Exit(1) - } - - cfg := config.New( - config.WithLogger(ctrl.Log.WithName("config")), - config.WithVersion(v), - config.WithCollectorImage(collectorImage), - config.WithCreateRBACPermissions(createRBACPermissions), - config.WithEnableMultiInstrumentation(enableMultiInstrumentation), - config.WithEnableApacheHttpdInstrumentation(enableApacheHttpdInstrumentation), - config.WithEnableDotNetInstrumentation(enableDotNetInstrumentation), - config.WithEnableNginxInstrumentation(enableNginxInstrumentation), - config.WithEnablePythonInstrumentation(enablePythonInstrumentation), - config.WithTargetAllocatorImage(targetAllocatorImage), - config.WithOperatorOpAMPBridgeImage(operatorOpAMPBridgeImage), - config.WithAutoInstrumentationJavaImage(autoInstrumentationJava), - config.WithAutoInstrumentationNodeJSImage(autoInstrumentationNodeJS), - config.WithAutoInstrumentationPythonImage(autoInstrumentationPython), - config.WithAutoInstrumentationDotNetImage(autoInstrumentationDotNet), - config.WithAutoInstrumentationGoImage(autoInstrumentationGo), - config.WithAutoInstrumentationApacheHttpdImage(autoInstrumentationApacheHttpd), - config.WithAutoInstrumentationNginxImage(autoInstrumentationNginx), - config.WithAutoDetect(ad), - config.WithLabelFilters(labelsFilter), - config.WithAnnotationFilters(annotationsFilter), - ) - err = cfg.AutoDetect() - if err != nil { - setupLog.Error(err, "failed to autodetect config variables") - } - // Only add these to the scheme if they are available - if cfg.PrometheusCRAvailability() == prometheus.Available { - setupLog.Info("Prometheus CRDs are installed, adding to scheme.") - utilruntime.Must(monitoringv1.AddToScheme(scheme)) - } else { - setupLog.Info("Prometheus CRDs are not installed, skipping adding to scheme.") - } - if cfg.OpenShiftRoutesAvailability() == openshift.RoutesAvailable { - setupLog.Info("Openshift CRDs are installed, adding to scheme.") - utilruntime.Must(routev1.Install(scheme)) - } else { - setupLog.Info("Openshift CRDs are not installed, skipping adding to scheme.") - } - var namespaces map[string]cache.Config watchNamespace, found := os.LookupEnv("WATCH_NAMESPACE") if found { @@ -284,17 +237,81 @@ func main() { os.Exit(1) } + clientset, clientErr := kubernetes.NewForConfig(mgr.GetConfig()) + if err != nil { + setupLog.Error(clientErr, "failed to create kubernetes clientset") + } + + reviewer := rbac.NewReviewer(clientset) + createRBACPermissions = true ctx := ctrl.SetupSignalHandler() - err = addDependencies(ctx, mgr, cfg, v) + w, err := checkRbacPermissions(reviewer, ctx) if err != nil { - setupLog.Error(err, "failed to add/run bootstrap dependencies to the controller manager") + createRBACPermissions = false + setupLog.Info("the operator has not permissions to create rbac resources", "error", err, "serviceAccount") + } + if w != nil { + createRBACPermissions = false + setupLog.Info("the operator has not permissions to create rbac resources", "permissions", w) + } + + if createRBACPermissions { + setupLog.Info("the operator has permissions to create rbac resources") + } + + // builds the operator's configuration + ad, err := autodetect.New(restConfig) + if err != nil { + setupLog.Error(err, "failed to setup auto-detect routine") os.Exit(1) } - clientset, clientErr := kubernetes.NewForConfig(mgr.GetConfig()) + + cfg := config.New( + config.WithLogger(ctrl.Log.WithName("config")), + config.WithVersion(v), + config.WithCollectorImage(collectorImage), + config.WithCreateRBACPermissions(createRBACPermissions), + config.WithEnableMultiInstrumentation(enableMultiInstrumentation), + config.WithEnableApacheHttpdInstrumentation(enableApacheHttpdInstrumentation), + config.WithEnableDotNetInstrumentation(enableDotNetInstrumentation), + config.WithEnableNginxInstrumentation(enableNginxInstrumentation), + config.WithEnablePythonInstrumentation(enablePythonInstrumentation), + config.WithTargetAllocatorImage(targetAllocatorImage), + config.WithOperatorOpAMPBridgeImage(operatorOpAMPBridgeImage), + config.WithAutoInstrumentationJavaImage(autoInstrumentationJava), + config.WithAutoInstrumentationNodeJSImage(autoInstrumentationNodeJS), + config.WithAutoInstrumentationPythonImage(autoInstrumentationPython), + config.WithAutoInstrumentationDotNetImage(autoInstrumentationDotNet), + config.WithAutoInstrumentationGoImage(autoInstrumentationGo), + config.WithAutoInstrumentationApacheHttpdImage(autoInstrumentationApacheHttpd), + config.WithAutoInstrumentationNginxImage(autoInstrumentationNginx), + config.WithAutoDetect(ad), + config.WithLabelFilters(labelsFilter), + config.WithAnnotationFilters(annotationsFilter), + ) + err = cfg.AutoDetect() if err != nil { - setupLog.Error(clientErr, "failed to create kubernetes clientset") + setupLog.Error(err, "failed to autodetect config variables") + } + // Only add these to the scheme if they are available + if cfg.PrometheusCRAvailability() == prometheus.Available { + setupLog.Info("Prometheus CRDs are installed, adding to scheme.") + utilruntime.Must(monitoringv1.AddToScheme(scheme)) + } else { + setupLog.Info("Prometheus CRDs are not installed, skipping adding to scheme.") + } + if cfg.OpenShiftRoutesAvailability() == openshift.RoutesAvailable { + setupLog.Info("Openshift CRDs are installed, adding to scheme.") + utilruntime.Must(routev1.Install(scheme)) + } else { + setupLog.Info("Openshift CRDs are not installed, skipping adding to scheme.") + } + + err = addDependencies(ctx, mgr, cfg, v) + if err != nil { + setupLog.Error(err, "failed to add/run bootstrap dependencies to the controller manager") + os.Exit(1) } - reviewer := rbac.NewReviewer(clientset) if err = controllers.NewReconciler(controllers.Params{ Client: mgr.GetClient(), @@ -410,3 +427,32 @@ func tlsConfigSetting(cfg *tls.Config, tlsOpt tlsConfig) { } cfg.CipherSuites = cipherSuiteIDs } + +func checkRbacPermissions(reviewer *rbac.Reviewer, ctx context.Context) (admission.Warnings, error) { + notPossibleToCheck := "unable to check rbac rules:" + + namespace, err := autodetect.GetOperatorNamespace() + if err != nil { + return nil, fmt.Errorf("%s: %w", notPossibleToCheck, err) + } + + serviceAccount, err := autodetect.GetOperatorServiceAccount() + if err != nil { + return nil, fmt.Errorf("%s: %w", notPossibleToCheck, err) + } + + rules := []*rbacv1.PolicyRule{ + { + APIGroups: []string{"rbac.authorization.k8s.io"}, + Resources: []string{"clusterrolebindings", "clusterroles"}, + Verbs: []string{"create", "delete", "get", "list", "patch", "update"}, + }, + } + + if subjectAccessReviews, err := reviewer.CheckPolicyRules(ctx, serviceAccount, namespace, rules...); err != nil { + return nil, fmt.Errorf("%s: %w", notPossibleToCheck, err) + } else if allowed, deniedReviews := rbac.AllSubjectAccessReviewsAllowed(subjectAccessReviews); !allowed { + return rbac.WarningsGroupedByResource(deniedReviews), nil + } + return nil, nil +} From e0bcbd7ba2367156e12ef59499e4773456ac7677 Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Mon, 8 Apr 2024 16:26:31 +0200 Subject: [PATCH 02/26] Fix bundle Signed-off-by: Israel Blancas --- .../opentelemetry-operator.clusterserviceversion.yaml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml b/bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml index e15503949a..fb677d882b 100644 --- a/bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml +++ b/bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml @@ -65,7 +65,7 @@ metadata: categories: Logging & Tracing,Monitoring certified: "false" containerImage: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator - createdAt: "2024-04-05T17:37:15Z" + createdAt: "2024-04-08T14:26:03Z" description: Provides the OpenTelemetry components, including the Collector operators.operatorframework.io/builder: operator-sdk-v1.29.0 operators.operatorframework.io/project_layout: go.kubebuilder.io/v3 @@ -413,6 +413,11 @@ spec: - --zap-time-encoding=rfc3339nano - --feature-gates=+operator.autoinstrumentation.go - --enable-nginx-instrumentation=true + env: + - name: SERVICE_ACCOUNT_NAME + valueFrom: + fieldRef: + fieldPath: spec.serviceAccountName image: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator:0.97.1 livenessProbe: httpGet: From 0dd7c54ade4e14b233cae9b056b0ed6d5c37c501 Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Mon, 8 Apr 2024 20:00:36 +0200 Subject: [PATCH 03/26] Fix log message Signed-off-by: Israel Blancas --- main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.go b/main.go index 59f8cec490..001951a8ca 100644 --- a/main.go +++ b/main.go @@ -248,7 +248,7 @@ func main() { w, err := checkRbacPermissions(reviewer, ctx) if err != nil { createRBACPermissions = false - setupLog.Info("the operator has not permissions to create rbac resources", "error", err, "serviceAccount") + setupLog.Info("the operator has not permissions to create rbac resources", "error", err) } if w != nil { createRBACPermissions = false From 07545f5bdf5edf7bc4b5e35ef6d0f68619325b1f Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Tue, 9 Apr 2024 16:16:06 +0200 Subject: [PATCH 04/26] Add E2E tests and fix #2833 Signed-off-by: Israel Blancas --- ...x-detector-resourcedetectionprocessor.yaml | 16 +++++ .github/workflows/e2e.yaml | 1 + Makefile | 13 ++++ hack/rbac-operator-permissions.yaml | 67 +++++++++++++++++++ .../processor/processor_resourcedetection.go | 2 +- .../processor-k8sattributes/00-install.yaml | 4 ++ .../processor-k8sattributes/01-assert.yaml | 14 ++++ .../processor-k8sattributes/01-install.yaml | 22 ++++++ .../processor-k8sattributes/02-assert.yaml | 22 ++++++ .../processor-k8sattributes/02-install.yaml | 25 +++++++ .../chainsaw-test.yaml | 24 +++++++ .../00-install.yaml | 4 ++ .../01-assert.yaml | 14 ++++ .../01-install.yaml | 25 +++++++ .../02-assert.yaml | 12 ++++ .../02-install.yaml | 25 +++++++ .../chainsaw-test.yaml | 24 +++++++ 17 files changed, 313 insertions(+), 1 deletion(-) create mode 100755 .chloggen/2833-fix-detector-resourcedetectionprocessor.yaml create mode 100644 hack/rbac-operator-permissions.yaml create mode 100644 tests/e2e-automatic-rbac/processor-k8sattributes/00-install.yaml create mode 100644 tests/e2e-automatic-rbac/processor-k8sattributes/01-assert.yaml create mode 100644 tests/e2e-automatic-rbac/processor-k8sattributes/01-install.yaml create mode 100644 tests/e2e-automatic-rbac/processor-k8sattributes/02-assert.yaml create mode 100644 tests/e2e-automatic-rbac/processor-k8sattributes/02-install.yaml create mode 100644 tests/e2e-automatic-rbac/processor-k8sattributes/chainsaw-test.yaml create mode 100644 tests/e2e-automatic-rbac/processor-resourcedetection/00-install.yaml create mode 100644 tests/e2e-automatic-rbac/processor-resourcedetection/01-assert.yaml create mode 100644 tests/e2e-automatic-rbac/processor-resourcedetection/01-install.yaml create mode 100644 tests/e2e-automatic-rbac/processor-resourcedetection/02-assert.yaml create mode 100644 tests/e2e-automatic-rbac/processor-resourcedetection/02-install.yaml create mode 100644 tests/e2e-automatic-rbac/processor-resourcedetection/chainsaw-test.yaml diff --git a/.chloggen/2833-fix-detector-resourcedetectionprocessor.yaml b/.chloggen/2833-fix-detector-resourcedetectionprocessor.yaml new file mode 100755 index 0000000000..e72d0bd20b --- /dev/null +++ b/.chloggen/2833-fix-detector-resourcedetectionprocessor.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: collector + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: "Use the k8snode detector instead of kubernetes for the automatic RBAC creation for the resourcedetector" + +# One or more tracking issues related to the change +issues: [] + +# (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/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index 7ee5c89d79..7255fefd4b 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -24,6 +24,7 @@ jobs: - "1.29" group: - e2e + - e2e-automatic-rbac - e2e-autoscale - e2e-instrumentation - e2e-opampbridge diff --git a/Makefile b/Makefile index 5a75ce1ea0..9e54af8094 100644 --- a/Makefile +++ b/Makefile @@ -207,6 +207,19 @@ generate: controller-gen e2e: chainsaw $(CHAINSAW) test --test-dir ./tests/e2e +# end-to-end-test for testing automatic RBAC creation +.PHONY: e2e-automatic-rbac +e2e-automatic-rbac: chainsaw + # Add extra needed permissions + kubectl apply -f hack/rbac-operator-permissions.yaml + kubectl rollout restart deployment opentelemetry-operator-controller-manager -n opentelemetry-operator-system + kubectl rollout status deployment opentelemetry-operator-controller-manager -n opentelemetry-operator-system + go run hack/check-operator-ready.go + $(CHAINSAW) test --test-dir ./tests/e2e-automatic-rbac + # Cleanup + kubectl delete -f hack/rbac-operator-permissions.yaml + kubectl rollout restart deployment opentelemetry-operator-controller-manager -n opentelemetry-operator-system + # end-to-end-test for testing autoscale .PHONY: e2e-autoscale e2e-autoscale: chainsaw diff --git a/hack/rbac-operator-permissions.yaml b/hack/rbac-operator-permissions.yaml new file mode 100644 index 0000000000..ca1eb91e08 --- /dev/null +++ b/hack/rbac-operator-permissions.yaml @@ -0,0 +1,67 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: generate-processors-rbac +rules: +- apiGroups: + - + resources: + - namespaces + verbs: + - create + - delete + - get + - list + - patch + - update + - watch +- apiGroups: + - + resources: + - nodes + verbs: + - get + - list + - watch +- apiGroups: + - rbac.authorization.k8s.io + resources: + - clusterrolebindings + - clusterroles + verbs: + - create + - delete + - get + - list + - patch + - update + - watch +- apiGroups: + - apps + resources: + - replicasets + verbs: + - get + - list + - watch +- apiGroups: + - extensions + resources: + - replicasets + verbs: + - get + - list + - watch +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: generate-processors-rbac +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: generate-processors-rbac +subjects: +- kind: ServiceAccount + name: opentelemetry-operator-controller-manager + namespace: opentelemetry-operator-system diff --git a/internal/manifests/collector/parser/processor/processor_resourcedetection.go b/internal/manifests/collector/parser/processor/processor_resourcedetection.go index b9038733c5..4145f6baa6 100644 --- a/internal/manifests/collector/parser/processor/processor_resourcedetection.go +++ b/internal/manifests/collector/parser/processor/processor_resourcedetection.go @@ -63,7 +63,7 @@ func (o *ResourceDetectionParser) GetRBACRules() []rbacv1.PolicyRule { for _, d := range detectors { detectorName := fmt.Sprint(d) switch detectorName { - case "kubernetes": + case "k8snode": policy := rbacv1.PolicyRule{ APIGroups: []string{""}, Resources: []string{"nodes"}, diff --git a/tests/e2e-automatic-rbac/processor-k8sattributes/00-install.yaml b/tests/e2e-automatic-rbac/processor-k8sattributes/00-install.yaml new file mode 100644 index 0000000000..274fa212c1 --- /dev/null +++ b/tests/e2e-automatic-rbac/processor-k8sattributes/00-install.yaml @@ -0,0 +1,4 @@ +apiVersion: v1 +kind: Namespace +metadata: + name: chainsaw-k8sattributes diff --git a/tests/e2e-automatic-rbac/processor-k8sattributes/01-assert.yaml b/tests/e2e-automatic-rbac/processor-k8sattributes/01-assert.yaml new file mode 100644 index 0000000000..12136c3776 --- /dev/null +++ b/tests/e2e-automatic-rbac/processor-k8sattributes/01-assert.yaml @@ -0,0 +1,14 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: simplest-chainsaw-k8sattributes-cluster-role +rules: +- apiGroups: + - "" + resources: + - pods + - namespaces + verbs: + - get + - watch + - list \ No newline at end of file diff --git a/tests/e2e-automatic-rbac/processor-k8sattributes/01-install.yaml b/tests/e2e-automatic-rbac/processor-k8sattributes/01-install.yaml new file mode 100644 index 0000000000..f498429a54 --- /dev/null +++ b/tests/e2e-automatic-rbac/processor-k8sattributes/01-install.yaml @@ -0,0 +1,22 @@ +apiVersion: opentelemetry.io/v1alpha1 +kind: OpenTelemetryCollector +metadata: + name: simplest + namespace: chainsaw-k8sattributes +spec: + config: | + receivers: + otlp: + protocols: + grpc: + http: + processors: + k8sattributes: + exporters: + debug: + service: + pipelines: + traces: + receivers: [otlp] + processors: [k8sattributes] + exporters: [debug] diff --git a/tests/e2e-automatic-rbac/processor-k8sattributes/02-assert.yaml b/tests/e2e-automatic-rbac/processor-k8sattributes/02-assert.yaml new file mode 100644 index 0000000000..113d182397 --- /dev/null +++ b/tests/e2e-automatic-rbac/processor-k8sattributes/02-assert.yaml @@ -0,0 +1,22 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: simplest-chainsaw-k8sattributes-cluster-role +rules: +- apiGroups: + - "" + resources: + - pods + - namespaces + verbs: + - get + - watch + - list +- apiGroups: + - "" + resources: + - nodes + verbs: + - get + - watch + - list \ No newline at end of file diff --git a/tests/e2e-automatic-rbac/processor-k8sattributes/02-install.yaml b/tests/e2e-automatic-rbac/processor-k8sattributes/02-install.yaml new file mode 100644 index 0000000000..612e9676b3 --- /dev/null +++ b/tests/e2e-automatic-rbac/processor-k8sattributes/02-install.yaml @@ -0,0 +1,25 @@ +apiVersion: opentelemetry.io/v1alpha1 +kind: OpenTelemetryCollector +metadata: + name: simplest + namespace: chainsaw-k8sattributes +spec: + config: | + receivers: + otlp: + protocols: + grpc: + http: + processors: + k8sattributes: + extract: + metadata: + - k8s.node.name + exporters: + debug: + service: + pipelines: + traces: + receivers: [otlp] + processors: [k8sattributes] + exporters: [debug] diff --git a/tests/e2e-automatic-rbac/processor-k8sattributes/chainsaw-test.yaml b/tests/e2e-automatic-rbac/processor-k8sattributes/chainsaw-test.yaml new file mode 100644 index 0000000000..b711740eef --- /dev/null +++ b/tests/e2e-automatic-rbac/processor-k8sattributes/chainsaw-test.yaml @@ -0,0 +1,24 @@ +# yaml-language-server: $schema=https://raw.githubusercontent.com/kyverno/chainsaw/main/.schemas/json/test-chainsaw-v1alpha1.json +apiVersion: chainsaw.kyverno.io/v1alpha1 +kind: Test +metadata: + creationTimestamp: null + name: processor-k8sattributes +spec: + steps: + - name: create-namespace + try: + - apply: + file: 01-install.yaml + - name: default-conf + try: + - apply: + file: 01-install.yaml + - assert: + file: 01-assert.yaml + - name: k8s.node + try: + - apply: + file: 02-install.yaml + - assert: + file: 02-assert.yaml diff --git a/tests/e2e-automatic-rbac/processor-resourcedetection/00-install.yaml b/tests/e2e-automatic-rbac/processor-resourcedetection/00-install.yaml new file mode 100644 index 0000000000..412a68b477 --- /dev/null +++ b/tests/e2e-automatic-rbac/processor-resourcedetection/00-install.yaml @@ -0,0 +1,4 @@ +apiVersion: v1 +kind: Namespace +metadata: + name: chainsaw-resourcedetection \ No newline at end of file diff --git a/tests/e2e-automatic-rbac/processor-resourcedetection/01-assert.yaml b/tests/e2e-automatic-rbac/processor-resourcedetection/01-assert.yaml new file mode 100644 index 0000000000..d87a840e05 --- /dev/null +++ b/tests/e2e-automatic-rbac/processor-resourcedetection/01-assert.yaml @@ -0,0 +1,14 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: simplest-chainsaw-resourcedetection-cluster-role +rules: +- apiGroups: + - config.openshift.io + resources: + - infrastructures + - infrastructures/status + verbs: + - get + - watch + - list \ No newline at end of file diff --git a/tests/e2e-automatic-rbac/processor-resourcedetection/01-install.yaml b/tests/e2e-automatic-rbac/processor-resourcedetection/01-install.yaml new file mode 100644 index 0000000000..139126ad34 --- /dev/null +++ b/tests/e2e-automatic-rbac/processor-resourcedetection/01-install.yaml @@ -0,0 +1,25 @@ +apiVersion: opentelemetry.io/v1alpha1 +kind: OpenTelemetryCollector +metadata: + name: simplest + namespace: chainsaw-resourcedetection +spec: + config: | + receivers: + otlp: + protocols: + grpc: + http: + processors: + resourcedetection: + detectors: [openshift] + timeout: 2s + override: false + exporters: + debug: + service: + pipelines: + traces: + receivers: [otlp] + processors: [resourcedetection] + exporters: [debug] diff --git a/tests/e2e-automatic-rbac/processor-resourcedetection/02-assert.yaml b/tests/e2e-automatic-rbac/processor-resourcedetection/02-assert.yaml new file mode 100644 index 0000000000..927e3fdc5a --- /dev/null +++ b/tests/e2e-automatic-rbac/processor-resourcedetection/02-assert.yaml @@ -0,0 +1,12 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: simplest-chainsaw-resourcedetection-cluster-role +rules: +- apiGroups: + - "" + resources: + - nodes + verbs: + - get + - list diff --git a/tests/e2e-automatic-rbac/processor-resourcedetection/02-install.yaml b/tests/e2e-automatic-rbac/processor-resourcedetection/02-install.yaml new file mode 100644 index 0000000000..f155149391 --- /dev/null +++ b/tests/e2e-automatic-rbac/processor-resourcedetection/02-install.yaml @@ -0,0 +1,25 @@ +apiVersion: opentelemetry.io/v1alpha1 +kind: OpenTelemetryCollector +metadata: + name: simplest + namespace: chainsaw-resourcedetection +spec: + config: | + receivers: + otlp: + protocols: + grpc: + http: + processors: + resourcedetection: + detectors: [k8snode] + timeout: 2s + override: false + exporters: + debug: + service: + pipelines: + traces: + receivers: [otlp] + processors: [resourcedetection] + exporters: [debug] diff --git a/tests/e2e-automatic-rbac/processor-resourcedetection/chainsaw-test.yaml b/tests/e2e-automatic-rbac/processor-resourcedetection/chainsaw-test.yaml new file mode 100644 index 0000000000..8d0e7ccbd9 --- /dev/null +++ b/tests/e2e-automatic-rbac/processor-resourcedetection/chainsaw-test.yaml @@ -0,0 +1,24 @@ +# yaml-language-server: $schema=https://raw.githubusercontent.com/kyverno/chainsaw/main/.schemas/json/test-chainsaw-v1alpha1.json +apiVersion: chainsaw.kyverno.io/v1alpha1 +kind: Test +metadata: + creationTimestamp: null + name: processor-resourcedetection +spec: + steps: + - name: create-namespace + try: + - apply: + file: 00-install.yaml + - name: openshift-detector + try: + - apply: + file: 01-install.yaml + - assert: + file: 01-assert.yaml + - name: k8snode-detector + try: + - apply: + file: 02-install.yaml + - assert: + file: 02-assert.yaml From 14b737c8cd590b7982062472d7f7042bfaaad2cd Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Tue, 9 Apr 2024 17:40:53 +0200 Subject: [PATCH 05/26] Fix tests Signed-off-by: Israel Blancas --- internal/manifests/collector/adapters/config_to_rbac_test.go | 2 +- .../collector/testdata/rbac_resourcedetectionprocessor_k8s.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/manifests/collector/adapters/config_to_rbac_test.go b/internal/manifests/collector/adapters/config_to_rbac_test.go index 28774ca846..8260d23648 100644 --- a/internal/manifests/collector/adapters/config_to_rbac_test.go +++ b/internal/manifests/collector/adapters/config_to_rbac_test.go @@ -51,7 +51,7 @@ service: desc: "resourcedetection-processor k8s", config: `processors: resourcedetection: - detectors: [kubernetes] + detectors: [k8snode] service: pipelines: traces: diff --git a/internal/manifests/collector/testdata/rbac_resourcedetectionprocessor_k8s.yaml b/internal/manifests/collector/testdata/rbac_resourcedetectionprocessor_k8s.yaml index 9cd7c5790d..4027b74a15 100644 --- a/internal/manifests/collector/testdata/rbac_resourcedetectionprocessor_k8s.yaml +++ b/internal/manifests/collector/testdata/rbac_resourcedetectionprocessor_k8s.yaml @@ -4,7 +4,7 @@ receivers: grpc: processors: resourcedetection: - detectors: [kubernetes] + detectors: [k8snode] exporters: otlp: endpoint: "otlp:4317" From 730e71f5b7b461559dee781992801ce0c37578dc Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Tue, 9 Apr 2024 18:51:37 +0200 Subject: [PATCH 06/26] Apply changes requested in code review Signed-off-by: Israel Blancas --- controllers/opampbridge_controller_test.go | 4 + controllers/suite_test.go | 9 +++ internal/autodetect/main.go | 28 ++++++- internal/autodetect/main_test.go | 5 +- internal/autodetect/rbac/check.go | 71 ++++++++++++++++++ internal/autodetect/{ => rbac}/operator.go | 33 ++++----- internal/config/main_test.go | 12 +++ internal/rbac/format_test.go | 74 +++++++++++++++++++ main.go | 58 ++++----------- .../chainsaw-test.yaml | 2 +- 10 files changed, 226 insertions(+), 70 deletions(-) create mode 100644 internal/autodetect/rbac/check.go rename internal/autodetect/{ => rbac}/operator.go (57%) create mode 100644 internal/rbac/format_test.go diff --git a/controllers/opampbridge_controller_test.go b/controllers/opampbridge_controller_test.go index 269a43c745..e0533cea63 100644 --- a/controllers/opampbridge_controller_test.go +++ b/controllers/opampbridge_controller_test.go @@ -37,6 +37,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/controllers" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" "github.com/open-telemetry/opentelemetry-operator/internal/config" ) @@ -48,6 +49,9 @@ var opampBridgeMockAutoDetector = &mockAutoDetect{ PrometheusCRsAvailabilityFunc: func() (prometheus.Availability, error) { return prometheus.Available, nil }, + RBACPermissionsFunc: func() (rbac.Availability, error) { + return rbac.Available, nil + }, } func TestNewObjectsOnReconciliation_OpAMPBridge(t *testing.T) { diff --git a/controllers/suite_test.go b/controllers/suite_test.go index f36f47d262..37f032d41c 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -55,6 +55,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" + autoRbac "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" "github.com/open-telemetry/opentelemetry-operator/internal/config" "github.com/open-telemetry/opentelemetry-operator/internal/manifests" "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/testdata" @@ -94,6 +95,7 @@ var _ autodetect.AutoDetect = (*mockAutoDetect)(nil) type mockAutoDetect struct { OpenShiftRoutesAvailabilityFunc func() (openshift.RoutesAvailability, error) PrometheusCRsAvailabilityFunc func() (prometheus.Availability, error) + RBACPermissionsFunc func() (autoRbac.Availability, error) } func (m *mockAutoDetect) PrometheusCRsAvailability() (prometheus.Availability, error) { @@ -110,6 +112,13 @@ func (m *mockAutoDetect) OpenShiftRoutesAvailability() (openshift.RoutesAvailabi return openshift.RoutesNotAvailable, nil } +func (m *mockAutoDetect) RBACPermissions() (autoRbac.Availability, error) { + if m.RBACPermissionsFunc != nil { + return m.RBACPermissionsFunc() + } + return autoRbac.NotAvailable, nil +} + func TestMain(m *testing.M) { ctx, cancel = context.WithCancel(context.TODO()) defer cancel() diff --git a/internal/autodetect/main.go b/internal/autodetect/main.go index e0dec72245..a279f28ac7 100644 --- a/internal/autodetect/main.go +++ b/internal/autodetect/main.go @@ -16,11 +16,16 @@ package autodetect import ( + "context" + "fmt" + "k8s.io/client-go/discovery" "k8s.io/client-go/rest" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" + autoRbac "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" + "github.com/open-telemetry/opentelemetry-operator/internal/rbac" ) var _ AutoDetect = (*autoDetect)(nil) @@ -29,14 +34,17 @@ var _ AutoDetect = (*autoDetect)(nil) type AutoDetect interface { OpenShiftRoutesAvailability() (openshift.RoutesAvailability, error) PrometheusCRsAvailability() (prometheus.Availability, error) + RBACPermissions() (autoRbac.Availability, error) } type autoDetect struct { - dcl discovery.DiscoveryInterface + ctx context.Context + dcl discovery.DiscoveryInterface + reviewer *rbac.Reviewer } // New creates a new auto-detection worker, using the given client when talking to the current cluster. -func New(restConfig *rest.Config) (AutoDetect, error) { +func New(restConfig *rest.Config, ctx context.Context, reviewer *rbac.Reviewer) (AutoDetect, error) { dcl, err := discovery.NewDiscoveryClientForConfig(restConfig) if err != nil { // it's pretty much impossible to get into this problem, as most of the @@ -46,7 +54,9 @@ func New(restConfig *rest.Config) (AutoDetect, error) { } return &autoDetect{ - dcl: dcl, + ctx: ctx, + dcl: dcl, + reviewer: reviewer, }, nil } @@ -83,3 +93,15 @@ func (a *autoDetect) OpenShiftRoutesAvailability() (openshift.RoutesAvailability return openshift.RoutesNotAvailable, nil } + +func (a *autoDetect) RBACPermissions() (autoRbac.Availability, error) { + w, err := autoRbac.CheckRbacPermissions(a.reviewer, a.ctx) + if err != nil { + return autoRbac.NotAvailable, err + } + if w != nil { + return autoRbac.NotAvailable, fmt.Errorf("missing permissions: %s", w) + } + + return autoRbac.Available, nil +} diff --git a/internal/autodetect/main_test.go b/internal/autodetect/main_test.go index 6b1e22369a..2217fedd1d 100644 --- a/internal/autodetect/main_test.go +++ b/internal/autodetect/main_test.go @@ -15,6 +15,7 @@ package autodetect_test import ( + "context" "encoding/json" "net/http" "net/http/httptest" @@ -61,7 +62,7 @@ func TestDetectPlatformBasedOnAvailableAPIGroups(t *testing.T) { })) defer server.Close() - autoDetect, err := autodetect.New(&rest.Config{Host: server.URL}) + autoDetect, err := autodetect.New(&rest.Config{Host: server.URL}, context.TODO(), nil) require.NoError(t, err) // test @@ -104,7 +105,7 @@ func TestDetectPlatformBasedOnAvailableAPIGroupsPrometheus(t *testing.T) { })) defer server.Close() - autoDetect, err := autodetect.New(&rest.Config{Host: server.URL}) + autoDetect, err := autodetect.New(&rest.Config{Host: server.URL}, nil, nil) require.NoError(t, err) // test diff --git a/internal/autodetect/rbac/check.go b/internal/autodetect/rbac/check.go new file mode 100644 index 0000000000..66603c6bed --- /dev/null +++ b/internal/autodetect/rbac/check.go @@ -0,0 +1,71 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package rbac + +import ( + "context" + "fmt" + "os" + + rbacv1 "k8s.io/api/rbac/v1" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "github.com/open-telemetry/opentelemetry-operator/internal/rbac" +) + +func getOperatorNamespace() (string, error) { + nsBytes, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace") + if err != nil { + return "", err + } + return string(nsBytes), nil +} + +func getOperatorServiceAccount() (string, error) { + sa := os.Getenv(saEnvVar) + if sa == "" { + return sa, fmt.Errorf("%s env variable not found", saEnvVar) + } + return sa, nil +} + +func CheckRbacPermissions(reviewer *rbac.Reviewer, ctx context.Context) (admission.Warnings, error) { + notPossibleToCheck := "unable to check rbac rules:" + + namespace, err := getOperatorNamespace() + if err != nil { + return nil, fmt.Errorf("%s: %w", notPossibleToCheck, err) + } + + serviceAccount, err := getOperatorServiceAccount() + if err != nil { + return nil, fmt.Errorf("%s: %w", notPossibleToCheck, err) + } + + rules := []*rbacv1.PolicyRule{ + { + APIGroups: []string{"rbac.authorization.k8s.io"}, + Resources: []string{"clusterrolebindings", "clusterroles"}, + Verbs: []string{"create", "delete", "get", "list", "patch", "update"}, + }, + } + + if subjectAccessReviews, err := reviewer.CheckPolicyRules(ctx, serviceAccount, namespace, rules...); err != nil { + return nil, fmt.Errorf("%s: %w", notPossibleToCheck, err) + } else if allowed, deniedReviews := rbac.AllSubjectAccessReviewsAllowed(subjectAccessReviews); !allowed { + return rbac.WarningsGroupedByResource(deniedReviews), nil + } + return nil, nil +} diff --git a/internal/autodetect/operator.go b/internal/autodetect/rbac/operator.go similarity index 57% rename from internal/autodetect/operator.go rename to internal/autodetect/rbac/operator.go index 13a4ea0a72..b3614f6252 100644 --- a/internal/autodetect/operator.go +++ b/internal/autodetect/rbac/operator.go @@ -12,26 +12,21 @@ // See the License for the specific language governing permissions and // limitations under the License. -package autodetect +package rbac -import ( - "fmt" - "os" -) +const saEnvVar = "SERVICE_ACCOUNT_NAME" -func GetOperatorNamespace() (string, error) { - nsBytes, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace") - if err != nil { - return "", err - } - return string(nsBytes), nil -} +// Availability represents that the opeerator service account has permissions to create RBAC resources. +type Availability int + +const ( + // NotAvailable RBAC permissions are not available. + NotAvailable Availability = iota + + // Available NotAvailable RBAC permissions are available. + Available +) -func GetOperatorServiceAccount() (string, error) { - saEnvVar := "SERVICE_ACCOUNT_NAME" - sa := os.Getenv(saEnvVar) - if sa == "" { - return sa, fmt.Errorf("%s env variable not found", saEnvVar) - } - return sa, nil +func (p Availability) String() string { + return [...]string{"NotAvailable", "Available"}[p] } diff --git a/internal/config/main_test.go b/internal/config/main_test.go index 7c22e65687..86b3046fbf 100644 --- a/internal/config/main_test.go +++ b/internal/config/main_test.go @@ -23,6 +23,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" "github.com/open-telemetry/opentelemetry-operator/internal/config" ) @@ -51,6 +52,9 @@ func TestConfigChangesOnAutoDetect(t *testing.T) { PrometheusCRsAvailabilityFunc: func() (prometheus.Availability, error) { return prometheus.Available, nil }, + RBACPermissionsFunc: func() (rbac.Availability, error) { + return rbac.Available, nil + }, } cfg := config.New( config.WithAutoDetect(mock), @@ -74,6 +78,7 @@ var _ autodetect.AutoDetect = (*mockAutoDetect)(nil) type mockAutoDetect struct { OpenShiftRoutesAvailabilityFunc func() (openshift.RoutesAvailability, error) PrometheusCRsAvailabilityFunc func() (prometheus.Availability, error) + RBACPermissionsFunc func() (rbac.Availability, error) } func (m *mockAutoDetect) OpenShiftRoutesAvailability() (openshift.RoutesAvailability, error) { @@ -89,3 +94,10 @@ func (m *mockAutoDetect) PrometheusCRsAvailability() (prometheus.Availability, e } return prometheus.NotAvailable, nil } + +func (m *mockAutoDetect) RBACPermissions() (rbac.Availability, error) { + if m.RBACPermissionsFunc != nil { + return m.RBACPermissionsFunc() + } + return rbac.NotAvailable, nil +} diff --git a/internal/rbac/format_test.go b/internal/rbac/format_test.go new file mode 100644 index 0000000000..82f97d25fe --- /dev/null +++ b/internal/rbac/format_test.go @@ -0,0 +1,74 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package rbac + +import ( + "testing" + + "github.com/stretchr/testify/assert" + v1 "k8s.io/api/authorization/v1" +) + +func TestWarningsGroupedByResource(t *testing.T) { + tests := []struct { + desc string + reviews []*v1.SubjectAccessReview + expected []string + }{ + { + desc: "No access reviews", + reviews: nil, + expected: nil, + }, + { + desc: "One access review with resource attributes", + reviews: []*v1.SubjectAccessReview{ + { + Spec: v1.SubjectAccessReviewSpec{ + ResourceAttributes: &v1.ResourceAttributes{ + Verb: "get", + Group: "", + Resource: "namespaces", + }, + }, + }, + }, + expected: []string{"missing the following rules for namespaces: [get]"}, + }, + { + desc: "One access review with non resource attributes", + reviews: []*v1.SubjectAccessReview{ + { + Spec: v1.SubjectAccessReviewSpec{ + ResourceAttributes: &v1.ResourceAttributes{ + Verb: "watch", + Group: "apps", + Resource: "replicasets", + }, + }, + }, + }, + expected: []string{"missing the following rules for apps/replicasets: [watch]"}, + }, + } + + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + w := WarningsGroupedByResource(tt.reviews) + assert.Equal(t, tt.expected, w) + }) + } + +} diff --git a/main.go b/main.go index 001951a8ca..ae8937d64d 100644 --- a/main.go +++ b/main.go @@ -29,7 +29,6 @@ import ( "github.com/spf13/pflag" colfeaturegate "go.opentelemetry.io/collector/featuregate" networkingv1 "k8s.io/api/networking/v1" - rbacv1 "k8s.io/api/rbac/v1" k8sruntime "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/client-go/kubernetes" @@ -51,6 +50,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" + autoRbac "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" "github.com/open-telemetry/opentelemetry-operator/internal/config" "github.com/open-telemetry/opentelemetry-operator/internal/rbac" "github.com/open-telemetry/opentelemetry-operator/internal/version" @@ -243,29 +243,26 @@ func main() { } reviewer := rbac.NewReviewer(clientset) - createRBACPermissions = true ctx := ctrl.SetupSignalHandler() - w, err := checkRbacPermissions(reviewer, ctx) - if err != nil { - createRBACPermissions = false - setupLog.Info("the operator has not permissions to create rbac resources", "error", err) - } - if w != nil { - createRBACPermissions = false - setupLog.Info("the operator has not permissions to create rbac resources", "permissions", w) - } - - if createRBACPermissions { - setupLog.Info("the operator has permissions to create rbac resources") - } // builds the operator's configuration - ad, err := autodetect.New(restConfig) + ad, err := autodetect.New(restConfig, ctx, reviewer) if err != nil { setupLog.Error(err, "failed to setup auto-detect routine") os.Exit(1) } + rbacAvailable, err := ad.RBACPermissions() + if err != nil { + createRBACPermissions = false + setupLog.Info("error checking if the operator has permissions to create RBAC resources", "reason", err) + } else if rbacAvailable == autoRbac.NotAvailable { + createRBACPermissions = false + setupLog.Info("The operator has not permissions to create RBAC resources, skipping.") + } else { + createRBACPermissions = true + } + cfg := config.New( config.WithLogger(ctrl.Log.WithName("config")), config.WithVersion(v), @@ -427,32 +424,3 @@ func tlsConfigSetting(cfg *tls.Config, tlsOpt tlsConfig) { } cfg.CipherSuites = cipherSuiteIDs } - -func checkRbacPermissions(reviewer *rbac.Reviewer, ctx context.Context) (admission.Warnings, error) { - notPossibleToCheck := "unable to check rbac rules:" - - namespace, err := autodetect.GetOperatorNamespace() - if err != nil { - return nil, fmt.Errorf("%s: %w", notPossibleToCheck, err) - } - - serviceAccount, err := autodetect.GetOperatorServiceAccount() - if err != nil { - return nil, fmt.Errorf("%s: %w", notPossibleToCheck, err) - } - - rules := []*rbacv1.PolicyRule{ - { - APIGroups: []string{"rbac.authorization.k8s.io"}, - Resources: []string{"clusterrolebindings", "clusterroles"}, - Verbs: []string{"create", "delete", "get", "list", "patch", "update"}, - }, - } - - if subjectAccessReviews, err := reviewer.CheckPolicyRules(ctx, serviceAccount, namespace, rules...); err != nil { - return nil, fmt.Errorf("%s: %w", notPossibleToCheck, err) - } else if allowed, deniedReviews := rbac.AllSubjectAccessReviewsAllowed(subjectAccessReviews); !allowed { - return rbac.WarningsGroupedByResource(deniedReviews), nil - } - return nil, nil -} diff --git a/tests/e2e-automatic-rbac/processor-k8sattributes/chainsaw-test.yaml b/tests/e2e-automatic-rbac/processor-k8sattributes/chainsaw-test.yaml index b711740eef..efceb94533 100644 --- a/tests/e2e-automatic-rbac/processor-k8sattributes/chainsaw-test.yaml +++ b/tests/e2e-automatic-rbac/processor-k8sattributes/chainsaw-test.yaml @@ -9,7 +9,7 @@ spec: - name: create-namespace try: - apply: - file: 01-install.yaml + file: 00-install.yaml - name: default-conf try: - apply: From a51d162f8ba6a1d7d0b78e53043a36b0e388ad11 Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Tue, 9 Apr 2024 18:54:57 +0200 Subject: [PATCH 07/26] Fix changelog Signed-off-by: Israel Blancas --- .chloggen/2833-fix-detector-resourcedetectionprocessor.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.chloggen/2833-fix-detector-resourcedetectionprocessor.yaml b/.chloggen/2833-fix-detector-resourcedetectionprocessor.yaml index e72d0bd20b..effa51536f 100755 --- a/.chloggen/2833-fix-detector-resourcedetectionprocessor.yaml +++ b/.chloggen/2833-fix-detector-resourcedetectionprocessor.yaml @@ -8,7 +8,7 @@ component: collector note: "Use the k8snode detector instead of kubernetes for the automatic RBAC creation for the resourcedetector" # One or more tracking issues related to the change -issues: [] +issues: [2833] # (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. From 9e764e9065cbe865e7d20c9a1a3a83c488a80183 Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Tue, 9 Apr 2024 19:07:18 +0200 Subject: [PATCH 08/26] Fix yaml Signed-off-by: Israel Blancas --- hack/rbac-operator-permissions.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hack/rbac-operator-permissions.yaml b/hack/rbac-operator-permissions.yaml index ca1eb91e08..1370fa9ec9 100644 --- a/hack/rbac-operator-permissions.yaml +++ b/hack/rbac-operator-permissions.yaml @@ -4,7 +4,7 @@ metadata: name: generate-processors-rbac rules: - apiGroups: - - + - "" resources: - namespaces verbs: @@ -16,7 +16,7 @@ rules: - update - watch - apiGroups: - - + - "" resources: - nodes verbs: From a12aade2316541c7f0be674704c53980378b2321 Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Wed, 10 Apr 2024 16:43:00 +0200 Subject: [PATCH 09/26] Apply changes requested in code review Signed-off-by: Israel Blancas --- controllers/opampbridge_controller_test.go | 2 +- .../opentelemetrycollector_controller.go | 3 +- controllers/suite_test.go | 6 +- internal/autodetect/main.go | 10 +- internal/autodetect/main_test.go | 115 +++++++++++++++++- internal/autodetect/rbac/check.go | 27 ++-- internal/autodetect/rbac/operator.go | 2 - internal/config/main.go | 23 ++-- internal/config/main_test.go | 9 +- internal/config/options.go | 14 ++- internal/manifests/collector/collector.go | 3 +- main.go | 15 +-- 12 files changed, 173 insertions(+), 56 deletions(-) diff --git a/controllers/opampbridge_controller_test.go b/controllers/opampbridge_controller_test.go index e0533cea63..99f4f43a78 100644 --- a/controllers/opampbridge_controller_test.go +++ b/controllers/opampbridge_controller_test.go @@ -49,7 +49,7 @@ var opampBridgeMockAutoDetector = &mockAutoDetect{ PrometheusCRsAvailabilityFunc: func() (prometheus.Availability, error) { return prometheus.Available, nil }, - RBACPermissionsFunc: func() (rbac.Availability, error) { + RBACPermissionsFunc: func(ctx context.Context) (rbac.Availability, error) { return rbac.Available, nil }, } diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index fac87a6f86..74e6a318fe 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -40,6 +40,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" "github.com/open-telemetry/opentelemetry-operator/internal/config" "github.com/open-telemetry/opentelemetry-operator/internal/manifests" "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector" @@ -245,7 +246,7 @@ func (r *OpenTelemetryCollectorReconciler) SetupWithManager(mgr ctrl.Manager) er Owns(&autoscalingv2.HorizontalPodAutoscaler{}). Owns(&policyV1.PodDisruptionBudget{}) - if r.config.CreateRBACPermissions() { + if r.config.CreateRBACPermissions() == rbac.Available { builder.Owns(&rbacv1.ClusterRoleBinding{}) builder.Owns(&rbacv1.ClusterRole{}) } diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 37f032d41c..ad389854c4 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -95,7 +95,7 @@ var _ autodetect.AutoDetect = (*mockAutoDetect)(nil) type mockAutoDetect struct { OpenShiftRoutesAvailabilityFunc func() (openshift.RoutesAvailability, error) PrometheusCRsAvailabilityFunc func() (prometheus.Availability, error) - RBACPermissionsFunc func() (autoRbac.Availability, error) + RBACPermissionsFunc func(ctx context.Context) (autoRbac.Availability, error) } func (m *mockAutoDetect) PrometheusCRsAvailability() (prometheus.Availability, error) { @@ -112,9 +112,9 @@ func (m *mockAutoDetect) OpenShiftRoutesAvailability() (openshift.RoutesAvailabi return openshift.RoutesNotAvailable, nil } -func (m *mockAutoDetect) RBACPermissions() (autoRbac.Availability, error) { +func (m *mockAutoDetect) RBACPermissions(ctx context.Context) (autoRbac.Availability, error) { if m.RBACPermissionsFunc != nil { - return m.RBACPermissionsFunc() + return m.RBACPermissionsFunc(ctx) } return autoRbac.NotAvailable, nil } diff --git a/internal/autodetect/main.go b/internal/autodetect/main.go index a279f28ac7..003dfd43d9 100644 --- a/internal/autodetect/main.go +++ b/internal/autodetect/main.go @@ -34,17 +34,16 @@ var _ AutoDetect = (*autoDetect)(nil) type AutoDetect interface { OpenShiftRoutesAvailability() (openshift.RoutesAvailability, error) PrometheusCRsAvailability() (prometheus.Availability, error) - RBACPermissions() (autoRbac.Availability, error) + RBACPermissions(ctx context.Context) (autoRbac.Availability, error) } type autoDetect struct { - ctx context.Context dcl discovery.DiscoveryInterface reviewer *rbac.Reviewer } // New creates a new auto-detection worker, using the given client when talking to the current cluster. -func New(restConfig *rest.Config, ctx context.Context, reviewer *rbac.Reviewer) (AutoDetect, error) { +func New(restConfig *rest.Config, reviewer *rbac.Reviewer) (AutoDetect, error) { dcl, err := discovery.NewDiscoveryClientForConfig(restConfig) if err != nil { // it's pretty much impossible to get into this problem, as most of the @@ -54,7 +53,6 @@ func New(restConfig *rest.Config, ctx context.Context, reviewer *rbac.Reviewer) } return &autoDetect{ - ctx: ctx, dcl: dcl, reviewer: reviewer, }, nil @@ -94,8 +92,8 @@ func (a *autoDetect) OpenShiftRoutesAvailability() (openshift.RoutesAvailability return openshift.RoutesNotAvailable, nil } -func (a *autoDetect) RBACPermissions() (autoRbac.Availability, error) { - w, err := autoRbac.CheckRbacPermissions(a.reviewer, a.ctx) +func (a *autoDetect) RBACPermissions(ctx context.Context) (autoRbac.Availability, error) { + w, err := autoRbac.CheckRbacPermissions(ctx, a.reviewer) if err != nil { return autoRbac.NotAvailable, err } diff --git a/internal/autodetect/main_test.go b/internal/autodetect/main_test.go index 2217fedd1d..88d7703a53 100644 --- a/internal/autodetect/main_test.go +++ b/internal/autodetect/main_test.go @@ -17,18 +17,26 @@ package autodetect_test import ( "context" "encoding/json" + "fmt" "net/http" "net/http/httptest" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + v1 "k8s.io/api/authorization/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/rest" + kubeTesting "k8s.io/client-go/testing" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" + autoRbac "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" + "github.com/open-telemetry/opentelemetry-operator/internal/rbac" ) func TestDetectPlatformBasedOnAvailableAPIGroups(t *testing.T) { @@ -62,7 +70,7 @@ func TestDetectPlatformBasedOnAvailableAPIGroups(t *testing.T) { })) defer server.Close() - autoDetect, err := autodetect.New(&rest.Config{Host: server.URL}, context.TODO(), nil) + autoDetect, err := autodetect.New(&rest.Config{Host: server.URL}, nil) require.NoError(t, err) // test @@ -105,7 +113,7 @@ func TestDetectPlatformBasedOnAvailableAPIGroupsPrometheus(t *testing.T) { })) defer server.Close() - autoDetect, err := autodetect.New(&rest.Config{Host: server.URL}, nil, nil) + autoDetect, err := autodetect.New(&rest.Config{Host: server.URL}, nil) require.NoError(t, err) // test @@ -116,3 +124,106 @@ func TestDetectPlatformBasedOnAvailableAPIGroupsPrometheus(t *testing.T) { assert.Equal(t, tt.expected, ora) } } + +type fakeClientGenerator func() kubernetes.Interface + +const ( + createVerb = "create" + sarResource = "subjectaccessreviews" +) + +func reactorFactory(status v1.SubjectAccessReviewStatus) fakeClientGenerator { + return func() kubernetes.Interface { + c := fake.NewSimpleClientset() + c.PrependReactor(createVerb, sarResource, func(action kubeTesting.Action) (handled bool, ret runtime.Object, err error) { + // check our expectation here + if !action.Matches(createVerb, sarResource) { + return false, nil, fmt.Errorf("must be a create for a SAR") + } + sar, ok := action.(kubeTesting.CreateAction).GetObject().DeepCopyObject().(*v1.SubjectAccessReview) + if !ok || sar == nil { + return false, nil, fmt.Errorf("bad object") + } + sar.Status = status + return true, sar, nil + }) + return c + } +} + +func TestDetectRBACPermissionsBasedOnAvailableClusterRoles(t *testing.T) { + + for _, tt := range []struct { + description string + expectedAvailability autoRbac.Availability + shouldError bool + namespace string + serviceAccount string + clientGenerator fakeClientGenerator + }{ + { + description: "Not possible to read the namespace", + namespace: "default", + shouldError: true, + expectedAvailability: autoRbac.NotAvailable, + clientGenerator: reactorFactory(v1.SubjectAccessReviewStatus{ + Allowed: true, + }), + }, + { + description: "Not possible to read the service account", + serviceAccount: "default", + shouldError: true, + clientGenerator: reactorFactory(v1.SubjectAccessReviewStatus{ + Allowed: true, + }), + }, + { + description: "RBAC resources are NOT there", + + shouldError: true, + namespace: "default", + serviceAccount: "defaultSA", + clientGenerator: reactorFactory(v1.SubjectAccessReviewStatus{ + Allowed: false, + }), + expectedAvailability: autoRbac.NotAvailable, + }, + { + description: "RBAC resources are there", + + shouldError: false, + namespace: "default", + serviceAccount: "defaultSA", + clientGenerator: reactorFactory(v1.SubjectAccessReviewStatus{ + Allowed: true, + }), + expectedAvailability: autoRbac.Available, + }, + } { + t.Run(tt.description, func(t *testing.T) { + // These settings can be get from env vars + t.Setenv(autoRbac.NAMESPACE_ENV_VAR, tt.namespace) + t.Setenv(autoRbac.SA_ENV_VAR, tt.serviceAccount) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {})) + defer server.Close() + + r := rbac.NewReviewer(tt.clientGenerator()) + + aD, err := autodetect.New(&rest.Config{Host: server.URL}, r) + require.NoError(t, err) + + // test + rAuto, err := aD.RBACPermissions(context.Background()) + + // verify + assert.Equal(t, tt.expectedAvailability, rAuto) + if tt.shouldError { + require.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} diff --git a/internal/autodetect/rbac/check.go b/internal/autodetect/rbac/check.go index 66603c6bed..945cd43fde 100644 --- a/internal/autodetect/rbac/check.go +++ b/internal/autodetect/rbac/check.go @@ -25,8 +25,19 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/rbac" ) +const ( + SA_ENV_VAR = "SERVICE_ACCOUNT_NAME" + NAMESPACE_ENV_VAR = "NAMESPACE" + NAMESPACE_FILE_PATH = "/var/run/secrets/kubernetes.io/serviceaccount/namespace" +) + func getOperatorNamespace() (string, error) { - nsBytes, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace") + namespace := os.Getenv(NAMESPACE_ENV_VAR) + if namespace != "" { + return namespace, nil + } + + nsBytes, err := os.ReadFile(NAMESPACE_FILE_PATH) if err != nil { return "", err } @@ -34,24 +45,22 @@ func getOperatorNamespace() (string, error) { } func getOperatorServiceAccount() (string, error) { - sa := os.Getenv(saEnvVar) + sa := os.Getenv(SA_ENV_VAR) if sa == "" { - return sa, fmt.Errorf("%s env variable not found", saEnvVar) + return sa, fmt.Errorf("%s env variable not found", SA_ENV_VAR) } return sa, nil } -func CheckRbacPermissions(reviewer *rbac.Reviewer, ctx context.Context) (admission.Warnings, error) { - notPossibleToCheck := "unable to check rbac rules:" - +func CheckRbacPermissions(ctx context.Context, reviewer *rbac.Reviewer) (admission.Warnings, error) { namespace, err := getOperatorNamespace() if err != nil { - return nil, fmt.Errorf("%s: %w", notPossibleToCheck, err) + return nil, fmt.Errorf("%s: %w", "not possible to check RBAC rules", err) } serviceAccount, err := getOperatorServiceAccount() if err != nil { - return nil, fmt.Errorf("%s: %w", notPossibleToCheck, err) + return nil, fmt.Errorf("%s: %w", "not possible to check RBAC rules", err) } rules := []*rbacv1.PolicyRule{ @@ -63,7 +72,7 @@ func CheckRbacPermissions(reviewer *rbac.Reviewer, ctx context.Context) (admissi } if subjectAccessReviews, err := reviewer.CheckPolicyRules(ctx, serviceAccount, namespace, rules...); err != nil { - return nil, fmt.Errorf("%s: %w", notPossibleToCheck, err) + return nil, fmt.Errorf("%s: %w", "unable to check rbac rules", err) } else if allowed, deniedReviews := rbac.AllSubjectAccessReviewsAllowed(subjectAccessReviews); !allowed { return rbac.WarningsGroupedByResource(deniedReviews), nil } diff --git a/internal/autodetect/rbac/operator.go b/internal/autodetect/rbac/operator.go index b3614f6252..03d0b892ea 100644 --- a/internal/autodetect/rbac/operator.go +++ b/internal/autodetect/rbac/operator.go @@ -14,8 +14,6 @@ package rbac -const saEnvVar = "SERVICE_ACCOUNT_NAME" - // Availability represents that the opeerator service account has permissions to create RBAC resources. type Availability int diff --git a/internal/config/main.go b/internal/config/main.go index aa64953463..ae0d4ef61e 100644 --- a/internal/config/main.go +++ b/internal/config/main.go @@ -16,6 +16,7 @@ package config import ( + "context" "time" "github.com/go-logr/logr" @@ -24,6 +25,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" + autoRbac "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" "github.com/open-telemetry/opentelemetry-operator/internal/version" ) @@ -43,7 +45,7 @@ type Config struct { autoInstrumentationPythonImage string collectorImage string collectorConfigMapEntry string - createRBACPermissions bool + createRBACPermissions autoRbac.Availability enableMultiInstrumentation bool enableApacheHttpdInstrumentation bool enableDotNetInstrumentation bool @@ -57,10 +59,11 @@ type Config struct { operatorOpAMPBridgeConfigMapEntry string autoInstrumentationNodeJSImage string autoInstrumentationJavaImage string - openshiftRoutesAvailability openshift.RoutesAvailability - prometheusCRAvailability prometheus.Availability - labelsFilter []string - annotationsFilter []string + + openshiftRoutesAvailability openshift.RoutesAvailability + prometheusCRAvailability prometheus.Availability + labelsFilter []string + annotationsFilter []string } // New constructs a new configuration based on the given options. @@ -83,7 +86,6 @@ func New(opts ...Option) Config { autoDetect: o.autoDetect, collectorImage: o.collectorImage, collectorConfigMapEntry: o.collectorConfigMapEntry, - createRBACPermissions: o.createRBACPermissions, enableMultiInstrumentation: o.enableMultiInstrumentation, enableApacheHttpdInstrumentation: o.enableApacheHttpdInstrumentation, enableDotNetInstrumentation: o.enableDotNetInstrumentation, @@ -123,6 +125,13 @@ func (c *Config) AutoDetect() error { return err } c.prometheusCRAvailability = pcrd + + rAuto, err := c.autoDetect.RBACPermissions(context.Background()) + if err != nil { + return err + } + c.createRBACPermissions = rAuto + return nil } @@ -162,7 +171,7 @@ func (c *Config) CollectorConfigMapEntry() string { } // CreateRBACPermissions is true when the operator can create RBAC permissions for SAs running a collector instance. Immutable. -func (c *Config) CreateRBACPermissions() bool { +func (c *Config) CreateRBACPermissions() autoRbac.Availability { return c.createRBACPermissions } diff --git a/internal/config/main_test.go b/internal/config/main_test.go index 86b3046fbf..1f3886f776 100644 --- a/internal/config/main_test.go +++ b/internal/config/main_test.go @@ -15,6 +15,7 @@ package config_test import ( + "context" "testing" "github.com/stretchr/testify/assert" @@ -52,7 +53,7 @@ func TestConfigChangesOnAutoDetect(t *testing.T) { PrometheusCRsAvailabilityFunc: func() (prometheus.Availability, error) { return prometheus.Available, nil }, - RBACPermissionsFunc: func() (rbac.Availability, error) { + RBACPermissionsFunc: func(ctx context.Context) (rbac.Availability, error) { return rbac.Available, nil }, } @@ -78,7 +79,7 @@ var _ autodetect.AutoDetect = (*mockAutoDetect)(nil) type mockAutoDetect struct { OpenShiftRoutesAvailabilityFunc func() (openshift.RoutesAvailability, error) PrometheusCRsAvailabilityFunc func() (prometheus.Availability, error) - RBACPermissionsFunc func() (rbac.Availability, error) + RBACPermissionsFunc func(ctx context.Context) (rbac.Availability, error) } func (m *mockAutoDetect) OpenShiftRoutesAvailability() (openshift.RoutesAvailability, error) { @@ -95,9 +96,9 @@ func (m *mockAutoDetect) PrometheusCRsAvailability() (prometheus.Availability, e return prometheus.NotAvailable, nil } -func (m *mockAutoDetect) RBACPermissions() (rbac.Availability, error) { +func (m *mockAutoDetect) RBACPermissions(ctx context.Context) (rbac.Availability, error) { if m.RBACPermissionsFunc != nil { - return m.RBACPermissionsFunc() + return m.RBACPermissionsFunc(ctx) } return rbac.NotAvailable, nil } diff --git a/internal/config/options.go b/internal/config/options.go index 6969081699..08eb8db876 100644 --- a/internal/config/options.go +++ b/internal/config/options.go @@ -23,6 +23,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" + autoRbac "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" "github.com/open-telemetry/opentelemetry-operator/internal/version" ) @@ -42,7 +43,7 @@ type options struct { autoInstrumentationNginxImage string collectorImage string collectorConfigMapEntry string - createRBACPermissions bool + createRBACPermissions autoRbac.Availability enableMultiInstrumentation bool enableApacheHttpdInstrumentation bool enableDotNetInstrumentation bool @@ -83,11 +84,6 @@ func WithCollectorConfigMapEntry(s string) Option { o.collectorConfigMapEntry = s } } -func WithCreateRBACPermissions(s bool) Option { - return func(o *options) { - o.createRBACPermissions = s - } -} func WithEnableMultiInstrumentation(s bool) Option { return func(o *options) { o.enableMultiInstrumentation = s @@ -188,6 +184,12 @@ func WithPrometheusCRAvailability(pcrd prometheus.Availability) Option { } } +func WithRBACPermissions(rAuto autoRbac.Availability) Option { + return func(o *options) { + o.createRBACPermissions = rAuto + } +} + func WithLabelFilters(labelFilters []string) Option { return func(o *options) { diff --git a/internal/manifests/collector/collector.go b/internal/manifests/collector/collector.go index 3b984d6918..9cb2302bba 100644 --- a/internal/manifests/collector/collector.go +++ b/internal/manifests/collector/collector.go @@ -18,6 +18,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" + "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" "github.com/open-telemetry/opentelemetry-operator/internal/manifests" "github.com/open-telemetry/opentelemetry-operator/pkg/featuregate" ) @@ -60,7 +61,7 @@ func Build(params manifests.Params) ([]client.Object, error) { } } - if params.Config.CreateRBACPermissions() { + if params.Config.CreateRBACPermissions() == rbac.Available { manifestFactories = append(manifestFactories, manifests.Factory(ClusterRole), manifests.Factory(ClusterRoleBinding), diff --git a/main.go b/main.go index ae8937d64d..4584cf9524 100644 --- a/main.go +++ b/main.go @@ -50,7 +50,6 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" - autoRbac "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" "github.com/open-telemetry/opentelemetry-operator/internal/config" "github.com/open-telemetry/opentelemetry-operator/internal/rbac" "github.com/open-telemetry/opentelemetry-operator/internal/version" @@ -246,28 +245,16 @@ func main() { ctx := ctrl.SetupSignalHandler() // builds the operator's configuration - ad, err := autodetect.New(restConfig, ctx, reviewer) + ad, err := autodetect.New(restConfig, reviewer) if err != nil { setupLog.Error(err, "failed to setup auto-detect routine") os.Exit(1) } - rbacAvailable, err := ad.RBACPermissions() - if err != nil { - createRBACPermissions = false - setupLog.Info("error checking if the operator has permissions to create RBAC resources", "reason", err) - } else if rbacAvailable == autoRbac.NotAvailable { - createRBACPermissions = false - setupLog.Info("The operator has not permissions to create RBAC resources, skipping.") - } else { - createRBACPermissions = true - } - cfg := config.New( config.WithLogger(ctrl.Log.WithName("config")), config.WithVersion(v), config.WithCollectorImage(collectorImage), - config.WithCreateRBACPermissions(createRBACPermissions), config.WithEnableMultiInstrumentation(enableMultiInstrumentation), config.WithEnableApacheHttpdInstrumentation(enableApacheHttpdInstrumentation), config.WithEnableDotNetInstrumentation(enableDotNetInstrumentation), From 4fc272433921c2be5fc92021b2f82c0cb943d36e Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Thu, 11 Apr 2024 11:48:29 +0200 Subject: [PATCH 10/26] Fix k8sattributes processor Signed-off-by: Israel Blancas --- config/manager/kustomization.yaml | 6 +++ config/manager/manager.yaml | 1 + .../processor/processor_k8sattributes.go | 33 ++++++------ .../processor/processor_k8sattributes_test.go | 54 +++++++++++++++---- tests/e2e/smoke-simplest/00-install.yaml | 24 +++++---- 5 files changed, 79 insertions(+), 39 deletions(-) diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 5c5f0b84cb..1f93e9d569 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -1,2 +1,8 @@ resources: - manager.yaml +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +images: +- name: controller + newName: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator + newTag: 0.97.1-7-gcafbc81a diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index 35c9072ffc..ef529ce138 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -34,6 +34,7 @@ spec: - "--zap-time-encoding=rfc3339nano" - "--feature-gates=+operator.autoinstrumentation.go" - "--enable-nginx-instrumentation=true" + - "--create-rbac-permissions=true" image: controller name: manager livenessProbe: diff --git a/internal/manifests/collector/parser/processor/processor_k8sattributes.go b/internal/manifests/collector/parser/processor/processor_k8sattributes.go index 3fcbfb0911..76fb45e08f 100644 --- a/internal/manifests/collector/parser/processor/processor_k8sattributes.go +++ b/internal/manifests/collector/parser/processor/processor_k8sattributes.go @@ -50,18 +50,26 @@ func (o *K8sAttributesParser) ParserName() string { } func (o *K8sAttributesParser) GetRBACRules() []rbacv1.PolicyRule { - var prs []rbacv1.PolicyRule + // These policies need to be added always + var prs []rbacv1.PolicyRule = []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"pods", "namespaces"}, + Verbs: []string{"get", "watch", "list"}, + }, + } - // This one needs to be added always - policy := rbacv1.PolicyRule{ - APIGroups: []string{""}, - Resources: []string{"pods", "namespaces"}, + replicasetPolicy := rbacv1.PolicyRule{ + APIGroups: []string{"apps"}, + Resources: []string{"replicasets"}, Verbs: []string{"get", "watch", "list"}, } - prs = append(prs, policy) extractCfg, ok := o.config["extract"] if !ok { + // k8s.deployment.name is enabled by default so, replicasets permissions are needed + // https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/32248#discussion_r1560077826 + prs = append(prs, replicasetPolicy) return prs } @@ -78,18 +86,7 @@ func (o *K8sAttributesParser) GetRBACRules() []rbacv1.PolicyRule { for _, m := range metadata { metadataField := fmt.Sprint(m) if metadataField == "k8s.deployment.uid" || metadataField == "k8s.deployment.name" { - prs = append(prs, - rbacv1.PolicyRule{ - APIGroups: []string{"apps"}, - Resources: []string{"replicasets"}, - Verbs: []string{"get", "watch", "list"}, - }, - rbacv1.PolicyRule{ - APIGroups: []string{"extensions"}, - Resources: []string{"replicasets"}, - Verbs: []string{"get", "watch", "list"}, - }, - ) + prs = append(prs, replicasetPolicy) } else if strings.Contains(metadataField, "k8s.node") { prs = append(prs, rbacv1.PolicyRule{ diff --git a/internal/manifests/collector/parser/processor/processor_k8sattributes_test.go b/internal/manifests/collector/parser/processor/processor_k8sattributes_test.go index 5b5c044bca..64afd88d86 100644 --- a/internal/manifests/collector/parser/processor/processor_k8sattributes_test.go +++ b/internal/manifests/collector/parser/processor/processor_k8sattributes_test.go @@ -40,14 +40,19 @@ func TestK8sAttributesRBAC(t *testing.T) { Resources: []string{"pods", "namespaces"}, Verbs: []string{"get", "watch", "list"}, }, + { + APIGroups: []string{"apps"}, + Resources: []string{"replicasets"}, + Verbs: []string{"get", "watch", "list"}, + }, }, }, { - name: "extract k8s.deployment.uid", + name: "extract k8s.deployment.name", config: map[interface{}]interface{}{ "extract": map[interface{}]interface{}{ "metadata": []interface{}{ - "k8s.deployment.uid", + "k8s.deployment.name", }, }, }, @@ -62,19 +67,36 @@ func TestK8sAttributesRBAC(t *testing.T) { Resources: []string{"replicasets"}, Verbs: []string{"get", "watch", "list"}, }, + }, + }, + { + name: "extract k8s.deployment.uid", + config: map[interface{}]interface{}{ + "extract": map[interface{}]interface{}{ + "metadata": []interface{}{ + "k8s.deployment.uid", + }, + }, + }, + expectedRules: []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"pods", "namespaces"}, + Verbs: []string{"get", "watch", "list"}, + }, { - APIGroups: []string{"extensions"}, + APIGroups: []string{"apps"}, Resources: []string{"replicasets"}, Verbs: []string{"get", "watch", "list"}, }, }, }, { - name: "extract k8s.deployment.name", + name: "extract k8s.pod.name", config: map[interface{}]interface{}{ "extract": map[interface{}]interface{}{ "metadata": []interface{}{ - "k8s.deployment.name", + "k8s.pod.name", }, }, }, @@ -84,14 +106,26 @@ func TestK8sAttributesRBAC(t *testing.T) { Resources: []string{"pods", "namespaces"}, Verbs: []string{"get", "watch", "list"}, }, + }, + }, + { + name: "extract k8s.node", + config: map[interface{}]interface{}{ + "extract": map[interface{}]interface{}{ + "metadata": []interface{}{ + "k8s.node", + }, + }, + }, + expectedRules: []rbacv1.PolicyRule{ { - APIGroups: []string{"apps"}, - Resources: []string{"replicasets"}, + APIGroups: []string{""}, + Resources: []string{"pods", "namespaces"}, Verbs: []string{"get", "watch", "list"}, }, - { - APIGroups: []string{"extensions"}, - Resources: []string{"replicasets"}, + rbacv1.PolicyRule{ + APIGroups: []string{""}, + Resources: []string{"nodes"}, Verbs: []string{"get", "watch", "list"}, }, }, diff --git a/tests/e2e/smoke-simplest/00-install.yaml b/tests/e2e/smoke-simplest/00-install.yaml index 0fb69308c7..2d9d45eb58 100644 --- a/tests/e2e/smoke-simplest/00-install.yaml +++ b/tests/e2e/smoke-simplest/00-install.yaml @@ -3,23 +3,25 @@ kind: OpenTelemetryCollector metadata: name: simplest spec: + image: otel/opentelemetry-collector-contrib:latest config: | receivers: - jaeger: - protocols: - grpc: otlp: protocols: - grpc: - http: + grpc: {} + http: {} processors: - + k8sattributes: {} + batch: {} exporters: debug: - service: pipelines: - traces: - receivers: [jaeger,otlp] - processors: [] - exporters: [debug] \ No newline at end of file + metrics: + receivers: + - otlp + processors: + - k8sattributes + - batch + exporters: + - debug \ No newline at end of file From 856440a0881ee0a4c801ea3dc21bc00399ce8cc2 Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Thu, 11 Apr 2024 12:10:13 +0200 Subject: [PATCH 11/26] Fix test Signed-off-by: Israel Blancas --- .../processor-k8sattributes/01-assert.yaml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/e2e-automatic-rbac/processor-k8sattributes/01-assert.yaml b/tests/e2e-automatic-rbac/processor-k8sattributes/01-assert.yaml index 12136c3776..20034a75c5 100644 --- a/tests/e2e-automatic-rbac/processor-k8sattributes/01-assert.yaml +++ b/tests/e2e-automatic-rbac/processor-k8sattributes/01-assert.yaml @@ -11,4 +11,12 @@ rules: verbs: - get - watch + - list +- apiGroups: + - "apps" + resources: + - replicasets + verbs: + - get + - watch - list \ No newline at end of file From 7347aed442ef3907f1da0ea4487acf37cb943578 Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Thu, 11 Apr 2024 17:20:56 +0200 Subject: [PATCH 12/26] Apply changes requested in code review Signed-off-by: Israel Blancas --- .github/workflows/e2e.yaml | 4 +- Makefile | 24 ++----- .../namespaces.yaml | 15 +++++ .../extra-permissions-operator/nodes.yaml | 12 ++++ .../rbac/extra-permissions-operator/rbac.yaml | 16 +++++ .../replicaset.yaml | 11 +++ hack/rbac-operator-permissions.yaml | 67 ------------------- internal/config/main.go | 7 +- .../processor/processor_k8sattributes_test.go | 2 +- 9 files changed, 69 insertions(+), 89 deletions(-) create mode 100644 config/rbac/extra-permissions-operator/namespaces.yaml create mode 100644 config/rbac/extra-permissions-operator/nodes.yaml create mode 100644 config/rbac/extra-permissions-operator/rbac.yaml create mode 100644 config/rbac/extra-permissions-operator/replicaset.yaml delete mode 100644 hack/rbac-operator-permissions.yaml diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index 7255fefd4b..c654900c7b 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -24,7 +24,6 @@ jobs: - "1.29" group: - e2e - - e2e-automatic-rbac - e2e-autoscale - e2e-instrumentation - e2e-opampbridge @@ -39,7 +38,8 @@ jobs: setup: "add-operator-arg OPERATOR_ARG=--enable-multi-instrumentation prepare-e2e" - group: e2e-metadata-filters setup: "add-operator-arg OPERATOR_ARG='--annotations-filter=*filter.out --labels=*filter.out' prepare-e2e" - + - group: e2e-automatic-rbac + setup: "add-rbac-permissions-to-operator prepare-e2e" steps: - name: Check out code into the Go module directory uses: actions/checkout@v4 diff --git a/Makefile b/Makefile index 9e54af8094..c8463a9c61 100644 --- a/Makefile +++ b/Makefile @@ -147,9 +147,12 @@ add-image-targetallocator: add-image-opampbridge: @$(MAKE) add-operator-arg OPERATOR_ARG=--operator-opamp-bridge-image=$(OPERATOROPAMPBRIDGE_IMG) -.PHONY: enable-operator-featuregates -enable-operator-featuregates: OPERATOR_ARG = --feature-gates=$(FEATUREGATES) -enable-operator-featuregates: add-operator-arg +.PHONY: add-rbac-permissions-to-operator +add-rbac-permissions-to-operator: manifests kustomize + cd config/rbac && $(KUSTOMIZE) edit add patch --kind ClusterRole --name manager-role --path extra-permissions-operator/namespaces.yaml + cd config/rbac && $(KUSTOMIZE) edit add patch --kind ClusterRole --name manager-role --path extra-permissions-operator/nodes.yaml + cd config/rbac && $(KUSTOMIZE) edit add patch --kind ClusterRole --name manager-role --path extra-permissions-operator/rbac.yaml + cd config/rbac && $(KUSTOMIZE) edit add patch --kind ClusterRole --name manager-role --path extra-permissions-operator/replicaset.yaml # Deploy controller in the current Kubernetes context, configured in ~/.kube/config .PHONY: deploy @@ -210,15 +213,7 @@ e2e: chainsaw # end-to-end-test for testing automatic RBAC creation .PHONY: e2e-automatic-rbac e2e-automatic-rbac: chainsaw - # Add extra needed permissions - kubectl apply -f hack/rbac-operator-permissions.yaml - kubectl rollout restart deployment opentelemetry-operator-controller-manager -n opentelemetry-operator-system - kubectl rollout status deployment opentelemetry-operator-controller-manager -n opentelemetry-operator-system - go run hack/check-operator-ready.go $(CHAINSAW) test --test-dir ./tests/e2e-automatic-rbac - # Cleanup - kubectl delete -f hack/rbac-operator-permissions.yaml - kubectl rollout restart deployment opentelemetry-operator-controller-manager -n opentelemetry-operator-system # end-to-end-test for testing autoscale .PHONY: e2e-autoscale @@ -275,9 +270,6 @@ e2e-upgrade: undeploy chainsaw .PHONY: prepare-e2e prepare-e2e: chainsaw set-image-controller add-image-targetallocator add-image-opampbridge container container-target-allocator container-operator-opamp-bridge start-kind cert-manager install-metrics-server install-targetallocator-prometheus-crds load-image-all deploy -.PHONY: prepare-e2e-with-featuregates -prepare-e2e-with-featuregates: chainsaw enable-operator-featuregates prepare-e2e - .PHONY: scorecard-tests scorecard-tests: operator-sdk $(OPERATOR_SDK) scorecard -w=5m bundle || (echo "scorecard test failed" && exit 1) @@ -323,10 +315,6 @@ endif install-metrics-server: ./hack/install-metrics-server.sh -.PHONY: install-prometheus-operator -install-prometheus-operator: - ./hack/install-prometheus-operator.sh - # This only installs the CRDs Target Allocator supports .PHONY: install-targetallocator-prometheus-crds install-targetallocator-prometheus-crds: diff --git a/config/rbac/extra-permissions-operator/namespaces.yaml b/config/rbac/extra-permissions-operator/namespaces.yaml new file mode 100644 index 0000000000..99a57e7386 --- /dev/null +++ b/config/rbac/extra-permissions-operator/namespaces.yaml @@ -0,0 +1,15 @@ +- op: add + path: /rules/- + value: + apiGroups: + - "" + resources: + - namespaces + verbs: + - create + - delete + - get + - list + - patch + - update + - watch diff --git a/config/rbac/extra-permissions-operator/nodes.yaml b/config/rbac/extra-permissions-operator/nodes.yaml new file mode 100644 index 0000000000..3971ded1a4 --- /dev/null +++ b/config/rbac/extra-permissions-operator/nodes.yaml @@ -0,0 +1,12 @@ +--- +- op: add + path: /rules/- + value: + apiGroups: + - "" + resources: + - nodes + verbs: + - get + - list + - watch diff --git a/config/rbac/extra-permissions-operator/rbac.yaml b/config/rbac/extra-permissions-operator/rbac.yaml new file mode 100644 index 0000000000..c36e0c2213 --- /dev/null +++ b/config/rbac/extra-permissions-operator/rbac.yaml @@ -0,0 +1,16 @@ +- op: add + path: /rules/- + value: + apiGroups: + - rbac.authorization.k8s.io + resources: + - clusterrolebindings + - clusterroles + verbs: + - create + - delete + - get + - list + - patch + - update + - watch diff --git a/config/rbac/extra-permissions-operator/replicaset.yaml b/config/rbac/extra-permissions-operator/replicaset.yaml new file mode 100644 index 0000000000..887c17da54 --- /dev/null +++ b/config/rbac/extra-permissions-operator/replicaset.yaml @@ -0,0 +1,11 @@ +- op: add + path: /rules/- + value: + apiGroups: + - apps + resources: + - replicasets + verbs: + - get + - list + - watch diff --git a/hack/rbac-operator-permissions.yaml b/hack/rbac-operator-permissions.yaml deleted file mode 100644 index 1370fa9ec9..0000000000 --- a/hack/rbac-operator-permissions.yaml +++ /dev/null @@ -1,67 +0,0 @@ -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRole -metadata: - name: generate-processors-rbac -rules: -- apiGroups: - - "" - resources: - - namespaces - verbs: - - create - - delete - - get - - list - - patch - - update - - watch -- apiGroups: - - "" - resources: - - nodes - verbs: - - get - - list - - watch -- apiGroups: - - rbac.authorization.k8s.io - resources: - - clusterrolebindings - - clusterroles - verbs: - - create - - delete - - get - - list - - patch - - update - - watch -- apiGroups: - - apps - resources: - - replicasets - verbs: - - get - - list - - watch -- apiGroups: - - extensions - resources: - - replicasets - verbs: - - get - - list - - watch ---- -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRoleBinding -metadata: - name: generate-processors-rbac -roleRef: - apiGroup: rbac.authorization.k8s.io - kind: ClusterRole - name: generate-processors-rbac -subjects: -- kind: ServiceAccount - name: opentelemetry-operator-controller-manager - namespace: opentelemetry-operator-system diff --git a/internal/config/main.go b/internal/config/main.go index ae0d4ef61e..3cf9a78c6a 100644 --- a/internal/config/main.go +++ b/internal/config/main.go @@ -72,12 +72,14 @@ func New(opts ...Option) Config { o := options{ prometheusCRAvailability: prometheus.NotAvailable, openshiftRoutesAvailability: openshift.RoutesNotAvailable, + createRBACPermissions: autoRbac.NotAvailable, collectorConfigMapEntry: defaultCollectorConfigMapEntry, targetAllocatorConfigMapEntry: defaultTargetAllocatorConfigMapEntry, operatorOpAMPBridgeConfigMapEntry: defaultOperatorOpAMPBridgeConfigMapEntry, logger: logf.Log.WithName("config"), version: version.Get(), } + for _, opt := range opts { opt(&o) } @@ -119,18 +121,21 @@ func (c *Config) AutoDetect() error { return err } c.openshiftRoutesAvailability = ora + c.logger.V(2).Info("openshift routes detected", "availability", ora) pcrd, err := c.autoDetect.PrometheusCRsAvailability() if err != nil { return err } c.prometheusCRAvailability = pcrd + c.logger.V(2).Info("prometheus cr detected", "availability", pcrd) rAuto, err := c.autoDetect.RBACPermissions(context.Background()) if err != nil { - return err + c.logger.V(2).Info("the rbac permissions are not set for the operator", "reason", err) } c.createRBACPermissions = rAuto + c.logger.V(2).Info("create rbac permissions detected", "availability", rAuto) return nil } diff --git a/internal/manifests/collector/parser/processor/processor_k8sattributes_test.go b/internal/manifests/collector/parser/processor/processor_k8sattributes_test.go index 64afd88d86..7274ffaa3d 100644 --- a/internal/manifests/collector/parser/processor/processor_k8sattributes_test.go +++ b/internal/manifests/collector/parser/processor/processor_k8sattributes_test.go @@ -123,7 +123,7 @@ func TestK8sAttributesRBAC(t *testing.T) { Resources: []string{"pods", "namespaces"}, Verbs: []string{"get", "watch", "list"}, }, - rbacv1.PolicyRule{ + { APIGroups: []string{""}, Resources: []string{"nodes"}, Verbs: []string{"get", "watch", "list"}, From f276b03415c53a6c3d9607893c78bed511dabeef Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Thu, 11 Apr 2024 17:25:58 +0200 Subject: [PATCH 13/26] Apply changes requested in code review Signed-off-by: Israel Blancas --- ...-create-rbac-permissions-by-checking-the-sa-permissions.yaml | 2 +- internal/autodetect/rbac/check.go | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/.chloggen/replace-create-rbac-permissions-by-checking-the-sa-permissions.yaml b/.chloggen/replace-create-rbac-permissions-by-checking-the-sa-permissions.yaml index 3cf4cf6f57..a29025c083 100755 --- a/.chloggen/replace-create-rbac-permissions-by-checking-the-sa-permissions.yaml +++ b/.chloggen/replace-create-rbac-permissions-by-checking-the-sa-permissions.yaml @@ -5,7 +5,7 @@ change_type: enhancement component: operator # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). -note: Check the permissions for the SA running the OpenTelemetry Operator instead of using the create-rbac-permissions flag. +note: Automatically enable RBAC creation if operator SA can create clusterroles and bindings. We should also add link to the manifest that defines the RBAC. --create-rbac-permissions flag is noop and deprecated now. # One or more tracking issues related to the change issues: [2588] diff --git a/internal/autodetect/rbac/check.go b/internal/autodetect/rbac/check.go index 945cd43fde..6d918cbe1f 100644 --- a/internal/autodetect/rbac/check.go +++ b/internal/autodetect/rbac/check.go @@ -52,6 +52,8 @@ func getOperatorServiceAccount() (string, error) { return sa, nil } +// CheckRbacPermissions checks if the operator has the needed permissions to create RBAC resources automatically. +// If the RBAC is there, no errors nor warnings are returned. func CheckRbacPermissions(ctx context.Context, reviewer *rbac.Reviewer) (admission.Warnings, error) { namespace, err := getOperatorNamespace() if err != nil { From fb2d5be8761678dfd2491184075ce0482b80411a Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Thu, 11 Apr 2024 17:27:39 +0200 Subject: [PATCH 14/26] Remove checked error Signed-off-by: Israel Blancas --- config/manager/manager.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index ef529ce138..35c9072ffc 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -34,7 +34,6 @@ spec: - "--zap-time-encoding=rfc3339nano" - "--feature-gates=+operator.autoinstrumentation.go" - "--enable-nginx-instrumentation=true" - - "--create-rbac-permissions=true" image: controller name: manager livenessProbe: From 1116be48d519e138e104e30528836bb895c46146 Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Thu, 11 Apr 2024 18:06:35 +0200 Subject: [PATCH 15/26] Revert change Signed-off-by: Israel Blancas --- ...-create-rbac-permissions-by-checking-the-sa-permissions.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.chloggen/replace-create-rbac-permissions-by-checking-the-sa-permissions.yaml b/.chloggen/replace-create-rbac-permissions-by-checking-the-sa-permissions.yaml index a29025c083..ab5895bb16 100755 --- a/.chloggen/replace-create-rbac-permissions-by-checking-the-sa-permissions.yaml +++ b/.chloggen/replace-create-rbac-permissions-by-checking-the-sa-permissions.yaml @@ -5,7 +5,7 @@ change_type: enhancement component: operator # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). -note: Automatically enable RBAC creation if operator SA can create clusterroles and bindings. We should also add link to the manifest that defines the RBAC. --create-rbac-permissions flag is noop and deprecated now. +note: Automatically enable RBAC creation if operator SA can create clusterroles and bindings. --create-rbac-permissions flag is noop and deprecated now. # One or more tracking issues related to the change issues: [2588] From 73afa80ad11d9f159cbc51cdb506bbc716432df4 Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Thu, 11 Apr 2024 18:19:36 +0200 Subject: [PATCH 16/26] Fix workflow Signed-off-by: Israel Blancas --- .github/workflows/e2e.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index c654900c7b..84ce49b954 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -24,6 +24,7 @@ jobs: - "1.29" group: - e2e + - e2e-automatic-rbac - e2e-autoscale - e2e-instrumentation - e2e-opampbridge From bd2716cd067ef66255553b535d7a335572dfb290 Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Thu, 11 Apr 2024 18:33:29 +0200 Subject: [PATCH 17/26] Fix E2E test Signed-off-by: Israel Blancas --- tests/e2e/smoke-simplest/00-install.yaml | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/tests/e2e/smoke-simplest/00-install.yaml b/tests/e2e/smoke-simplest/00-install.yaml index 2d9d45eb58..0fb69308c7 100644 --- a/tests/e2e/smoke-simplest/00-install.yaml +++ b/tests/e2e/smoke-simplest/00-install.yaml @@ -3,25 +3,23 @@ kind: OpenTelemetryCollector metadata: name: simplest spec: - image: otel/opentelemetry-collector-contrib:latest config: | receivers: + jaeger: + protocols: + grpc: otlp: protocols: - grpc: {} - http: {} + grpc: + http: processors: - k8sattributes: {} - batch: {} + exporters: debug: + service: pipelines: - metrics: - receivers: - - otlp - processors: - - k8sattributes - - batch - exporters: - - debug \ No newline at end of file + traces: + receivers: [jaeger,otlp] + processors: [] + exporters: [debug] \ No newline at end of file From 0558a69a99be581c89e46227b5b1c6a705d702d7 Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Fri, 12 Apr 2024 12:37:54 +0200 Subject: [PATCH 18/26] Fix bundle Signed-off-by: Israel Blancas --- .../opentelemetry-operator.clusterserviceversion.yaml | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml b/bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml index 9c5d55f5ef..b3189e546c 100644 --- a/bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml +++ b/bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml @@ -65,11 +65,7 @@ metadata: categories: Logging & Tracing,Monitoring certified: "false" containerImage: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator -<<<<<<< HEAD - createdAt: "2024-04-08T14:26:03Z" -======= - createdAt: "2024-04-11T16:00:15Z" ->>>>>>> 9a05c3ebd8736be55686de1b853c77bf30f9211d + createdAt: "2024-04-12T10:37:24Z" description: Provides the OpenTelemetry components, including the Collector operators.operatorframework.io/builder: operator-sdk-v1.29.0 operators.operatorframework.io/project_layout: go.kubebuilder.io/v3 From b548a41999aa39a1ae4b534acb6af27258335fc8 Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Tue, 16 Apr 2024 06:50:38 +0200 Subject: [PATCH 19/26] Apply changes requested in code review Signed-off-by: Israel Blancas --- controllers/suite_test.go | 8 ++++---- internal/autodetect/main.go | 14 +++++++------- internal/autodetect/main_test.go | 14 +++++++------- internal/autodetect/rbac/check.go | 4 ++-- internal/config/main.go | 8 ++++---- internal/config/options.go | 6 +++--- .../processor-k8sattributes/01-assert.yaml | 19 ++++++++++++++++++- .../processor-k8sattributes/02-assert.yaml | 19 ++++++++++++++++++- .../01-assert.yaml | 19 ++++++++++++++++++- .../02-assert.yaml | 17 +++++++++++++++++ 10 files changed, 98 insertions(+), 30 deletions(-) diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 767ffea9e9..94051b4a68 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -56,7 +56,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" - autoRbac "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" + autoRBAC "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" "github.com/open-telemetry/opentelemetry-operator/internal/config" "github.com/open-telemetry/opentelemetry-operator/internal/manifests" "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/testdata" @@ -96,7 +96,7 @@ var _ autodetect.AutoDetect = (*mockAutoDetect)(nil) type mockAutoDetect struct { OpenShiftRoutesAvailabilityFunc func() (openshift.RoutesAvailability, error) PrometheusCRsAvailabilityFunc func() (prometheus.Availability, error) - RBACPermissionsFunc func(ctx context.Context) (autoRbac.Availability, error) + RBACPermissionsFunc func(ctx context.Context) (autoRBAC.Availability, error) } func (m *mockAutoDetect) PrometheusCRsAvailability() (prometheus.Availability, error) { @@ -113,11 +113,11 @@ func (m *mockAutoDetect) OpenShiftRoutesAvailability() (openshift.RoutesAvailabi return openshift.RoutesNotAvailable, nil } -func (m *mockAutoDetect) RBACPermissions(ctx context.Context) (autoRbac.Availability, error) { +func (m *mockAutoDetect) RBACPermissions(ctx context.Context) (autoRBAC.Availability, error) { if m.RBACPermissionsFunc != nil { return m.RBACPermissionsFunc(ctx) } - return autoRbac.NotAvailable, nil + return autoRBAC.NotAvailable, nil } func TestMain(m *testing.M) { diff --git a/internal/autodetect/main.go b/internal/autodetect/main.go index 003dfd43d9..359843dcd0 100644 --- a/internal/autodetect/main.go +++ b/internal/autodetect/main.go @@ -24,7 +24,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" - autoRbac "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" + autoRBAC "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" "github.com/open-telemetry/opentelemetry-operator/internal/rbac" ) @@ -34,7 +34,7 @@ var _ AutoDetect = (*autoDetect)(nil) type AutoDetect interface { OpenShiftRoutesAvailability() (openshift.RoutesAvailability, error) PrometheusCRsAvailability() (prometheus.Availability, error) - RBACPermissions(ctx context.Context) (autoRbac.Availability, error) + RBACPermissions(ctx context.Context) (autoRBAC.Availability, error) } type autoDetect struct { @@ -92,14 +92,14 @@ func (a *autoDetect) OpenShiftRoutesAvailability() (openshift.RoutesAvailability return openshift.RoutesNotAvailable, nil } -func (a *autoDetect) RBACPermissions(ctx context.Context) (autoRbac.Availability, error) { - w, err := autoRbac.CheckRbacPermissions(ctx, a.reviewer) +func (a *autoDetect) RBACPermissions(ctx context.Context) (autoRBAC.Availability, error) { + w, err := autoRBAC.CheckRBACPermissions(ctx, a.reviewer) if err != nil { - return autoRbac.NotAvailable, err + return autoRBAC.NotAvailable, err } if w != nil { - return autoRbac.NotAvailable, fmt.Errorf("missing permissions: %s", w) + return autoRBAC.NotAvailable, fmt.Errorf("missing permissions: %s", w) } - return autoRbac.Available, nil + return autoRBAC.Available, nil } diff --git a/internal/autodetect/main_test.go b/internal/autodetect/main_test.go index 88d7703a53..d5dbbc707e 100644 --- a/internal/autodetect/main_test.go +++ b/internal/autodetect/main_test.go @@ -35,7 +35,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" - autoRbac "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" + autoRBAC "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" "github.com/open-telemetry/opentelemetry-operator/internal/rbac" ) @@ -155,7 +155,7 @@ func TestDetectRBACPermissionsBasedOnAvailableClusterRoles(t *testing.T) { for _, tt := range []struct { description string - expectedAvailability autoRbac.Availability + expectedAvailability autoRBAC.Availability shouldError bool namespace string serviceAccount string @@ -165,7 +165,7 @@ func TestDetectRBACPermissionsBasedOnAvailableClusterRoles(t *testing.T) { description: "Not possible to read the namespace", namespace: "default", shouldError: true, - expectedAvailability: autoRbac.NotAvailable, + expectedAvailability: autoRBAC.NotAvailable, clientGenerator: reactorFactory(v1.SubjectAccessReviewStatus{ Allowed: true, }), @@ -187,7 +187,7 @@ func TestDetectRBACPermissionsBasedOnAvailableClusterRoles(t *testing.T) { clientGenerator: reactorFactory(v1.SubjectAccessReviewStatus{ Allowed: false, }), - expectedAvailability: autoRbac.NotAvailable, + expectedAvailability: autoRBAC.NotAvailable, }, { description: "RBAC resources are there", @@ -198,13 +198,13 @@ func TestDetectRBACPermissionsBasedOnAvailableClusterRoles(t *testing.T) { clientGenerator: reactorFactory(v1.SubjectAccessReviewStatus{ Allowed: true, }), - expectedAvailability: autoRbac.Available, + expectedAvailability: autoRBAC.Available, }, } { t.Run(tt.description, func(t *testing.T) { // These settings can be get from env vars - t.Setenv(autoRbac.NAMESPACE_ENV_VAR, tt.namespace) - t.Setenv(autoRbac.SA_ENV_VAR, tt.serviceAccount) + t.Setenv(autoRBAC.NAMESPACE_ENV_VAR, tt.namespace) + t.Setenv(autoRBAC.SA_ENV_VAR, tt.serviceAccount) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {})) defer server.Close() diff --git a/internal/autodetect/rbac/check.go b/internal/autodetect/rbac/check.go index 6d918cbe1f..1e133ebf49 100644 --- a/internal/autodetect/rbac/check.go +++ b/internal/autodetect/rbac/check.go @@ -52,9 +52,9 @@ func getOperatorServiceAccount() (string, error) { return sa, nil } -// CheckRbacPermissions checks if the operator has the needed permissions to create RBAC resources automatically. +// CheckRBACPermissions checks if the operator has the needed permissions to create RBAC resources automatically. // If the RBAC is there, no errors nor warnings are returned. -func CheckRbacPermissions(ctx context.Context, reviewer *rbac.Reviewer) (admission.Warnings, error) { +func CheckRBACPermissions(ctx context.Context, reviewer *rbac.Reviewer) (admission.Warnings, error) { namespace, err := getOperatorNamespace() if err != nil { return nil, fmt.Errorf("%s: %w", "not possible to check RBAC rules", err) diff --git a/internal/config/main.go b/internal/config/main.go index 3cf9a78c6a..541ab97d71 100644 --- a/internal/config/main.go +++ b/internal/config/main.go @@ -25,7 +25,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" - autoRbac "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" + autoRBAC "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" "github.com/open-telemetry/opentelemetry-operator/internal/version" ) @@ -45,7 +45,7 @@ type Config struct { autoInstrumentationPythonImage string collectorImage string collectorConfigMapEntry string - createRBACPermissions autoRbac.Availability + createRBACPermissions autoRBAC.Availability enableMultiInstrumentation bool enableApacheHttpdInstrumentation bool enableDotNetInstrumentation bool @@ -72,7 +72,7 @@ func New(opts ...Option) Config { o := options{ prometheusCRAvailability: prometheus.NotAvailable, openshiftRoutesAvailability: openshift.RoutesNotAvailable, - createRBACPermissions: autoRbac.NotAvailable, + createRBACPermissions: autoRBAC.NotAvailable, collectorConfigMapEntry: defaultCollectorConfigMapEntry, targetAllocatorConfigMapEntry: defaultTargetAllocatorConfigMapEntry, operatorOpAMPBridgeConfigMapEntry: defaultOperatorOpAMPBridgeConfigMapEntry, @@ -176,7 +176,7 @@ func (c *Config) CollectorConfigMapEntry() string { } // CreateRBACPermissions is true when the operator can create RBAC permissions for SAs running a collector instance. Immutable. -func (c *Config) CreateRBACPermissions() autoRbac.Availability { +func (c *Config) CreateRBACPermissions() autoRBAC.Availability { return c.createRBACPermissions } diff --git a/internal/config/options.go b/internal/config/options.go index 08eb8db876..99c25b5a3c 100644 --- a/internal/config/options.go +++ b/internal/config/options.go @@ -23,7 +23,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/internal/autodetect" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift" "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/prometheus" - autoRbac "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" + autoRBAC "github.com/open-telemetry/opentelemetry-operator/internal/autodetect/rbac" "github.com/open-telemetry/opentelemetry-operator/internal/version" ) @@ -43,7 +43,7 @@ type options struct { autoInstrumentationNginxImage string collectorImage string collectorConfigMapEntry string - createRBACPermissions autoRbac.Availability + createRBACPermissions autoRBAC.Availability enableMultiInstrumentation bool enableApacheHttpdInstrumentation bool enableDotNetInstrumentation bool @@ -184,7 +184,7 @@ func WithPrometheusCRAvailability(pcrd prometheus.Availability) Option { } } -func WithRBACPermissions(rAuto autoRbac.Availability) Option { +func WithRBACPermissions(rAuto autoRBAC.Availability) Option { return func(o *options) { o.createRBACPermissions = rAuto } diff --git a/tests/e2e-automatic-rbac/processor-k8sattributes/01-assert.yaml b/tests/e2e-automatic-rbac/processor-k8sattributes/01-assert.yaml index 20034a75c5..a9e20d795c 100644 --- a/tests/e2e-automatic-rbac/processor-k8sattributes/01-assert.yaml +++ b/tests/e2e-automatic-rbac/processor-k8sattributes/01-assert.yaml @@ -19,4 +19,21 @@ rules: verbs: - get - watch - - list \ No newline at end of file + - list +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + labels: + app.kubernetes.io/component: opentelemetry-collector + app.kubernetes.io/instance: chainsaw-k8sattributes.simplest + app.kubernetes.io/managed-by: opentelemetry-operator + app.kubernetes.io/name: simplest-collector +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: simplest-chainsaw-resourcedetection-cluster-role +subjects: +- kind: ServiceAccount + name: simplest-collector + namespace: chainsaw-k8sattributes diff --git a/tests/e2e-automatic-rbac/processor-k8sattributes/02-assert.yaml b/tests/e2e-automatic-rbac/processor-k8sattributes/02-assert.yaml index 113d182397..067779e384 100644 --- a/tests/e2e-automatic-rbac/processor-k8sattributes/02-assert.yaml +++ b/tests/e2e-automatic-rbac/processor-k8sattributes/02-assert.yaml @@ -19,4 +19,21 @@ rules: verbs: - get - watch - - list \ No newline at end of file + - list +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + labels: + app.kubernetes.io/component: opentelemetry-collector + app.kubernetes.io/instance: chainsaw-k8sattributes.simplest + app.kubernetes.io/managed-by: opentelemetry-operator + app.kubernetes.io/name: simplest-collector +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: simplest-chainsaw-resourcedetection-cluster-role +subjects: +- kind: ServiceAccount + name: simplest-collector + namespace: chainsaw-k8sattributes diff --git a/tests/e2e-automatic-rbac/processor-resourcedetection/01-assert.yaml b/tests/e2e-automatic-rbac/processor-resourcedetection/01-assert.yaml index d87a840e05..32a1faed64 100644 --- a/tests/e2e-automatic-rbac/processor-resourcedetection/01-assert.yaml +++ b/tests/e2e-automatic-rbac/processor-resourcedetection/01-assert.yaml @@ -11,4 +11,21 @@ rules: verbs: - get - watch - - list \ No newline at end of file + - list +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + labels: + app.kubernetes.io/component: opentelemetry-collector + app.kubernetes.io/instance: chainsaw-resourcedetection.simplest + app.kubernetes.io/managed-by: opentelemetry-operator + app.kubernetes.io/name: simplest-collector +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: simplest-chainsaw-resourcedetection-cluster-role +subjects: +- kind: ServiceAccount + name: simplest-collector + namespace: chainsaw-resourcedetection diff --git a/tests/e2e-automatic-rbac/processor-resourcedetection/02-assert.yaml b/tests/e2e-automatic-rbac/processor-resourcedetection/02-assert.yaml index 927e3fdc5a..c1cef0a139 100644 --- a/tests/e2e-automatic-rbac/processor-resourcedetection/02-assert.yaml +++ b/tests/e2e-automatic-rbac/processor-resourcedetection/02-assert.yaml @@ -10,3 +10,20 @@ rules: verbs: - get - list +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + labels: + app.kubernetes.io/component: opentelemetry-collector + app.kubernetes.io/instance: chainsaw-resourcedetection.simplest + app.kubernetes.io/managed-by: opentelemetry-operator + app.kubernetes.io/name: simplest-collector +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: simplest-chainsaw-resourcedetection-cluster-role +subjects: +- kind: ServiceAccount + name: simplest-collector + namespace: chainsaw-resourcedetection From 019c516cca8b63e0c911fab46e7036904f90c5de Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Tue, 16 Apr 2024 06:52:04 +0200 Subject: [PATCH 20/26] Fix docs Signed-off-by: Israel Blancas --- docs/api.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/api.md b/docs/api.md index f38a729e4b..842764eab1 100644 --- a/docs/api.md +++ b/docs/api.md @@ -40457,7 +40457,8 @@ TargetAllocator indicates a value which determines whether to spawn a target all AllocationStrategy determines which strategy the target allocator should use for allocation. The current options are least-weighted, consistent-hashing and per-node. The default is -consistent-hashing.
+consistent-hashing. +WARNING: The per-node strategy currently ignores targets without a Node, like control plane components.

Enum: least-weighted, consistent-hashing
Default: consistent-hashing
From 38528402eb8c4d867c5a1d8d996f740ec83d288e Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Tue, 16 Apr 2024 18:21:09 +0200 Subject: [PATCH 21/26] Fixes #2862 Signed-off-by: Israel Blancas --- .chloggen/2862-fix-clusterrolebinding-names.yaml | 16 ++++++++++++++++ internal/manifests/collector/rbac.go | 2 +- internal/naming/main.go | 4 ++-- 3 files changed, 19 insertions(+), 3 deletions(-) create mode 100755 .chloggen/2862-fix-clusterrolebinding-names.yaml diff --git a/.chloggen/2862-fix-clusterrolebinding-names.yaml b/.chloggen/2862-fix-clusterrolebinding-names.yaml new file mode 100755 index 0000000000..44307f7670 --- /dev/null +++ b/.chloggen/2862-fix-clusterrolebinding-names.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: collector + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: "When two Collectors are created with the same name but different namespaces, the ClusterRoleBinding created by the first will be overriden by the second one." + +# One or more tracking issues related to the change +issues: [2862] + +# (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/internal/manifests/collector/rbac.go b/internal/manifests/collector/rbac.go index 4af9223996..25bad9dcb1 100644 --- a/internal/manifests/collector/rbac.go +++ b/internal/manifests/collector/rbac.go @@ -70,7 +70,7 @@ func ClusterRoleBinding(params manifests.Params) (*rbacv1.ClusterRoleBinding, er return nil, nil } - name := naming.ClusterRoleBinding(params.OtelCol.Name) + name := naming.ClusterRoleBinding(params.OtelCol.Name, params.OtelCol.Namespace) labels := manifestutils.Labels(params.OtelCol.ObjectMeta, name, params.OtelCol.Spec.Image, ComponentOpenTelemetryCollector, params.Config.LabelsFilter()) return &rbacv1.ClusterRoleBinding{ diff --git a/internal/naming/main.go b/internal/naming/main.go index 8b801ce75e..def5adbf2a 100644 --- a/internal/naming/main.go +++ b/internal/naming/main.go @@ -136,8 +136,8 @@ func ClusterRole(otelcol string, namespace string) string { } // ClusterRoleBinding builds the cluster role binding name based on the instance. -func ClusterRoleBinding(otelcol string) string { - return DNSName(Truncate("%s-collector", 63, otelcol)) +func ClusterRoleBinding(otelcol, namespace string) string { + return DNSName(Truncate("%s-%s-collector", 63, otelcol, namespace)) } // TAService returns the name to use for the TargetAllocator service. From 9b7e65390334e81f8eac1b30a1de0caf80ba76c4 Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Tue, 16 Apr 2024 18:21:18 +0200 Subject: [PATCH 22/26] Fix E2E tests Signed-off-by: Israel Blancas --- .../processor-k8sattributes/01-assert.yaml | 7 ++++--- .../processor-k8sattributes/02-assert.yaml | 5 +++-- .../processor-resourcedetection/01-assert.yaml | 4 +++- .../processor-resourcedetection/02-assert.yaml | 4 +++- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/tests/e2e-automatic-rbac/processor-k8sattributes/01-assert.yaml b/tests/e2e-automatic-rbac/processor-k8sattributes/01-assert.yaml index a9e20d795c..ae72093f6f 100644 --- a/tests/e2e-automatic-rbac/processor-k8sattributes/01-assert.yaml +++ b/tests/e2e-automatic-rbac/processor-k8sattributes/01-assert.yaml @@ -28,12 +28,13 @@ metadata: app.kubernetes.io/component: opentelemetry-collector app.kubernetes.io/instance: chainsaw-k8sattributes.simplest app.kubernetes.io/managed-by: opentelemetry-operator - app.kubernetes.io/name: simplest-collector + app.kubernetes.io/name: simplest-chainsaw-k8sattributes-collector + name: simplest-chainsaw-k8sattributes-collector roleRef: apiGroup: rbac.authorization.k8s.io kind: ClusterRole - name: simplest-chainsaw-resourcedetection-cluster-role + name: simplest-chainsaw-k8sattributes-cluster-role subjects: - kind: ServiceAccount name: simplest-collector - namespace: chainsaw-k8sattributes + namespace: chainsaw-k8sattributes \ No newline at end of file diff --git a/tests/e2e-automatic-rbac/processor-k8sattributes/02-assert.yaml b/tests/e2e-automatic-rbac/processor-k8sattributes/02-assert.yaml index 067779e384..b1a5b4a58d 100644 --- a/tests/e2e-automatic-rbac/processor-k8sattributes/02-assert.yaml +++ b/tests/e2e-automatic-rbac/processor-k8sattributes/02-assert.yaml @@ -28,11 +28,12 @@ metadata: app.kubernetes.io/component: opentelemetry-collector app.kubernetes.io/instance: chainsaw-k8sattributes.simplest app.kubernetes.io/managed-by: opentelemetry-operator - app.kubernetes.io/name: simplest-collector + app.kubernetes.io/name: simplest-chainsaw-k8sattributes-collector + name: simplest-chainsaw-k8sattributes-collector roleRef: apiGroup: rbac.authorization.k8s.io kind: ClusterRole - name: simplest-chainsaw-resourcedetection-cluster-role + name: simplest-chainsaw-k8sattributes-cluster-role subjects: - kind: ServiceAccount name: simplest-collector diff --git a/tests/e2e-automatic-rbac/processor-resourcedetection/01-assert.yaml b/tests/e2e-automatic-rbac/processor-resourcedetection/01-assert.yaml index 32a1faed64..077ce1a5e3 100644 --- a/tests/e2e-automatic-rbac/processor-resourcedetection/01-assert.yaml +++ b/tests/e2e-automatic-rbac/processor-resourcedetection/01-assert.yaml @@ -20,7 +20,9 @@ metadata: app.kubernetes.io/component: opentelemetry-collector app.kubernetes.io/instance: chainsaw-resourcedetection.simplest app.kubernetes.io/managed-by: opentelemetry-operator - app.kubernetes.io/name: simplest-collector + app.kubernetes.io/name: simplest-chainsaw-resourcedetection-collector + app.kubernetes.io/part-of: opentelemetry + name: simplest-chainsaw-resourcedetection-collector roleRef: apiGroup: rbac.authorization.k8s.io kind: ClusterRole diff --git a/tests/e2e-automatic-rbac/processor-resourcedetection/02-assert.yaml b/tests/e2e-automatic-rbac/processor-resourcedetection/02-assert.yaml index c1cef0a139..ecdea43a56 100644 --- a/tests/e2e-automatic-rbac/processor-resourcedetection/02-assert.yaml +++ b/tests/e2e-automatic-rbac/processor-resourcedetection/02-assert.yaml @@ -18,7 +18,9 @@ metadata: app.kubernetes.io/component: opentelemetry-collector app.kubernetes.io/instance: chainsaw-resourcedetection.simplest app.kubernetes.io/managed-by: opentelemetry-operator - app.kubernetes.io/name: simplest-collector + app.kubernetes.io/name: simplest-chainsaw-resourcedetection-collector + app.kubernetes.io/part-of: opentelemetry + name: simplest-chainsaw-resourcedetection-collector roleRef: apiGroup: rbac.authorization.k8s.io kind: ClusterRole From 08915d225b8691490242c2fed4d04eb236d86a02 Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Wed, 17 Apr 2024 11:29:21 +0200 Subject: [PATCH 23/26] Apply changes requested in code review Signed-off-by: Israel Blancas --- .gitignore | 1 + Makefile | 3 +++ .../extra-permissions-operator/namespaces.yaml | 0 .../e2e-automatic-rbac}/extra-permissions-operator/nodes.yaml | 0 .../e2e-automatic-rbac}/extra-permissions-operator/rbac.yaml | 0 .../extra-permissions-operator/replicaset.yaml | 0 6 files changed, 4 insertions(+) rename {config/rbac => tests/e2e-automatic-rbac}/extra-permissions-operator/namespaces.yaml (100%) rename {config/rbac => tests/e2e-automatic-rbac}/extra-permissions-operator/nodes.yaml (100%) rename {config/rbac => tests/e2e-automatic-rbac}/extra-permissions-operator/rbac.yaml (100%) rename {config/rbac => tests/e2e-automatic-rbac}/extra-permissions-operator/replicaset.yaml (100%) diff --git a/.gitignore b/.gitignore index 88abe862c0..1438657894 100644 --- a/.gitignore +++ b/.gitignore @@ -38,6 +38,7 @@ config/manager/kustomization.yaml # test resources kubeconfig tests/_build/ +config/rbac/extra-permissions-operator/ # autoinstrumentation artifacts build diff --git a/Makefile b/Makefile index 644c0a5000..3af82538a5 100644 --- a/Makefile +++ b/Makefile @@ -151,6 +151,9 @@ add-image-opampbridge: .PHONY: add-rbac-permissions-to-operator add-rbac-permissions-to-operator: manifests kustomize + # Kustomize only allows patches in the folder where the kustomization is located + # This folder is ignored by .gitignore + cp -r tests/e2e-automatic-rbac/extra-permissions-operator/ config/rbac/extra-permissions-operator cd config/rbac && $(KUSTOMIZE) edit add patch --kind ClusterRole --name manager-role --path extra-permissions-operator/namespaces.yaml cd config/rbac && $(KUSTOMIZE) edit add patch --kind ClusterRole --name manager-role --path extra-permissions-operator/nodes.yaml cd config/rbac && $(KUSTOMIZE) edit add patch --kind ClusterRole --name manager-role --path extra-permissions-operator/rbac.yaml diff --git a/config/rbac/extra-permissions-operator/namespaces.yaml b/tests/e2e-automatic-rbac/extra-permissions-operator/namespaces.yaml similarity index 100% rename from config/rbac/extra-permissions-operator/namespaces.yaml rename to tests/e2e-automatic-rbac/extra-permissions-operator/namespaces.yaml diff --git a/config/rbac/extra-permissions-operator/nodes.yaml b/tests/e2e-automatic-rbac/extra-permissions-operator/nodes.yaml similarity index 100% rename from config/rbac/extra-permissions-operator/nodes.yaml rename to tests/e2e-automatic-rbac/extra-permissions-operator/nodes.yaml diff --git a/config/rbac/extra-permissions-operator/rbac.yaml b/tests/e2e-automatic-rbac/extra-permissions-operator/rbac.yaml similarity index 100% rename from config/rbac/extra-permissions-operator/rbac.yaml rename to tests/e2e-automatic-rbac/extra-permissions-operator/rbac.yaml diff --git a/config/rbac/extra-permissions-operator/replicaset.yaml b/tests/e2e-automatic-rbac/extra-permissions-operator/replicaset.yaml similarity index 100% rename from config/rbac/extra-permissions-operator/replicaset.yaml rename to tests/e2e-automatic-rbac/extra-permissions-operator/replicaset.yaml From 4bbefe93cf80475b2211bb045701ded3a510aec9 Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Thu, 25 Apr 2024 18:12:00 +0200 Subject: [PATCH 24/26] Remove kustomization Signed-off-by: Israel Blancas --- config/manager/kustomization.yaml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 1f93e9d569..5c5f0b84cb 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -1,8 +1,2 @@ resources: - manager.yaml -apiVersion: kustomize.config.k8s.io/v1beta1 -kind: Kustomization -images: -- name: controller - newName: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator - newTag: 0.97.1-7-gcafbc81a From 26e8830f2849c603a139d1b0138b746095dc1e3d Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Fri, 3 May 2024 17:22:36 +0200 Subject: [PATCH 25/26] Update bundle Signed-off-by: Israel Blancas --- .../opentelemetry-operator.clusterserviceversion.yaml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml b/bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml index 062e10cb27..abcf23573b 100644 --- a/bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml +++ b/bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml @@ -99,7 +99,7 @@ metadata: categories: Logging & Tracing,Monitoring certified: "false" containerImage: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator - createdAt: "2024-04-30T12:37:39Z" + createdAt: "2024-05-03T15:21:44Z" description: Provides the OpenTelemetry components, including the Collector operators.operatorframework.io/builder: operator-sdk-v1.29.0 operators.operatorframework.io/project_layout: go.kubebuilder.io/v3 @@ -499,6 +499,11 @@ spec: - --zap-log-level=info - --zap-time-encoding=rfc3339nano - --enable-nginx-instrumentation=true + env: + - name: SERVICE_ACCOUNT_NAME + valueFrom: + fieldRef: + fieldPath: spec.serviceAccountName image: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator:0.98.0 livenessProbe: httpGet: From 603d6d3837421adc5505b7a45b6fae4fed18cd65 Mon Sep 17 00:00:00 2001 From: Israel Blancas Date: Fri, 3 May 2024 19:16:34 +0200 Subject: [PATCH 26/26] Fix typo Signed-off-by: Israel Blancas --- .../manifests/opentelemetry-operator.clusterserviceversion.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml b/bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml index abcf23573b..c14ddf4e76 100644 --- a/bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml +++ b/bundle/manifests/opentelemetry-operator.clusterserviceversion.yaml @@ -504,7 +504,7 @@ spec: valueFrom: fieldRef: fieldPath: spec.serviceAccountName - image: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator:0.98.0 + image: ghcr.io/open-telemetry/opentelemetry-operator/opentelemetry-operator:0.99.0 livenessProbe: httpGet: path: /healthz