Skip to content

Commit 6ba0921

Browse files
author
Leonid Podolinskiy
authored
revert handling of the space in the env var (#24)
1 parent 04d4b21 commit 6ba0921

File tree

6 files changed

+48
-111
lines changed

6 files changed

+48
-111
lines changed

‎.github/workflows/release.yaml

+1-1
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ jobs:
5252
make test
5353
5454
- name: Install Helm
55-
uses: azure/setup-helm@v1
55+
uses: azure/setup-helm@v4
5656

5757
- name: Pack Helm chart
5858
shell: bash

‎internal/controller/helpers.go

+1-8
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"errors"
66
"hash/fnv"
77
"sort"
8-
"strings"
98
"time"
109

1110
agentv1beta "github.com/lightrun-platform/lightrun-k8s-operator/api/v1beta"
@@ -230,7 +229,7 @@ func SetStatusCondition(conditions *[]metav1.Condition, newCondition metav1.Cond
230229
}
231230

232231
func agentEnvVarArgument(mountPath string, agentCliFlags string) (string, error) {
233-
agentArg := " -agentpath:" + mountPath + "/agent/lightrun_agent.so"
232+
agentArg := "-agentpath:" + mountPath + "/agent/lightrun_agent.so"
234233
if agentCliFlags != "" {
235234
agentArg += "=" + agentCliFlags
236235
if len(agentArg) > 1024 {
@@ -240,12 +239,6 @@ func agentEnvVarArgument(mountPath string, agentCliFlags string) (string, error)
240239
return agentArg, nil
241240
}
242241

243-
// Removes from env var value. Removes env var from the list if value is empty after the update
244-
func unpatchEnvVarValue(origValue string, removalValue string) string {
245-
value := strings.ReplaceAll(origValue, removalValue, "")
246-
return value
247-
}
248-
249242
// Return index if the env var in the []corev1.EnvVar, otherwise -1
250243
func findEnvVarIndex(envVarName string, envVarList []corev1.EnvVar) int {
251244
for i, envVar := range envVarList {

‎internal/controller/helpers_test.go

+2-54
Original file line numberDiff line numberDiff line change
@@ -53,58 +53,6 @@ func Test_findEnvVarIndex(t *testing.T) {
5353
}
5454
}
5555

56-
func Test_unpatchEnvVarValue(t *testing.T) {
57-
type args struct {
58-
origValue string
59-
removalValue string
60-
}
61-
tests := []struct {
62-
name string
63-
args args
64-
want string
65-
}{
66-
{
67-
name: "correctly removes the value from the env var",
68-
args: args{
69-
origValue: "test",
70-
removalValue: "test",
71-
},
72-
want: "",
73-
},
74-
{
75-
name: "not found substring",
76-
args: args{
77-
origValue: "test",
78-
removalValue: "test1",
79-
},
80-
want: "test",
81-
},
82-
{
83-
name: "with space",
84-
args: args{
85-
origValue: "test this string",
86-
removalValue: " this",
87-
},
88-
want: "test string",
89-
},
90-
{
91-
name: "unpatch empty value",
92-
args: args{
93-
origValue: "test this string",
94-
removalValue: "",
95-
},
96-
want: "test this string",
97-
},
98-
}
99-
for _, tt := range tests {
100-
t.Run(tt.name, func(t *testing.T) {
101-
if got := unpatchEnvVarValue(tt.args.origValue, tt.args.removalValue); got != tt.want {
102-
t.Errorf("unpatchEnvVarValue() = %v, want %v", got, tt.want)
103-
}
104-
})
105-
}
106-
}
107-
10856
func Test_agentEnvVarArgument(t *testing.T) {
10957
type args struct {
11058
mountPath string
@@ -122,7 +70,7 @@ func Test_agentEnvVarArgument(t *testing.T) {
12270
mountPath: "test",
12371
agentCliFlags: "test",
12472
},
125-
want: " -agentpath:test/agent/lightrun_agent.so=test",
73+
want: "-agentpath:test/agent/lightrun_agent.so=test",
12674
wantErr: false,
12775
},
12876
{
@@ -131,7 +79,7 @@ func Test_agentEnvVarArgument(t *testing.T) {
13179
mountPath: "test",
13280
agentCliFlags: "",
13381
},
134-
want: " -agentpath:test/agent/lightrun_agent.so",
82+
want: "-agentpath:test/agent/lightrun_agent.so",
13583
wantErr: false,
13684
},
13785
{

‎internal/controller/lightrunjavaagent_controller.go

+5-5
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ func (r *LightrunJavaAgentReconciler) Reconcile(ctx context.Context, req ctrl.Re
8888
}
8989
}
9090

91-
if oldLrjaName, ok := originalDeployment.Annotations["lightrun.com/lightrunjavaagent"]; ok && oldLrjaName != lightrunJavaAgent.Name {
91+
if oldLrjaName, ok := originalDeployment.Annotations[annotationAgentName]; ok && oldLrjaName != lightrunJavaAgent.Name {
9292
log.Error(err, "Deployment already patched by LightrunJavaAgent", "Existing LightrunJavaAgent", oldLrjaName)
9393
return r.errorStatus(ctx, lightrunJavaAgent, errors.New("deployment already patched"))
9494
}
@@ -143,8 +143,8 @@ func (r *LightrunJavaAgentReconciler) Reconcile(ctx context.Context, req ctrl.Re
143143
}
144144

145145
}
146-
delete(originalDeployment.Annotations, "lightrun.com/patched-env-name")
147-
delete(originalDeployment.Annotations, "lightrun.com/patched-env-value")
146+
delete(originalDeployment.Annotations, annotationPatchedEnvName)
147+
delete(originalDeployment.Annotations, annotationPatchedEnvValue)
148148
err = r.Patch(ctx, originalDeployment, clientSidePatch)
149149
if err != nil {
150150
log.Error(err, "unable to unpatch "+lightrunJavaAgent.Spec.AgentEnvVarName)
@@ -276,8 +276,8 @@ func (r *LightrunJavaAgentReconciler) Reconcile(ctx context.Context, req ctrl.Re
276276
}
277277
}
278278
}
279-
originalDeployment.Annotations["lightrun.com/patched-env-name"] = lightrunJavaAgent.Spec.AgentEnvVarName
280-
originalDeployment.Annotations["lightrun.com/patched-env-value"] = agentArg
279+
originalDeployment.Annotations[annotationPatchedEnvName] = lightrunJavaAgent.Spec.AgentEnvVarName
280+
originalDeployment.Annotations[annotationPatchedEnvValue] = agentArg
281281
err = r.Patch(ctx, originalDeployment, clientSidePatch)
282282
if err != nil {
283283
log.Error(err, "failed to patch "+lightrunJavaAgent.Spec.AgentEnvVarName)

‎internal/controller/lightrunjavaagent_controller_test.go

+21-21
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ var _ = Describe("LightrunJavaAgent controller", func() {
3232
agentPlatform = "linux"
3333
initVolumeName = "lightrun-agent-init"
3434
javaEnv = "JAVA_TOOL_OPTIONS"
35-
defaultAgentPath = " -agentpath:/lightrun/agent/lightrun_agent.so"
35+
defaultAgentPath = "-agentpath:/lightrun/agent/lightrun_agent.so"
3636
agentCliFlags = "--lightrun_extra_class_path=<PATH_TO_JAR>"
3737
javaEnvNonEmptyValue = "-Djava.net.preferIPv4Stack=true"
3838
)
@@ -264,7 +264,7 @@ var _ = Describe("LightrunJavaAgent controller", func() {
264264
return false
265265
}
266266
} else if container.Name == "app2" {
267-
if envVar.Value != javaEnvNonEmptyValue+defaultAgentPath+"="+agentCliFlags {
267+
if envVar.Value != javaEnvNonEmptyValue+" "+defaultAgentPath+"="+agentCliFlags {
268268
return false
269269
}
270270
}
@@ -344,12 +344,12 @@ var _ = Describe("LightrunJavaAgent controller", func() {
344344
Eventually(func() bool {
345345
flag := 0
346346
for k, v := range patchedDepl.ObjectMeta.Annotations {
347-
if k == "lightrun.com/lightrunjavaagent" && v == lragent1Name {
347+
if k == annotationAgentName && v == lragent1Name {
348348
flag += 1
349349
}
350350
}
351351
for k := range patchedDepl.Spec.Template.Annotations {
352-
if k == "lightrun.com/configmap-hash" {
352+
if k == annotationConfigMapHash {
353353
flag += 1
354354
}
355355
}
@@ -358,7 +358,7 @@ var _ = Describe("LightrunJavaAgent controller", func() {
358358
})
359359
It("Should not change hash of the configmap in the deployment metadata", func() {
360360
Eventually(func() bool {
361-
return patchedDepl.Spec.Template.Annotations["lightrun.com/configmap-hash"] == fmt.Sprint(hash(cm.Data["config"]+cm.Data["metadata"]))
361+
return patchedDepl.Spec.Template.Annotations[annotationConfigMapHash] == fmt.Sprint(hash(cm.Data["config"]+cm.Data["metadata"]))
362362
}).Should(BeTrue())
363363
})
364364

@@ -461,12 +461,12 @@ var _ = Describe("LightrunJavaAgent controller", func() {
461461
It("Should delete annotations from deployment", func() {
462462
Eventually(func() bool {
463463
for k := range patchedDepl.ObjectMeta.Annotations {
464-
if k == "lightrun.com/lightrunjavaagent" {
464+
if k == annotationAgentName {
465465
return false
466466
}
467467
}
468468
for k := range patchedDepl.Spec.Template.Annotations {
469-
if k == "lightrun.com/configmap-hash" {
469+
if k == annotationConfigMapHash {
470470
return false
471471
}
472472
}
@@ -561,7 +561,7 @@ var _ = Describe("LightrunJavaAgent controller", func() {
561561
return false
562562
}
563563
} else if container.Name == "app2" {
564-
if envVar.Value != javaEnvNonEmptyValue+defaultAgentPath {
564+
if envVar.Value != javaEnvNonEmptyValue+" "+defaultAgentPath {
565565
return false
566566
}
567567
}
@@ -695,7 +695,7 @@ var _ = Describe("LightrunJavaAgent controller", func() {
695695
if err := k8sClient.Get(ctx, deplRequest2, &patchedDepl2); err != nil {
696696
return false
697697
}
698-
return patchedDepl2.Annotations["lightrun.com/lightrunjavaagent"] == lrAgent2.Name
698+
return patchedDepl2.Annotations[annotationAgentName] == lrAgent2.Name
699699
}).Should(BeTrue())
700700
})
701701

@@ -782,7 +782,7 @@ var _ = Describe("LightrunJavaAgent controller", func() {
782782
if err := k8sClient.Get(ctx, deplRequest3, &patchedDepl3); err != nil {
783783
return false
784784
}
785-
if _, ok := patchedDepl3.Annotations["lightrun.com/lightrunjavaagent"]; !ok && len(patchedDepl3.Finalizers) == 0 {
785+
if _, ok := patchedDepl3.Annotations[annotationAgentName]; !ok && len(patchedDepl3.Finalizers) == 0 {
786786
return true
787787
}
788788
return false
@@ -871,7 +871,7 @@ var _ = Describe("LightrunJavaAgent controller", func() {
871871
return false
872872
}
873873
} else if container.Name == "app2" {
874-
if envVar.Value != javaEnvNonEmptyValue+defaultAgentPath {
874+
if envVar.Value != javaEnvNonEmptyValue+" "+defaultAgentPath {
875875
return false
876876
}
877877
}
@@ -886,16 +886,16 @@ var _ = Describe("LightrunJavaAgent controller", func() {
886886
if err := k8sClient.Get(ctx, lrAgentRequest5, &lrAgent5); err != nil {
887887
return false
888888
}
889-
if patchedDepl4.Annotations["lightrun.com/lightrunjavaagent"] != lrAgent5.Name {
890-
// logger.Info("annotations", "lightrun.com/lightrunjavaagent", patchedDepl4.Annotations["lightrun.com/lightrunjavaagent"])
889+
if patchedDepl4.Annotations[annotationAgentName] != lrAgent5.Name {
890+
// logger.Info("annotations", "annotationAgentName", patchedDepl4.Annotations["annotationAgentName"])
891891
return false
892892
}
893-
if patchedDepl4.Annotations["lightrun.com/patched-env-name"] != javaEnv {
894-
// logger.Info("annotations", "lightrun.com/patched-env-name", patchedDepl4.Annotations["lightrun.com/patched-env-name"])
893+
if patchedDepl4.Annotations[annotationPatchedEnvName] != javaEnv {
894+
// logger.Info("annotations", annotationPatchedEnvName, patchedDepl4.Annotations[annotationPatchedEnvName])
895895
return false
896896
}
897-
if patchedDepl4.Annotations["lightrun.com/patched-env-value"] != defaultAgentPath {
898-
// logger.Info("annotations", "lightrun.com/patched-env-value", patchedDepl4.Annotations["lightrun.com/patched-env-value"])
897+
if patchedDepl4.Annotations[annotationPatchedEnvValue] != defaultAgentPath {
898+
// logger.Info("annotations", annotationPatchedEnvValue, patchedDepl4.Annotations[annotationPatchedEnvValue])
899899
return false
900900
}
901901
return true
@@ -938,10 +938,10 @@ var _ = Describe("LightrunJavaAgent controller", func() {
938938
}
939939
}
940940
}
941-
if patchedDepl4.Annotations["lightrun.com/patched-env-name"] != "NEW_ENV_NAME" {
941+
if patchedDepl4.Annotations[annotationPatchedEnvName] != "NEW_ENV_NAME" {
942942
return false
943943
}
944-
if patchedDepl4.Annotations["lightrun.com/patched-env-value"] != defaultAgentPath {
944+
if patchedDepl4.Annotations[annotationPatchedEnvValue] != defaultAgentPath {
945945
return false
946946
}
947947
return true
@@ -965,8 +965,8 @@ var _ = Describe("LightrunJavaAgent controller", func() {
965965
if err := k8sClient.Get(ctx, deplRequest4, &patchedDepl4); err != nil {
966966
return false
967967
}
968-
if patchedDepl4.Annotations["lightrun.com/patched-env-value"] != defaultAgentPath+"=--new-flags" {
969-
logger.Info("annotations", "lightrun.com/patched-env-value", patchedDepl4.Annotations["lightrun.com/patched-env-value"])
968+
if patchedDepl4.Annotations[annotationPatchedEnvValue] != defaultAgentPath+"=--new-flags" {
969+
logger.Info("annotations", annotationPatchedEnvValue, patchedDepl4.Annotations[annotationPatchedEnvValue])
970970
return false
971971
}
972972
for _, container := range patchedDepl4.Spec.Template.Spec.Containers {

‎internal/controller/patch_funcs.go

+18-22
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,13 @@ import (
1818
)
1919

2020
const (
21-
cmNamePrefix = "lightrunagent-cm-"
22-
cmVolumeName = "lightrunagent-config"
23-
initContainerName = "lightrun-installer"
21+
cmNamePrefix = "lightrunagent-cm-"
22+
cmVolumeName = "lightrunagent-config"
23+
initContainerName = "lightrun-installer"
24+
annotationPatchedEnvName = "lightrun.com/patched-env-name"
25+
annotationPatchedEnvValue = "lightrun.com/patched-env-value"
26+
annotationConfigMapHash = "lightrun.com/configmap-hash"
27+
annotationAgentName = "lightrun.com/lightrunjavaagent"
2428
)
2529

2630
func (r *LightrunJavaAgentReconciler) createAgentConfig(lightrunJavaAgent *agentv1beta.LightrunJavaAgent) (corev1.ConfigMap, error) {
@@ -55,12 +59,12 @@ func (r *LightrunJavaAgentReconciler) patchDeployment(lightrunJavaAgent *agentv1
5559
corev1ac.PodTemplateSpec().WithSpec(
5660
corev1ac.PodSpec(),
5761
).WithAnnotations(map[string]string{
58-
"lightrun.com/configmap-hash": fmt.Sprint(cmDataHash),
62+
annotationConfigMapHash: fmt.Sprint(cmDataHash),
5963
},
6064
),
6165
),
6266
).WithAnnotations(map[string]string{
63-
"lightrun.com/lightrunjavaagent": lightrunJavaAgent.Name,
67+
annotationAgentName: lightrunJavaAgent.Name,
6468
})
6569
r.addVolume(deploymentApplyConfig, lightrunJavaAgent)
6670
r.addInitContainer(deploymentApplyConfig, lightrunJavaAgent, secret)
@@ -172,20 +176,12 @@ func (r *LightrunJavaAgentReconciler) patchAppContainers(lightrunJavaAgent *agen
172176
// Client side patch, as we can't update value from 2 sources
173177
func (r *LightrunJavaAgentReconciler) patchJavaToolEnv(deplAnnotations map[string]string, container *corev1.Container, targetEnvVar string, agentArg string) error {
174178
// Check if some env was already patched before
175-
patchedEnv := deplAnnotations["lightrun.com/patched-env-name"]
176-
patchedEnvValue := deplAnnotations["lightrun.com/patched-env-value"]
179+
patchedEnv := deplAnnotations[annotationPatchedEnvName]
180+
patchedEnvValue := deplAnnotations[annotationPatchedEnvValue]
177181

178182
if patchedEnv != targetEnvVar || patchedEnvValue != agentArg {
179183
// If different env was patched before - unpatch it
180-
patchedEnvVarIndex := findEnvVarIndex(patchedEnv, container.Env)
181-
if patchedEnvVarIndex != -1 {
182-
unpatchedEnvValue := unpatchEnvVarValue(container.Env[patchedEnvVarIndex].Value, patchedEnvValue)
183-
if unpatchedEnvValue == "" {
184-
container.Env = slices.Delete(container.Env, patchedEnvVarIndex, patchedEnvVarIndex+1)
185-
} else {
186-
container.Env[patchedEnvVarIndex].Value = unpatchedEnvValue
187-
}
188-
}
184+
r.unpatchJavaToolEnv(deplAnnotations, container)
189185
}
190186

191187
targetEnvVarIndex := findEnvVarIndex(targetEnvVar, container.Env)
@@ -197,7 +193,7 @@ func (r *LightrunJavaAgentReconciler) patchJavaToolEnv(deplAnnotations map[strin
197193
})
198194
} else {
199195
if !strings.Contains(container.Env[targetEnvVarIndex].Value, agentArg) {
200-
container.Env[targetEnvVarIndex].Value = container.Env[targetEnvVarIndex].Value + agentArg
196+
container.Env[targetEnvVarIndex].Value = container.Env[targetEnvVarIndex].Value + " " + agentArg
201197
if len(container.Env[targetEnvVarIndex].Value) > 1024 {
202198
return errors.New(targetEnvVar + " has more that 1024 chars. This is a limitation of Java")
203199
}
@@ -206,21 +202,21 @@ func (r *LightrunJavaAgentReconciler) patchJavaToolEnv(deplAnnotations map[strin
206202
return nil
207203
}
208204

209-
func (r *LightrunJavaAgentReconciler) unpatchJavaToolEnv(deplAnnotations map[string]string, container *corev1.Container) *corev1.Container {
210-
patchedEnv := deplAnnotations["lightrun.com/patched-env-name"]
211-
patchedEnvValue := deplAnnotations["lightrun.com/patched-env-value"]
205+
func (r *LightrunJavaAgentReconciler) unpatchJavaToolEnv(deplAnnotations map[string]string, container *corev1.Container) {
206+
patchedEnv := deplAnnotations[annotationPatchedEnvName]
207+
patchedEnvValue := deplAnnotations[annotationPatchedEnvValue]
212208
if patchedEnv == "" && patchedEnvValue == "" {
213-
return container
209+
return
214210
}
215211

216212
envVarIndex := findEnvVarIndex(patchedEnv, container.Env)
217213
if envVarIndex != -1 {
218214
value := strings.ReplaceAll(container.Env[envVarIndex].Value, patchedEnvValue, "")
215+
value = strings.TrimSpace(value)
219216
if value == "" {
220217
container.Env = slices.Delete(container.Env, envVarIndex, envVarIndex+1)
221218
} else {
222219
container.Env[envVarIndex].Value = value
223220
}
224221
}
225-
return container
226222
}

0 commit comments

Comments
 (0)