revert handling of the space in the env var (#24)

lightrun-platform · May 5, 2024 · 6ba0921 · foxweba3 · Aug 19, 2024 · 6ba0921
1 parent 04d4b21
commit 6ba0921
Show file tree

Hide file tree

Showing 6 changed files with 48 additions and 111 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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=<PATH_TO_JAR>"
 		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
@@ -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
 }