code review feedback

Signed-off-by: Irbe Krumina <irbe@tailscale.com>
tailscale · Jun 7, 2024 · 4ef5918 · 4ef5918
1 parent 88ab26f
commit 4ef5918
Show file tree

Hide file tree

Showing 7 changed files with 37 additions and 18 deletions.
diff --git a/cmd/k8s-operator/deploy/crds/tailscale.com_proxyclasses.yaml b/cmd/k8s-operator/deploy/crds/tailscale.com_proxyclasses.yaml
@@ -722,7 +722,7 @@ spec:
                                     description: 'Variable references $(VAR_NAME) are expanded using the previously defined environment variables in the container and any service environment variables. If a variable cannot be resolved, the reference in the input string will be unchanged. Double $$ are reduced to a single $, which allows for escaping the $(VAR_NAME) syntax: i.e. "$$(VAR_NAME)" will produce the string literal "$(VAR_NAME)". Escaped references will never be expanded, regardless of whether the variable exists or not. Defaults to "".'
                                     type: string
                             image:
-                              description: Container image name. By default images are pulled from docker.io/tailscale/tailscale, but the official images are also available at ghcr.io/tailscale/tailscale. Image name provided here will override any proxy image values specified via the Kubernetes operator's Helm chart values or PROXY_IMAGE env var to the operator deployment. https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#image
+                              description: Container image name. By default images are pulled from docker.io/tailscale/tailscale, but the official images are also available at ghcr.io/tailscale/tailscale. Specifying image name here will override any proxy image values specified via the Kubernetes operator's Helm chart values or PROXY_IMAGE env var in the operator Deployment. https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#image
                               type: string
                             imagePullPolicy:
                               description: Image pull policy. One of Always, Never, IfNotPresent. Defaults to Always. https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#image
@@ -875,7 +875,7 @@ spec:
                                     description: 'Variable references $(VAR_NAME) are expanded using the previously defined environment variables in the container and any service environment variables. If a variable cannot be resolved, the reference in the input string will be unchanged. Double $$ are reduced to a single $, which allows for escaping the $(VAR_NAME) syntax: i.e. "$$(VAR_NAME)" will produce the string literal "$(VAR_NAME)". Escaped references will never be expanded, regardless of whether the variable exists or not. Defaults to "".'
                                     type: string
                             image:
-                              description: Container image name. By default images are pulled from docker.io/tailscale/tailscale, but the official images are also available at ghcr.io/tailscale/tailscale. Image name provided here will override any proxy image values specified via the Kubernetes operator's Helm chart values or PROXY_IMAGE env var to the operator deployment. https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#image
+                              description: Container image name. By default images are pulled from docker.io/tailscale/tailscale, but the official images are also available at ghcr.io/tailscale/tailscale. Specifying image name here will override any proxy image values specified via the Kubernetes operator's Helm chart values or PROXY_IMAGE env var in the operator Deployment. https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#image
                               type: string
                             imagePullPolicy:
                               description: Image pull policy. One of Always, Never, IfNotPresent. Defaults to Always. https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#image

diff --git a/cmd/k8s-operator/deploy/examples/proxyclass.yaml b/cmd/k8s-operator/deploy/examples/proxyclass.yaml
@@ -16,8 +16,8 @@ spec:
       imagePullSecrets:
       - name: "foo"
       tailscaleContainer:
-        image: "ghcr.io/tailscale/tailscale:v1.64.0"
+        image: "ghcr.io/tailscale/tailscale:v1.64"
         imagePullPolicy: IfNotPresent
       tailscaleInitContainer:
-        image: "ghcr.io/tailscale/tailscale:v1.64.0"
+        image: "ghcr.io/tailscale/tailscale:v1.64"
         imagePullPolicy: IfNotPresent
diff --git a/cmd/k8s-operator/deploy/manifests/operator.yaml b/cmd/k8s-operator/deploy/manifests/operator.yaml
@@ -971,7 +971,7 @@ spec:
                                                             type: object
                                                         type: array
                                                     image:
-                                                        description: Container image name. By default images are pulled from docker.io/tailscale/tailscale, but the official images are also available at ghcr.io/tailscale/tailscale. Image name provided here will override any proxy image values specified via the Kubernetes operator's Helm chart values or PROXY_IMAGE env var to the operator deployment. https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#image
+                                                        description: Container image name. By default images are pulled from docker.io/tailscale/tailscale, but the official images are also available at ghcr.io/tailscale/tailscale. Specifying image name here will override any proxy image values specified via the Kubernetes operator's Helm chart values or PROXY_IMAGE env var in the operator Deployment. https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#image
                                                         type: string
                                                     imagePullPolicy:
                                                         description: Image pull policy. One of Always, Never, IfNotPresent. Defaults to Always. https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#image
@@ -1124,7 +1124,7 @@ spec:
                                                             type: object
                                                         type: array
                                                     image:
