diff --git a/apis/v1alpha1/opentelemetrycollector_webhook.go b/apis/v1alpha1/collector_webhook.go similarity index 63% rename from apis/v1alpha1/opentelemetrycollector_webhook.go rename to apis/v1alpha1/collector_webhook.go index 3d1d9828ba..cc61f72b59 100644 --- a/apis/v1alpha1/opentelemetrycollector_webhook.go +++ b/apis/v1alpha1/collector_webhook.go @@ -15,38 +15,71 @@ package v1alpha1 import ( + "context" "fmt" + "github.com/go-logr/logr" autoscalingv2 "k8s.io/api/autoscaling/v2" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/validation" ctrl "sigs.k8s.io/controller-runtime" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + "github.com/open-telemetry/opentelemetry-operator/internal/config" ta "github.com/open-telemetry/opentelemetry-operator/internal/manifests/targetallocator/adapters" "github.com/open-telemetry/opentelemetry-operator/pkg/featuregate" ) -// log is for logging in this package. -var opentelemetrycollectorlog = logf.Log.WithName("opentelemetrycollector-resource") +var ( + _ admission.CustomValidator = &CollectorWebhook{} + _ admission.CustomDefaulter = &CollectorWebhook{} +) -func (r *OpenTelemetryCollector) SetupWebhookWithManager(mgr ctrl.Manager) error { - return ctrl.NewWebhookManagedBy(mgr). - For(r). - Complete() +// +kubebuilder:webhook:path=/mutate-opentelemetry-io-v1alpha1-opentelemetrycollector,mutating=true,failurePolicy=fail,groups=opentelemetry.io,resources=opentelemetrycollectors,verbs=create;update,versions=v1alpha1,name=mopentelemetrycollector.kb.io,sideEffects=none,admissionReviewVersions=v1 +// +kubebuilder:webhook:verbs=create;update,path=/validate-opentelemetry-io-v1alpha1-opentelemetrycollector,mutating=false,failurePolicy=fail,groups=opentelemetry.io,resources=opentelemetrycollectors,versions=v1alpha1,name=vopentelemetrycollectorcreateupdate.kb.io,sideEffects=none,admissionReviewVersions=v1 +// +kubebuilder:webhook:verbs=delete,path=/validate-opentelemetry-io-v1alpha1-opentelemetrycollector,mutating=false,failurePolicy=ignore,groups=opentelemetry.io,resources=opentelemetrycollectors,versions=v1alpha1,name=vopentelemetrycollectordelete.kb.io,sideEffects=none,admissionReviewVersions=v1 +// +kubebuilder:object:generate=false + +type CollectorWebhook struct { + logger logr.Logger + cfg config.Config + scheme *runtime.Scheme } -// +kubebuilder:webhook:path=/mutate-opentelemetry-io-v1alpha1-opentelemetrycollector,mutating=true,failurePolicy=fail,groups=opentelemetry.io,resources=opentelemetrycollectors,verbs=create;update,versions=v1alpha1,name=mopentelemetrycollector.kb.io,sideEffects=none,admissionReviewVersions=v1 +func (c CollectorWebhook) Default(ctx context.Context, obj runtime.Object) error { + otelcol, ok := obj.(*OpenTelemetryCollector) + if !ok { + return fmt.Errorf("expected an OpenTelemetryCollector, received %T", obj) + } + return c.defaulter(otelcol) +} -var _ webhook.Defaulter = &OpenTelemetryCollector{} +func (c CollectorWebhook) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + otelcol, ok := obj.(*OpenTelemetryCollector) + if !ok { + return nil, fmt.Errorf("expected an OpenTelemetryCollector, received %T", obj) + } + return c.validate(otelcol) +} -// Default implements webhook.Defaulter so a webhook will be registered for the type. -func (r *OpenTelemetryCollector) Default() { - opentelemetrycollectorlog.Info("default", "name", r.Name) +func (c CollectorWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { + otelcol, ok := newObj.(*OpenTelemetryCollector) + if !ok { + return nil, fmt.Errorf("expected an OpenTelemetryCollector, received %T", newObj) + } + return c.validate(otelcol) +} +func (c CollectorWebhook) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + otelcol, ok := obj.(*OpenTelemetryCollector) + if !ok || otelcol == nil { + return nil, fmt.Errorf("expected an OpenTelemetryCollector, received %T", obj) + } + return c.validate(otelcol) +} + +func (c CollectorWebhook) defaulter(r *OpenTelemetryCollector) error { if len(r.Spec.Mode) == 0 { r.Spec.Mode = ModeDeployment } @@ -117,74 +150,53 @@ func (r *OpenTelemetryCollector) Default() { if len(r.Spec.ManagementState) == 0 { r.Spec.ManagementState = ManagementStateManaged } + return nil } -// +kubebuilder:webhook:verbs=create;update,path=/validate-opentelemetry-io-v1alpha1-opentelemetrycollector,mutating=false,failurePolicy=fail,groups=opentelemetry.io,resources=opentelemetrycollectors,versions=v1alpha1,name=vopentelemetrycollectorcreateupdate.kb.io,sideEffects=none,admissionReviewVersions=v1 -// +kubebuilder:webhook:verbs=delete,path=/validate-opentelemetry-io-v1alpha1-opentelemetrycollector,mutating=false,failurePolicy=ignore,groups=opentelemetry.io,resources=opentelemetrycollectors,versions=v1alpha1,name=vopentelemetrycollectordelete.kb.io,sideEffects=none,admissionReviewVersions=v1 - -var _ webhook.Validator = &OpenTelemetryCollector{} - -// ValidateCreate implements webhook.Validator so a webhook will be registered for the type. -func (r *OpenTelemetryCollector) ValidateCreate() (admission.Warnings, error) { - opentelemetrycollectorlog.Info("validate create", "name", r.Name) - return nil, r.validateCRDSpec() -} - -// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. -func (r *OpenTelemetryCollector) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { - opentelemetrycollectorlog.Info("validate update", "name", r.Name) - return nil, r.validateCRDSpec() -} - -// ValidateDelete implements webhook.Validator so a webhook will be registered for the type. -func (r *OpenTelemetryCollector) ValidateDelete() (admission.Warnings, error) { - opentelemetrycollectorlog.Info("validate delete", "name", r.Name) - return nil, nil -} - -func (r *OpenTelemetryCollector) validateCRDSpec() error { +func (c CollectorWebhook) validate(r *OpenTelemetryCollector) (admission.Warnings, error) { + warnings := admission.Warnings{} // validate volumeClaimTemplates if r.Spec.Mode != ModeStatefulSet && len(r.Spec.VolumeClaimTemplates) > 0 { - return fmt.Errorf("the OpenTelemetry Collector mode is set to %s, which does not support the attribute 'volumeClaimTemplates'", r.Spec.Mode) + return warnings, fmt.Errorf("the OpenTelemetry Collector mode is set to %s, which does not support the attribute 'volumeClaimTemplates'", r.Spec.Mode) } // validate tolerations if r.Spec.Mode == ModeSidecar && len(r.Spec.Tolerations) > 0 { - return fmt.Errorf("the OpenTelemetry Collector mode is set to %s, which does not support the attribute 'tolerations'", r.Spec.Mode) + return warnings, fmt.Errorf("the OpenTelemetry Collector mode is set to %s, which does not support the attribute 'tolerations'", r.Spec.Mode) } // validate priorityClassName if r.Spec.Mode == ModeSidecar && r.Spec.PriorityClassName != "" { - return fmt.Errorf("the OpenTelemetry Collector mode is set to %s, which does not support the attribute 'priorityClassName'", r.Spec.Mode) + return warnings, fmt.Errorf("the OpenTelemetry Collector mode is set to %s, which does not support the attribute 'priorityClassName'", r.Spec.Mode) } // validate affinity if r.Spec.Mode == ModeSidecar && r.Spec.Affinity != nil { - return fmt.Errorf("the OpenTelemetry Collector mode is set to %s, which does not support the attribute 'affinity'", r.Spec.Mode) + return warnings, fmt.Errorf("the OpenTelemetry Collector mode is set to %s, which does not support the attribute 'affinity'", r.Spec.Mode) } if r.Spec.Mode == ModeSidecar && len(r.Spec.AdditionalContainers) > 0 { - return fmt.Errorf("the OpenTelemetry Collector mode is set to %s, which does not support the attribute 'AdditionalContainers'", r.Spec.Mode) + return warnings, fmt.Errorf("the OpenTelemetry Collector mode is set to %s, which does not support the attribute 'AdditionalContainers'", r.Spec.Mode) } // validate target allocation if r.Spec.TargetAllocator.Enabled && r.Spec.Mode != ModeStatefulSet { - return fmt.Errorf("the OpenTelemetry Collector mode is set to %s, which does not support the target allocation deployment", r.Spec.Mode) + return warnings, fmt.Errorf("the OpenTelemetry Collector mode is set to %s, which does not support the target allocation deployment", r.Spec.Mode) } // validate Prometheus config for target allocation if r.Spec.TargetAllocator.Enabled { promCfg, err := ta.ConfigToPromConfig(r.Spec.Config) if err != nil { - return fmt.Errorf("the OpenTelemetry Spec Prometheus configuration is incorrect, %w", err) + return warnings, fmt.Errorf("the OpenTelemetry Spec Prometheus configuration is incorrect, %w", err) } err = ta.ValidatePromConfig(promCfg, r.Spec.TargetAllocator.Enabled, featuregate.EnableTargetAllocatorRewrite.IsEnabled()) if err != nil { - return fmt.Errorf("the OpenTelemetry Spec Prometheus configuration is incorrect, %w", err) + return warnings, fmt.Errorf("the OpenTelemetry Spec Prometheus configuration is incorrect, %w", err) } err = ta.ValidateTargetAllocatorConfig(r.Spec.TargetAllocator.PrometheusCR.Enabled, promCfg) if err != nil { - return fmt.Errorf("the OpenTelemetry Spec Prometheus configuration is incorrect, %w", err) + return warnings, fmt.Errorf("the OpenTelemetry Spec Prometheus configuration is incorrect, %w", err) } } @@ -193,29 +205,31 @@ func (r *OpenTelemetryCollector) validateCRDSpec() error { nameErrs := validation.IsValidPortName(p.Name) numErrs := validation.IsValidPortNum(int(p.Port)) if len(nameErrs) > 0 || len(numErrs) > 0 { - return fmt.Errorf("the OpenTelemetry Spec Ports configuration is incorrect, port name '%s' errors: %s, num '%d' errors: %s", + return warnings, fmt.Errorf("the OpenTelemetry Spec Ports configuration is incorrect, port name '%s' errors: %s, num '%d' errors: %s", p.Name, nameErrs, p.Port, numErrs) } } - maxReplicas := new(int32) + var maxReplicas *int32 if r.Spec.Autoscaler != nil && r.Spec.Autoscaler.MaxReplicas != nil { maxReplicas = r.Spec.Autoscaler.MaxReplicas } // check deprecated .Spec.MaxReplicas if maxReplicas is not set - if *maxReplicas == 0 { + if maxReplicas == nil && r.Spec.MaxReplicas != nil { + warnings = append(warnings, "MaxReplicas is deprecated") maxReplicas = r.Spec.MaxReplicas } - minReplicas := new(int32) + var minReplicas *int32 if r.Spec.Autoscaler != nil && r.Spec.Autoscaler.MinReplicas != nil { minReplicas = r.Spec.Autoscaler.MinReplicas } // check deprecated .Spec.MinReplicas if minReplicas is not set - if *minReplicas == 0 { + if minReplicas == nil { if r.Spec.MinReplicas != nil { + warnings = append(warnings, "MinReplicas is deprecated") minReplicas = r.Spec.MinReplicas } else { minReplicas = r.Spec.Replicas @@ -223,7 +237,7 @@ func (r *OpenTelemetryCollector) validateCRDSpec() error { } if r.Spec.Ingress.Type == IngressTypeNginx && r.Spec.Mode == ModeSidecar { - return fmt.Errorf("the OpenTelemetry Spec Ingress configuration is incorrect. Ingress can only be used in combination with the modes: %s, %s, %s", + return warnings, fmt.Errorf("the OpenTelemetry Spec Ingress configuration is incorrect. Ingress can only be used in combination with the modes: %s, %s, %s", ModeDeployment, ModeDaemonSet, ModeStatefulSet, ) } @@ -231,65 +245,57 @@ func (r *OpenTelemetryCollector) validateCRDSpec() error { // validate autoscale with horizontal pod autoscaler if maxReplicas != nil { if *maxReplicas < int32(1) { - return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, maxReplicas should be defined and one or more") + return warnings, fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, maxReplicas should be defined and one or more") } if r.Spec.Replicas != nil && *r.Spec.Replicas > *maxReplicas { - return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, replicas must not be greater than maxReplicas") + return warnings, fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, replicas must not be greater than maxReplicas") } if minReplicas != nil && *minReplicas > *maxReplicas { - return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, minReplicas must not be greater than maxReplicas") + return warnings, fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, minReplicas must not be greater than maxReplicas") } if minReplicas != nil && *minReplicas < int32(1) { - return fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, minReplicas should be one or more") + return warnings, fmt.Errorf("the OpenTelemetry Spec autoscale configuration is incorrect, minReplicas should be one or more") } if r.Spec.Autoscaler != nil { - return checkAutoscalerSpec(r.Spec.Autoscaler) - } - } - - // validate pod disruption budget - - if r.Spec.PodDisruptionBudget != nil { - if r.Spec.PodDisruptionBudget.MaxUnavailable != nil && r.Spec.PodDisruptionBudget.MinAvailable != nil { - return fmt.Errorf("the OpenTelemetry Spec podDisruptionBudget configuration is incorrect, minAvailable and maxUnavailable are mutually exclusive") + return warnings, checkAutoscalerSpec(r.Spec.Autoscaler) } } if r.Spec.Ingress.Type == IngressTypeNginx && r.Spec.Mode == ModeSidecar { - return fmt.Errorf("the OpenTelemetry Spec Ingress configuiration is incorrect. Ingress can only be used in combination with the modes: %s, %s, %s", + return warnings, fmt.Errorf("the OpenTelemetry Spec Ingress configuiration is incorrect. Ingress can only be used in combination with the modes: %s, %s, %s", ModeDeployment, ModeDaemonSet, ModeStatefulSet, ) } if r.Spec.Ingress.RuleType == IngressRuleTypeSubdomain && (r.Spec.Ingress.Hostname == "" || r.Spec.Ingress.Hostname == "*") { - return fmt.Errorf("a valid Ingress hostname has to be defined for subdomain ruleType") + return warnings, fmt.Errorf("a valid Ingress hostname has to be defined for subdomain ruleType") } if r.Spec.LivenessProbe != nil { if r.Spec.LivenessProbe.InitialDelaySeconds != nil && *r.Spec.LivenessProbe.InitialDelaySeconds < 0 { - return fmt.Errorf("the OpenTelemetry Spec LivenessProbe InitialDelaySeconds configuration is incorrect. InitialDelaySeconds should be greater than or equal to 0") + return warnings, fmt.Errorf("the OpenTelemetry Spec LivenessProbe InitialDelaySeconds configuration is incorrect. InitialDelaySeconds should be greater than or equal to 0") } if r.Spec.LivenessProbe.PeriodSeconds != nil && *r.Spec.LivenessProbe.PeriodSeconds < 1 { - return fmt.Errorf("the OpenTelemetry Spec LivenessProbe PeriodSeconds configuration is incorrect. PeriodSeconds should be greater than or equal to 1") + return warnings, fmt.Errorf("the OpenTelemetry Spec LivenessProbe PeriodSeconds configuration is incorrect. PeriodSeconds should be greater than or equal to 1") } if r.Spec.LivenessProbe.TimeoutSeconds != nil && *r.Spec.LivenessProbe.TimeoutSeconds < 1 { - return fmt.Errorf("the OpenTelemetry Spec LivenessProbe TimeoutSeconds configuration is incorrect. TimeoutSeconds should be greater than or equal to 1") + return warnings, fmt.Errorf("the OpenTelemetry Spec LivenessProbe TimeoutSeconds configuration is incorrect. TimeoutSeconds should be greater than or equal to 1") } if r.Spec.LivenessProbe.SuccessThreshold != nil && *r.Spec.LivenessProbe.SuccessThreshold < 1 { - return fmt.Errorf("the OpenTelemetry Spec LivenessProbe SuccessThreshold configuration is incorrect. SuccessThreshold should be greater than or equal to 1") + return warnings, fmt.Errorf("the OpenTelemetry Spec LivenessProbe SuccessThreshold configuration is incorrect. SuccessThreshold should be greater than or equal to 1") } if r.Spec.LivenessProbe.FailureThreshold != nil && *r.Spec.LivenessProbe.FailureThreshold < 1 { - return fmt.Errorf("the OpenTelemetry Spec LivenessProbe FailureThreshold configuration is incorrect. FailureThreshold should be greater than or equal to 1") + return warnings, fmt.Errorf("the OpenTelemetry Spec LivenessProbe FailureThreshold configuration is incorrect. FailureThreshold should be greater than or equal to 1") } if r.Spec.LivenessProbe.TerminationGracePeriodSeconds != nil && *r.Spec.LivenessProbe.TerminationGracePeriodSeconds < 1 { - return fmt.Errorf("the OpenTelemetry Spec LivenessProbe TerminationGracePeriodSeconds configuration is incorrect. TerminationGracePeriodSeconds should be greater than or equal to 1") + return warnings, fmt.Errorf("the OpenTelemetry Spec LivenessProbe TerminationGracePeriodSeconds configuration is incorrect. TerminationGracePeriodSeconds should be greater than or equal to 1") } } - return nil + return warnings, nil } func checkAutoscalerSpec(autoscaler *AutoscalerSpec) error { @@ -332,3 +338,16 @@ func checkAutoscalerSpec(autoscaler *AutoscalerSpec) error { return nil } + +func SetupCollectorWebhook(mgr ctrl.Manager, cfg config.Config) error { + cvw := &CollectorWebhook{ + logger: mgr.GetLogger().WithValues("handler", "CollectorWebhook"), + scheme: mgr.GetScheme(), + cfg: cfg, + } + return ctrl.NewWebhookManagedBy(mgr). + For(&OpenTelemetryCollector{}). + WithValidator(cvw). + WithDefaulter(cvw). + Complete() +} diff --git a/apis/v1alpha1/opentelemetrycollector_webhook_test.go b/apis/v1alpha1/collector_webhook_test.go similarity index 87% rename from apis/v1alpha1/opentelemetrycollector_webhook_test.go rename to apis/v1alpha1/collector_webhook_test.go index b4b4b9e2be..0f23134717 100644 --- a/apis/v1alpha1/opentelemetrycollector_webhook_test.go +++ b/apis/v1alpha1/collector_webhook_test.go @@ -15,15 +15,26 @@ package v1alpha1 import ( + "context" "fmt" + "os" "testing" + "github.com/go-logr/logr" "github.com/stretchr/testify/assert" autoscalingv2 "k8s.io/api/autoscaling/v2" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/client-go/kubernetes/scheme" + + "github.com/open-telemetry/opentelemetry-operator/internal/config" +) + +var ( + testScheme *runtime.Scheme = scheme.Scheme ) func TestOTELColDefaultingWebhook(t *testing.T) { @@ -31,6 +42,11 @@ func TestOTELColDefaultingWebhook(t *testing.T) { five := int32(5) defaultCPUTarget := int32(90) + if err := AddToScheme(testScheme); err != nil { + fmt.Printf("failed to register scheme: %v", err) + os.Exit(1) + } + tests := []struct { name string otelcol OpenTelemetryCollector @@ -261,7 +277,17 @@ func TestOTELColDefaultingWebhook(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - test.otelcol.Default() + cvw := &CollectorWebhook{ + logger: logr.Discard(), + scheme: testScheme, + cfg: config.New( + config.WithCollectorImage("collector:v0.0.0"), + config.WithTargetAllocatorImage("ta:v0.0.0"), + ), + } + ctx := context.Background() + err := cvw.Default(ctx, &test.otelcol) + assert.NoError(t, err) assert.Equal(t, test.expected, test.otelcol) }) } @@ -279,9 +305,10 @@ func TestOTELColValidatingWebhook(t *testing.T) { five := int32(5) tests := []struct { //nolint:govet - name string - otelcol OpenTelemetryCollector - expectedErr string + name string + otelcol OpenTelemetryCollector + expectedErr string + expectedWarnings []string }{ { name: "valid empty spec", @@ -336,12 +363,6 @@ func TestOTELColValidatingWebhook(t *testing.T) { }, TargetCPUUtilization: &five, }, - PodDisruptionBudget: &PodDisruptionBudgetSpec{ - MinAvailable: &intstr.IntOrString{ - Type: intstr.Int, - IntVal: 1, - }, - }, }, }, }, @@ -440,7 +461,8 @@ func TestOTELColValidatingWebhook(t *testing.T) { MaxReplicas: &zero, }, }, - expectedErr: "maxReplicas should be defined and one or more", + expectedErr: "maxReplicas should be defined and one or more", + expectedWarnings: []string{"MaxReplicas is deprecated"}, }, { name: "invalid replicas, greater than max", @@ -450,7 +472,8 @@ func TestOTELColValidatingWebhook(t *testing.T) { Replicas: &five, }, }, - expectedErr: "replicas must not be greater than maxReplicas", + expectedErr: "replicas must not be greater than maxReplicas", + expectedWarnings: []string{"MaxReplicas is deprecated"}, }, { name: "invalid min replicas, greater than max", @@ -460,7 +483,8 @@ func TestOTELColValidatingWebhook(t *testing.T) { MinReplicas: &five, }, }, - expectedErr: "minReplicas must not be greater than maxReplicas", + expectedErr: "minReplicas must not be greater than maxReplicas", + expectedWarnings: []string{"MaxReplicas is deprecated", "MinReplicas is deprecated"}, }, { name: "invalid min replicas, lesser than 1", @@ -470,7 +494,8 @@ func TestOTELColValidatingWebhook(t *testing.T) { MinReplicas: &zero, }, }, - expectedErr: "minReplicas should be one or more", + expectedErr: "minReplicas should be one or more", + expectedWarnings: []string{"MaxReplicas is deprecated", "MinReplicas is deprecated"}, }, { name: "invalid autoscaler scale down", @@ -486,7 +511,8 @@ func TestOTELColValidatingWebhook(t *testing.T) { }, }, }, - expectedErr: "scaleDown should be one or more", + expectedErr: "scaleDown should be one or more", + expectedWarnings: []string{"MaxReplicas is deprecated"}, }, { name: "invalid autoscaler scale up", @@ -502,7 +528,8 @@ func TestOTELColValidatingWebhook(t *testing.T) { }, }, }, - expectedErr: "scaleUp should be one or more", + expectedErr: "scaleUp should be one or more", + expectedWarnings: []string{"MaxReplicas is deprecated"}, }, { name: "invalid autoscaler target cpu utilization", @@ -514,7 +541,8 @@ func TestOTELColValidatingWebhook(t *testing.T) { }, }, }, - expectedErr: "targetCPUUtilization should be greater than 0 and less than 100", + expectedErr: "targetCPUUtilization should be greater than 0 and less than 100", + expectedWarnings: []string{"MaxReplicas is deprecated"}, }, { name: "autoscaler minReplicas is less than maxReplicas", @@ -542,7 +570,8 @@ func TestOTELColValidatingWebhook(t *testing.T) { }, }, }, - expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, metric type unsupported. Expected metric of source type Pod", + expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, metric type unsupported. Expected metric of source type Pod", + expectedWarnings: []string{"MaxReplicas is deprecated"}, }, { name: "invalid pod metric average value", @@ -567,7 +596,8 @@ func TestOTELColValidatingWebhook(t *testing.T) { }, }, }, - expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, average value should be greater than 0", + expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, average value should be greater than 0", + expectedWarnings: []string{"MaxReplicas is deprecated"}, }, { name: "utilization target is not valid with pod metrics", @@ -592,26 +622,8 @@ func TestOTELColValidatingWebhook(t *testing.T) { }, }, }, - expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, invalid pods target type", - }, - { - name: "pdb minAvailable and maxUnavailable have been set together", - otelcol: OpenTelemetryCollector{ - Spec: OpenTelemetryCollectorSpec{ - MaxReplicas: &three, - PodDisruptionBudget: &PodDisruptionBudgetSpec{ - MinAvailable: &intstr.IntOrString{ - Type: intstr.Int, - IntVal: 1, - }, - MaxUnavailable: &intstr.IntOrString{ - Type: intstr.Int, - IntVal: 1, - }, - }, - }, - }, - expectedErr: "the OpenTelemetry Spec podDisruptionBudget configuration is incorrect, minAvailable and maxUnavailable are mutually exclusive", + expectedErr: "the OpenTelemetry Spec autoscale configuration is incorrect, invalid pods target type", + expectedWarnings: []string{"MaxReplicas is deprecated"}, }, { name: "invalid deployment mode incompabible with ingress settings", @@ -758,11 +770,25 @@ func TestOTELColValidatingWebhook(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - err := test.otelcol.validateCRDSpec() + cvw := &CollectorWebhook{ + logger: logr.Discard(), + scheme: testScheme, + cfg: config.New( + config.WithCollectorImage("collector:v0.0.0"), + config.WithTargetAllocatorImage("ta:v0.0.0"), + ), + } + ctx := context.Background() + warnings, err := cvw.ValidateCreate(ctx, &test.otelcol) if test.expectedErr == "" { assert.NoError(t, err) return } + if len(test.expectedWarnings) == 0 { + assert.Empty(t, warnings, test.expectedWarnings) + } else { + assert.ElementsMatch(t, warnings, test.expectedWarnings) + } assert.ErrorContains(t, err, test.expectedErr) }) } diff --git a/apis/v1alpha1/instrumentation_webhook.go b/apis/v1alpha1/instrumentation_webhook.go index 234784dd15..beeaedf362 100644 --- a/apis/v1alpha1/instrumentation_webhook.go +++ b/apis/v1alpha1/instrumentation_webhook.go @@ -15,56 +15,84 @@ package v1alpha1 import ( + "context" "fmt" "strconv" "strings" + "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/pkg/constants" ) const ( - AnnotationDefaultAutoInstrumentationJava = "instrumentation.opentelemetry.io/default-auto-instrumentation-java-image" - AnnotationDefaultAutoInstrumentationNodeJS = "instrumentation.opentelemetry.io/default-auto-instrumentation-nodejs-image" - AnnotationDefaultAutoInstrumentationPython = "instrumentation.opentelemetry.io/default-auto-instrumentation-python-image" - AnnotationDefaultAutoInstrumentationDotNet = "instrumentation.opentelemetry.io/default-auto-instrumentation-dotnet-image" - AnnotationDefaultAutoInstrumentationGo = "instrumentation.opentelemetry.io/default-auto-instrumentation-go-image" - AnnotationDefaultAutoInstrumentationApacheHttpd = "instrumentation.opentelemetry.io/default-auto-instrumentation-apache-httpd-image" - AnnotationDefaultAutoInstrumentationNginx = "instrumentation.opentelemetry.io/default-auto-instrumentation-nginx-image" - envPrefix = "OTEL_" - envSplunkPrefix = "SPLUNK_" + envPrefix = "OTEL_" + envSplunkPrefix = "SPLUNK_" ) -// log is for logging in this package. -var instrumentationlog = logf.Log.WithName("instrumentation-resource") +var ( + _ admission.CustomValidator = &InstrumentationWebhook{} + _ admission.CustomDefaulter = &InstrumentationWebhook{} + initContainerDefaultLimitResources = corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("500m"), + corev1.ResourceMemory: resource.MustParse("128Mi"), + } + initContainerDefaultRequestedResources = corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1m"), + corev1.ResourceMemory: resource.MustParse("128Mi"), + } +) + +// +kubebuilder:webhook:path=/mutate-opentelemetry-io-v1alpha1-instrumentation,mutating=true,failurePolicy=fail,sideEffects=None,groups=opentelemetry.io,resources=instrumentations,verbs=create;update,versions=v1alpha1,name=minstrumentation.kb.io,admissionReviewVersions=v1 +// +kubebuilder:webhook:verbs=create;update,path=/validate-opentelemetry-io-v1alpha1-instrumentation,mutating=false,failurePolicy=fail,groups=opentelemetry.io,resources=instrumentations,versions=v1alpha1,name=vinstrumentationcreateupdate.kb.io,sideEffects=none,admissionReviewVersions=v1 +// +kubebuilder:webhook:verbs=delete,path=/validate-opentelemetry-io-v1alpha1-instrumentation,mutating=false,failurePolicy=ignore,groups=opentelemetry.io,resources=instrumentations,versions=v1alpha1,name=vinstrumentationdelete.kb.io,sideEffects=none,admissionReviewVersions=v1 +// +kubebuilder:object:generate=false -var initContainerDefaultLimitResources = corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("500m"), - corev1.ResourceMemory: resource.MustParse("128Mi"), +type InstrumentationWebhook struct { + logger logr.Logger + cfg config.Config + scheme *runtime.Scheme } -var initContainerDefaultRequestedResources = corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("1m"), - corev1.ResourceMemory: resource.MustParse("128Mi"), + +func (w InstrumentationWebhook) Default(ctx context.Context, obj runtime.Object) error { + instrumentation, ok := obj.(*Instrumentation) + if !ok { + return fmt.Errorf("expected an Instrumentation, received %T", obj) + } + return w.defaulter(instrumentation) } -func (r *Instrumentation) SetupWebhookWithManager(mgr ctrl.Manager) error { - return ctrl.NewWebhookManagedBy(mgr). - For(r). - Complete() +func (w InstrumentationWebhook) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + inst, ok := obj.(*Instrumentation) + if !ok { + return nil, fmt.Errorf("expected an Instrumentation, received %T", obj) + } + return w.validate(inst) } -//+kubebuilder:webhook:path=/mutate-opentelemetry-io-v1alpha1-instrumentation,mutating=true,failurePolicy=fail,sideEffects=None,groups=opentelemetry.io,resources=instrumentations,verbs=create;update,versions=v1alpha1,name=minstrumentation.kb.io,admissionReviewVersions=v1 +func (w InstrumentationWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { + inst, ok := newObj.(*Instrumentation) + if !ok { + return nil, fmt.Errorf("expected an Instrumentation, received %T", newObj) + } + return w.validate(inst) +} -var _ webhook.Defaulter = &Instrumentation{} +func (w InstrumentationWebhook) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + inst, ok := obj.(*Instrumentation) + if !ok || inst == nil { + return nil, fmt.Errorf("expected an Instrumentation, received %T", obj) + } + return w.validate(inst) +} -// Default implements webhook.Defaulter so a webhook will be registered for the type. -func (r *Instrumentation) Default() { - instrumentationlog.Info("default", "name", r.Name) +func (w InstrumentationWebhook) defaulter(r *Instrumentation) error { if r.Labels == nil { r.Labels = map[string]string{} } @@ -73,9 +101,7 @@ func (r *Instrumentation) Default() { } if r.Spec.Java.Image == "" { - if val, ok := r.Annotations[AnnotationDefaultAutoInstrumentationJava]; ok { - r.Spec.Java.Image = val - } + r.Spec.Java.Image = w.cfg.AutoInstrumentationJavaImage() } if r.Spec.Java.Resources.Limits == nil { r.Spec.Java.Resources.Limits = corev1.ResourceList{ @@ -90,9 +116,7 @@ func (r *Instrumentation) Default() { } } if r.Spec.NodeJS.Image == "" { - if val, ok := r.Annotations[AnnotationDefaultAutoInstrumentationNodeJS]; ok { - r.Spec.NodeJS.Image = val - } + r.Spec.NodeJS.Image = w.cfg.AutoInstrumentationNodeJSImage() } if r.Spec.NodeJS.Resources.Limits == nil { r.Spec.NodeJS.Resources.Limits = corev1.ResourceList{ @@ -107,9 +131,7 @@ func (r *Instrumentation) Default() { } } if r.Spec.Python.Image == "" { - if val, ok := r.Annotations[AnnotationDefaultAutoInstrumentationPython]; ok { - r.Spec.Python.Image = val - } + r.Spec.Python.Image = w.cfg.AutoInstrumentationPythonImage() } if r.Spec.Python.Resources.Limits == nil { r.Spec.Python.Resources.Limits = corev1.ResourceList{ @@ -124,9 +146,7 @@ func (r *Instrumentation) Default() { } } if r.Spec.DotNet.Image == "" { - if val, ok := r.Annotations[AnnotationDefaultAutoInstrumentationDotNet]; ok { - r.Spec.DotNet.Image = val - } + r.Spec.DotNet.Image = w.cfg.AutoInstrumentationDotNetImage() } if r.Spec.DotNet.Resources.Limits == nil { r.Spec.DotNet.Resources.Limits = corev1.ResourceList{ @@ -141,9 +161,7 @@ func (r *Instrumentation) Default() { } } if r.Spec.Go.Image == "" { - if val, ok := r.Annotations[AnnotationDefaultAutoInstrumentationGo]; ok { - r.Spec.Go.Image = val - } + r.Spec.Go.Image = w.cfg.AutoInstrumentationGoImage() } if r.Spec.Go.Resources.Limits == nil { r.Spec.Go.Resources.Limits = corev1.ResourceList{ @@ -158,9 +176,7 @@ func (r *Instrumentation) Default() { } } if r.Spec.ApacheHttpd.Image == "" { - if val, ok := r.Annotations[AnnotationDefaultAutoInstrumentationApacheHttpd]; ok { - r.Spec.ApacheHttpd.Image = val - } + r.Spec.ApacheHttpd.Image = w.cfg.AutoInstrumentationApacheHttpdImage() } if r.Spec.ApacheHttpd.Resources.Limits == nil { r.Spec.ApacheHttpd.Resources.Limits = initContainerDefaultLimitResources @@ -175,9 +191,7 @@ func (r *Instrumentation) Default() { r.Spec.ApacheHttpd.ConfigPath = "/usr/local/apache2/conf" } if r.Spec.Nginx.Image == "" { - if val, ok := r.Annotations[AnnotationDefaultAutoInstrumentationNginx]; ok { - r.Spec.Nginx.Image = val - } + r.Spec.Nginx.Image = w.cfg.AutoInstrumentationNginxImage() } if r.Spec.Nginx.Resources.Limits == nil { r.Spec.Nginx.Resources.Limits = initContainerDefaultLimitResources @@ -188,73 +202,33 @@ func (r *Instrumentation) Default() { if r.Spec.Nginx.ConfigFile == "" { r.Spec.Nginx.ConfigFile = "/etc/nginx/nginx.conf" } -} - -// +kubebuilder:webhook:verbs=create;update,path=/validate-opentelemetry-io-v1alpha1-instrumentation,mutating=false,failurePolicy=fail,groups=opentelemetry.io,resources=instrumentations,versions=v1alpha1,name=vinstrumentationcreateupdate.kb.io,sideEffects=none,admissionReviewVersions=v1 -// +kubebuilder:webhook:verbs=delete,path=/validate-opentelemetry-io-v1alpha1-instrumentation,mutating=false,failurePolicy=ignore,groups=opentelemetry.io,resources=instrumentations,versions=v1alpha1,name=vinstrumentationdelete.kb.io,sideEffects=none,admissionReviewVersions=v1 - -var _ webhook.Validator = &Instrumentation{} - -// ValidateCreate implements webhook.Validator so a webhook will be registered for the type. -func (r *Instrumentation) ValidateCreate() (admission.Warnings, error) { - instrumentationlog.Info("validate create", "name", r.Name) - return nil, r.validate() -} - -// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. -func (r *Instrumentation) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { - instrumentationlog.Info("validate update", "name", r.Name) - return nil, r.validate() -} - -// ValidateDelete implements webhook.Validator so a webhook will be registered for the type. -func (r *Instrumentation) ValidateDelete() (admission.Warnings, error) { - instrumentationlog.Info("validate delete", "name", r.Name) - return nil, nil -} - -func validateJaegerRemoteSamplerArgument(argument string) error { - parts := strings.Split(argument, ",") - - for _, part := range parts { - kv := strings.Split(part, "=") - if len(kv) != 2 { - return fmt.Errorf("invalid argument: %s, the argument should be in the form of key=value", part) - } - - switch kv[0] { - case "endpoint": - if kv[1] == "" { - return fmt.Errorf("endpoint cannot be empty") - } - case "pollingIntervalMs": - if _, err := strconv.Atoi(kv[1]); err != nil { - return fmt.Errorf("invalid pollingIntervalMs: %s", kv[1]) - } - case "initialSamplingRate": - rate, err := strconv.ParseFloat(kv[1], 64) - if err != nil { - return fmt.Errorf("invalid initialSamplingRate: %s", kv[1]) - } - if rate < 0 || rate > 1 { - return fmt.Errorf("initialSamplingRate should be in rage [0..1]: %s", kv[1]) - } - } - } + // Set the defaulting annotations + if r.Annotations == nil { + r.Annotations = map[string]string{} + } + r.Annotations[constants.AnnotationDefaultAutoInstrumentationJava] = w.cfg.AutoInstrumentationJavaImage() + r.Annotations[constants.AnnotationDefaultAutoInstrumentationNodeJS] = w.cfg.AutoInstrumentationNodeJSImage() + r.Annotations[constants.AnnotationDefaultAutoInstrumentationPython] = w.cfg.AutoInstrumentationPythonImage() + r.Annotations[constants.AnnotationDefaultAutoInstrumentationDotNet] = w.cfg.AutoInstrumentationDotNetImage() + r.Annotations[constants.AnnotationDefaultAutoInstrumentationGo] = w.cfg.AutoInstrumentationGoImage() + r.Annotations[constants.AnnotationDefaultAutoInstrumentationApacheHttpd] = w.cfg.AutoInstrumentationApacheHttpdImage() + r.Annotations[constants.AnnotationDefaultAutoInstrumentationNginx] = w.cfg.AutoInstrumentationNginxImage() return nil } -func (r *Instrumentation) validate() error { +func (w InstrumentationWebhook) validate(r *Instrumentation) (admission.Warnings, error) { + var warnings []string switch r.Spec.Sampler.Type { - case "": // not set, do nothing + case "": + warnings = append(warnings, "sampler type not set") case TraceIDRatio, ParentBasedTraceIDRatio: if r.Spec.Sampler.Argument != "" { rate, err := strconv.ParseFloat(r.Spec.Sampler.Argument, 64) if err != nil { - return fmt.Errorf("spec.sampler.argument is not a number: %s", r.Spec.Sampler.Argument) + return warnings, fmt.Errorf("spec.sampler.argument is not a number: %s", r.Spec.Sampler.Argument) } if rate < 0 || rate > 1 { - return fmt.Errorf("spec.sampler.argument should be in rage [0..1]: %s", r.Spec.Sampler.Argument) + return warnings, fmt.Errorf("spec.sampler.argument should be in rage [0..1]: %s", r.Spec.Sampler.Argument) } } case JaegerRemote, ParentBasedJaegerRemote: @@ -264,44 +238,43 @@ func (r *Instrumentation) validate() error { err := validateJaegerRemoteSamplerArgument(r.Spec.Sampler.Argument) if err != nil { - return fmt.Errorf("spec.sampler.argument is not a valid argument for sampler %s: %w", r.Spec.Sampler.Type, err) + return warnings, fmt.Errorf("spec.sampler.argument is not a valid argument for sampler %s: %w", r.Spec.Sampler.Type, err) } } case AlwaysOn, AlwaysOff, ParentBasedAlwaysOn, ParentBasedAlwaysOff, XRaySampler: default: - return fmt.Errorf("spec.sampler.type is not valid: %s", r.Spec.Sampler.Type) + return warnings, fmt.Errorf("spec.sampler.type is not valid: %s", r.Spec.Sampler.Type) } // validate env vars - if err := r.validateEnv(r.Spec.Env); err != nil { - return err + if err := w.validateEnv(r.Spec.Env); err != nil { + return warnings, err } - if err := r.validateEnv(r.Spec.Java.Env); err != nil { - return err + if err := w.validateEnv(r.Spec.Java.Env); err != nil { + return warnings, err } - if err := r.validateEnv(r.Spec.NodeJS.Env); err != nil { - return err + if err := w.validateEnv(r.Spec.NodeJS.Env); err != nil { + return warnings, err } - if err := r.validateEnv(r.Spec.Python.Env); err != nil { - return err + if err := w.validateEnv(r.Spec.Python.Env); err != nil { + return warnings, err } - if err := r.validateEnv(r.Spec.DotNet.Env); err != nil { - return err + if err := w.validateEnv(r.Spec.DotNet.Env); err != nil { + return warnings, err } - if err := r.validateEnv(r.Spec.Go.Env); err != nil { - return err + if err := w.validateEnv(r.Spec.Go.Env); err != nil { + return warnings, err } - if err := r.validateEnv(r.Spec.ApacheHttpd.Env); err != nil { - return err + if err := w.validateEnv(r.Spec.ApacheHttpd.Env); err != nil { + return warnings, err } - if err := r.validateEnv(r.Spec.Nginx.Env); err != nil { - return err + if err := w.validateEnv(r.Spec.Nginx.Env); err != nil { + return warnings, err } - - return nil + return warnings, nil } -func (r *Instrumentation) validateEnv(envs []corev1.EnvVar) error { +func (w InstrumentationWebhook) validateEnv(envs []corev1.EnvVar) error { for _, env := range envs { if !strings.HasPrefix(env.Name, envPrefix) && !strings.HasPrefix(env.Name, envSplunkPrefix) { return fmt.Errorf("env name should start with \"OTEL_\" or \"SPLUNK_\": %s", env.Name) @@ -309,3 +282,55 @@ func (r *Instrumentation) validateEnv(envs []corev1.EnvVar) error { } return nil } + +func validateJaegerRemoteSamplerArgument(argument string) error { + parts := strings.Split(argument, ",") + + for _, part := range parts { + kv := strings.Split(part, "=") + if len(kv) != 2 { + return fmt.Errorf("invalid argument: %s, the argument should be in the form of key=value", part) + } + + switch kv[0] { + case "endpoint": + if kv[1] == "" { + return fmt.Errorf("endpoint cannot be empty") + } + case "pollingIntervalMs": + if _, err := strconv.Atoi(kv[1]); err != nil { + return fmt.Errorf("invalid pollingIntervalMs: %s", kv[1]) + } + case "initialSamplingRate": + rate, err := strconv.ParseFloat(kv[1], 64) + if err != nil { + return fmt.Errorf("invalid initialSamplingRate: %s", kv[1]) + } + if rate < 0 || rate > 1 { + return fmt.Errorf("initialSamplingRate should be in rage [0..1]: %s", kv[1]) + } + } + } + return nil +} + +func NewInstrumentationWebhook(logger logr.Logger, scheme *runtime.Scheme, cfg config.Config) *InstrumentationWebhook { + return &InstrumentationWebhook{ + logger: logger, + scheme: scheme, + cfg: cfg, + } +} + +func SetupInstrumentationWebhook(mgr ctrl.Manager, cfg config.Config) error { + ivw := NewInstrumentationWebhook( + mgr.GetLogger().WithValues("handler", "InstrumentationWebhook"), + mgr.GetScheme(), + cfg, + ) + return ctrl.NewWebhookManagedBy(mgr). + For(&Instrumentation{}). + WithValidator(ivw). + WithDefaulter(ivw). + Complete() +} diff --git a/apis/v1alpha1/instrumentation_webhook_test.go b/apis/v1alpha1/instrumentation_webhook_test.go index 9791a93014..46f7327ad8 100644 --- a/apis/v1alpha1/instrumentation_webhook_test.go +++ b/apis/v1alpha1/instrumentation_webhook_test.go @@ -15,26 +15,28 @@ package v1alpha1 import ( + "context" "testing" "github.com/stretchr/testify/assert" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "github.com/open-telemetry/opentelemetry-operator/internal/config" ) func TestInstrumentationDefaultingWebhook(t *testing.T) { - inst := &Instrumentation{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - AnnotationDefaultAutoInstrumentationJava: "java-img:1", - AnnotationDefaultAutoInstrumentationNodeJS: "nodejs-img:1", - AnnotationDefaultAutoInstrumentationPython: "python-img:1", - AnnotationDefaultAutoInstrumentationDotNet: "dotnet-img:1", - AnnotationDefaultAutoInstrumentationApacheHttpd: "apache-httpd-img:1", - AnnotationDefaultAutoInstrumentationNginx: "nginx-img:1", - }, - }, - } - inst.Default() + inst := &Instrumentation{} + err := InstrumentationWebhook{ + cfg: config.New( + config.WithAutoInstrumentationJavaImage("java-img:1"), + config.WithAutoInstrumentationNodeJSImage("nodejs-img:1"), + config.WithAutoInstrumentationPythonImage("python-img:1"), + config.WithAutoInstrumentationDotNetImage("dotnet-img:1"), + config.WithAutoInstrumentationApacheHttpdImage("apache-httpd-img:1"), + config.WithAutoInstrumentationNginxImage("nginx-img:1"), + ), + }.Default(context.Background(), inst) + assert.NoError(t, err) assert.Equal(t, "java-img:1", inst.Spec.Java.Image) assert.Equal(t, "nodejs-img:1", inst.Spec.NodeJS.Image) assert.Equal(t, "python-img:1", inst.Spec.Python.Image) @@ -45,15 +47,17 @@ func TestInstrumentationDefaultingWebhook(t *testing.T) { func TestInstrumentationValidatingWebhook(t *testing.T) { tests := []struct { - name string - err string - inst Instrumentation + name string + err string + warnings admission.Warnings + inst Instrumentation }{ { name: "all defaults", inst: Instrumentation{ Spec: InstrumentationSpec{}, }, + warnings: []string{"sampler type not set"}, }, { name: "sampler configuration not present", @@ -62,6 +66,7 @@ func TestInstrumentationValidatingWebhook(t *testing.T) { Sampler: Sampler{}, }, }, + warnings: []string{"sampler type not set"}, }, { name: "argument is not a number", @@ -112,19 +117,20 @@ func TestInstrumentationValidatingWebhook(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { + ctx := context.Background() if test.err == "" { - warnings, err := test.inst.ValidateCreate() - assert.Nil(t, warnings) + warnings, err := InstrumentationWebhook{}.ValidateCreate(ctx, &test.inst) + assert.Equal(t, test.warnings, warnings) assert.Nil(t, err) - warnings, err = test.inst.ValidateUpdate(nil) - assert.Nil(t, warnings) + warnings, err = InstrumentationWebhook{}.ValidateUpdate(ctx, nil, &test.inst) + assert.Equal(t, test.warnings, warnings) assert.Nil(t, err) } else { - warnings, err := test.inst.ValidateCreate() - assert.Nil(t, warnings) + warnings, err := InstrumentationWebhook{}.ValidateCreate(ctx, &test.inst) + assert.Equal(t, test.warnings, warnings) assert.Contains(t, err.Error(), test.err) - warnings, err = test.inst.ValidateUpdate(nil) - assert.Nil(t, warnings) + warnings, err = InstrumentationWebhook{}.ValidateUpdate(ctx, nil, &test.inst) + assert.Equal(t, test.warnings, warnings) assert.Contains(t, err.Error(), test.err) } }) @@ -171,18 +177,19 @@ func TestInstrumentationJaegerRemote(t *testing.T) { }, }, } + ctx := context.Background() if test.err == "" { - warnings, err := inst.ValidateCreate() + warnings, err := InstrumentationWebhook{}.ValidateCreate(ctx, &inst) assert.Nil(t, warnings) assert.Nil(t, err) - warnings, err = inst.ValidateUpdate(nil) + warnings, err = InstrumentationWebhook{}.ValidateUpdate(ctx, nil, &inst) assert.Nil(t, warnings) assert.Nil(t, err) } else { - warnings, err := inst.ValidateCreate() + warnings, err := InstrumentationWebhook{}.ValidateCreate(ctx, &inst) assert.Nil(t, warnings) assert.Contains(t, err.Error(), test.err) - warnings, err = inst.ValidateUpdate(nil) + warnings, err = InstrumentationWebhook{}.ValidateUpdate(ctx, nil, &inst) assert.Nil(t, warnings) assert.Contains(t, err.Error(), test.err) } diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 4d91e64486..acf46e64ef 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -125,7 +125,7 @@ func TestMain(m *testing.M) { os.Exit(1) } - if err = (&v1alpha1.OpenTelemetryCollector{}).SetupWebhookWithManager(mgr); err != nil { + if err = v1alpha1.SetupCollectorWebhook(mgr, config.New()); err != nil { fmt.Printf("failed to SetupWebhookWithManager: %v", err) os.Exit(1) } diff --git a/internal/config/main.go b/internal/config/main.go index 951340136b..3c2c594143 100644 --- a/internal/config/main.go +++ b/internal/config/main.go @@ -85,6 +85,7 @@ func New(opts ...Option) Config { autoInstrumentationNodeJSImage: o.autoInstrumentationNodeJSImage, autoInstrumentationPythonImage: o.autoInstrumentationPythonImage, autoInstrumentationDotNetImage: o.autoInstrumentationDotNetImage, + autoInstrumentationGoImage: o.autoInstrumentationGoImage, autoInstrumentationApacheHttpdImage: o.autoInstrumentationApacheHttpdImage, autoInstrumentationNginxImage: o.autoInstrumentationNginxImage, labelsFilter: o.labelsFilter, diff --git a/internal/webhookhandler/webhookhandler.go b/internal/webhook/podmutation/webhookhandler.go similarity index 92% rename from internal/webhookhandler/webhookhandler.go rename to internal/webhook/podmutation/webhookhandler.go index 11f166b790..9d9fa6e743 100644 --- a/internal/webhookhandler/webhookhandler.go +++ b/internal/webhook/podmutation/webhookhandler.go @@ -12,8 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package webhookhandler contains the webhook that injects sidecars into pods. -package webhookhandler +// Package podmutation contains the webhook that injects sidecars into pods. +package podmutation import ( "context" @@ -35,7 +35,7 @@ import ( // +kubebuilder:rbac:groups=opentelemetry.io,resources=instrumentations,verbs=get;list;watch // +kubebuilder:rbac:groups="apps",resources=replicasets,verbs=get;list;watch -var _ WebhookHandler = (*podSidecarInjector)(nil) +var _ WebhookHandler = (*podMutationWebhook)(nil) // WebhookHandler is a webhook handler that analyzes new pods and injects appropriate sidecars into it. type WebhookHandler interface { @@ -43,7 +43,7 @@ type WebhookHandler interface { } // the implementation. -type podSidecarInjector struct { +type podMutationWebhook struct { client client.Client decoder *admission.Decoder logger logr.Logger @@ -58,7 +58,7 @@ type PodMutator interface { // NewWebhookHandler creates a new WebhookHandler. func NewWebhookHandler(cfg config.Config, logger logr.Logger, decoder *admission.Decoder, cl client.Client, podMutators []PodMutator) WebhookHandler { - return &podSidecarInjector{ + return &podMutationWebhook{ config: cfg, decoder: decoder, logger: logger, @@ -67,7 +67,7 @@ func NewWebhookHandler(cfg config.Config, logger logr.Logger, decoder *admission } } -func (p *podSidecarInjector) Handle(ctx context.Context, req admission.Request) admission.Response { +func (p *podMutationWebhook) Handle(ctx context.Context, req admission.Request) admission.Response { pod := corev1.Pod{} err := p.decoder.Decode(req, &pod) if err != nil { diff --git a/internal/webhookhandler/webhookhandler_suite_test.go b/internal/webhook/podmutation/webhookhandler_suite_test.go similarity index 92% rename from internal/webhookhandler/webhookhandler_suite_test.go rename to internal/webhook/podmutation/webhookhandler_suite_test.go index 5c188e7ddd..05f6c062be 100644 --- a/internal/webhookhandler/webhookhandler_suite_test.go +++ b/internal/webhook/podmutation/webhookhandler_suite_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package webhookhandler_test +package podmutation_test import ( "context" @@ -37,6 +37,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" + "github.com/open-telemetry/opentelemetry-operator/internal/config" // +kubebuilder:scaffold:imports ) @@ -55,9 +56,9 @@ func TestMain(m *testing.M) { defer cancel() testEnv = &envtest.Environment{ - CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")}, + CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "config", "crd", "bases")}, WebhookInstallOptions: envtest.WebhookInstallOptions{ - Paths: []string{filepath.Join("..", "..", "config", "webhook")}, + Paths: []string{filepath.Join("..", "..", "..", "config", "webhook")}, }, } cfg, err = testEnv.Start() @@ -97,7 +98,7 @@ func TestMain(m *testing.M) { os.Exit(1) } - if err = (&v1alpha1.OpenTelemetryCollector{}).SetupWebhookWithManager(mgr); err != nil { + if err = v1alpha1.SetupCollectorWebhook(mgr, config.New()); err != nil { fmt.Printf("failed to SetupWebhookWithManager: %v", err) os.Exit(1) } diff --git a/internal/webhookhandler/webhookhandler_test.go b/internal/webhook/podmutation/webhookhandler_test.go similarity index 99% rename from internal/webhookhandler/webhookhandler_test.go rename to internal/webhook/podmutation/webhookhandler_test.go index 91aa2c3ccc..d5bb69b795 100644 --- a/internal/webhookhandler/webhookhandler_test.go +++ b/internal/webhook/podmutation/webhookhandler_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package webhookhandler_test +package podmutation_test import ( "context" @@ -33,7 +33,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" "github.com/open-telemetry/opentelemetry-operator/internal/config" "github.com/open-telemetry/opentelemetry-operator/internal/naming" - . "github.com/open-telemetry/opentelemetry-operator/internal/webhookhandler" + . "github.com/open-telemetry/opentelemetry-operator/internal/webhook/podmutation" "github.com/open-telemetry/opentelemetry-operator/pkg/sidecar" ) diff --git a/main.go b/main.go index 2f04fb0835..80872048b6 100644 --- a/main.go +++ b/main.go @@ -28,7 +28,6 @@ import ( monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" "github.com/spf13/pflag" colfeaturegate "go.opentelemetry.io/collector/featuregate" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" k8sruntime "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" @@ -48,7 +47,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/controllers" "github.com/open-telemetry/opentelemetry-operator/internal/config" "github.com/open-telemetry/opentelemetry-operator/internal/version" - "github.com/open-telemetry/opentelemetry-operator/internal/webhookhandler" + "github.com/open-telemetry/opentelemetry-operator/internal/webhook/podmutation" "github.com/open-telemetry/opentelemetry-operator/pkg/autodetect" collectorupgrade "github.com/open-telemetry/opentelemetry-operator/pkg/collector/upgrade" "github.com/open-telemetry/opentelemetry-operator/pkg/featuregate" @@ -70,7 +69,6 @@ type tlsConfig struct { func init() { utilruntime.Must(clientgoscheme.AddToScheme(scheme)) - utilruntime.Must(otelv1alpha1.AddToScheme(scheme)) utilruntime.Must(routev1.AddToScheme(scheme)) utilruntime.Must(monitoringv1.AddToScheme(scheme)) @@ -248,29 +246,18 @@ func main() { } if os.Getenv("ENABLE_WEBHOOKS") != "false" { - if err = (&otelv1alpha1.OpenTelemetryCollector{}).SetupWebhookWithManager(mgr); err != nil { + if err = otelv1alpha1.SetupCollectorWebhook(mgr, cfg); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "OpenTelemetryCollector") os.Exit(1) } - if err = (&otelv1alpha1.Instrumentation{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - otelv1alpha1.AnnotationDefaultAutoInstrumentationJava: autoInstrumentationJava, - otelv1alpha1.AnnotationDefaultAutoInstrumentationNodeJS: autoInstrumentationNodeJS, - otelv1alpha1.AnnotationDefaultAutoInstrumentationPython: autoInstrumentationPython, - otelv1alpha1.AnnotationDefaultAutoInstrumentationDotNet: autoInstrumentationDotNet, - otelv1alpha1.AnnotationDefaultAutoInstrumentationGo: autoInstrumentationGo, - otelv1alpha1.AnnotationDefaultAutoInstrumentationApacheHttpd: autoInstrumentationApacheHttpd, - otelv1alpha1.AnnotationDefaultAutoInstrumentationNginx: autoInstrumentationNginx}, - }, - }).SetupWebhookWithManager(mgr); err != nil { + if err = otelv1alpha1.SetupInstrumentationWebhook(mgr, cfg); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "Instrumentation") os.Exit(1) } decoder := admission.NewDecoder(mgr.GetScheme()) mgr.GetWebhookServer().Register("/mutate-v1-pod", &webhook.Admission{ - Handler: webhookhandler.NewWebhookHandler(cfg, ctrl.Log.WithName("pod-webhook"), decoder, mgr.GetClient(), - []webhookhandler.PodMutator{ + Handler: podmutation.NewWebhookHandler(cfg, ctrl.Log.WithName("pod-webhook"), decoder, mgr.GetClient(), + []podmutation.PodMutator{ sidecar.NewMutator(logger, cfg, mgr.GetClient()), instrumentation.NewMutator(logger, mgr.GetClient(), mgr.GetEventRecorderFor("opentelemetry-operator")), }), diff --git a/pkg/collector/reconcile/suite_test.go b/pkg/collector/reconcile/suite_test.go index 9feec99458..cfe28fc28a 100644 --- a/pkg/collector/reconcile/suite_test.go +++ b/pkg/collector/reconcile/suite_test.go @@ -135,7 +135,7 @@ func TestMain(m *testing.M) { os.Exit(1) } - if err = (&v1alpha1.OpenTelemetryCollector{}).SetupWebhookWithManager(mgr); err != nil { + if err = v1alpha1.SetupCollectorWebhook(mgr, config.New()); err != nil { fmt.Printf("failed to SetupWebhookWithManager: %v", err) os.Exit(1) } diff --git a/pkg/collector/upgrade/suite_test.go b/pkg/collector/upgrade/suite_test.go index 63c5cf4934..9496b9f47d 100644 --- a/pkg/collector/upgrade/suite_test.go +++ b/pkg/collector/upgrade/suite_test.go @@ -37,6 +37,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" + "github.com/open-telemetry/opentelemetry-operator/internal/config" // +kubebuilder:scaffold:imports ) @@ -48,6 +49,7 @@ var ( cancel context.CancelFunc err error cfg *rest.Config + conf = config.New() ) func TestMain(m *testing.M) { @@ -98,7 +100,7 @@ func TestMain(m *testing.M) { os.Exit(1) } - if err = (&v1alpha1.OpenTelemetryCollector{}).SetupWebhookWithManager(mgr); err != nil { + if err = v1alpha1.SetupCollectorWebhook(mgr, conf); err != nil { fmt.Printf("failed to SetupWebhookWithManager: %v", err) os.Exit(1) } diff --git a/pkg/constants/env.go b/pkg/constants/env.go index 0c4070905c..93f0e98bf6 100644 --- a/pkg/constants/env.go +++ b/pkg/constants/env.go @@ -22,6 +22,15 @@ const ( EnvOTELTracesSampler = "OTEL_TRACES_SAMPLER" EnvOTELTracesSamplerArg = "OTEL_TRACES_SAMPLER_ARG" + InstrumentationPrefix = "instrumentation.opentelemetry.io/" + AnnotationDefaultAutoInstrumentationJava = InstrumentationPrefix + "default-auto-instrumentation-java-image" + AnnotationDefaultAutoInstrumentationNodeJS = InstrumentationPrefix + "default-auto-instrumentation-nodejs-image" + AnnotationDefaultAutoInstrumentationPython = InstrumentationPrefix + "default-auto-instrumentation-python-image" + AnnotationDefaultAutoInstrumentationDotNet = InstrumentationPrefix + "default-auto-instrumentation-dotnet-image" + AnnotationDefaultAutoInstrumentationGo = InstrumentationPrefix + "default-auto-instrumentation-go-image" + AnnotationDefaultAutoInstrumentationApacheHttpd = InstrumentationPrefix + "default-auto-instrumentation-apache-httpd-image" + AnnotationDefaultAutoInstrumentationNginx = InstrumentationPrefix + "default-auto-instrumentation-nginx-image" + EnvPodName = "OTEL_RESOURCE_ATTRIBUTES_POD_NAME" EnvPodUID = "OTEL_RESOURCE_ATTRIBUTES_POD_UID" EnvNodeName = "OTEL_RESOURCE_ATTRIBUTES_NODE_NAME" diff --git a/pkg/instrumentation/podmutator.go b/pkg/instrumentation/podmutator.go index 49f93d7067..0a7878ae19 100644 --- a/pkg/instrumentation/podmutator.go +++ b/pkg/instrumentation/podmutator.go @@ -27,7 +27,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" - "github.com/open-telemetry/opentelemetry-operator/internal/webhookhandler" + "github.com/open-telemetry/opentelemetry-operator/internal/webhook/podmutation" "github.com/open-telemetry/opentelemetry-operator/pkg/featuregate" ) @@ -191,7 +191,7 @@ func (langInsts *languageInstrumentations) setInstrumentationLanguageContainers( } } -var _ webhookhandler.PodMutator = (*instPodMutator)(nil) +var _ podmutation.PodMutator = (*instPodMutator)(nil) func NewMutator(logger logr.Logger, client client.Client, recorder record.EventRecorder) *instPodMutator { return &instPodMutator{ diff --git a/pkg/instrumentation/upgrade/upgrade.go b/pkg/instrumentation/upgrade/upgrade.go index 0b45be4fb0..d9dbec6c6f 100644 --- a/pkg/instrumentation/upgrade/upgrade.go +++ b/pkg/instrumentation/upgrade/upgrade.go @@ -20,13 +20,27 @@ import ( "reflect" "github.com/go-logr/logr" + featuregate2 "go.opentelemetry.io/collector/featuregate" "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" + "github.com/open-telemetry/opentelemetry-operator/pkg/constants" "github.com/open-telemetry/opentelemetry-operator/pkg/featuregate" ) +var ( + defaultAnnotationToGate = map[string]*featuregate2.Gate{ + constants.AnnotationDefaultAutoInstrumentationJava: featuregate.EnableJavaAutoInstrumentationSupport, + constants.AnnotationDefaultAutoInstrumentationNodeJS: featuregate.EnableNodeJSAutoInstrumentationSupport, + constants.AnnotationDefaultAutoInstrumentationPython: featuregate.EnablePythonAutoInstrumentationSupport, + constants.AnnotationDefaultAutoInstrumentationDotNet: featuregate.EnableDotnetAutoInstrumentationSupport, + constants.AnnotationDefaultAutoInstrumentationGo: featuregate.EnableGoAutoInstrumentationSupport, + constants.AnnotationDefaultAutoInstrumentationApacheHttpd: featuregate.EnableApacheHTTPAutoInstrumentationSupport, + constants.AnnotationDefaultAutoInstrumentationNginx: featuregate.EnableNginxAutoInstrumentationSupport, + } +) + type InstrumentationUpgrade struct { Client client.Client Logger logr.Logger @@ -61,7 +75,7 @@ func (u *InstrumentationUpgrade) ManagedInstances(ctx context.Context) error { upgraded := u.upgrade(ctx, toUpgrade) if !reflect.DeepEqual(upgraded, toUpgrade) { // use update instead of patch because the patch does not upgrade annotations - if err := u.Client.Update(ctx, &upgraded); err != nil { + if err := u.Client.Update(ctx, upgraded); err != nil { u.Logger.Error(err, "failed to apply changes to instance", "name", upgraded.Name, "namespace", upgraded.Namespace) continue } @@ -74,97 +88,54 @@ func (u *InstrumentationUpgrade) ManagedInstances(ctx context.Context) error { return nil } -func (u *InstrumentationUpgrade) upgrade(_ context.Context, inst v1alpha1.Instrumentation) v1alpha1.Instrumentation { - autoInstJava := inst.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationJava] - if autoInstJava != "" { - if featuregate.EnableJavaAutoInstrumentationSupport.IsEnabled() { - // upgrade the image only if the image matches the annotation - if inst.Spec.Java.Image == autoInstJava { - inst.Spec.Java.Image = u.DefaultAutoInstJava - inst.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationJava] = u.DefaultAutoInstJava - } - } else { - u.Logger.Error(nil, "support for Java auto instrumentation is not enabled") - u.Recorder.Event(inst.DeepCopy(), "Warning", "InstrumentationUpgradeRejected", "support for Java auto instrumentation is not enabled") - } - } - autoInstNodeJS := inst.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationNodeJS] - if autoInstNodeJS != "" { - if featuregate.EnableNodeJSAutoInstrumentationSupport.IsEnabled() { - // upgrade the image only if the image matches the annotation - if inst.Spec.NodeJS.Image == autoInstNodeJS { - inst.Spec.NodeJS.Image = u.DefaultAutoInstNodeJS - inst.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationNodeJS] = u.DefaultAutoInstNodeJS - } - } else { - u.Logger.Error(nil, "support for NodeJS auto instrumentation is not enabled") - u.Recorder.Event(inst.DeepCopy(), "Warning", "InstrumentationUpgradeRejected", "support for NodeJS auto instrumentation is not enabled") - } - } - autoInstPython := inst.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationPython] - if autoInstPython != "" { - if featuregate.EnablePythonAutoInstrumentationSupport.IsEnabled() { - // upgrade the image only if the image matches the annotation - if inst.Spec.Python.Image == autoInstPython { - inst.Spec.Python.Image = u.DefaultAutoInstPython - inst.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationPython] = u.DefaultAutoInstPython - } - } else { - u.Logger.Error(nil, "support for Python auto instrumentation is not enabled") - u.Recorder.Event(inst.DeepCopy(), "Warning", "InstrumentationUpgradeRejected", "support for Python auto instrumentation is not enabled") - } - } - autoInstDotnet := inst.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationDotNet] - if autoInstDotnet != "" { - if featuregate.EnableDotnetAutoInstrumentationSupport.IsEnabled() { - // upgrade the image only if the image matches the annotation - if inst.Spec.DotNet.Image == autoInstDotnet { - inst.Spec.DotNet.Image = u.DefaultAutoInstDotNet - inst.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationDotNet] = u.DefaultAutoInstDotNet - } - } else { - u.Logger.Error(nil, "support for .NET auto instrumentation is not enabled") - u.Recorder.Event(inst.DeepCopy(), "Warning", "InstrumentationUpgradeRejected", "support for .NET auto instrumentation is not enabled") - } - } - autoInstGo := inst.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationGo] - if autoInstGo != "" { - if featuregate.EnableGoAutoInstrumentationSupport.IsEnabled() { - // upgrade the image only if the image matches the annotation - if inst.Spec.Go.Image == autoInstGo { - inst.Spec.Go.Image = u.DefaultAutoInstGo - inst.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationGo] = u.DefaultAutoInstGo - } - } else { - u.Logger.Error(nil, "support for Go auto instrumentation is not enabled") - u.Recorder.Event(inst.DeepCopy(), "Warning", "InstrumentationUpgradeRejected", "support for Go auto instrumentation is not enabled") - } - } - autoInstApacheHttpd := inst.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationApacheHttpd] - if autoInstApacheHttpd != "" { - if featuregate.EnableApacheHTTPAutoInstrumentationSupport.IsEnabled() { - // upgrade the image only if the image matches the annotation - if inst.Spec.ApacheHttpd.Image == autoInstApacheHttpd { - inst.Spec.ApacheHttpd.Image = u.DefaultAutoInstApacheHttpd - inst.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationApacheHttpd] = u.DefaultAutoInstApacheHttpd - } - } else { - u.Logger.Error(nil, "support for Apache HTTPD auto instrumentation is not enabled") - u.Recorder.Event(inst.DeepCopy(), "Warning", "InstrumentationUpgradeRejected", "support for Apache HTTPD auto instrumentation is not enabled") - } - } - autoInstNginx := inst.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationNginx] - if autoInstNginx != "" { - if featuregate.EnableNginxAutoInstrumentationSupport.IsEnabled() { - // upgrade the image only if the image matches the annotation - if inst.Spec.Nginx.Image == autoInstNginx { - inst.Spec.Nginx.Image = u.DefaultAutoInstNginx - inst.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationNginx] = u.DefaultAutoInstNginx +func (u *InstrumentationUpgrade) upgrade(_ context.Context, inst v1alpha1.Instrumentation) *v1alpha1.Instrumentation { + upgraded := inst.DeepCopy() + for annotation, gate := range defaultAnnotationToGate { + autoInst := upgraded.Annotations[annotation] + if autoInst != "" { + if gate.IsEnabled() { + switch annotation { + case constants.AnnotationDefaultAutoInstrumentationJava: + if inst.Spec.Java.Image == autoInst { + upgraded.Spec.Java.Image = u.DefaultAutoInstJava + upgraded.Annotations[annotation] = u.DefaultAutoInstJava + } + case constants.AnnotationDefaultAutoInstrumentationNodeJS: + if inst.Spec.NodeJS.Image == autoInst { + upgraded.Spec.NodeJS.Image = u.DefaultAutoInstNodeJS + upgraded.Annotations[annotation] = u.DefaultAutoInstNodeJS + } + case constants.AnnotationDefaultAutoInstrumentationPython: + if inst.Spec.Python.Image == autoInst { + upgraded.Spec.Python.Image = u.DefaultAutoInstPython + upgraded.Annotations[annotation] = u.DefaultAutoInstPython + } + case constants.AnnotationDefaultAutoInstrumentationDotNet: + if inst.Spec.DotNet.Image == autoInst { + upgraded.Spec.DotNet.Image = u.DefaultAutoInstDotNet + upgraded.Annotations[annotation] = u.DefaultAutoInstDotNet + } + case constants.AnnotationDefaultAutoInstrumentationGo: + if inst.Spec.Go.Image == autoInst { + upgraded.Spec.Go.Image = u.DefaultAutoInstGo + upgraded.Annotations[annotation] = u.DefaultAutoInstGo + } + case constants.AnnotationDefaultAutoInstrumentationApacheHttpd: + if inst.Spec.ApacheHttpd.Image == autoInst { + upgraded.Spec.ApacheHttpd.Image = u.DefaultAutoInstApacheHttpd + upgraded.Annotations[annotation] = u.DefaultAutoInstApacheHttpd + } + case constants.AnnotationDefaultAutoInstrumentationNginx: + if inst.Spec.Nginx.Image == autoInst { + upgraded.Spec.Nginx.Image = u.DefaultAutoInstNginx + upgraded.Annotations[annotation] = u.DefaultAutoInstNginx + } + } + } else { + u.Logger.Error(nil, "autoinstrumentation not enabled for this language", "flag", gate.ID()) + u.Recorder.Event(upgraded, "Warning", "InstrumentationUpgradeRejected", fmt.Sprintf("support for is not enabled for %s", gate.ID())) } - } else { - u.Logger.Error(nil, "support for Nginx auto instrumentation is not enabled") - u.Recorder.Event(inst.DeepCopy(), "Warning", "InstrumentationUpgradeRejected", "support for Nginx auto instrumentation is not enabled") } } - return inst + return upgraded } diff --git a/pkg/instrumentation/upgrade/upgrade_test.go b/pkg/instrumentation/upgrade/upgrade_test.go index f8d989810b..e4f6448e45 100644 --- a/pkg/instrumentation/upgrade/upgrade_test.go +++ b/pkg/instrumentation/upgrade/upgrade_test.go @@ -28,6 +28,8 @@ import ( "k8s.io/apimachinery/pkg/types" "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" + "github.com/open-telemetry/opentelemetry-operator/internal/config" + "github.com/open-telemetry/opentelemetry-operator/pkg/constants" "github.com/open-telemetry/opentelemetry-operator/pkg/featuregate" ) @@ -62,15 +64,6 @@ func TestUpgrade(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "my-inst", Namespace: nsName, - Annotations: map[string]string{ - v1alpha1.AnnotationDefaultAutoInstrumentationJava: "java:1", - v1alpha1.AnnotationDefaultAutoInstrumentationNodeJS: "nodejs:1", - v1alpha1.AnnotationDefaultAutoInstrumentationPython: "python:1", - v1alpha1.AnnotationDefaultAutoInstrumentationDotNet: "dotnet:1", - v1alpha1.AnnotationDefaultAutoInstrumentationGo: "go:1", - v1alpha1.AnnotationDefaultAutoInstrumentationApacheHttpd: "apache-httpd:1", - v1alpha1.AnnotationDefaultAutoInstrumentationNginx: "nginx:1", - }, }, Spec: v1alpha1.InstrumentationSpec{ Sampler: v1alpha1.Sampler{ @@ -78,7 +71,20 @@ func TestUpgrade(t *testing.T) { }, }, } - inst.Default() + err = v1alpha1.NewInstrumentationWebhook( + logr.Discard(), + testScheme, + config.New( + config.WithAutoInstrumentationJavaImage("java:1"), + config.WithAutoInstrumentationNodeJSImage("nodejs:1"), + config.WithAutoInstrumentationPythonImage("python:1"), + config.WithAutoInstrumentationDotNetImage("dotnet:1"), + config.WithAutoInstrumentationGoImage("go:1"), + config.WithAutoInstrumentationApacheHttpdImage("apache-httpd:1"), + config.WithAutoInstrumentationNginxImage("nginx:1"), + ), + ).Default(context.Background(), inst) + assert.Nil(t, err) assert.Equal(t, "java:1", inst.Spec.Java.Image) assert.Equal(t, "nodejs:1", inst.Spec.NodeJS.Image) assert.Equal(t, "python:1", inst.Spec.Python.Image) @@ -109,18 +115,18 @@ func TestUpgrade(t *testing.T) { Name: "my-inst", }, &updated) require.NoError(t, err) - assert.Equal(t, "java:2", updated.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationJava]) + assert.Equal(t, "java:2", updated.Annotations[constants.AnnotationDefaultAutoInstrumentationJava]) assert.Equal(t, "java:2", updated.Spec.Java.Image) - assert.Equal(t, "nodejs:2", updated.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationNodeJS]) + assert.Equal(t, "nodejs:2", updated.Annotations[constants.AnnotationDefaultAutoInstrumentationNodeJS]) assert.Equal(t, "nodejs:2", updated.Spec.NodeJS.Image) - assert.Equal(t, "python:2", updated.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationPython]) + assert.Equal(t, "python:2", updated.Annotations[constants.AnnotationDefaultAutoInstrumentationPython]) assert.Equal(t, "python:2", updated.Spec.Python.Image) - assert.Equal(t, "dotnet:2", updated.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationDotNet]) + assert.Equal(t, "dotnet:2", updated.Annotations[constants.AnnotationDefaultAutoInstrumentationDotNet]) assert.Equal(t, "dotnet:2", updated.Spec.DotNet.Image) - assert.Equal(t, "go:2", updated.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationGo]) + assert.Equal(t, "go:2", updated.Annotations[constants.AnnotationDefaultAutoInstrumentationGo]) assert.Equal(t, "go:2", updated.Spec.Go.Image) - assert.Equal(t, "apache-httpd:2", updated.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationApacheHttpd]) + assert.Equal(t, "apache-httpd:2", updated.Annotations[constants.AnnotationDefaultAutoInstrumentationApacheHttpd]) assert.Equal(t, "apache-httpd:2", updated.Spec.ApacheHttpd.Image) - assert.Equal(t, "nginx:2", updated.Annotations[v1alpha1.AnnotationDefaultAutoInstrumentationNginx]) + assert.Equal(t, "nginx:2", updated.Annotations[constants.AnnotationDefaultAutoInstrumentationNginx]) assert.Equal(t, "nginx:2", updated.Spec.Nginx.Image) } diff --git a/pkg/sidecar/podmutator.go b/pkg/sidecar/podmutator.go index 7f1956e4dd..a7a0737b53 100644 --- a/pkg/sidecar/podmutator.go +++ b/pkg/sidecar/podmutator.go @@ -28,7 +28,7 @@ import ( "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" "github.com/open-telemetry/opentelemetry-operator/internal/config" - "github.com/open-telemetry/opentelemetry-operator/internal/webhookhandler" + "github.com/open-telemetry/opentelemetry-operator/internal/webhook/podmutation" ) var ( @@ -43,7 +43,7 @@ type sidecarPodMutator struct { config config.Config } -var _ webhookhandler.PodMutator = (*sidecarPodMutator)(nil) +var _ podmutation.PodMutator = (*sidecarPodMutator)(nil) func NewMutator(logger logr.Logger, config config.Config, client client.Client) *sidecarPodMutator { return &sidecarPodMutator{