From 300fe72ff0e5d6ea442481bc5ec41faab8ebbb13 Mon Sep 17 00:00:00 2001 From: irbekrm Date: Tue, 25 Apr 2023 12:12:00 +0100 Subject: [PATCH] Code review Signed-off-by: irbekrm --- cmd/controller/app/controller.go | 2 +- pkg/controller/acmechallenges/controller.go | 4 ++-- pkg/controller/context.go | 24 ++++++++++----------- pkg/controller/test/context_builder.go | 6 +++--- pkg/issuer/acme/http/http.go | 4 ++-- pkg/issuer/acme/http/pod_test.go | 4 ++-- pkg/issuer/acme/http/service.go | 1 - pkg/issuer/acme/http/service_test.go | 4 ++-- 8 files changed, 24 insertions(+), 25 deletions(-) diff --git a/cmd/controller/app/controller.go b/cmd/controller/app/controller.go index 58d27c8e7..325726861 100644 --- a/cmd/controller/app/controller.go +++ b/cmd/controller/app/controller.go @@ -211,7 +211,7 @@ func Run(opts *options.ControllerOptions, stopCh <-chan struct{}) error { log.V(logf.DebugLevel).Info("starting shared informer factories") ctx.SharedInformerFactory.Start(rootCtx.Done()) ctx.KubeSharedInformerFactory.Start(rootCtx.Done()) - ctx.MetadataInformerFactory.Start(rootCtx.Done()) + ctx.HTTP01ResourceMetadataInformersFactory.Start(rootCtx.Done()) if utilfeature.DefaultFeatureGate.Enabled(feature.ExperimentalGatewayAPISupport) { ctx.GWShared.Start(rootCtx.Done()) diff --git a/pkg/controller/acmechallenges/controller.go b/pkg/controller/acmechallenges/controller.go index 735f2c04f..5a9d82c2b 100644 --- a/pkg/controller/acmechallenges/controller.go +++ b/pkg/controller/acmechallenges/controller.go @@ -94,8 +94,8 @@ func (c *controller) Register(ctx *controllerpkg.Context) (workqueue.RateLimitin secretInformer := ctx.KubeSharedInformerFactory.Secrets() // we register these informers here so the HTTP01 solver has a synced // cache when managing pod/service/ingress resources - podInformer := ctx.MetadataInformerFactory.ForResource(corev1.SchemeGroupVersion.WithResource("pods")) - serviceInformer := ctx.MetadataInformerFactory.ForResource(corev1.SchemeGroupVersion.WithResource("services")) + podInformer := ctx.HTTP01ResourceMetadataInformersFactory.ForResource(corev1.SchemeGroupVersion.WithResource("pods")) + serviceInformer := ctx.HTTP01ResourceMetadataInformersFactory.ForResource(corev1.SchemeGroupVersion.WithResource("services")) ingressInformer := ctx.KubeSharedInformerFactory.Ingresses() // build a list of InformerSynced functions that will be returned by the Register method. diff --git a/pkg/controller/context.go b/pkg/controller/context.go index 98f6981ae..355b38636 100644 --- a/pkg/controller/context.go +++ b/pkg/controller/context.go @@ -105,9 +105,9 @@ type Context struct { // instances for cert-manager.io types SharedInformerFactory informers.SharedInformerFactory - // MetadataInformerFactory can be used to start partial metadata - // informers - MetadataInformerFactory metadatainformer.SharedInformerFactory + // HTTP01ResourceMetadataInformersFactory is a metadata only informers + // factory with a http-01 resource label filter selector + HTTP01ResourceMetadataInformersFactory metadatainformer.SharedInformerFactory // GWShared can be used to obtain SharedIndexInformer instances for // gateway.networking.k8s.io types @@ -289,7 +289,7 @@ func NewContextFactory(ctx context.Context, opts ContextOptions) (*ContextFactor panic(fmt.Errorf("internal error: failed to build label selector to filter HTTP-01 challenge resources: %w", err)) } isHTTP01ChallengeResourceLabelSelector := labels.NewSelector().Add(*r) - metadataInformerFactory := metadatainformer.NewFilteredSharedInformerFactory(clients.metadataOnlyClient, resyncPeriod, opts.Namespace, func(listOptions *metav1.ListOptions) { + http01ResourceMetadataInformerFactory := metadatainformer.NewFilteredSharedInformerFactory(clients.metadataOnlyClient, resyncPeriod, opts.Namespace, func(listOptions *metav1.ListOptions) { // metadataInformersFactory is at the moment only used for pods // and services for http-01 challenge which can be identified by // the same label keys, so it is okay to set the label selector @@ -305,14 +305,14 @@ func NewContextFactory(ctx context.Context, opts ContextOptions) (*ContextFactor baseRestConfig: restConfig, log: logf.FromContext(ctx), ctx: &Context{ - RootContext: ctx, - StopCh: ctx.Done(), - KubeSharedInformerFactory: kubeSharedInformerFactory, - SharedInformerFactory: sharedInformerFactory, - GWShared: gwSharedInformerFactory, - GatewaySolverEnabled: clients.gatewayAvailable, - MetadataInformerFactory: metadataInformerFactory, - ContextOptions: opts, + RootContext: ctx, + StopCh: ctx.Done(), + KubeSharedInformerFactory: kubeSharedInformerFactory, + SharedInformerFactory: sharedInformerFactory, + GWShared: gwSharedInformerFactory, + GatewaySolverEnabled: clients.gatewayAvailable, + HTTP01ResourceMetadataInformersFactory: http01ResourceMetadataInformerFactory, + ContextOptions: opts, }, }, nil } diff --git a/pkg/controller/test/context_builder.go b/pkg/controller/test/context_builder.go index 8c4f28d77..10228c89e 100644 --- a/pkg/controller/test/context_builder.go +++ b/pkg/controller/test/context_builder.go @@ -151,7 +151,7 @@ func (b *Builder) Init() { b.KubeSharedInformerFactory = internalinformers.NewBaseKubeInformerFactory(b.Client, informerResyncPeriod, "") b.SharedInformerFactory = informers.NewSharedInformerFactory(b.CMClient, informerResyncPeriod) b.GWShared = gwinformers.NewSharedInformerFactory(b.GWClient, informerResyncPeriod) - b.MetadataInformerFactory = metadatainformer.NewFilteredSharedInformerFactory(b.MetadataClient, informerResyncPeriod, "", func(listOptions *metav1.ListOptions) {}) + b.HTTP01ResourceMetadataInformersFactory = metadatainformer.NewFilteredSharedInformerFactory(b.MetadataClient, informerResyncPeriod, "", func(listOptions *metav1.ListOptions) {}) b.stopCh = make(chan struct{}) b.Metrics = metrics.New(logs.Log, clock.RealClock{}) @@ -322,7 +322,7 @@ func (b *Builder) Start() { b.KubeSharedInformerFactory.Start(b.stopCh) b.SharedInformerFactory.Start(b.stopCh) b.GWShared.Start(b.stopCh) - b.MetadataInformerFactory.Start(b.stopCh) + b.HTTP01ResourceMetadataInformersFactory.Start(b.stopCh) // wait for caches to sync b.Sync() @@ -338,7 +338,7 @@ func (b *Builder) Sync() { if err := mustAllSync(b.GWShared.WaitForCacheSync(b.stopCh)); err != nil { panic("Error waiting for GWShared to sync: " + err.Error()) } - if err := mustAllSyncGVR(b.MetadataInformerFactory.WaitForCacheSync(b.stopCh)); err != nil { + if err := mustAllSyncGVR(b.HTTP01ResourceMetadataInformersFactory.WaitForCacheSync(b.stopCh)); err != nil { panic("Error waiting for MetadataInformerFactory to sync:" + err.Error()) } if b.additionalSyncFuncs != nil { diff --git a/pkg/issuer/acme/http/http.go b/pkg/issuer/acme/http/http.go index 6ac1a19a8..6173d5714 100644 --- a/pkg/issuer/acme/http/http.go +++ b/pkg/issuer/acme/http/http.go @@ -74,8 +74,8 @@ type reachabilityTest func(ctx context.Context, url *url.URL, key string, dnsSer func NewSolver(ctx *controller.Context) (*Solver, error) { return &Solver{ Context: ctx, - podLister: ctx.MetadataInformerFactory.ForResource(corev1.SchemeGroupVersion.WithResource("pods")).Lister(), - serviceLister: ctx.MetadataInformerFactory.ForResource(corev1.SchemeGroupVersion.WithResource("services")).Lister(), + podLister: ctx.HTTP01ResourceMetadataInformersFactory.ForResource(corev1.SchemeGroupVersion.WithResource("pods")).Lister(), + serviceLister: ctx.HTTP01ResourceMetadataInformersFactory.ForResource(corev1.SchemeGroupVersion.WithResource("services")).Lister(), ingressLister: ctx.KubeSharedInformerFactory.Ingresses().Lister(), httpRouteLister: ctx.GWShared.Gateway().V1beta1().HTTPRoutes().Lister(), testReachability: testReachability, diff --git a/pkg/issuer/acme/http/pod_test.go b/pkg/issuer/acme/http/pod_test.go index bc7f7e04e..9bab7b8ca 100644 --- a/pkg/issuer/acme/http/pod_test.go +++ b/pkg/issuer/acme/http/pod_test.go @@ -163,7 +163,7 @@ func TestEnsurePod(t *testing.T) { scenario.builder.InitWithRESTConfig() s := &Solver{ Context: scenario.builder.Context, - podLister: scenario.builder.MetadataInformerFactory.ForResource(corev1.SchemeGroupVersion.WithResource("pods")).Lister(), + podLister: scenario.builder.HTTP01ResourceMetadataInformersFactory.ForResource(corev1.SchemeGroupVersion.WithResource("pods")).Lister(), } s.Context.ACMEOptions = controller.ACMEOptions{ HTTP01SolverResourceRequestCPU: cpuRequest, @@ -254,7 +254,7 @@ func TestGetPodsForChallenge(t *testing.T) { scenario.builder.InitWithRESTConfig() s := &Solver{ Context: scenario.builder.Context, - podLister: scenario.builder.MetadataInformerFactory.ForResource(corev1.SchemeGroupVersion.WithResource("pods")).Lister(), + podLister: scenario.builder.HTTP01ResourceMetadataInformersFactory.ForResource(corev1.SchemeGroupVersion.WithResource("pods")).Lister(), } defer scenario.builder.Stop() scenario.builder.Start() diff --git a/pkg/issuer/acme/http/service.go b/pkg/issuer/acme/http/service.go index 6ef54e0c5..563cb45d5 100644 --- a/pkg/issuer/acme/http/service.go +++ b/pkg/issuer/acme/http/service.go @@ -81,7 +81,6 @@ func (s *Solver) getServicesForChallenge(ctx context.Context, ch *cmacme.Challen var relevantServices []*metav1.PartialObjectMetadata for _, service := range serviceList { - // TODO: can we use a metadata specific lister instead? s, ok := service.(*metav1.PartialObjectMetadata) if !ok { return nil, fmt.Errorf("internal error: cannot cast Service PartialObjectMetadata") diff --git a/pkg/issuer/acme/http/service_test.go b/pkg/issuer/acme/http/service_test.go index 4668bdcfd..0fc2d61e4 100644 --- a/pkg/issuer/acme/http/service_test.go +++ b/pkg/issuer/acme/http/service_test.go @@ -145,7 +145,7 @@ func TestEnsureService(t *testing.T) { scenario.builder.InitWithRESTConfig() s := &Solver{ Context: scenario.builder.Context, - serviceLister: scenario.builder.MetadataInformerFactory.ForResource(corev1.SchemeGroupVersion.WithResource("services")).Lister(), + serviceLister: scenario.builder.HTTP01ResourceMetadataInformersFactory.ForResource(corev1.SchemeGroupVersion.WithResource("services")).Lister(), } scenario.builder.Start() defer scenario.builder.Stop() @@ -229,7 +229,7 @@ func TestGetServicesForChallenge(t *testing.T) { scenario.builder.InitWithRESTConfig() s := &Solver{ Context: scenario.builder.Context, - serviceLister: scenario.builder.MetadataInformerFactory.ForResource(corev1.SchemeGroupVersion.WithResource("services")).Lister(), + serviceLister: scenario.builder.HTTP01ResourceMetadataInformersFactory.ForResource(corev1.SchemeGroupVersion.WithResource("services")).Lister(), } scenario.builder.Start() defer scenario.builder.Stop()