shoot webhook: Clean up obsolete mutations and add object selector (#730

)

* Drop obsolete code for the vpn-shoot Service mutation

After ReversedVPN feature is unconditionally enabled, there is no longer a Service kube-system/vpn-shoot in the Shoot cluster

* Drop obsolete code for the metrics-server Deployment mutation

After gardener/gardener#4884 it should no longer be required to have the mutation of the metrics-server Deployment.

* Add object selector to the shoot webhook

pkg/webhook/shoot/add.go

@@ -7,8 +7,8 @@ package shoot
 import (
 	extensionswebhook "github.com/gardener/gardener/extensions/pkg/webhook"
 	"github.com/gardener/gardener/extensions/pkg/webhook/shoot"
-	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 
@@ -34,9 +34,15 @@ func AddToManagerWithOptions(mgr manager.Manager, opts AddOptions) (*extensionsw
 	return shoot.New(mgr, shoot.Args{
 		Types: []extensionswebhook.Type{
 			{Obj: &corev1.Service{}},
-			{Obj: &appsv1.Deployment{}},
 		},
 		Mutator: NewMutator(&opts.Service),
+		ObjectSelector: &metav1.LabelSelector{
+			MatchLabels: map[string]string{
+				"app":       "nginx-ingress",
+				"component": "controller",
+				"release":   "addons",
+			},
+		},
 	})
 }
 

pkg/webhook/shoot/mutator.go

@@ -10,9 +10,7 @@ import (
 
 	extensionswebhook "github.com/gardener/gardener/extensions/pkg/webhook"
 	"github.com/go-logr/logr"
-	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/meta"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
@@ -34,44 +32,28 @@ func NewMutator(service *config.Service) extensionswebhook.Mutator {
 }
 
 // Mutate mutates resources.
-func (m *mutator) Mutate(ctx context.Context, new, old client.Object) error {
-	acc, err := meta.Accessor(new)
-	if err != nil {
-		return fmt.Errorf("could not create accessor during webhook: %w", err)
+func (m *mutator) Mutate(_ context.Context, new, old client.Object) error {
+	svc, ok := new.(*corev1.Service)
+	if !ok {
+		return fmt.Errorf("wrong object type %T", new)
 	}
-	// If the object does have a deletion timestamp then we don't want to mutate anything.
-	if acc.GetDeletionTimestamp() != nil {
+
+	if svc.GetDeletionTimestamp() != nil {
 		return nil
 	}
 
-	switch x := new.(type) {
-	case *corev1.Service:
-		if x.Name == "vpn-shoot" || x.Name == "addons-nginx-ingress-controller" {
-			var oldSvc *corev1.Service
-			if old != nil {
-				var ok bool
-				oldSvc, ok = old.(*corev1.Service)
-				if !ok {
-					return fmt.Errorf("could not cast old object to corev1.Service: %w", err)
-				}
-			}
-
-			extensionswebhook.LogMutation(logger, x.Kind, x.Namespace, x.Name)
-			webhookutils.MutateAnnotation(x, oldSvc, m.service.BackendLoadBalancerSpec)
-			webhookutils.MutateExternalTrafficPolicy(x, oldSvc)
-		}
-	case *appsv1.Deployment:
-		if x.Name == "metrics-server" {
-			extensionswebhook.LogMutation(logger, x.Kind, x.Namespace, x.Name)
-			m.mutateMetricsServerDeployment(ctx, x)
+	var oldSvc *corev1.Service
+	if old != nil {
+		var ok bool
+		oldSvc, ok = old.(*corev1.Service)
+		if !ok {
+			return fmt.Errorf("wrong object type %T", old)
 		}
 	}
-	return nil
-}
 
-func (m *mutator) mutateMetricsServerDeployment(_ context.Context, dep *appsv1.Deployment) {
-	ps := &dep.Spec.Template.Spec
-	if c := extensionswebhook.ContainerWithName(ps.Containers, "metrics-server"); c != nil {
-		c.Command = extensionswebhook.EnsureStringWithPrefix(c.Command, "--kubelet-preferred-address-types=", "InternalIP")
-	}
+	extensionswebhook.LogMutation(logger, svc.Kind, svc.Namespace, svc.Name)
+	webhookutils.MutateAnnotation(svc, oldSvc, m.service.BackendLoadBalancerSpec)
+	webhookutils.MutateExternalTrafficPolicy(svc, oldSvc)
+
+	return nil
 }

pkg/webhook/shoot/mutator_test.go

@@ -7,10 +7,9 @@ package shoot
 import (
 	"context"
 
-	extensionswebhook "github.com/gardener/gardener/extensions/pkg/webhook"
+	"github.com/gardener/gardener/extensions/pkg/webhook"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
-	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
@@ -19,40 +18,44 @@ import (
 
 var _ = Describe("Mutator", func() {
 	var (
-		service = &config.Service{BackendLoadBalancerSpec: "slb.s1.small"}
-
-		mutator = NewMutator(service)
-		dep     = &appsv1.Deployment{
-			ObjectMeta: metav1.ObjectMeta{Name: "metrics-server"},
-			Spec: appsv1.DeploymentSpec{
-				Template: corev1.PodTemplateSpec{
-					Spec: corev1.PodSpec{
-						Containers: []corev1.Container{
-							{
-								Name: "metrics-server",
-								Command: []string{
-									"--profiling=false",
-									"--cert-dir=/home/certdir",
-									"--secure-port=8443",
-									"--kubelet-insecure-tls",
-									"--tls-cert-file=/srv/metrics-server/tls/tls.crt",
-									"--tls-private-key-file=/srv/metrics-server/tls/tls.key",
-									"--v=2",
-								},
-							},
-						},
-					},
-				},
+		mutator         webhook.Mutator
+		serviceConfig   = &config.Service{BackendLoadBalancerSpec: "slb.s1.small"}
+		nginxIngressSvc *corev1.Service
+	)
+
+	BeforeEach(func() {
+		mutator = NewMutator(serviceConfig)
+
+		nginxIngressSvc = &corev1.Service{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "addons-nginx-ingress-controller",
+				Namespace: metav1.NamespaceSystem,
+			},
+			Spec: corev1.ServiceSpec{
+				Type:                  corev1.ServiceTypeLoadBalancer,
+				ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeCluster,
 			},
 		}
-	)
-	Describe("#MutateMetricsServerDeployment", func() {
-		It("should modify existing elements of metrics-server deployment", func() {
-			err := mutator.Mutate(context.TODO(), dep, nil)
-			c := extensionswebhook.ContainerWithName(dep.Spec.Template.Spec.Containers, "metrics-server")
-			Expect(c).To(Not(BeNil()))
-			Expect(c.Command).To(ContainElement("--kubelet-preferred-address-types=InternalIP"))
+	})
+
+	Describe("#Mutate", func() {
+		It("should set ExternalTrafficPolicy to Local", func() {
+			err := mutator.Mutate(context.TODO(), nginxIngressSvc, nil)
+
+			Expect(err).To(Not(HaveOccurred()))
+			Expect(nginxIngressSvc.Spec.ExternalTrafficPolicy).To(Equal(corev1.ServiceExternalTrafficPolicyTypeLocal))
+		})
+
+		It("should not overwrite .spec.healthCheckNodePort", func() {
+			oldNginxIngressSvc := nginxIngressSvc.DeepCopy()
+			oldNginxIngressSvc.Spec.ExternalTrafficPolicy = corev1.ServiceExternalTrafficPolicyTypeLocal
+			oldNginxIngressSvc.Spec.HealthCheckNodePort = 31280
+
+			err := mutator.Mutate(context.TODO(), nginxIngressSvc, oldNginxIngressSvc)
+
 			Expect(err).To(Not(HaveOccurred()))
+			Expect(oldNginxIngressSvc.Spec.ExternalTrafficPolicy).To(Equal(corev1.ServiceExternalTrafficPolicyTypeLocal))
+			Expect(oldNginxIngressSvc.Spec.HealthCheckNodePort).To(Equal(int32(31280)))
 		})
 	})
 })