[Multicluster] Fix cache options for controller Manager (#6150)

A few issues were introduced by #5843 because of changes in the sigs.k8s.io/controller-runtime interface. The biggest issue was that the call to ctrl.NewManager was not using the Options object populated earlier in the setupManagerAndCertController function. Instead it was creating and using a new, incomplete Options object. Additionally, the decoder is no longer injected automatically, it needs to be instantiated by us. Otherwise the admission webhook panics. See kubernetes-sigs/controller-runtime#2695 Fixes #6149 Signed-off-by: Antonin Bas <[email protected]>
antrea-io · Mar 27, 2024 · 3b1169c · 3b1169c
1 parent e366d58
commit 3b1169c
Show file tree

Hide file tree

Showing 11 changed files with 62 additions and 61 deletions.
diff --git a/multicluster/cmd/multicluster-controller/clusterset_webhook.go b/multicluster/cmd/multicluster-controller/clusterset_webhook.go
@@ -97,8 +97,3 @@ func (v *clusterSetValidator) Handle(ctx context.Context, req admission.Request)
 	}
 	return admission.Allowed("")
 }
-
-func (v *clusterSetValidator) InjectDecoder(d *admission.Decoder) error {
-	v.decoder = d
-	return nil
-}
diff --git a/multicluster/cmd/multicluster-controller/clusterset_webhook_test.go b/multicluster/cmd/multicluster-controller/clusterset_webhook_test.go
@@ -181,7 +181,6 @@ func TestWebhookClusterSetEvents(t *testing.T) {
 	}
 
 	decoder := admission.NewDecoder(common.TestScheme)
