Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Devops 831 fix env var cleanup upon change #23

Merged
merged 6 commits into from
May 2, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions internal/controller/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@ package controller

import (
"context"
"errors"
"hash/fnv"
"sort"
"strings"
"time"

agentv1beta "github.com/lightrun-platform/lightrun-k8s-operator/api/v1beta"
Expand Down Expand Up @@ -226,3 +228,30 @@ func SetStatusCondition(conditions *[]metav1.Condition, newCondition metav1.Cond
existingCondition.Message = newCondition.Message
existingCondition.ObservedGeneration = newCondition.ObservedGeneration
}

func agentEnvVarArgument(mountPath string, agentCliFlags string) (string, error) {
agentArg := " -agentpath:" + mountPath + "/agent/lightrun_agent.so"
if agentCliFlags != "" {
agentArg += "=" + agentCliFlags
if len(agentArg) > 1024 {
return "", errors.New("agentpath with agentCliFlags has more than 1024 chars. This is a limitation of Java")
}
}
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 {
if envVar.Name == envVarName {
return i
}
}
return -1
}
143 changes: 143 additions & 0 deletions internal/controller/helpers_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
package controller

import (
"strings"
"testing"

corev1 "k8s.io/api/core/v1"
)

func Test_findEnvVarIndex(t *testing.T) {
type args struct {
envVarName string
envVarList []corev1.EnvVar
}
tests := []struct {
name string
args args
want int
}{
{
name: "correctly finds the index of the env var",
args: args{
envVarName: "test",
envVarList: []corev1.EnvVar{
{
Name: "test",
Value: "test",
},
},
},
want: 0,
},
{
name: "correctly finds the index of the env var",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this test represent a scenario where the arg envVarName is not in envVarList and therefore function return -1 , right ? in that case it's still true to name it "correctly finds the index of the env var" ? maybe test envVarName does not exist or something like that

args: args{
envVarName: "test",
envVarList: []corev1.EnvVar{
{
Name: "test1",
Value: "test",
},
},
},
want: -1,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := findEnvVarIndex(tt.args.envVarName, tt.args.envVarList); got != tt.want {
t.Errorf("findEnvVarIndex() = %v, want %v", got, tt.want)
}
})
}
}

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: "correctly removes the value from the env var",
args: args{
origValue: "test",
removalValue: "test1",
},
want: "test",
},
}
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
agentCliFlags string
}
tests := []struct {
name string
args args
want string
wantErr bool
}{
{
name: "correctly returns the agent env var argument",
args: args{
mountPath: "test",
agentCliFlags: "test",
},
want: " -agentpath:test/agent/lightrun_agent.so=test",
wantErr: false,
},
{
name: "correctly returns the agent env var argument",
args: args{
mountPath: "test",
agentCliFlags: "",
},
want: " -agentpath:test/agent/lightrun_agent.so",
wantErr: false,
},
{
name: "return error when agentpath with agentCliFlags has more than 1024 chars",
args: args{
mountPath: "test",
agentCliFlags: strings.Repeat("a", 1024),
},
want: "",
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := agentEnvVarArgument(tt.args.mountPath, tt.args.agentCliFlags)
if (err != nil) != tt.wantErr {
t.Errorf("agentEnvVarArgument() error = %v, wantErr %v", err, tt.wantErr)
return
}
if got != tt.want {
t.Errorf("agentEnvVarArgument() = %v, want %v", got, tt.want)
}
})
}
}
30 changes: 18 additions & 12 deletions internal/controller/lightrunjavaagent_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,17 +134,17 @@ func (r *LightrunJavaAgentReconciler) Reconcile(ctx context.Context, req ctrl.Re
log.Info("Unpatching deployment", "Deployment", originalDeployment.Name)

// Remove agent from JAVA_TOOL_OPTIONS. Client side patch
conIndex := -1
clientSidePatch := client.MergeFrom(originalDeployment.DeepCopy())
for i, container := range originalDeployment.Spec.Template.Spec.Containers {
for _, targetContainer := range lightrunJavaAgent.Spec.ContainerSelector {
if targetContainer == container.Name {
conIndex = i
break
r.unpatchJavaToolEnv(originalDeployment.Annotations, &originalDeployment.Spec.Template.Spec.Containers[i])
}
}
r.unpatchJavaToolEnv(&originalDeployment.Spec.Template.Spec.Containers[conIndex], lightrunJavaAgent.Spec.AgentEnvVarName, lightrunJavaAgent.Spec.InitContainer.SharedVolumeMountPath, lightrunJavaAgent.Spec.AgentCliFlags)

}
delete(originalDeployment.Annotations, "lightrun.com/patched-env-name")
delete(originalDeployment.Annotations, "lightrun.com/patched-env-value")
err = r.Patch(ctx, originalDeployment, clientSidePatch)
if err != nil {
log.Error(err, "unable to unpatch "+lightrunJavaAgent.Spec.AgentEnvVarName)
Expand Down Expand Up @@ -185,6 +185,13 @@ func (r *LightrunJavaAgentReconciler) Reconcile(ctx context.Context, req ctrl.Re
}
}

// Verify that env var won't exceed 1024 chars
agentArg, err := agentEnvVarArgument(lightrunJavaAgent.Spec.InitContainer.SharedVolumeMountPath, lightrunJavaAgent.Spec.AgentCliFlags)
if err != nil {
log.Error(err, "agentEnvVarArgument exceeds 1024 chars")
return r.errorStatus(ctx, lightrunJavaAgent, err)
}

// Create config map
log.V(2).Info("Reconciling config map with agent configuration")
configMap, err := r.createAgentConfig(lightrunJavaAgent)
Expand Down Expand Up @@ -258,20 +265,19 @@ func (r *LightrunJavaAgentReconciler) Reconcile(ctx context.Context, req ctrl.Re
return r.errorStatus(ctx, lightrunJavaAgent, err)
}
clientSidePatch := client.MergeFrom(originalDeployment.DeepCopy())
conIndex := -1
for i, container := range originalDeployment.Spec.Template.Spec.Containers {
for _, targetContainer := range lightrunJavaAgent.Spec.ContainerSelector {
if targetContainer == container.Name {
conIndex = i
break
err = r.patchJavaToolEnv(originalDeployment.Annotations, &originalDeployment.Spec.Template.Spec.Containers[i], lightrunJavaAgent.Spec.AgentEnvVarName, agentArg)
if err != nil {
log.Error(err, "failed to patch "+lightrunJavaAgent.Spec.AgentEnvVarName)
return r.errorStatus(ctx, lightrunJavaAgent, err)
}
}
}
err = r.patchJavaToolEnv(&originalDeployment.Spec.Template.Spec.Containers[conIndex], lightrunJavaAgent.Spec.AgentEnvVarName, lightrunJavaAgent.Spec.InitContainer.SharedVolumeMountPath, lightrunJavaAgent.Spec.AgentCliFlags)
if err != nil {
log.Error(err, "failed to patch "+lightrunJavaAgent.Spec.AgentEnvVarName)
return r.errorStatus(ctx, lightrunJavaAgent, err)
}
}
originalDeployment.Annotations["lightrun.com/patched-env-name"] = lightrunJavaAgent.Spec.AgentEnvVarName
originalDeployment.Annotations["lightrun.com/patched-env-value"] = agentArg
err = r.Patch(ctx, originalDeployment, clientSidePatch)
if err != nil {
log.Error(err, "failed to patch "+lightrunJavaAgent.Spec.AgentEnvVarName)
Expand Down
Loading
Loading