Skip to content

Commit 55c857c

Browse files
ThomasK33ThomasK33
committed
fix: only honor envFrom when access URL is present
1 parent fe3789b commit 55c857c

2 files changed

Lines changed: 160 additions & 8 deletions

File tree

‎internal/controller/codercontrolplane_controller.go‎

Lines changed: 72 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -784,7 +784,7 @@ func (r *CoderControlPlaneReconciler) reconcileDeployment(ctx context.Context, c
784784
accessURLConfiguredViaEnvFrom := false
785785
if injectClusterAccessURL {
786786
var err error
787-
accessURLConfiguredViaEnvFrom, err = envFromMayDefineEnvVar(coderControlPlane.Spec.EnvFrom, "CODER_ACCESS_URL")
787+
accessURLConfiguredViaEnvFrom, err = r.envFromDefinesEnvVar(ctx, coderControlPlane.Namespace, coderControlPlane.Spec.EnvFrom, "CODER_ACCESS_URL")
788788
if err != nil {
789789
return nil, err
790790
}
@@ -2078,40 +2078,104 @@ func (r *CoderControlPlaneReconciler) resolvePostgresURLFromExtraEnv(
20782078
return r.readSecretValue(ctx, coderControlPlane.Namespace, secretRef.Name, secretRef.Key)
20792079
}
20802080

2081-
func envFromMayDefineEnvVar(envFromSources []corev1.EnvFromSource, envVarName string) (bool, error) {
2081+
func (r *CoderControlPlaneReconciler) envFromDefinesEnvVar(
2082+
ctx context.Context,
2083+
namespace string,
2084+
envFromSources []corev1.EnvFromSource,
2085+
envVarName string,
2086+
) (bool, error) {
2087+
if strings.TrimSpace(namespace) == "" {
2088+
return false, fmt.Errorf("assertion failed: namespace must not be empty")
2089+
}
20822090
if strings.TrimSpace(envVarName) == "" {
20832091
return false, fmt.Errorf("assertion failed: environment variable name must not be empty")
20842092
}
20852093

2094+
var reader client.Reader = r.Client
2095+
if r.APIReader != nil {
2096+
reader = r.APIReader
2097+
}
2098+
if reader == nil {
2099+
return false, fmt.Errorf("assertion failed: reader must not be nil")
2100+
}
2101+
20862102
for i := range envFromSources {
20872103
envFromSource := envFromSources[i]
20882104
if envFromSource.ConfigMapRef != nil && envFromSource.SecretRef != nil {
20892105
return false, fmt.Errorf("assertion failed: envFrom[%d] must not set both configMapRef and secretRef", i)
20902106
}
20912107

2092-
prefix := strings.TrimSpace(envFromSource.Prefix)
2093-
if prefix != "" && !strings.HasPrefix(envVarName, prefix) {
2108+
lookupKey, includeSource, err := envFromLookupKeyForEnvVar(i, envFromSource.Prefix, envVarName)
2109+
if err != nil {
2110+
return false, err
2111+
}
2112+
if !includeSource {
20942113
continue
20952114
}
20962115

20972116
if envFromSource.ConfigMapRef != nil {
2098-
if strings.TrimSpace(envFromSource.ConfigMapRef.Name) == "" {
2117+
configMapName := strings.TrimSpace(envFromSource.ConfigMapRef.Name)
2118+
if configMapName == "" {
20992119
return false, fmt.Errorf("assertion failed: envFrom[%d].configMapRef.name must not be empty", i)
21002120
}
2101-
return true, nil
2121+
2122+
configMap := &corev1.ConfigMap{}
2123+
err := reader.Get(ctx, types.NamespacedName{Namespace: namespace, Name: configMapName}, configMap)
2124+
if err != nil {
2125+
if apierrors.IsNotFound(err) {
2126+
continue
2127+
}
2128+
return false, fmt.Errorf("get envFrom[%d] configmap %s/%s: %w", i, namespace, configMapName, err)
2129+
}
2130+
if _, ok := configMap.Data[lookupKey]; ok {
2131+
return true, nil
2132+
}
2133+
if _, ok := configMap.BinaryData[lookupKey]; ok {
2134+
return true, nil
2135+
}
2136+
continue
21022137
}
21032138

21042139
if envFromSource.SecretRef != nil {
2105-
if strings.TrimSpace(envFromSource.SecretRef.Name) == "" {
2140+
secretName := strings.TrimSpace(envFromSource.SecretRef.Name)
2141+
if secretName == "" {
21062142
return false, fmt.Errorf("assertion failed: envFrom[%d].secretRef.name must not be empty", i)
21072143
}
2108-
return true, nil
2144+
2145+
secret := &corev1.Secret{}
2146+
err := reader.Get(ctx, types.NamespacedName{Namespace: namespace, Name: secretName}, secret)
2147+
if err != nil {
2148+
if apierrors.IsNotFound(err) {
2149+
continue
2150+
}
2151+
return false, fmt.Errorf("get envFrom[%d] secret %s/%s: %w", i, namespace, secretName, err)
2152+
}
2153+
if _, ok := secret.Data[lookupKey]; ok {
2154+
return true, nil
2155+
}
21092156
}
21102157
}
21112158

21122159
return false, nil
21132160
}
21142161

2162+
func envFromLookupKeyForEnvVar(index int, prefix, envVarName string) (string, bool, error) {
2163+
prefix = strings.TrimSpace(prefix)
2164+
if prefix == "" {
2165+
return envVarName, true, nil
2166+
}
2167+
if !strings.HasPrefix(envVarName, prefix) {
2168+
return "", false, nil
2169+
}
2170+
2171+
lookupKey := strings.TrimPrefix(envVarName, prefix)
2172+
if strings.TrimSpace(lookupKey) == "" {
2173+
return "", false, fmt.Errorf("assertion failed: envFrom[%d] prefix %q must not consume the entire environment variable name %q", index, prefix, envVarName)
2174+
}
2175+
2176+
return lookupKey, true, nil
2177+
}
2178+
21152179
func findEnvVar(envVars []corev1.EnvVar, name string) (*corev1.EnvVar, error) {
21162180
if strings.TrimSpace(name) == "" {
21172181
return nil, fmt.Errorf("assertion failed: environment variable name must not be empty")

‎internal/controller/codercontrolplane_controller_test.go‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2894,6 +2894,94 @@ func TestReconcile_DeploymentAlignment(t *testing.T) {
28942894
}
28952895
})
28962896

2897+
t.Run("EnvFromWithoutAccessURLKeepsDefault", func(t *testing.T) {
2898+
configMap := &corev1.ConfigMap{
2899+
ObjectMeta: metav1.ObjectMeta{Name: "test-deployment-alignment-envfrom-unrelated", Namespace: "default"},
2900+
Data: map[string]string{
2901+
"UNRELATED_ENV": "value",
2902+
},
2903+
}
2904+
if err := k8sClient.Create(ctx, configMap); err != nil {
2905+
t.Fatalf("create configmap: %v", err)
2906+
}
2907+
t.Cleanup(func() {
2908+
_ = k8sClient.Delete(ctx, configMap)
2909+
})
2910+
2911+
cp := &coderv1alpha1.CoderControlPlane{
2912+
ObjectMeta: metav1.ObjectMeta{Name: "test-deployment-alignment-envfrom-unrelated", Namespace: "default"},
2913+
Spec: coderv1alpha1.CoderControlPlaneSpec{
2914+
Image: "test-deployment-alignment:latest",
2915+
EnvFrom: []corev1.EnvFromSource{{
2916+
ConfigMapRef: &corev1.ConfigMapEnvSource{
2917+
LocalObjectReference: corev1.LocalObjectReference{Name: configMap.Name},
2918+
},
2919+
}},
2920+
},
2921+
}
2922+
if err := k8sClient.Create(ctx, cp); err != nil {
2923+
t.Fatalf("create control plane: %v", err)
2924+
}
2925+
t.Cleanup(func() {
2926+
_ = k8sClient.Delete(ctx, cp)
2927+
})
2928+
2929+
r := &controller.CoderControlPlaneReconciler{Client: k8sClient, APIReader: k8sClient, Scheme: scheme}
2930+
if _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: cp.Name, Namespace: cp.Namespace}}); err != nil {
2931+
t.Fatalf("reconcile control plane: %v", err)
2932+
}
2933+
2934+
deployment := &appsv1.Deployment{}
2935+
if err := k8sClient.Get(ctx, types.NamespacedName{Name: cp.Name, Namespace: cp.Namespace}, deployment); err != nil {
2936+
t.Fatalf("get deployment: %v", err)
2937+
}
2938+
container := deployment.Spec.Template.Spec.Containers[0]
2939+
if countEnvVar(container.Env, "CODER_ACCESS_URL") != 1 {
2940+
t.Fatalf("expected exactly one default CODER_ACCESS_URL env var, got %d", countEnvVar(container.Env, "CODER_ACCESS_URL"))
2941+
}
2942+
expectedAccessURL := "http://" + cp.Name + "." + cp.Namespace + ".svc.cluster.local"
2943+
if got := mustFindEnvVar(t, container.Env, "CODER_ACCESS_URL").Value; got != expectedAccessURL {
2944+
t.Fatalf("expected default CODER_ACCESS_URL %q, got %q", expectedAccessURL, got)
2945+
}
2946+
})
2947+
2948+
t.Run("OptionalMissingEnvFromKeepsDefault", func(t *testing.T) {
2949+
optional := true
2950+
cp := &coderv1alpha1.CoderControlPlane{
2951+
ObjectMeta: metav1.ObjectMeta{Name: "test-deployment-alignment-envfrom-optional-missing", Namespace: "default"},
2952+
Spec: coderv1alpha1.CoderControlPlaneSpec{
2953+
Image: "test-deployment-alignment:latest",
2954+
EnvFrom: []corev1.EnvFromSource{{
2955+
ConfigMapRef: &corev1.ConfigMapEnvSource{
2956+
LocalObjectReference: corev1.LocalObjectReference{Name: "missing-optional-envfrom"},
2957+
Optional: &optional,
2958+
},
2959+
}},
2960+
},
2961+
}
2962+
if err := k8sClient.Create(ctx, cp); err != nil {
2963+
t.Fatalf("create control plane: %v", err)
2964+
}
2965+
t.Cleanup(func() {
2966+
_ = k8sClient.Delete(ctx, cp)
2967+
})
2968+
2969+
r := &controller.CoderControlPlaneReconciler{Client: k8sClient, APIReader: k8sClient, Scheme: scheme}
2970+
if _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: cp.Name, Namespace: cp.Namespace}}); err != nil {
2971+
t.Fatalf("reconcile control plane: %v", err)
2972+
}
2973+
2974+
deployment := &appsv1.Deployment{}
2975+
if err := k8sClient.Get(ctx, types.NamespacedName{Name: cp.Name, Namespace: cp.Namespace}, deployment); err != nil {
2976+
t.Fatalf("get deployment: %v", err)
2977+
}
2978+
container := deployment.Spec.Template.Spec.Containers[0]
2979+
expectedAccessURL := "http://" + cp.Name + "." + cp.Namespace + ".svc.cluster.local"
2980+
if got := mustFindEnvVar(t, container.Env, "CODER_ACCESS_URL").Value; got != expectedAccessURL {
2981+
t.Fatalf("expected default CODER_ACCESS_URL %q, got %q", expectedAccessURL, got)
2982+
}
2983+
})
2984+
28972985
t.Run("ResourcesAndSecurityContextsApplied", func(t *testing.T) {
28982986
runAsUser := int64(1001)
28992987
allowPrivilegeEscalation := false

0 commit comments

Comments
 (0)