From 94fdb527e012120817584f77a398419fffc3e667 Mon Sep 17 00:00:00 2001 From: Leonid Podolinskiy Date: Sun, 5 May 2024 10:34:02 +0300 Subject: [PATCH] revert handling of the space in the env var --- .github/workflows/release.yaml | 2 +- internal/controller/helpers.go | 9 +-- internal/controller/helpers_test.go | 56 +------------------ .../lightrunjavaagent_controller.go | 10 ++-- .../lightrunjavaagent_controller_test.go | 42 +++++++------- internal/controller/patch_funcs.go | 40 ++++++------- 6 files changed, 48 insertions(+), 111 deletions(-) diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index 63fdbb5..18609fd 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -52,7 +52,7 @@ jobs: make test - name: Install Helm - uses: azure/setup-helm@v1 + uses: azure/setup-helm@v4 - name: Pack Helm chart shell: bash diff --git a/internal/controller/helpers.go b/internal/controller/helpers.go index ca8a281..c3645e2 100644 --- a/internal/controller/helpers.go +++ b/internal/controller/helpers.go @@ -5,7 +5,6 @@ import ( "errors" "hash/fnv" "sort" - "strings" "time" agentv1beta "github.com/lightrun-platform/lightrun-k8s-operator/api/v1beta" @@ -230,7 +229,7 @@ func SetStatusCondition(conditions *[]metav1.Condition, newCondition metav1.Cond } func agentEnvVarArgument(mountPath string, agentCliFlags string) (string, error) { - agentArg := " -agentpath:" + mountPath + "/agent/lightrun_agent.so" + agentArg := "-agentpath:" + mountPath + "/agent/lightrun_agent.so" if agentCliFlags != "" { agentArg += "=" + agentCliFlags if len(agentArg) > 1024 { @@ -240,12 +239,6 @@ func agentEnvVarArgument(mountPath string, agentCliFlags string) (string, error) return agentArg, nil } -// Removes from env var value. Removes env var from the list if value is empty after the update -func unpatchEnvVarValue(origValue string, removalValue string) string { - value := strings.ReplaceAll(origValue, removalValue, "") - return value -} - // Return index if the env var in the []corev1.EnvVar, otherwise -1 func findEnvVarIndex(envVarName string, envVarList []corev1.EnvVar) int { for i, envVar := range envVarList { diff --git a/internal/controller/helpers_test.go b/internal/controller/helpers_test.go index aa9ae8e..58946d2 100644 --- a/internal/controller/helpers_test.go +++ b/internal/controller/helpers_test.go @@ -53,58 +53,6 @@ func Test_findEnvVarIndex(t *testing.T) { } } -func Test_unpatchEnvVarValue(t *testing.T) { - type args struct { - origValue string - removalValue string - } - tests := []struct { - name string - args args - want string - }{ - { - name: "correctly removes the value from the env var", - args: args{ - origValue: "test", - removalValue: "test", - }, - want: "", - }, - { - name: "not found substring", - args: args{ - origValue: "test", - removalValue: "test1", - }, - want: "test", - }, - { - name: "with space", - args: args{ - origValue: "test this string", - removalValue: " this", - }, - want: "test string", - }, - { - name: "unpatch empty value", - args: args{ - origValue: "test this string", - removalValue: "", - }, - want: "test this string", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := unpatchEnvVarValue(tt.args.origValue, tt.args.removalValue); got != tt.want { - t.Errorf("unpatchEnvVarValue() = %v, want %v", got, tt.want) - } - }) - } -} - func Test_agentEnvVarArgument(t *testing.T) { type args struct { mountPath string @@ -122,7 +70,7 @@ func Test_agentEnvVarArgument(t *testing.T) { mountPath: "test", agentCliFlags: "test", }, - want: " -agentpath:test/agent/lightrun_agent.so=test", + want: "-agentpath:test/agent/lightrun_agent.so=test", wantErr: false, }, { @@ -131,7 +79,7 @@ func Test_agentEnvVarArgument(t *testing.T) { mountPath: "test", agentCliFlags: "", }, - want: " -agentpath:test/agent/lightrun_agent.so", + want: "-agentpath:test/agent/lightrun_agent.so", wantErr: false, }, { diff --git a/internal/controller/lightrunjavaagent_controller.go b/internal/controller/lightrunjavaagent_controller.go index 476f667..e0c0b0b 100644 --- a/internal/controller/lightrunjavaagent_controller.go +++ b/internal/controller/lightrunjavaagent_controller.go @@ -88,7 +88,7 @@ func (r *LightrunJavaAgentReconciler) Reconcile(ctx context.Context, req ctrl.Re } } - if oldLrjaName, ok := originalDeployment.Annotations["lightrun.com/lightrunjavaagent"]; ok && oldLrjaName != lightrunJavaAgent.Name { + if oldLrjaName, ok := originalDeployment.Annotations[annotationAgentName]; ok && oldLrjaName != lightrunJavaAgent.Name { log.Error(err, "Deployment already patched by LightrunJavaAgent", "Existing LightrunJavaAgent", oldLrjaName) return r.errorStatus(ctx, lightrunJavaAgent, errors.New("deployment already patched")) } @@ -143,8 +143,8 @@ func (r *LightrunJavaAgentReconciler) Reconcile(ctx context.Context, req ctrl.Re } } - delete(originalDeployment.Annotations, "lightrun.com/patched-env-name") - delete(originalDeployment.Annotations, "lightrun.com/patched-env-value") + delete(originalDeployment.Annotations, annotationPatchedEnvName) + delete(originalDeployment.Annotations, annotationPatchedEnvValue) err = r.Patch(ctx, originalDeployment, clientSidePatch) if err != nil { log.Error(err, "unable to unpatch "+lightrunJavaAgent.Spec.AgentEnvVarName) @@ -276,8 +276,8 @@ func (r *LightrunJavaAgentReconciler) Reconcile(ctx context.Context, req ctrl.Re } } } - originalDeployment.Annotations["lightrun.com/patched-env-name"] = lightrunJavaAgent.Spec.AgentEnvVarName - originalDeployment.Annotations["lightrun.com/patched-env-value"] = agentArg + originalDeployment.Annotations[annotationPatchedEnvName] = lightrunJavaAgent.Spec.AgentEnvVarName + originalDeployment.Annotations[annotationPatchedEnvValue] = agentArg err = r.Patch(ctx, originalDeployment, clientSidePatch) if err != nil { log.Error(err, "failed to patch "+lightrunJavaAgent.Spec.AgentEnvVarName) diff --git a/internal/controller/lightrunjavaagent_controller_test.go b/internal/controller/lightrunjavaagent_controller_test.go index 6030cd4..46aaa83 100644 --- a/internal/controller/lightrunjavaagent_controller_test.go +++ b/internal/controller/lightrunjavaagent_controller_test.go @@ -32,7 +32,7 @@ var _ = Describe("LightrunJavaAgent controller", func() { agentPlatform = "linux" initVolumeName = "lightrun-agent-init" javaEnv = "JAVA_TOOL_OPTIONS" - defaultAgentPath = " -agentpath:/lightrun/agent/lightrun_agent.so" + defaultAgentPath = "-agentpath:/lightrun/agent/lightrun_agent.so" agentCliFlags = "--lightrun_extra_class_path=" javaEnvNonEmptyValue = "-Djava.net.preferIPv4Stack=true" ) @@ -264,7 +264,7 @@ var _ = Describe("LightrunJavaAgent controller", func() { return false } } else if container.Name == "app2" { - if envVar.Value != javaEnvNonEmptyValue+defaultAgentPath+"="+agentCliFlags { + if envVar.Value != javaEnvNonEmptyValue+" "+defaultAgentPath+"="+agentCliFlags { return false } } @@ -344,12 +344,12 @@ var _ = Describe("LightrunJavaAgent controller", func() { Eventually(func() bool { flag := 0 for k, v := range patchedDepl.ObjectMeta.Annotations { - if k == "lightrun.com/lightrunjavaagent" && v == lragent1Name { + if k == annotationAgentName && v == lragent1Name { flag += 1 } } for k := range patchedDepl.Spec.Template.Annotations { - if k == "lightrun.com/configmap-hash" { + if k == annotationConfigMapHash { flag += 1 } } @@ -358,7 +358,7 @@ var _ = Describe("LightrunJavaAgent controller", func() { }) It("Should not change hash of the configmap in the deployment metadata", func() { Eventually(func() bool { - return patchedDepl.Spec.Template.Annotations["lightrun.com/configmap-hash"] == fmt.Sprint(hash(cm.Data["config"]+cm.Data["metadata"])) + return patchedDepl.Spec.Template.Annotations[annotationConfigMapHash] == fmt.Sprint(hash(cm.Data["config"]+cm.Data["metadata"])) }).Should(BeTrue()) }) @@ -461,12 +461,12 @@ var _ = Describe("LightrunJavaAgent controller", func() { It("Should delete annotations from deployment", func() { Eventually(func() bool { for k := range patchedDepl.ObjectMeta.Annotations { - if k == "lightrun.com/lightrunjavaagent" { + if k == annotationAgentName { return false } } for k := range patchedDepl.Spec.Template.Annotations { - if k == "lightrun.com/configmap-hash" { + if k == annotationConfigMapHash { return false } } @@ -561,7 +561,7 @@ var _ = Describe("LightrunJavaAgent controller", func() { return false } } else if container.Name == "app2" { - if envVar.Value != javaEnvNonEmptyValue+defaultAgentPath { + if envVar.Value != javaEnvNonEmptyValue+" "+defaultAgentPath { return false } } @@ -695,7 +695,7 @@ var _ = Describe("LightrunJavaAgent controller", func() { if err := k8sClient.Get(ctx, deplRequest2, &patchedDepl2); err != nil { return false } - return patchedDepl2.Annotations["lightrun.com/lightrunjavaagent"] == lrAgent2.Name + return patchedDepl2.Annotations[annotationAgentName] == lrAgent2.Name }).Should(BeTrue()) }) @@ -782,7 +782,7 @@ var _ = Describe("LightrunJavaAgent controller", func() { if err := k8sClient.Get(ctx, deplRequest3, &patchedDepl3); err != nil { return false } - if _, ok := patchedDepl3.Annotations["lightrun.com/lightrunjavaagent"]; !ok && len(patchedDepl3.Finalizers) == 0 { + if _, ok := patchedDepl3.Annotations[annotationAgentName]; !ok && len(patchedDepl3.Finalizers) == 0 { return true } return false @@ -871,7 +871,7 @@ var _ = Describe("LightrunJavaAgent controller", func() { return false } } else if container.Name == "app2" { - if envVar.Value != javaEnvNonEmptyValue+defaultAgentPath { + if envVar.Value != javaEnvNonEmptyValue+" "+defaultAgentPath { return false } } @@ -886,16 +886,16 @@ var _ = Describe("LightrunJavaAgent controller", func() { if err := k8sClient.Get(ctx, lrAgentRequest5, &lrAgent5); err != nil { return false } - if patchedDepl4.Annotations["lightrun.com/lightrunjavaagent"] != lrAgent5.Name { - // logger.Info("annotations", "lightrun.com/lightrunjavaagent", patchedDepl4.Annotations["lightrun.com/lightrunjavaagent"]) + if patchedDepl4.Annotations[annotationAgentName] != lrAgent5.Name { + // logger.Info("annotations", "annotationAgentName", patchedDepl4.Annotations["annotationAgentName"]) return false } - if patchedDepl4.Annotations["lightrun.com/patched-env-name"] != javaEnv { - // logger.Info("annotations", "lightrun.com/patched-env-name", patchedDepl4.Annotations["lightrun.com/patched-env-name"]) + if patchedDepl4.Annotations[annotationPatchedEnvName] != javaEnv { + // logger.Info("annotations", annotationPatchedEnvName, patchedDepl4.Annotations[annotationPatchedEnvName]) return false } - if patchedDepl4.Annotations["lightrun.com/patched-env-value"] != defaultAgentPath { - // logger.Info("annotations", "lightrun.com/patched-env-value", patchedDepl4.Annotations["lightrun.com/patched-env-value"]) + if patchedDepl4.Annotations[annotationPatchedEnvValue] != defaultAgentPath { + // logger.Info("annotations", annotationPatchedEnvValue, patchedDepl4.Annotations[annotationPatchedEnvValue]) return false } return true @@ -938,10 +938,10 @@ var _ = Describe("LightrunJavaAgent controller", func() { } } } - if patchedDepl4.Annotations["lightrun.com/patched-env-name"] != "NEW_ENV_NAME" { + if patchedDepl4.Annotations[annotationPatchedEnvName] != "NEW_ENV_NAME" { return false } - if patchedDepl4.Annotations["lightrun.com/patched-env-value"] != defaultAgentPath { + if patchedDepl4.Annotations[annotationPatchedEnvValue] != defaultAgentPath { return false } return true @@ -965,8 +965,8 @@ var _ = Describe("LightrunJavaAgent controller", func() { if err := k8sClient.Get(ctx, deplRequest4, &patchedDepl4); err != nil { return false } - if patchedDepl4.Annotations["lightrun.com/patched-env-value"] != defaultAgentPath+"=--new-flags" { - logger.Info("annotations", "lightrun.com/patched-env-value", patchedDepl4.Annotations["lightrun.com/patched-env-value"]) + if patchedDepl4.Annotations[annotationPatchedEnvValue] != defaultAgentPath+"=--new-flags" { + logger.Info("annotations", annotationPatchedEnvValue, patchedDepl4.Annotations[annotationPatchedEnvValue]) return false } for _, container := range patchedDepl4.Spec.Template.Spec.Containers { diff --git a/internal/controller/patch_funcs.go b/internal/controller/patch_funcs.go index a9b9e94..ccf8684 100644 --- a/internal/controller/patch_funcs.go +++ b/internal/controller/patch_funcs.go @@ -18,9 +18,13 @@ import ( ) const ( - cmNamePrefix = "lightrunagent-cm-" - cmVolumeName = "lightrunagent-config" - initContainerName = "lightrun-installer" + cmNamePrefix = "lightrunagent-cm-" + cmVolumeName = "lightrunagent-config" + initContainerName = "lightrun-installer" + annotationPatchedEnvName = "lightrun.com/patched-env-name" + annotationPatchedEnvValue = "lightrun.com/patched-env-value" + annotationConfigMapHash = "lightrun.com/configmap-hash" + annotationAgentName = "lightrun.com/lightrunjavaagent" ) func (r *LightrunJavaAgentReconciler) createAgentConfig(lightrunJavaAgent *agentv1beta.LightrunJavaAgent) (corev1.ConfigMap, error) { @@ -55,12 +59,12 @@ func (r *LightrunJavaAgentReconciler) patchDeployment(lightrunJavaAgent *agentv1 corev1ac.PodTemplateSpec().WithSpec( corev1ac.PodSpec(), ).WithAnnotations(map[string]string{ - "lightrun.com/configmap-hash": fmt.Sprint(cmDataHash), + annotationConfigMapHash: fmt.Sprint(cmDataHash), }, ), ), ).WithAnnotations(map[string]string{ - "lightrun.com/lightrunjavaagent": lightrunJavaAgent.Name, + annotationAgentName: lightrunJavaAgent.Name, }) r.addVolume(deploymentApplyConfig, lightrunJavaAgent) r.addInitContainer(deploymentApplyConfig, lightrunJavaAgent, secret) @@ -172,20 +176,12 @@ func (r *LightrunJavaAgentReconciler) patchAppContainers(lightrunJavaAgent *agen // Client side patch, as we can't update value from 2 sources func (r *LightrunJavaAgentReconciler) patchJavaToolEnv(deplAnnotations map[string]string, container *corev1.Container, targetEnvVar string, agentArg string) error { // Check if some env was already patched before - patchedEnv := deplAnnotations["lightrun.com/patched-env-name"] - patchedEnvValue := deplAnnotations["lightrun.com/patched-env-value"] + patchedEnv := deplAnnotations[annotationPatchedEnvName] + patchedEnvValue := deplAnnotations[annotationPatchedEnvValue] if patchedEnv != targetEnvVar || patchedEnvValue != agentArg { // If different env was patched before - unpatch it - patchedEnvVarIndex := findEnvVarIndex(patchedEnv, container.Env) - if patchedEnvVarIndex != -1 { - unpatchedEnvValue := unpatchEnvVarValue(container.Env[patchedEnvVarIndex].Value, patchedEnvValue) - if unpatchedEnvValue == "" { - container.Env = slices.Delete(container.Env, patchedEnvVarIndex, patchedEnvVarIndex+1) - } else { - container.Env[patchedEnvVarIndex].Value = unpatchedEnvValue - } - } + r.unpatchJavaToolEnv(deplAnnotations, container) } targetEnvVarIndex := findEnvVarIndex(targetEnvVar, container.Env) @@ -197,7 +193,7 @@ func (r *LightrunJavaAgentReconciler) patchJavaToolEnv(deplAnnotations map[strin }) } else { if !strings.Contains(container.Env[targetEnvVarIndex].Value, agentArg) { - container.Env[targetEnvVarIndex].Value = container.Env[targetEnvVarIndex].Value + agentArg + container.Env[targetEnvVarIndex].Value = container.Env[targetEnvVarIndex].Value + " " + agentArg if len(container.Env[targetEnvVarIndex].Value) > 1024 { return errors.New(targetEnvVar + " has more that 1024 chars. This is a limitation of Java") } @@ -206,21 +202,21 @@ func (r *LightrunJavaAgentReconciler) patchJavaToolEnv(deplAnnotations map[strin return nil } -func (r *LightrunJavaAgentReconciler) unpatchJavaToolEnv(deplAnnotations map[string]string, container *corev1.Container) *corev1.Container { - patchedEnv := deplAnnotations["lightrun.com/patched-env-name"] - patchedEnvValue := deplAnnotations["lightrun.com/patched-env-value"] +func (r *LightrunJavaAgentReconciler) unpatchJavaToolEnv(deplAnnotations map[string]string, container *corev1.Container) { + patchedEnv := deplAnnotations[annotationPatchedEnvName] + patchedEnvValue := deplAnnotations[annotationPatchedEnvValue] if patchedEnv == "" && patchedEnvValue == "" { - return container + return } envVarIndex := findEnvVarIndex(patchedEnv, container.Env) if envVarIndex != -1 { value := strings.ReplaceAll(container.Env[envVarIndex].Value, patchedEnvValue, "") + value = strings.TrimSpace(value) if value == "" { container.Env = slices.Delete(container.Env, envVarIndex, envVarIndex+1) } else { container.Env[envVarIndex].Value = value } } - return container }