-                                                        description: Container image name. By default images are pulled from docker.io/tailscale/tailscale, but the official images are also available at ghcr.io/tailscale/tailscale. Image name provided here will override any proxy image values specified via the Kubernetes operator's Helm chart values or PROXY_IMAGE env var to the operator deployment. https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#image
+                                                        description: Container image name. By default images are pulled from docker.io/tailscale/tailscale, but the official images are also available at ghcr.io/tailscale/tailscale. Specifying image name here will override any proxy image values specified via the Kubernetes operator's Helm chart values or PROXY_IMAGE env var in the operator Deployment. https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#image
                                                         type: string
                                                     imagePullPolicy:
                                                         description: Image pull policy. One of Always, Never, IfNotPresent. Defaults to Always. https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#image

diff --git a/cmd/k8s-operator/proxyclass.go b/cmd/k8s-operator/proxyclass.go
@@ -116,15 +116,14 @@ func (a *ProxyClassReconciler) validate(pc *tsapi.ProxyClass) (violations field.
 					// Same validation as used by kubelet https://github.com/kubernetes/kubernetes/blob/release-1.30/pkg/kubelet/images/image_manager.go#L212
 					if _, err := dockerref.ParseNormalizedNamed(tc.Image); err != nil {
 						violations = append(violations, field.TypeInvalid(field.NewPath("spec", "statefulSet", "pod", "tailscaleContainer", "image"), tc.Image, err.Error()))
-
 					}
 				}
 			}
-			if tc := pod.TailscaleContainer; tc != nil {
+			if tc := pod.TailscaleInitContainer; tc != nil {
 				if tc.Image != "" {
 					// Same validation as used by kubelet https://github.com/kubernetes/kubernetes/blob/release-1.30/pkg/kubelet/images/image_manager.go#L212
 					if _, err := dockerref.ParseNormalizedNamed(tc.Image); err != nil {
-						violations = append(violations, field.TypeInvalid(field.NewPath("spec", "statefulset", "pod", "tailscaleInitContainer", "image"), tc.Image, err.Error()))
+						violations = append(violations, field.TypeInvalid(field.NewPath("spec", "statefulSet", "pod", "tailscaleInitContainer", "image"), tc.Image, err.Error()))
 					}
 				}
 			}

diff --git a/cmd/k8s-operator/proxyclass_test.go b/cmd/k8s-operator/proxyclass_test.go
@@ -97,15 +97,35 @@ func TestProxyClass(t *testing.T) {
 		proxyClass.Spec.StatefulSet.Pod.TailscaleContainer.Image = pc.Spec.StatefulSet.Pod.TailscaleContainer.Image
 	})
 	expectReconciled(t, pcr, "", "test")
-	msg = `ProxyClass is not valid: [spec.statefulSet.pod.tailscaleContainer.image: Invalid value: "FOO bar": invalid reference format: repository name (library/FOO bar) must be lowercase, spec.statefulset.pod.tailscaleInitContainer.image: Invalid value: "FOO bar": invalid reference format: repository name (library/FOO bar) must be lowercase]`
+	msg = `ProxyClass is not valid: spec.statefulSet.pod.tailscaleContainer.image: Invalid value: "FOO bar": invalid reference format: repository name (library/FOO bar) must be lowercase`
 	tsoperator.SetProxyClassCondition(pc, tsapi.ProxyClassready, metav1.ConditionFalse, reasonProxyClassInvalid, msg, 0, cl, zl.Sugar())
 	expectEqual(t, fc, pc, nil)
-	expectedEvent = `Warning ProxyClassInvalid ProxyClass is not valid: [spec.statefulSet.pod.tailscaleContainer.image: Invalid value: "FOO bar": invalid reference format: repository name (library/FOO bar) must be lowercase, spec.statefulset.pod.tailscaleInitContainer.image: Invalid value: "FOO bar": invalid reference format: repository name (library/FOO bar) must be lowercase]`
+	expectedEvent = `Warning ProxyClassInvalid ProxyClass is not valid: spec.statefulSet.pod.tailscaleContainer.image: Invalid value: "FOO bar": invalid reference format: repository name (library/FOO bar) must be lowercase`
 	expectEvents(t, fr, []string{expectedEvent})
 
