Skip to content

Commit

Permalink
fix: env variables not applied when Deployment is deleted IPT-1134
Browse files Browse the repository at this point in the history 
- Improve detection of when Deployment does not contain correct env. variables
- Remove deprecated feature that allowed users to manually edit Deployment to specify env. variables. This has caused the code to be more complex and increased chance of bugs, and is not feasible to support it together with the podTemplateSpecPreview feature.
  • Loading branch information
@jsenko
jsenko committed Jun 21, 2024
1 parent f112323 commit 9344c1e
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 104 deletions.
115 changes: 52 additions & 63 deletions controllers/cf/cf_env_apply.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,36 +8,34 @@ import (
"github.com/Apicurio/apicurio-registry-operator/controllers/svc/resources"
"go.uber.org/zap"
apps "k8s.io/api/apps/v1"
"k8s.io/apimachinery/pkg/types"
)

var _ loop.ControlFunction = &EnvApplyCF{}

type EnvApplyCF struct {
ctx context.LoopContext
log *zap.SugaredLogger
svcResourceCache resources.ResourceCache
svcEnvCache env.EnvCache
deploymentExists bool
deploymentEntry resources.ResourceCacheEntry
deploymentName string
envCacheUpdated bool
lastDeploymentName string
deploymentUID types.UID
lastDeploymentUID types.UID
ctx context.LoopContext
log *zap.SugaredLogger
svcResourceCache resources.ResourceCache

svcEnvCache env.EnvCache
envCacheUpdated bool

deploymentExists bool
deploymentEntry resources.ResourceCacheEntry
deploymentNeedsUpdate bool
}