-
 	for _, tt := range tests {
 		objects := []client.Object{}
 		if tt.existingClusterSet != nil {
@@ -193,10 +192,10 @@ func TestWebhookClusterSetEvents(t *testing.T) {
 		fakeClient := fake.NewClientBuilder().WithScheme(common.TestScheme).WithObjects(objects...).Build()
 		clusterSetWebhookUnderTest = &clusterSetValidator{
 			Client:    fakeClient,
+			decoder:   decoder,
 			namespace: "mcs1",
 			role:      tt.role,
 		}
-		clusterSetWebhookUnderTest.InjectDecoder(decoder)
 
 		t.Run(tt.name, func(t *testing.T) {
 			response := clusterSetWebhookUnderTest.Handle(context.Background(), tt.req)

diff --git a/multicluster/cmd/multicluster-controller/controller.go b/multicluster/cmd/multicluster-controller/controller.go
@@ -25,7 +25,6 @@ import (
 	apiextensionclientset "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/fields"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	genericoptions "k8s.io/apiserver/pkg/server/options"
 	clientset "k8s.io/client-go/kubernetes"
@@ -139,6 +138,15 @@ func getWebhookLabel(isLeader bool, controllerNs string) *metav1.LabelSelector {
 func setupManagerAndCertController(isLeader bool, o *Options) (manager.Manager, error) {
 	ctrl.SetLogger(klog.NewKlogr())
 
+	podNamespace := env.GetPodNamespace()
+
+	var caConfig *certificate.CAConfig
+	if isLeader {
+		caConfig = getCaConfig(isLeader, podNamespace)
+	} else {
+		caConfig = getCaConfig(isLeader, "")
+	}
+
 	// build up cert controller to manage certificate for MC Controller
 	k8sConfig := ctrl.GetConfigOrDie()
 	k8sConfig.QPS = common.ResourceExchangeQPS
@@ -149,40 +157,48 @@ func setupManagerAndCertController(isLeader bool, o *Options) (manager.Manager,
 	}
 
 	secureServing := genericoptions.NewSecureServingOptions().WithLoopback()
-	caCertController, err := certificate.ApplyServerCert(o.SelfSignedCert, client, aggregatorClient, apiExtensionClient,
-		secureServing, getCaConfig(isLeader, o.Namespace))
+	caCertController, err := certificate.ApplyServerCert(o.SelfSignedCert, client, aggregatorClient, apiExtensionClient, secureServing, caConfig)
 	if err != nil {
 		return nil, fmt.Errorf("error applying server cert: %v", err)
 	}
 	if err := caCertController.RunOnce(context.TODO()); err != nil {
 		return nil, err
 	}
 
+	options := o.options
 	if o.SelfSignedCert {
-		o.options.Metrics.CertDir = selfSignedCertDir
+		options.Metrics.CertDir = selfSignedCertDir
 		o.WebhookConfig.CertDir = selfSignedCertDir
 	} else {
-		o.options.Metrics.CertDir = certDir
+		options.Metrics.CertDir = certDir
 		o.WebhookConfig.CertDir = certDir
 	}
-	o.options.WebhookServer = webhook.NewServer(webhook.Options{
+	options.WebhookServer = webhook.NewServer(webhook.Options{
 		Port:    *o.WebhookConfig.Port,
 		Host:    o.WebhookConfig.Host,
 		CertDir: o.WebhookConfig.CertDir,
 	})
 
-	namespaceFieldSelector := fields.SelectorFromSet(fields.Set{"metadata.namespace": env.GetPodNamespace()})
-	o.options.Cache.DefaultFieldSelector = namespaceFieldSelector
-	o.options.Cache.ByObject = map[controllerruntimeclient.Object]cache.ByObject{
-		&mcv1alpha1.Gateway{}: {
-			Field: namespaceFieldSelector,
-		},
-		&mcv1alpha2.ClusterSet{}: {
-			Field: namespaceFieldSelector,
-		},
-		&mcv1alpha1.MemberClusterAnnounce{}: {
-			Field: namespaceFieldSelector,
-		},
+	cacheOptions := &options.Cache
+	if isLeader {
+		// For the leader, restrict the cache to the controller's Namespace.
+		cacheOptions.DefaultNamespaces = map[string]cache.Config{
+			podNamespace: {},
+		}
+	} else {
+		// For a member, restict the cache to the controller's Namespace for the following objects.
+		cacheOptions.ByObject = map[controllerruntimeclient.Object]cache.ByObject{
+			&mcv1alpha1.Gateway{}: {
+				Namespaces: map[string]cache.Config{
+					podNamespace: {},
+				},
+			},
+			&mcv1alpha2.ClusterSet{}: {
+				Namespaces: map[string]cache.Config{
+					podNamespace: {},
+				},
+			},
+		}
 	}
 
 	// EndpointSlice is enabled in AntreaProxy by default since v1.11, so Antrea MC
@@ -206,11 +222,7 @@ func setupManagerAndCertController(isLeader bool, o *Options) (manager.Manager,
 	}
 	o.ClusterCalimCRDAvailable = clusterClaimCRDAvailable
 
-	mgr, err := ctrl.NewManager(k8sConfig, manager.Options{
-		Scheme:                 o.options.Scheme,
-		Metrics:                o.options.Metrics,
-		HealthProbeBindAddress: o.options.HealthProbeBindAddress,
-	})
+	mgr, err := ctrl.NewManager(k8sConfig, options)
 	if err != nil {
 		return nil, fmt.Errorf("error creating manager: %v", err)
 	}

diff --git a/multicluster/cmd/multicluster-controller/gateway_webhook.go b/multicluster/cmd/multicluster-controller/gateway_webhook.go
@@ -67,8 +67,3 @@ func (v *gatewayValidator) Handle(ctx context.Context, req admission.Request) ad
 	}
 	return admission.Allowed("")
 }
-
-func (v *gatewayValidator) InjectDecoder(d *admission.Decoder) error {
-	v.decoder = d
-	return nil
-}
diff --git a/multicluster/cmd/multicluster-controller/gateway_webhook_test.go b/multicluster/cmd/multicluster-controller/gateway_webhook_test.go
@@ -180,8 +180,9 @@ func TestWebhookGatewayEvents(t *testing.T) {
 		}
 		gatewayWebhookUnderTest = &gatewayValidator{
 			Client:    fakeClient,
-			namespace: "default"}
-		gatewayWebhookUnderTest.InjectDecoder(decoder)
+			decoder:   decoder,
+			namespace: "default",
+		}
 
 		t.Run(tt.name, func(t *testing.T) {
 			response := gatewayWebhookUnderTest.Handle(context.Background(), tt.req)

diff --git a/multicluster/cmd/multicluster-controller/leader.go b/multicluster/cmd/multicluster-controller/leader.go
@@ -21,6 +21,7 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	multiclusterv1alpha1 "antrea.io/antrea/multicluster/apis/multicluster/v1alpha1"
 	"antrea.io/antrea/multicluster/controllers/multicluster/leader"
@@ -50,9 +51,7 @@ func newLeaderCommand() *cobra.Command {
 }
 
 func runLeader(o *Options) error {
-	// on the leader we want the reconciler to run for a given Namespace instead of cluster scope
 	podNamespace := env.GetPodNamespace()
-	o.Namespace = podNamespace
 	stopCh := signals.RegisterSignalHandlers()
 
 	mgr, err := setupManagerAndCertControllerFunc(true, o)
@@ -76,14 +75,19 @@ func runLeader(o *Options) error {
 	hookServer.Register("/validate-multicluster-crd-antrea-io-v1alpha1-memberclusterannounce",
 		&webhook.Admission{Handler: &memberClusterAnnounceValidator{
 			Client:    noCachedClient,
-			namespace: podNamespace}})
+			decoder:   admission.NewDecoder(mgr.GetScheme()),
+			namespace: podNamespace,
+		}},
+	)
 
 	hookServer.Register("/validate-multicluster-crd-antrea-io-v1alpha2-clusterset",
 		&webhook.Admission{Handler: &clusterSetValidator{
 			Client:    mgr.GetClient(),
+			decoder:   admission.NewDecoder(mgr.GetScheme()),
 			namespace: env.GetPodNamespace(),
-			role:      leaderRole},
-		})
+			role:      leaderRole,
+		}},
+	)
 
 	clusterSetReconciler := leader.NewLeaderClusterSetReconciler(mgrClient, podNamespace,
 		o.ClusterCalimCRDAvailable, memberClusterStatusManager)

diff --git a/multicluster/cmd/multicluster-controller/member.go b/multicluster/cmd/multicluster-controller/member.go
@@ -22,6 +22,7 @@ import (
 	"k8s.io/klog/v2"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	"antrea.io/antrea/multicluster/controllers/multicluster/member"
 	"antrea.io/antrea/pkg/log"
@@ -62,14 +63,19 @@ func runMember(o *Options) error {
 	hookServer.Register("/validate-multicluster-crd-antrea-io-v1alpha1-gateway",
 		&webhook.Admission{Handler: &gatewayValidator{
 			Client:    mgrClient,
-			namespace: podNamespace}})
+			decoder:   admission.NewDecoder(mgr.GetScheme()),
+			namespace: podNamespace,
+		}},
+	)
 
 	hookServer.Register("/validate-multicluster-crd-antrea-io-v1alpha2-clusterset",
 		&webhook.Admission{Handler: &clusterSetValidator{
 			Client:    mgrClient,
+			decoder:   admission.NewDecoder(mgr.GetScheme()),
 			namespace: podNamespace,
-			role:      memberRole},
-		})
+			role:      memberRole,
+		}},
+	)
 
 	commonAreaCreationCh := make(chan struct{}, 1)
 	clusterSetReconciler := member.NewMemberClusterSetReconciler(mgr.GetClient(),

diff --git a/multicluster/cmd/multicluster-controller/memberclusterannounce_webhook.go b/multicluster/cmd/multicluster-controller/memberclusterannounce_webhook.go
@@ -108,8 +108,3 @@ func (v *memberClusterAnnounceValidator) Handle(ctx context.Context, req admissi
 		return admission.Allowed("")
 	}
 }
-
-func (v *memberClusterAnnounceValidator) InjectDecoder(d *admission.Decoder) error {
-	v.decoder = d
-	return nil
-}
diff --git a/multicluster/cmd/multicluster-controller/memberclusterannounce_webhook_test.go b/multicluster/cmd/multicluster-controller/memberclusterannounce_webhook_test.go
@@ -259,8 +259,9 @@ func TestMemberClusterAnnounceWebhook(t *testing.T) {
 		}
 		mcaWebhookUnderTest = &memberClusterAnnounceValidator{
 			Client:    fakeClient,
-			namespace: "mcs1"}
-		mcaWebhookUnderTest.InjectDecoder(decoder)
+			decoder:   decoder,
+			namespace: "mcs1",
+		}
 		t.Run(tt.name, func(t *testing.T) {
 			response := mcaWebhookUnderTest.Handle(context.Background(), tt.req)
 			assert.Equal(t, tt.isAllowed, response.Allowed)

diff --git a/multicluster/cmd/multicluster-controller/options.go b/multicluster/cmd/multicluster-controller/options.go
@@ -35,8 +35,8 @@ type Options struct {
 	// The path of configuration file.
 	configFile     string
 	SelfSignedCert bool
-	options        ctrl.Options
-	Namespace      string
+	// options store some base controller Manager options (initialized from the provided config).
+	options ctrl.Options
 	// The Service ClusterIP range used in the member cluster.
 	ServiceCIDR string
 	// PodCIDRs is the Pod IP address CIDRs of the member cluster.
@@ -68,14 +68,12 @@ func newOptions() *Options {
 func (o *Options) complete(args []string) error {
 	var err error
 	o.setDefaults()
-	options := ctrl.Options{Scheme: scheme}
 	ctrlConfig := &mcsv1alpha1.MultiClusterConfig{}
 	if len(o.configFile) > 0 {
 		klog.InfoS("Loading config", "file", o.configFile)
 		if err = o.loadConfigFromFile(ctrlConfig); err != nil {
 			return err
 		}
-		o.options = options
 		if ctrlConfig.ServiceCIDR != "" {
 			if _, _, err := net.ParseCIDR(ctrlConfig.ServiceCIDR); err != nil {
 				return fmt.Errorf("failed to parse serviceCIDR, invalid CIDR string %s", ctrlConfig.ServiceCIDR)

diff --git a/multicluster/cmd/multicluster-controller/options_test.go b/multicluster/cmd/multicluster-controller/options_test.go
@@ -19,7 +19,6 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
-	ctrl "sigs.k8s.io/controller-runtime"
 )
 
 func TestComplete(t *testing.T) {
@@ -34,7 +33,6 @@ func TestComplete(t *testing.T) {
 			o: Options{
 				configFile:          "./testdata/antrea-mc-config-with-valid-podcidrs.yml",
 				SelfSignedCert:      false,
-				options:             ctrl.Options{},
 				ServiceCIDR:         "",
 				PodCIDRs:            nil,
 				GatewayIPPrecedence: "",
@@ -47,7 +45,6 @@ func TestComplete(t *testing.T) {
 			o: Options{
 				configFile:          "./testdata/antrea-mc-config-with-empty-podcidrs.yml",
 				SelfSignedCert:      false,
-				options:             ctrl.Options{},
 				ServiceCIDR:         "",
 				PodCIDRs:            nil,
 				GatewayIPPrecedence: "",
@@ -60,7 +57,6 @@ func TestComplete(t *testing.T) {
 			o: Options{
 				configFile:          "./testdata/antrea-mc-config-with-invalid-podcidrs.yml",
 				SelfSignedCert:      false,
-				options:             ctrl.Options{},
 				ServiceCIDR:         "10.100.0.0/16",
 				PodCIDRs:            nil,
 				GatewayIPPrecedence: "",
@@ -73,7 +69,6 @@ func TestComplete(t *testing.T) {
 			o: Options{
 				configFile:          "./testdata/antrea-mc-config-with-invalid-endpointiptype.yml",
 				SelfSignedCert:      false,
-				options:             ctrl.Options{},
 				ServiceCIDR:         "10.100.0.0/16",
 				PodCIDRs:            nil,
 				GatewayIPPrecedence: "",