Repository: GoogleCloudPlatform/cloud-sql-proxy-operator
Pull request #206 (2023-02-10 19:14:52)

chore: use environment vars to configure global proxy settings
Use environment variables instead of CLI flags to configure the global proxy settings
for proxy containers. 


Patch:
diff --git a/internal/workload/podspec_updates.go b/internal/workload/podspec_updates.go
index c815a44..30fc942 100644
--- a/internal/workload/podspec_updates.go
+++ b/internal/workload/podspec_updates.go
@@ -204,38 +204,30 @@ func (u *Updater) ConfigureWorkload(wl *PodWorkload, matches []*cloudsqlapi.Auth
 }
 
 type managedEnvVar struct {
-	Instance             dbInstance        `json:"dbInstance"`
-	OperatorManagedValue corev1.EnvVar     `json:"operatorManagedValue"`
-	OriginalValues       map[string]string `json:"originalValues,omitempty"`
+	Instance             proxyInstanceID `json:"proxyInstanceID"`
+	ContainerName        string          `json:"containerName"`
+	OperatorManagedValue corev1.EnvVar   `json:"operatorManagedValue"`
 }
 
 type managedPort struct {
-	Instance       dbInstance       `json:"dbInstance"`
-	OriginalValues map[string]int32 `json:"originalValues,omitempty"`
-	Port           int32            `json:"port,omitempty"`
+	Instance proxyInstanceID `json:"proxyInstanceID"`
+	Port     int32           `json:"port,omitempty"`
 }
 
 type managedVolume struct {
 	Volume      corev1.Volume      `json:"volume"`
 	VolumeMount corev1.VolumeMount `json:"volumeMount"`
-	Instance    dbInstance         `json:"dbInstance"`
+	Instance    proxyInstanceID    `json:"proxyInstanceID"`
 }
 
