From 6811710ba3a6fe5c526d027dc0d5a318bc45c3ce Mon Sep 17 00:00:00 2001 From: jnarezo Date: Thu, 12 Sep 2024 22:39:23 -0700 Subject: [PATCH] feat(vol): move validation to webhook --- apis/v1alpha1/instrumentation_webhook.go | 39 ++++++++++++++++++++++ pkg/instrumentation/apachehttpd.go | 4 +-- pkg/instrumentation/dotnet.go | 7 ++-- pkg/instrumentation/helper.go | 13 ++++---- pkg/instrumentation/helper_test.go | 42 ++++++++++-------------- pkg/instrumentation/javaagent.go | 7 ++-- pkg/instrumentation/nodejs.go | 7 ++-- pkg/instrumentation/python.go | 7 ++-- 8 files changed, 73 insertions(+), 53 deletions(-) diff --git a/apis/v1alpha1/instrumentation_webhook.go b/apis/v1alpha1/instrumentation_webhook.go index 004992f795..bd7eb8b2b6 100644 --- a/apis/v1alpha1/instrumentation_webhook.go +++ b/apis/v1alpha1/instrumentation_webhook.go @@ -17,6 +17,7 @@ package v1alpha1 import ( "context" "fmt" + "reflect" "strconv" "strings" @@ -236,6 +237,37 @@ func (w InstrumentationWebhook) validate(r *Instrumentation) (admission.Warnings default: return warnings, fmt.Errorf("spec.sampler.type is not valid: %s", r.Spec.Sampler.Type) } + + var err error + err = validateInstrVolume(r.Spec.ApacheHttpd.Volume, r.Spec.ApacheHttpd.VolumeSizeLimit) + if err != nil { + return warnings, fmt.Errorf("spec.apachehttpd.volume and spec.apachehttpd.volumeSizeLimit cannot both be defined: %w", err) + } + err = validateInstrVolume(r.Spec.DotNet.Volume, r.Spec.DotNet.VolumeSizeLimit) + if err != nil { + return warnings, fmt.Errorf("spec.dotnet.volume and spec.dotnet.volumeSizeLimit cannot both be defined: %w", err) + } + err = validateInstrVolume(r.Spec.Go.Volume, r.Spec.Go.VolumeSizeLimit) + if err != nil { + return warnings, fmt.Errorf("spec.go.volume and spec.go.volumeSizeLimit cannot both be defined: %w", err) + } + err = validateInstrVolume(r.Spec.Java.Volume, r.Spec.Java.VolumeSizeLimit) + if err != nil { + return warnings, fmt.Errorf("spec.java.volume and spec.java.volumeSizeLimit cannot both be defined: %w", err) + } + err = validateInstrVolume(r.Spec.Nginx.Volume, r.Spec.Nginx.VolumeSizeLimit) + if err != nil { + return warnings, fmt.Errorf("spec.nginx.volume and spec.nginx.volumeSizeLimit cannot both be defined: %w", err) + } + err = validateInstrVolume(r.Spec.NodeJS.Volume, r.Spec.NodeJS.VolumeSizeLimit) + if err != nil { + return warnings, fmt.Errorf("spec.nodejs.volume and spec.nodejs.volumeSizeLimit cannot both be defined: %w", err) + } + err = validateInstrVolume(r.Spec.Python.Volume, r.Spec.Python.VolumeSizeLimit) + if err != nil { + return warnings, fmt.Errorf("spec.python.volume and spec.python.volumeSizeLimit cannot both be defined: %w", err) + } + return warnings, nil } @@ -270,6 +302,13 @@ func validateJaegerRemoteSamplerArgument(argument string) error { return nil } +func validateInstrVolume(volume corev1.Volume, volumeSizeLimit *resource.Quantity) error { + if !reflect.ValueOf(volume).IsZero() && volumeSizeLimit != nil { + return fmt.Errorf("unable to resolve volume size") + } + return nil +} + func NewInstrumentationWebhook(logger logr.Logger, scheme *runtime.Scheme, cfg config.Config) *InstrumentationWebhook { return &InstrumentationWebhook{ logger: logger, diff --git a/pkg/instrumentation/apachehttpd.go b/pkg/instrumentation/apachehttpd.go index c545bc356d..ca2228ed8f 100644 --- a/pkg/instrumentation/apachehttpd.go +++ b/pkg/instrumentation/apachehttpd.go @@ -61,11 +61,11 @@ const ( func injectApacheHttpdagent(_ logr.Logger, apacheSpec v1alpha1.ApacheHttpd, pod corev1.Pod, index int, otlpEndpoint string, resourceMap map[string]string) corev1.Pod { + volume := instrVolume(apacheSpec.Volume, apacheAgentVolume, apacheSpec.VolumeSizeLimit) + // caller checks if there is at least one container container := &pod.Spec.Containers[index] - volume, _ := instrVolume(apacheSpec.Volume, apacheAgentVolume, apacheSpec.VolumeSizeLimit) - // inject env vars for _, env := range apacheSpec.Env { idx := getIndexOfEnv(container.Env, env.Name) diff --git a/pkg/instrumentation/dotnet.go b/pkg/instrumentation/dotnet.go index ca86ef7cfe..ec01a0b039 100644 --- a/pkg/instrumentation/dotnet.go +++ b/pkg/instrumentation/dotnet.go @@ -52,6 +52,8 @@ const ( func injectDotNetSDK(dotNetSpec v1alpha1.DotNet, pod corev1.Pod, index int, runtime string) (corev1.Pod, error) { + volume := instrVolume(dotNetSpec.Volume, dotnetVolumeName, dotNetSpec.VolumeSizeLimit) + // caller checks if there is at least one container. container := &pod.Spec.Containers[index] @@ -60,11 +62,6 @@ func injectDotNetSDK(dotNetSpec v1alpha1.DotNet, pod corev1.Pod, index int, runt return pod, err } - volume, err := instrVolume(dotNetSpec.Volume, dotnetVolumeName, dotNetSpec.VolumeSizeLimit) - if err != nil { - return pod, err - } - // check if OTEL_DOTNET_AUTO_HOME env var is already set in the container // if it is already set, then we assume that .NET Auto-instrumentation is already configured for this container if getIndexOfEnv(container.Env, envDotNetOTelAutoHome) > -1 { diff --git a/pkg/instrumentation/helper.go b/pkg/instrumentation/helper.go index 45a08a214b..19270a4ae5 100644 --- a/pkg/instrumentation/helper.go +++ b/pkg/instrumentation/helper.go @@ -124,20 +124,19 @@ func isInstrWithoutContainers(inst instrumentationWithContainers) int { return 0 } -func instrVolume(volume corev1.Volume, name string, volumeSizeLimit *resource.Quantity) (corev1.Volume, error) { - if !reflect.ValueOf(volume).IsZero() && volumeSizeLimit != nil { - return volume, fmt.Errorf("both Volume and VolumeSizeLimit cannot be defined simultaneously") - } else if !reflect.ValueOf(volume).IsZero() { - return volume, nil +// Return volume if defined, otherwise return emptyDir with given name and size limit. +func instrVolume(volume corev1.Volume, name string, quantity *resource.Quantity) corev1.Volume { + if !reflect.ValueOf(volume).IsZero() { + return volume } return corev1.Volume{ Name: name, VolumeSource: corev1.VolumeSource{ EmptyDir: &corev1.EmptyDirVolumeSource{ - SizeLimit: volumeSize(volumeSizeLimit), + SizeLimit: volumeSize(quantity), }, - }}, nil + }} } func volumeSize(quantity *resource.Quantity) *resource.Quantity { diff --git a/pkg/instrumentation/helper_test.go b/pkg/instrumentation/helper_test.go index 7aaf3e1d48..d20880b5f6 100644 --- a/pkg/instrumentation/helper_test.go +++ b/pkg/instrumentation/helper_test.go @@ -191,12 +191,11 @@ func TestDuplicatedContainers(t *testing.T) { func TestInstrVolume(t *testing.T) { tests := []struct { - name string - volume corev1.Volume - volumeName string - volumeSizeLimit *resource.Quantity - expected corev1.Volume - err error + name string + volume corev1.Volume + volumeName string + quantity *resource.Quantity + expected corev1.Volume }{ { name: "With volume", @@ -207,8 +206,8 @@ func TestInstrVolume(t *testing.T) { SizeLimit: &resource.Quantity{}, }, }}, - volumeName: "default-vol", - volumeSizeLimit: nil, + volumeName: "default-vol", + quantity: nil, expected: corev1.Volume{ Name: "vol1", VolumeSource: corev1.VolumeSource{ @@ -216,13 +215,12 @@ func TestInstrVolume(t *testing.T) { SizeLimit: &resource.Quantity{}, }, }}, - err: nil, }, { - name: "With volume size limit", - volume: corev1.Volume{}, - volumeName: "default-vol", - volumeSizeLimit: &defaultVolumeLimitSize, + name: "With volume size limit", + volume: corev1.Volume{}, + volumeName: "default-vol", + quantity: &defaultVolumeLimitSize, expected: corev1.Volume{ Name: "default-vol", VolumeSource: corev1.VolumeSource{ @@ -230,13 +228,12 @@ func TestInstrVolume(t *testing.T) { SizeLimit: &defaultVolumeLimitSize, }, }}, - err: nil, }, { - name: "No volume or size limit", - volume: corev1.Volume{}, - volumeName: "default-vol", - volumeSizeLimit: nil, + name: "No volume or size limit", + volume: corev1.Volume{}, + volumeName: "default-vol", + quantity: nil, expected: corev1.Volume{ Name: "default-vol", VolumeSource: corev1.VolumeSource{ @@ -244,7 +241,6 @@ func TestInstrVolume(t *testing.T) { SizeLimit: &defaultSize, }, }}, - err: nil, }, { name: "With volume and size limit", @@ -255,8 +251,8 @@ func TestInstrVolume(t *testing.T) { SizeLimit: &resource.Quantity{}, }, }}, - volumeName: "default-vol", - volumeSizeLimit: &defaultVolumeLimitSize, + volumeName: "default-vol", + quantity: &defaultVolumeLimitSize, expected: corev1.Volume{ Name: "vol1", VolumeSource: corev1.VolumeSource{ @@ -264,15 +260,13 @@ func TestInstrVolume(t *testing.T) { SizeLimit: &resource.Quantity{}, }, }}, - err: fmt.Errorf("both Volume and VolumeSizeLimit cannot be defined simultaneously"), }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - res, err := instrVolume(test.volume, test.volumeName, test.volumeSizeLimit) + res := instrVolume(test.volume, test.volumeName, test.quantity) assert.Equal(t, test.expected, res) - assert.Equal(t, test.err, err) }) } } diff --git a/pkg/instrumentation/javaagent.go b/pkg/instrumentation/javaagent.go index f5b65d8455..903238a93b 100644 --- a/pkg/instrumentation/javaagent.go +++ b/pkg/instrumentation/javaagent.go @@ -31,6 +31,8 @@ const ( ) func injectJavaagent(javaSpec v1alpha1.Java, pod corev1.Pod, index int) (corev1.Pod, error) { + volume := instrVolume(javaSpec.Volume, javaVolumeName, javaSpec.VolumeSizeLimit) + // caller checks if there is at least one container. container := &pod.Spec.Containers[index] @@ -39,11 +41,6 @@ func injectJavaagent(javaSpec v1alpha1.Java, pod corev1.Pod, index int) (corev1. return pod, err } - volume, err := instrVolume(javaSpec.Volume, javaVolumeName, javaSpec.VolumeSizeLimit) - if err != nil { - return pod, err - } - // inject Java instrumentation spec env vars. for _, env := range javaSpec.Env { idx := getIndexOfEnv(container.Env, env.Name) diff --git a/pkg/instrumentation/nodejs.go b/pkg/instrumentation/nodejs.go index 8476653e92..5039f6490d 100644 --- a/pkg/instrumentation/nodejs.go +++ b/pkg/instrumentation/nodejs.go @@ -29,6 +29,8 @@ const ( ) func injectNodeJSSDK(nodeJSSpec v1alpha1.NodeJS, pod corev1.Pod, index int) (corev1.Pod, error) { + volume := instrVolume(nodeJSSpec.Volume, nodejsVolumeName, nodeJSSpec.VolumeSizeLimit) + // caller checks if there is at least one container. container := &pod.Spec.Containers[index] @@ -37,11 +39,6 @@ func injectNodeJSSDK(nodeJSSpec v1alpha1.NodeJS, pod corev1.Pod, index int) (cor return pod, err } - volume, err := instrVolume(nodeJSSpec.Volume, nodejsVolumeName, nodeJSSpec.VolumeSizeLimit) - if err != nil { - return pod, err - } - // inject NodeJS instrumentation spec env vars. for _, env := range nodeJSSpec.Env { idx := getIndexOfEnv(container.Env, env.Name) diff --git a/pkg/instrumentation/python.go b/pkg/instrumentation/python.go index 72bb91388d..0e2ec2c17f 100644 --- a/pkg/instrumentation/python.go +++ b/pkg/instrumentation/python.go @@ -35,6 +35,8 @@ const ( ) func injectPythonSDK(pythonSpec v1alpha1.Python, pod corev1.Pod, index int) (corev1.Pod, error) { + volume := instrVolume(pythonSpec.Volume, pythonVolumeName, pythonSpec.VolumeSizeLimit) + // caller checks if there is at least one container. container := &pod.Spec.Containers[index] @@ -43,11 +45,6 @@ func injectPythonSDK(pythonSpec v1alpha1.Python, pod corev1.Pod, index int) (cor return pod, err } - volume, err := instrVolume(pythonSpec.Volume, pythonVolumeName, pythonSpec.VolumeSizeLimit) - if err != nil { - return pod, err - } - // inject Python instrumentation spec env vars. for _, env := range pythonSpec.Env { idx := getIndexOfEnv(container.Env, env.Name)