refactor: code quality improvements and bug fixes

High priority fixes:
- Add type assertion panic protection in informer handlers
- Add webhook path duplicate validation

Code deduplication:
- Extract buildCABundlePatch helper function
- Remove redundant HookType definition in server package
- Unify errCh close behavior in HTTP servers

New features:
- Add CertSyncInterval config option (default: 1m)

Code simplification:
- Simplify determineWebhookRefs using map for seen types

Author: jimyag

internal/cabundle/syncer.go

@@ -69,13 +69,21 @@ func (s *Syncer) Start(ctx context.Context) error {
 
 	_, err := cmInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
 		AddFunc: func(obj interface{}) {
-			cm := obj.(*corev1.ConfigMap)
+			cm, ok := obj.(*corev1.ConfigMap)
+			if !ok {
+				klog.Warningf("unexpected object type in AddFunc: %T", obj)
+				return
+			}
 			if cm.Name == s.caBundleConfigMapName {
 				s.onConfigMapUpdate(ctx, cm)
 			}
 		},
 		UpdateFunc: func(oldObj, newObj interface{}) {
-			cm := newObj.(*corev1.ConfigMap)
+			cm, ok := newObj.(*corev1.ConfigMap)
+			if !ok {
+				klog.Warningf("unexpected object type in UpdateFunc: %T", newObj)
+				return
+			}
 			if cm.Name == s.caBundleConfigMapName {
 				s.onConfigMapUpdate(ctx, cm)
 			}
@@ -141,9 +149,21 @@ func (s *Syncer) patchWebhook(ctx context.Context, ref WebhookRef, caBundle []by
 	}
 }
 
+// buildCABundlePatch builds a JSON patch for updating caBundle on all webhooks.
+func buildCABundlePatch(webhookCount int, caBundle []byte) ([]byte, error) {
+	var patches []map[string]interface{}
+	for i := 0; i < webhookCount; i++ {
+		patches = append(patches, map[string]interface{}{
+			"op":    "replace",
+			"path":  fmt.Sprintf("/webhooks/%d/clientConfig/caBundle", i),
+			"value": caBundle,
+		})
+	}
+	return json.Marshal(patches)
+}
+
 // patchValidatingWebhook patches a ValidatingWebhookConfiguration.
 func (s *Syncer) patchValidatingWebhook(ctx context.Context, name string, caBundle []byte) error {
-	// Get current configuration to determine how many webhooks need patching
 	current, err := s.client.AdmissionregistrationV1().ValidatingWebhookConfigurations().Get(ctx, name, metav1.GetOptions{})
 	if err != nil {
 		if errors.IsNotFound(err) {
@@ -153,17 +173,7 @@ func (s *Syncer) patchValidatingWebhook(ctx context.Context, name string, caBund
 		return err
 	}
 
-	// Build patch for all webhooks
-	var patches []map[string]interface{}
-	for i := range current.Webhooks {
-		patches = append(patches, map[string]interface{}{
-			"op":    "replace",
-			"path":  fmt.Sprintf("/webhooks/%d/clientConfig/caBundle", i),
-			"value": caBundle,
-		})
-	}
-
-	patchBytes, err := json.Marshal(patches)
+	patchBytes, err := buildCABundlePatch(len(current.Webhooks), caBundle)
 	if err != nil {
 		return fmt.Errorf("failed to marshal patch: %w", err)
 	}
@@ -175,7 +185,6 @@ func (s *Syncer) patchValidatingWebhook(ctx context.Context, name string, caBund
 
 // patchMutatingWebhook patches a MutatingWebhookConfiguration.
 func (s *Syncer) patchMutatingWebhook(ctx context.Context, name string, caBundle []byte) error {
-	// Get current configuration to determine how many webhooks need patching
 	current, err := s.client.AdmissionregistrationV1().MutatingWebhookConfigurations().Get(ctx, name, metav1.GetOptions{})
 	if err != nil {
 		if errors.IsNotFound(err) {
@@ -185,17 +194,7 @@ func (s *Syncer) patchMutatingWebhook(ctx context.Context, name string, caBundle
 		return err
 	}
 
-	// Build patch for all webhooks
-	var patches []map[string]interface{}
-	for i := range current.Webhooks {
-		patches = append(patches, map[string]interface{}{
-			"op":    "replace",
-			"path":  fmt.Sprintf("/webhooks/%d/clientConfig/caBundle", i),
-			"value": caBundle,
-		})
-	}
-
-	patchBytes, err := json.Marshal(patches)
+	patchBytes, err := buildCABundlePatch(len(current.Webhooks), caBundle)
 	if err != nil {
 		return fmt.Errorf("failed to marshal patch: %w", err)
 	}

internal/certmanager/manager.go

@@ -49,6 +49,9 @@ type Config struct {
 
 	// CertRefresh is the refresh interval for the server certificate.
 	CertRefresh time.Duration
+
+	// SyncInterval is the interval between certificate sync checks.
+	SyncInterval time.Duration
 }
 
 // Manager handles certificate rotation using openshift/library-go.
@@ -101,7 +104,11 @@ func (m *Manager) Start(ctx context.Context) error {
 	m.configMapLister = m.informers.InformersFor(m.config.Namespace).Core().V1().ConfigMaps().Lister()
 
 	// Start the sync loop
-	ticker := time.NewTicker(time.Minute)
+	syncInterval := m.config.SyncInterval
+	if syncInterval <= 0 {
+		syncInterval = time.Minute
+	}
+	ticker := time.NewTicker(syncInterval)
 	defer ticker.Stop()
 
 	// Run immediately on start

internal/certprovider/provider.go

@@ -55,13 +55,21 @@ func (p *Provider) Start(ctx context.Context) error {
 
 	_, err := secretInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
 		AddFunc: func(obj interface{}) {
-			secret := obj.(*corev1.Secret)
+			secret, ok := obj.(*corev1.Secret)
+			if !ok {
+				klog.Warningf("unexpected object type in AddFunc: %T", obj)
+				return
+			}
 			if secret.Name == p.name {
 				p.onSecretUpdate(secret)
 			}
 		},
 		UpdateFunc: func(oldObj, newObj interface{}) {
-			secret := newObj.(*corev1.Secret)
+			secret, ok := newObj.(*corev1.Secret)
+			if !ok {
+				klog.Warningf("unexpected object type in UpdateFunc: %T", newObj)
+				return
+			}
 			if secret.Name == p.name {
 				p.onSecretUpdate(secret)
 			}

internal/metrics/server.go

@@ -58,7 +58,6 @@ func (s *Server) Start(ctx context.Context) error {
 		if err := s.server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
 			errCh <- err
 		}
-		close(errCh)
 	}()
 
 	select {

internal/server/server.go

@@ -14,9 +14,6 @@ import (
 	"github.com/jimyag/auto-cert-webhook/internal/certprovider"
 )
 
-// HookType defines the type of admission webhook.
-type HookType string
-
 // AdmitFunc is the function signature for handling admission requests.
 // This is defined here to match the public API type signature.
 type AdmitFunc = func(ar admissionv1.AdmissionReview) *admissionv1.AdmissionResponse
@@ -54,7 +51,7 @@ func New(certProvider *certprovider.Provider, config Config) *Server {
 }
 
 // RegisterHook registers a webhook handler at the given path.
-func (s *Server) RegisterHook(path string, hookType HookType, admit AdmitFunc) {
+func (s *Server) RegisterHook(path string, hookType string, admit AdmitFunc) {
 	s.mux.Handle(path, newAdmissionHandler(admit))
 	klog.V(2).Infof("Registered %s webhook at %s", hookType, path)
 }

run.go

@@ -64,13 +64,18 @@ func RunWithContext(ctx context.Context, admission Admission) error {
 	}
 
 	// Validate hooks
+	seenPaths := make(map[string]int)
 	for i, hook := range hooks {
 		if hook.Path == "" {
 			return fmt.Errorf("hook[%d]: path is required", i)
 		}
 		if hook.Path[0] != '/' {
 			return fmt.Errorf("hook[%d]: path must start with '/'", i)
 		}
+		if prev, exists := seenPaths[hook.Path]; exists {
+			return fmt.Errorf("hook[%d]: path %q already defined by hook[%d]", i, hook.Path, prev)
+		}
+		seenPaths[hook.Path] = i
 		if hook.Admit == nil {
 			return fmt.Errorf("hook[%d]: admit function is required", i)
 		}
@@ -125,7 +130,7 @@ func RunWithContext(ctx context.Context, admission Admission) error {
 
 	// Register webhook handlers
 	for _, hook := range hooks {
-		srv.RegisterHook(hook.Path, server.HookType(hook.Type), hook.Admit)
+		srv.RegisterHook(hook.Path, string(hook.Type), hook.Admit)
 		klog.Infof("Registered %s webhook at path %s", hook.Type, hook.Path)
 	}
 
@@ -163,6 +168,7 @@ func RunWithContext(ctx context.Context, admission Admission) error {
 		CARefresh:             cfg.CARefresh,
 		CertValidity:          cfg.CertValidity,
 		CertRefresh:           cfg.CertRefresh,
+		SyncInterval:          cfg.CertSyncInterval,
 	})
 
 	caBundleSyncer := cabundle.NewSyncer(client, cfg.Namespace, cfg.CABundleConfigMapName, webhookRefs)
@@ -352,25 +358,27 @@ func validateCertDurations(cfg *Config) error {
 // determineWebhookRefs determines webhook references for CA bundle syncing.
 func determineWebhookRefs(name string, hooks []Hook) []cabundle.WebhookRef {
 	var refs []cabundle.WebhookRef
-
-	hasMutating := false
-	hasValidating := false
+	seen := make(map[HookType]bool)
 
 	for _, hook := range hooks {
-		if hook.Type == Mutating && !hasMutating {
-			refs = append(refs, cabundle.WebhookRef{
-				Name: name,
-				Type: cabundle.MutatingWebhook,
-			})
-			hasMutating = true
+		if seen[hook.Type] {
+			continue
 		}
-		if hook.Type == Validating && !hasValidating {
-			refs = append(refs, cabundle.WebhookRef{
-				Name: name,
-				Type: cabundle.ValidatingWebhook,
-			})
-			hasValidating = true
+		seen[hook.Type] = true
+
+		var webhookType cabundle.WebhookType
+		switch hook.Type {
+		case Mutating:
+			webhookType = cabundle.MutatingWebhook
+		case Validating:
+			webhookType = cabundle.ValidatingWebhook
+		default:
+			continue
 		}
+		refs = append(refs, cabundle.WebhookRef{
+			Name: name,
+			Type: webhookType,
+		})
 	}
 
 	return refs

types.go

@@ -113,6 +113,10 @@ type Config struct {
 	// Env: ACW_CERT_REFRESH (e.g., "12h")
 	CertRefresh time.Duration `envconfig:"CERT_REFRESH" default:"12h"`
 
+	// CertSyncInterval is the interval between certificate sync checks.
+	// Env: ACW_CERT_SYNC_INTERVAL (e.g., "1m")
+	CertSyncInterval time.Duration `envconfig:"CERT_SYNC_INTERVAL" default:"1m"`
+
 	// LeaderElection enables leader election for certificate rotation.
 	// Env: ACW_LEADER_ELECTION
 	LeaderElection *bool `envconfig:"LEADER_ELECTION"`