-	// 4. An valid ProxyClass but with a Tailscale env vars set results in warning events.
+	// 4. A ProxyClass resource with invalid init container image reference gets it status updated to Invalid with an error message.
+	pc.Spec.StatefulSet.Labels = nil
+	pc.Spec.StatefulSet.Pod.TailscaleContainer.Image = ""
+	pc.Spec.StatefulSet.Pod.TailscaleInitContainer = &tsapi.Container{
+		Image: "FOO bar",
+	}
+	mustUpdate(t, fc, "", "test", func(proxyClass *tsapi.ProxyClass) {
+		proxyClass.Spec.StatefulSet.Pod.TailscaleContainer.Image = pc.Spec.StatefulSet.Pod.TailscaleContainer.Image
+		proxyClass.Spec.StatefulSet.Pod.TailscaleInitContainer = &tsapi.Container{
+			Image: pc.Spec.StatefulSet.Pod.TailscaleInitContainer.Image,
+		}
+	})
+	expectReconciled(t, pcr, "", "test")
+	msg = `ProxyClass is not valid: spec.statefulSet.pod.tailscaleInitContainer.image: Invalid value: "FOO bar": invalid reference format: repository name (library/FOO bar) must be lowercase`
+	tsoperator.SetProxyClassCondition(pc, tsapi.ProxyClassready, metav1.ConditionFalse, reasonProxyClassInvalid, msg, 0, cl, zl.Sugar())
+	expectEqual(t, fc, pc, nil)
+	expectedEvent = `Warning ProxyClassInvalid ProxyClass is not valid: spec.statefulSet.pod.tailscaleInitContainer.image: Invalid value: "FOO bar": invalid reference format: repository name (library/FOO bar) must be lowercase`
+	expectEvents(t, fr, []string{expectedEvent})
+
+	// 5. An valid ProxyClass but with a Tailscale env vars set results in warning events.
+	pc.Spec.StatefulSet.Pod.TailscaleInitContainer.Image = "" // unset previous test
 	mustUpdate(t, fc, "", "test", func(proxyClass *tsapi.ProxyClass) {
-		proxyClass.Spec.StatefulSet.Pod.TailscaleContainer.Image = "" // unset invalid image from the previous test
+		proxyClass.Spec.StatefulSet.Pod.TailscaleInitContainer.Image = pc.Spec.StatefulSet.Pod.TailscaleInitContainer.Image
 		proxyClass.Spec.StatefulSet.Pod.TailscaleContainer.Env = []tsapi.Env{{Name: "TS_USERSPACE", Value: "true"}, {Name: "EXPERIMENTAL_TS_CONFIGFILE_PATH"}, {Name: "EXPERIMENTAL_ALLOW_PROXYING_CLUSTER_TRAFFIC_VIA_INGRESS"}}
 	})
 	expectedEvents := []string{"Warning CustomTSEnvVar ProxyClass overrides the default value for TS_USERSPACE env var for tailscale container. Running with custom values for Tailscale env vars is not recommended and might break in the future.",

diff --git a/k8s-operator/api.md b/k8s-operator/api.md
@@ -2450,7 +2450,7 @@ Configuration for the proxy container running tailscale.
         <td><b>image</b></td>
         <td>string</td>
         <td>
-          Container image name. By default images are pulled from docker.io/tailscale/tailscale, but the official images are also available at ghcr.io/tailscale/tailscale. Image name provided here will override any proxy image values specified via the Kubernetes operator's Helm chart values or PROXY_IMAGE env var to the operator deployment. https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#image<br/>
+          Container image name. By default images are pulled from docker.io/tailscale/tailscale, but the official images are also available at ghcr.io/tailscale/tailscale. Specifying image name here will override any proxy image values specified via the Kubernetes operator's Helm chart values or PROXY_IMAGE env var in the operator Deployment. https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#image<br/>
         </td>
         <td>false</td>
       </tr><tr>
@@ -2877,7 +2877,7 @@ Configuration for the proxy init container that enables forwarding.
         <td><b>image</b></td>
         <td>string</td>
         <td>
-          Container image name. By default images are pulled from docker.io/tailscale/tailscale, but the official images are also available at ghcr.io/tailscale/tailscale. Image name provided here will override any proxy image values specified via the Kubernetes operator's Helm chart values or PROXY_IMAGE env var to the operator deployment. https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#image<br/>
+          Container image name. By default images are pulled from docker.io/tailscale/tailscale, but the official images are also available at ghcr.io/tailscale/tailscale. Specifying image name here will override any proxy image values specified via the Kubernetes operator's Helm chart values or PROXY_IMAGE env var in the operator Deployment. https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#image<br/>
         </td>
         <td>false</td>
       </tr><tr>

diff --git a/k8s-operator/apis/v1alpha1/types_proxyclass.go b/k8s-operator/apis/v1alpha1/types_proxyclass.go
@@ -162,10 +162,10 @@ type Container struct {
 	Env []Env `json:"env,omitempty"`
 	// Container image name. By default images are pulled from
 	// docker.io/tailscale/tailscale, but the official images are also
-	// available at ghcr.io/tailscale/tailscale. Image name provided here
+	// available at ghcr.io/tailscale/tailscale. Specifying image name here
 	// will override any proxy image values specified via the Kubernetes
-	// operator's Helm chart values or PROXY_IMAGE env var to the operator
-	// deployment.
+	// operator's Helm chart values or PROXY_IMAGE env var in the operator
+	// Deployment.
 	// https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#image
 	// +optional
 	Image string `json:"image,omitempty"`