-type dbInstance struct {
+// proxyInstanceID is an identifier for a proxy and/or specific proxy database
+// instance that created the EnvVar or Port. When this is empty, means that the
+// EnvVar or Port was created by the user, and is not associated with a proxy
+type proxyInstanceID struct {
 	AuthProxyWorkload types.NamespacedName `json:"authProxyWorkload"`
 	ConnectionString  string               `json:"connectionString"`
 }
 
-func dbInst(namespace, name, connectionString string) dbInstance {
-	return dbInstance{
-		AuthProxyWorkload: types.NamespacedName{
-			Namespace: namespace,
-			Name:      name,
-		},
-		ConnectionString: connectionString,
-	}
-}
-
 // updateState holds internal state while a particular workload being configured
 // with one or more DBInstances.
 type updateState struct {
@@ -248,14 +240,22 @@ type updateState struct {
 // workloadMods holds all modifications to this workload done by the operator so
 // so that it can be undone later.
 type workloadMods struct {
-	DBInstances  []*dbInstance    `json:"dbInstances"`
-	EnvVars      []*managedEnvVar `json:"envVars"`
-	VolumeMounts []*managedVolume `json:"volumeMounts"`
-	Ports        []*managedPort   `json:"ports"`
+	DBInstances  []*proxyInstanceID `json:"dbInstances"`
+	EnvVars      []*managedEnvVar   `json:"envVars"`
+	VolumeMounts []*managedVolume   `json:"volumeMounts"`
+	Ports        []*managedPort     `json:"ports"`
 }
 
-func (s *updateState) addInUsePort(p int32, containerName string) {
-	s.addPort(p, containerName, types.NamespacedName{}, "")
+func (s *updateState) addWorkloadPort(p int32) {
+	// This port is associated with the workload, not the proxy.
+	// so this uses an empty proxyInstanceID{}
+	s.addPort(p, proxyInstanceID{})
+}
+
+func (s *updateState) addProxyPort(p int32) {
+	// This port is associated with the workload, not the proxy.
+	// so this uses an empty proxyInstanceID{}
+	s.addPort(p, proxyInstanceID{})
 }
 
 // isPortInUse checks if the port is in use.
@@ -314,12 +314,18 @@ func (s *updateState) useInstancePort(p *cloudsqlapi.AuthProxyWorkload, is *clou
 				port, is.ConnectionString), p)
 	}
 
-	s.addPort(port, "", n, is.ConnectionString)
+	s.addPort(port, proxyInstanceID{
+		AuthProxyWorkload: types.NamespacedName{
+			Name:      p.Name,
+			Namespace: p.Namespace,
+		},
+		ConnectionString: is.ConnectionString,
+	})
 
 	return port
 }
 
-func (s *updateState) addPort(p int32, containerName string, n types.NamespacedName, connectionString string) {
+func (s *updateState) addPort(p int32, instance proxyInstanceID) {
 	var mp *managedPort
 
 	for i := 0; i < len(s.mods.Ports); i++ {
@@ -330,46 +336,78 @@ func (s *updateState) addPort(p int32, containerName string, n types.NamespacedN
 
 	if mp == nil {
 		mp = &managedPort{
-			Instance:       dbInst(n.Namespace, n.Name, connectionString),
-			Port:           p,
-			OriginalValues: map[string]int32{},
+			Instance: instance,
+			Port:     p,
 		}
 		s.mods.Ports = append(s.mods.Ports, mp)
 	}
-	if containerName != "" && !strings.HasPrefix(containerName, ContainerPrefix) {
-		mp.OriginalValues[containerName] = p
-	}
+}
 
+func (s *updateState) addProxyContainerEnvVar(p *cloudsqlapi.AuthProxyWorkload, k, v string) {
+	s.addEnvVar(p, managedEnvVar{
+		Instance: proxyInstanceID{
+			AuthProxyWorkload: types.NamespacedName{
+				Namespace: p.Namespace,
+				Name:      p.Name,
+			},
+		},
+		ContainerName:        ContainerName(p),
+		OperatorManagedValue: corev1.EnvVar{Name: k, Value: v},
+	})
 }
 
 // addWorkloadEnvVar adds or replaces the envVar based on its Name, returning the old and new values
 func (s *updateState) addWorkloadEnvVar(p *cloudsqlapi.AuthProxyWorkload, is *cloudsqlapi.InstanceSpec, ev corev1.EnvVar) {
+	s.addEnvVar(p, managedEnvVar{
+		Instance: proxyInstanceID{
+			AuthProxyWorkload: types.NamespacedName{
+				Namespace: p.Namespace,
+				Name:      p.Name,
+			},
+			ConnectionString: is.ConnectionString,
+		},
+		OperatorManagedValue: ev,
+	})
+}
+func (s *updateState) addEnvVar(p *cloudsqlapi.AuthProxyWorkload, v managedEnvVar) {
 	for i := 0; i < len(s.mods.EnvVars); i++ {
-		if s.mods.EnvVars[i].OperatorManagedValue.Name == ev.Name {
-			old := s.mods.EnvVars[i].OperatorManagedValue
-			s.mods.EnvVars[i].OperatorManagedValue = ev
-			if old.Value != ev.Value {
-				s.addError(cloudsqlapi.ErrorCodeEnvConflict,
-					fmt.Sprintf("environment variable named %s already exists", ev.Name), p)
-			}
+		oldEnv := s.mods.EnvVars[i]
+		// if the values don't match and either one is global, or its set twice
+		if isEnvVarConflict(oldEnv, v) {
+			s.addError(cloudsqlapi.ErrorCodeEnvConflict,
+				fmt.Sprintf("environment variable named %s is set more than once",
+					oldEnv.OperatorManagedValue.Name),
+				p)
 			return
 		}
 	}
-	s.mods.EnvVars = append(s.mods.EnvVars, &managedEnvVar{
-		Instance:             dbInst(p.Namespace, p.Name, is.ConnectionString),
-		OriginalValues:       map[string]string{},
-		OperatorManagedValue: ev,
-	})
+
+	s.mods.EnvVars = append(s.mods.EnvVars, &v)
+}
+
+func isEnvVarConflict(oldEnv *managedEnvVar, v managedEnvVar) bool {
+	// it's a different name, no conflict
+	if oldEnv.OperatorManagedValue.Name != v.OperatorManagedValue.Name {
+		return false
+	}
+
+	// if the envvar is intended for a different container
+	if oldEnv.ContainerName != v.ContainerName && oldEnv.ContainerName != "" && v.ContainerName != "" {
+		return false
+	}
+
+	// different value, therefore conflict
+	return oldEnv.OperatorManagedValue.Value != v.OperatorManagedValue.Value
 }
 
 func (s *updateState) initState(pl []*cloudsqlapi.AuthProxyWorkload) {
 	// Reset the mods.DBInstances to the list of pl being
 	// applied right now.
-	s.mods.DBInstances = make([]*dbInstance, 0, len(pl))
+	s.mods.DBInstances = make([]*proxyInstanceID, 0, len(pl))
 	for _, wl := range pl {
 		for _, instance := range wl.Spec.Instances {
 			s.mods.DBInstances = append(s.mods.DBInstances,
-				&dbInstance{
+				&proxyInstanceID{
 					AuthProxyWorkload: types.NamespacedName{
 						Namespace: wl.Namespace,
 						Name:      wl.Name,
@@ -399,7 +437,7 @@ func (s *updateState) update(wl *PodWorkload, matches []*cloudsqlapi.AuthProxyWo
 	for i := 0; i < len(nonAuthProxyContainers); i++ {
 		c := nonAuthProxyContainers[i]
 		for j := 0; j < len(c.Ports); j++ {
-			s.addInUsePort(c.Ports[j].ContainerPort, c.Name)
+			s.addWorkloadPort(c.Ports[j].ContainerPort)
 		}
 	}
 
@@ -455,19 +493,22 @@ func (s *updateState) updateContainer(p *cloudsqlapi.AuthProxyWorkload, wl Workl
 		return
 	}
 
-	// Build the c
-	var cliArgs []string
-
 	// always enable http port healthchecks on 0.0.0.0 and structured logs
-	cliArgs = s.addHealthCheck(p, c, cliArgs)
+	s.addHealthCheck(p, c)
 
 	// add the user agent
-	cliArgs = append(cliArgs, fmt.Sprintf("--user-agent=%v", s.updater.userAgent))
+	s.addProxyContainerEnvVar(p, "CSQL_PROXY_USER_AGENT", s.updater.userAgent)
+
+	// configure structured logs
+	s.addProxyContainerEnvVar(p, "CSQL_PROXY_STRUCTURED_LOGS", "true")
 
 	c.Name = ContainerName(p)
 	c.ImagePullPolicy = "IfNotPresent"
 
-	cliArgs = s.applyContainerSpec(p, c, cliArgs)
+	s.applyContainerSpec(p, c)
+
+	// Build the c
+	var cliArgs []string
 
 	// Instances
 	for i := range p.Spec.Instances {
@@ -527,12 +568,12 @@ func (s *updateState) updateContainer(p *cloudsqlapi.AuthProxyWorkload, wl Workl
 
 // applyContainerSpec applies settings from cloudsqlapi.AuthProxyContainerSpec
 // to the container
-func (s *updateState) applyContainerSpec(p *cloudsqlapi.AuthProxyWorkload, c *corev1.Container, cliArgs []string) []string {
+func (s *updateState) applyContainerSpec(p *cloudsqlapi.AuthProxyWorkload, c *corev1.Container) {
 	c.Image = s.defaultProxyImage()
 	c.Resources = defaultContainerResources
 
 	if p.Spec.AuthProxyContainer == nil {
-		return cliArgs
+		return
 	}
 
 	if p.Spec.AuthProxyContainer.Image != "" {
@@ -544,25 +585,32 @@ func (s *updateState) applyContainerSpec(p *cloudsqlapi.AuthProxyWorkload, c *co
 	}
 
 	if p.Spec.AuthProxyContainer.SQLAdminAPIEndpoint != "" {
-		cliArgs = append(cliArgs, "--sqladmin-api-endpoint="+p.Spec.AuthProxyContainer.SQLAdminAPIEndpoint)
+		s.addProxyContainerEnvVar(p, "CSQL_PROXY_SQLADMIN_API_ENDPOINT", p.Spec.AuthProxyContainer.SQLAdminAPIEndpoint)
 	}
 	if p.Spec.AuthProxyContainer.MaxConnections != nil &&
 		*p.Spec.AuthProxyContainer.MaxConnections != 0 {
-		cliArgs = append(cliArgs, fmt.Sprintf("--max-connections=%d", *p.Spec.AuthProxyContainer.MaxConnections))
+		s.addProxyContainerEnvVar(p, "CSQL_PROXY_MAX_CONNECTIONS", fmt.Sprintf("%d", *p.Spec.AuthProxyContainer.MaxConnections))
 	}
 	if p.Spec.AuthProxyContainer.MaxSigtermDelay != nil &&
 		*p.Spec.AuthProxyContainer.MaxSigtermDelay != 0 {
-		cliArgs = append(cliArgs, fmt.Sprintf("--max-sigterm-delay=%d", *p.Spec.AuthProxyContainer.MaxSigtermDelay))
+		s.addProxyContainerEnvVar(p, "CSQL_PROXY_MAX_SIGTERM_DELAY", fmt.Sprintf("%d", *p.Spec.AuthProxyContainer.MaxSigtermDelay))
 	}
 
-	return cliArgs
+	return
 }
 
 // updateContainerEnv applies global container state to all containers
 func (s *updateState) updateContainerEnv(c *corev1.Container) {
 	for i := 0; i < len(s.mods.EnvVars); i++ {
 		var found bool
-		operatorEnv := s.mods.EnvVars[i].OperatorManagedValue
+		v := s.mods.EnvVars[i]
+		operatorEnv := v.OperatorManagedValue
+
+		// If this EnvVar is not for this container and not for all containers
+		// don't add it to this container.
+		if v.ContainerName != c.Name && v.ContainerName != "" {
+			continue
+		}
 
 		for j := 0; j < len(c.Env); j++ {
 			if operatorEnv.Name == c.Env[j].Name {
@@ -578,11 +626,8 @@ func (s *updateState) updateContainerEnv(c *corev1.Container) {
 }
 
 // addHealthCheck adds the health check declaration to this workload.
-func (s *updateState) addHealthCheck(_ *cloudsqlapi.AuthProxyWorkload, c *corev1.Container, cliArgs []string) []string {
+func (s *updateState) addHealthCheck(p *cloudsqlapi.AuthProxyWorkload, c *corev1.Container) {
 	port := DefaultHealthCheckPort
-	for s.isPortInUse(port) {
-		port++
-	}
 
 	c.StartupProbe = &corev1.Probe{
 		ProbeHandler: corev1.ProbeHandler{HTTPGet: &corev1.HTTPGetAction{
@@ -605,12 +650,12 @@ func (s *updateState) addHealthCheck(_ *cloudsqlapi.AuthProxyWorkload, c *corev1
 		}},
 		PeriodSeconds: 30,
 	}
-	cliArgs = append(cliArgs,
-		fmt.Sprintf("--http-port=%d", port),
-		"--http-address=0.0.0.0",
-		"--health-check",
-		"--structured-logs")
-	return cliArgs
+	// Add a port that is associated with the proxy, but not a specific db instance
+	s.addPort(port, proxyInstanceID{AuthProxyWorkload: types.NamespacedName{Namespace: p.Namespace, Name: p.Name}})
+	s.addProxyContainerEnvVar(p, "CSQL_PROXY_HTTP_PORT", fmt.Sprintf("%d", port))
+	s.addProxyContainerEnvVar(p, "CSQL_PROXY_HTTP_ADDRESS", "0.0.0.0")
+	s.addProxyContainerEnvVar(p, "CSQL_PROXY_HEALTH_CHECK", "true")
+	return
 }
 
 func (s *updateState) addError(errorCode, description string, p *cloudsqlapi.AuthProxyWorkload) {


Test patch:
diff --git a/internal/workload/podspec_updates_test.go b/internal/workload/podspec_updates_test.go
index 37d0ac8..a4b7dd0 100644
--- a/internal/workload/podspec_updates_test.go
+++ b/internal/workload/podspec_updates_test.go
@@ -517,6 +517,7 @@ func TestProxyCLIArgs(t *testing.T) {
 		proxySpec            v1alpha1.AuthProxyWorkloadSpec
 		wantProxyArgContains []string
 		wantErrorCodes       []string
+		wantWorkloadEnv      map[string]string
 	}
 	wantTrue := true
 	wantFalse := false
@@ -533,12 +534,12 @@ func TestProxyCLIArgs(t *testing.T) {
 					PortEnvName:      "DB_PORT",
 				}},
 			},
-			wantProxyArgContains: []string{
-				"--structured-logs",
-				"--health-check",
-				fmt.Sprintf("--http-port=%d", workload.DefaultHealthCheckPort),
-				"--http-address=0.0.0.0",
-				"--user-agent=cloud-sql-proxy-operator/dev",
+			wantWorkloadEnv: map[string]string{
+				"CSQL_PROXY_STRUCTURED_LOGS": "true",
+				"CSQL_PROXY_HEALTH_CHECK":    "true",
+				"CSQL_PROXY_HTTP_PORT":       fmt.Sprintf("%d", workload.DefaultHealthCheckPort),
+				"CSQL_PROXY_HTTP_ADDRESS":    "0.0.0.0",
+				"CSQL_PROXY_USER_AGENT":      "cloud-sql-proxy-operator/dev",
 			},
 		},
 		{
@@ -621,25 +622,6 @@ func TestProxyCLIArgs(t *testing.T) {
 				fmt.Sprintf("hello:world:one?port=%d&private-ip=true", workload.DefaultFirstPort),
 				fmt.Sprintf("hello:world:two?port=%d&private-ip=false", workload.DefaultFirstPort+1)},
 		},
-		{
-			desc: "global flags",
-			proxySpec: v1alpha1.AuthProxyWorkloadSpec{
-				AuthProxyContainer: &v1alpha1.AuthProxyContainerSpec{
-					SQLAdminAPIEndpoint: "https://example.com",
-					MaxConnections:      ptr(int64(10)),
-					MaxSigtermDelay:     ptr(int64(20)),
-				},
-				Instances: []v1alpha1.InstanceSpec{{
-					ConnectionString: "hello:world:one",
-				}},
-			},
-			wantProxyArgContains: []string{
-				fmt.Sprintf("hello:world:one?port=%d", workload.DefaultFirstPort),
-				"--sqladmin-api-endpoint=https://example.com",
-				"--max-connections=10",
-				"--max-sigterm-delay=20",
-			},
-		},
 		{
 			desc: "port conflict with other instance causes error",
 			proxySpec: v1alpha1.AuthProxyWorkloadSpec{
@@ -714,6 +696,19 @@ func TestProxyCLIArgs(t *testing.T) {
 			// test that port cli args are set correctly
 			assertContainerArgsContains(t, csqlContainer.Args, tc.wantProxyArgContains)
 
+			// Test that workload has the right env vars
+			for wantKey, wantValue := range tc.wantWorkloadEnv {
+				gotEnvVar, err := findEnvVar(wl, csqlContainer.Name, wantKey)
+				if err != nil {
+					t.Error(err)
+					continue
+				}
+
+				if gotEnvVar.Value != wantValue {
+					t.Errorf("got %v, wants %v workload env var %v", gotEnvVar, wantValue, wantKey)
+				}
+			}
+
 		})
 	}