// Is responsible for managing environment variables from the env cache
func NewEnvApplyCF(ctx context.LoopContext) loop.ControlFunction {
res := &EnvApplyCF{
ctx: ctx,
svcResourceCache: ctx.GetResourceCache(),
svcEnvCache: ctx.GetEnvCache(),
deploymentExists: false,
deploymentEntry: nil,
deploymentName: resources.RC_NOT_CREATED_NAME_EMPTY,
lastDeploymentName: resources.RC_NOT_CREATED_NAME_EMPTY,
envCacheUpdated: false,
ctx: ctx,
svcResourceCache: ctx.GetResourceCache(),

svcEnvCache: ctx.GetEnvCache(),
envCacheUpdated: false,

deploymentExists: false,
deploymentEntry: nil,
}
res.log = ctx.GetLog().Sugar().With("cf", res.Describe())
return res
Expand All @@ -48,63 +46,49 @@ func (this *EnvApplyCF) Describe() string {
}

func (this *EnvApplyCF) Sense() {
this.log.Debugw("env cache before", "value", this.svcEnvCache.GetSorted())
// Observation #1
// Is deployment available and/or is it already created
var deploymentEntry resources.ResourceCacheEntry
if deploymentEntry, this.deploymentExists = this.svcResourceCache.Get(resources.RC_KEY_DEPLOYMENT); this.deploymentExists {
this.deploymentEntry = deploymentEntry
this.deploymentName = deploymentEntry.GetName().Str()

// Observation #2
// First, read the existing env variables, and the add them to cache,
// keeping the original ordering.
// The operator overwrites user defined ones only when necessary.
deployment := this.deploymentEntry.GetValue().(*apps.Deployment)

// Observation #2
// Determine whether any env. variables are missing or need to be removed
for i, c := range deployment.Spec.Template.Spec.Containers {
if c.Name == factory.REGISTRY_CONTAINER_NAME {
prevName := "" // To maintain ordering in case of interpolation
// Copy variables in the cache
deleted := make(map[string]bool, 0)

this.deploymentNeedsUpdate = false

// Remember which variables are in the cache, to see if any is NOT present in the deployment
cachedVariablePresentInDeployment := make(map[string]bool, 0)
for _, v := range this.svcEnvCache.GetSorted() {
if v.Name == env.JAVA_OPTIONS_OPERATOR || v.Name == env.JAVA_OPTIONS_COMBINED {
continue // Do not delete, these are special internal-only env. variables.
// TODO: Consider refactoring this so the special case is no longer needed.
continue // Ignore, these are special internal-only env. variables.
}
deleted[v.Name] = true // deletes spec stuff as well
cachedVariablePresentInDeployment[v.Name] = false
}
for _, e := range deployment.Spec.Template.Spec.Containers[i].Env {

// Remove from deleted if in spec
delete(deleted, e.Name)

// If already marked as deleted, do not re-add them
if this.svcEnvCache.WasDeleted(e.Name) {
continue
}

if e.Name == env.JAVA_OPTIONS {
/*
Do not read this variable from deployment, we can't support this specific use-case anymore.
It has been deprecated for some time, and it's very unlikely someone relies on this behavior.
*/
continue
}

// Add to the cache
entryBuilder := env.NewEnvCacheEntryBuilder(&e).SetPriority(env.PRIORITY_DEPLOYMENT)
if prevName != "" {
entryBuilder.SetDependency(prevName)
// Mark as present:
cachedVariablePresentInDeployment[e.Name] = true
// Also ensure the other direction, each variable in deployment is present in the cache:
if _, exists := this.svcEnvCache.Get(e.Name); !exists {
_, combinedExists := this.svcEnvCache.Get(env.JAVA_OPTIONS_COMBINED)
if e.Name == env.JAVA_OPTIONS && combinedExists {
// We transform env.JAVA_OPTIONS_COMBINED into env.JAVA_OPTIONS, which may not be present in the cache beforehand.
this.log.Debugln("ignoring that variable " + env.JAVA_OPTIONS + " is in deployment but not in cache")
} else {
this.log.Debugln("variable is in deployment but not in cache", e.Name)
this.deploymentNeedsUpdate = true
}
}
this.svcEnvCache.Set(entryBuilder.Build())
prevName = e.Name
}
// Remove things from the cache that are not in the spec
// IF the cache was not changed already.
// This would otherwise prevent new things from being added.
if !this.svcEnvCache.IsChanged() {
for k, _ := range deleted {
this.svcEnvCache.DeleteByName(k)
// Check that all variables in the cache are present in the deployment:
for k, v := range cachedVariablePresentInDeployment {
if !v {
this.log.Debugln("variable is in cache but not in deployment", k)
this.deploymentNeedsUpdate = true
}
}
}
Expand All @@ -128,7 +112,11 @@ func (this *EnvApplyCF) Compare() bool {
// We have something to update
// Condition #2
// There is a deployment
return (this.envCacheUpdated || this.deploymentName != this.lastDeploymentName) && this.deploymentExists
this.log.Debugw("env apply compare", "this.envCacheUpdated", this.envCacheUpdated,
"this.deploymentExists", this.deploymentExists,
"this.deploymentNeedsUpdate", this.deploymentNeedsUpdate,
)
return this.deploymentExists && (this.envCacheUpdated || this.deploymentNeedsUpdate)
}

func (this *EnvApplyCF) Respond() {
Expand All @@ -147,6 +135,8 @@ func (this *EnvApplyCF) Respond() {
v, _ := env.GetEnv(sorted, env.JAVA_OPTIONS_COMBINED)
v.Name = env.JAVA_OPTIONS
}

this.log.Debugw("deployment env after", "env", sorted)
deployment.Spec.Template.Spec.Containers[i].Env = sorted
}
} // TODO report a problem if not found?
Expand All @@ -157,7 +147,6 @@ func (this *EnvApplyCF) Respond() {
// Do not clear the cache, but reset the change mark
this.svcEnvCache.ProcessAndAdvanceToNextPeriod()

this.lastDeploymentName = this.deploymentName
}

func (this *EnvApplyCF) Cleanup() bool {
Expand Down
55 changes: 14 additions & 41 deletions controllers/cf/env_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,16 +139,7 @@ func TestEnvOrdering(t *testing.T) {
Containers: []corev1.Container{
{
Name: factory.REGISTRY_CONTAINER_NAME,
Env: []corev1.EnvVar{
{
Name: "DEPLOYMENT_VAR_1_NAME",
Value: "DEPLOYMENT_VAR_1_VALUE",
},
{
Name: "DEPLOYMENT_VAR_2_NAME",
Value: "DEPLOYMENT_VAR_2_VALUE",
},
},
Env: []corev1.EnvVar{},
},
},
},
Expand Down Expand Up @@ -202,15 +193,6 @@ func TestEnvOrdering(t *testing.T) {
Name: "SPEC_VAR_3_NAME",
Value: "SPEC_VAR_3_VALUE",
})
c.AssertIsInOrder(t, sortedI,
corev1.EnvVar{
Name: "DEPLOYMENT_VAR_1_NAME",
Value: "DEPLOYMENT_VAR_1_VALUE",
},
corev1.EnvVar{
Name: "DEPLOYMENT_VAR_2_NAME",
Value: "DEPLOYMENT_VAR_2_VALUE",
})
}

func TestEnvPriority(t *testing.T) {
Expand All @@ -227,7 +209,11 @@ func TestEnvPriority(t *testing.T) {
Configuration: v1.ApicurioRegistrySpecConfiguration{
Env: []corev1.EnvVar{
{
Name: "VAR_3_NAME",
Name: "VAR_1_NAME",
Value: "SPEC",
},
{
Name: "VAR_2_NAME",
Value: "SPEC",
},
},
Expand All @@ -238,8 +224,8 @@ func TestEnvPriority(t *testing.T) {
ctx.Finalize()
// TODO When overwriting an entry, previous dependencies are removed!
// Operator Managed
ctx.GetEnvCache().Set(env.NewSimpleEnvCacheEntryBuilder("VAR_2_NAME", "OPERATOR").Build())
ctx.GetEnvCache().Set(env.NewSimpleEnvCacheEntryBuilder("VAR_3_NAME", "OPERATOR").Build())
ctx.GetEnvCache().Set(env.NewSimpleEnvCacheEntryBuilder("VAR_2_NAME", "OPERATOR").SetDependency("VAR_3_NAME").Build())
loop.Run()
ctx.Finalize()
// Deployment - User Managed
Expand All @@ -253,20 +239,7 @@ func TestEnvPriority(t *testing.T) {
Containers: []corev1.Container{
{
Name: factory.REGISTRY_CONTAINER_NAME,
Env: []corev1.EnvVar{
{
Name: "VAR_1_NAME",
Value: "DEPLOYMENT",
},
{
Name: "VAR_2_NAME",
Value: "DEPLOYMENT",
},
{
Name: "VAR_3_NAME",
Value: "DEPLOYMENT",
},
},
Env: []corev1.EnvVar{},
},
},
},
Expand All @@ -280,7 +253,7 @@ func TestEnvPriority(t *testing.T) {
sortedI := convert(sorted)
c.AssertSliceContains(t, sortedI, corev1.EnvVar{
Name: "VAR_1_NAME",
Value: "DEPLOYMENT",
Value: "SPEC",
})
c.AssertSliceContains(t, sortedI, corev1.EnvVar{
Name: "VAR_2_NAME",
Expand All @@ -293,19 +266,19 @@ func TestEnvPriority(t *testing.T) {
c.AssertIsInOrder(t, sortedI,
corev1.EnvVar{
Name: "VAR_1_NAME",
Value: "DEPLOYMENT",
Value: "SPEC",
},
corev1.EnvVar{
Name: "VAR_2_NAME",
Name: "VAR_3_NAME",
Value: "OPERATOR",
})
c.AssertIsInOrder(t, sortedI,
corev1.EnvVar{
Name: "VAR_1_NAME",
Value: "DEPLOYMENT",
Name: "VAR_3_NAME",
Value: "OPERATOR",
},
corev1.EnvVar{
Name: "VAR_3_NAME",
Name: "VAR_2_NAME",
Value: "OPERATOR",
})
}
Expand Down

0 comments on commit 9344c1e

Please sign in to comment.