From 8db6228952dc458d82db8a98b8232539763c28cb Mon Sep 17 00:00:00 2001 From: Jamo Luhrsen Date: Tue, 21 Apr 2026 19:37:49 -0700 Subject: [PATCH] Watch Network and Infrastructure in proxyconfig controller The proxyconfig controller reads Network.Status.ClusterNetwork and Infrastructure.Status to compute Proxy.Status.NoProxy, but only watched Proxy and ConfigMaps. Network or Infrastructure changes would not trigger reconciliation, leaving proxy status stale. Add watches for Network and Infrastructure resources to ensure reconciliation occurs when these resources change. Refactor Reconcile so non-ConfigMap events re-resolve proxy config from the current cluster state rather than depending on the triggering request. Also add Proxy status subresource support to fake client and unit tests covering reconciliation logic. Signed-off-by: Jamo Luhrsen Co-authored-by: Claude Code --- pkg/client/fake/fake_client.go | 3 +- pkg/controller/proxyconfig/controller.go | 47 ++-- pkg/controller/proxyconfig/controller_test.go | 227 ++++++++++++++++++ 3 files changed, 258 insertions(+), 19 deletions(-) create mode 100644 pkg/controller/proxyconfig/controller_test.go diff --git a/pkg/client/fake/fake_client.go b/pkg/client/fake/fake_client.go index fdad7d229d..876eba57ab 100644 --- a/pkg/client/fake/fake_client.go +++ b/pkg/client/fake/fake_client.go @@ -100,10 +100,11 @@ func NewFakeClient(objs ...crclient.Object) cnoclient.Client { } } co := &configv1.ClusterOperator{ObjectMeta: metav1.ObjectMeta{Name: ""}} + proxy := &configv1.Proxy{ObjectMeta: metav1.ObjectMeta{Name: ""}} fc := FakeClusterClient{ kClient: faketyped.NewClientset(ooTyped...), dynclient: fakedynamic.NewSimpleDynamicClient(scheme.Scheme, oo...), - crclient: crfake.NewClientBuilder().WithStatusSubresource(co).WithObjects(objs...).Build(), + crclient: crfake.NewClientBuilder().WithStatusSubresource(co, proxy).WithObjects(objs...).Build(), osOperClient: osoperfakeclient.NewClientset(), } return &FakeClient{ diff --git a/pkg/controller/proxyconfig/controller.go b/pkg/controller/proxyconfig/controller.go index fbc9b8ecb9..9c1958a66c 100644 --- a/pkg/controller/proxyconfig/controller.go +++ b/pkg/controller/proxyconfig/controller.go @@ -79,6 +79,18 @@ func add(mgr manager.Manager, r *ReconcileProxyConfig) error { return err } + // Watch for changes to the network resource. + err = c.Watch(source.Kind[crclient.Object](mgr.GetCache(), &configv1.Network{}, &handler.EnqueueRequestForObject{})) + if err != nil { + return err + } + + // Watch for changes to the infrastructure resource. + err = c.Watch(source.Kind[crclient.Object](mgr.GetCache(), &configv1.Infrastructure{}, &handler.EnqueueRequestForObject{})) + if err != nil { + return err + } + return nil } @@ -92,32 +104,35 @@ type ReconcileProxyConfig struct { cmInformer cache.SharedIndexInformer } -// Reconcile expects request to refer to a cluster-scoped proxy object -// named "cluster" or a configmap object in namespace "openshift-config" -// and will ensure either object is in the desired state. +// Reconcile expects request to refer to a configmap object in namespace +// "openshift-config" or another watched resource indicating that the cluster +// Proxy should be reconciled from current state. func (r *ReconcileProxyConfig) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { defer utilruntime.HandleCrash(r.status.SetDegradedOnPanicAndCrash) validate := true trustBundle := &corev1.ConfigMap{} switch { - case request.NamespacedName == names.Proxy(): + case request.Namespace != names.ADDL_TRUST_BUNDLE_CONFIGMAP_NS: + // Additional trust bundles are handled below; for all other config changes, + // regardless of which object changed, we just re-resolve everything starting + // from the Proxy config. var err error proxyConfig := &configv1.Proxy{} infraConfig := &configv1.Infrastructure{} netConfig := &configv1.Network{} clusterConfig := &corev1.ConfigMap{} - log.Printf("Reconciling proxy '%s'", request.Name) - if err := r.client.Get(ctx, request.NamespacedName, proxyConfig); err != nil { + log.Printf("Reconciling proxy '%s'", names.PROXY_CONFIG) + if err := r.client.Get(ctx, names.Proxy(), proxyConfig); err != nil { if apierrors.IsNotFound(err) { // Request object not found, could have been deleted after reconcile request. // Return and don't requeue - log.Println("proxy not found; reconciliation will be skipped", "request", request) + log.Println("proxy not found; reconciliation will be skipped") return reconcile.Result{}, nil } // Error reading the object - requeue the request. - return reconcile.Result{}, fmt.Errorf("failed to get proxy '%s': %v", request.Name, err) + return reconcile.Result{}, fmt.Errorf("failed to get proxy '%s': %v", names.PROXY_CONFIG, err) } // A nil proxy is generated by upgrades and installs not requiring a proxy. @@ -125,7 +140,7 @@ func (r *ReconcileProxyConfig) Reconcile(ctx context.Context, request reconcile. !isSpecHTTPSProxySet(&proxyConfig.Spec) && !isSpecNoProxySet(&proxyConfig.Spec) { log.Printf("httpProxy, httpsProxy and noProxy not defined for proxy '%s'; validation will be skipped", - request.Name) + proxyConfig.Name) validate = false } @@ -202,14 +217,14 @@ func (r *ReconcileProxyConfig) Reconcile(ctx context.Context, request reconcile. fmt.Sprintf("Could not update proxy '%s' status: %v", proxyConfig.Name, err)) return reconcile.Result{}, fmt.Errorf("failed to sync proxy '%s': %v", names.PROXY_CONFIG, err) } - log.Printf("Reconciling proxy '%s' complete", request.Name) + log.Printf("Reconciling proxy '%s' complete", names.PROXY_CONFIG) case request.Namespace == names.ADDL_TRUST_BUNDLE_CONFIGMAP_NS: log.Printf("Reconciling additional trust bundle configmap '%s/%s'", request.Namespace, request.Name) if err := r.client.Get(ctx, request.NamespacedName, trustBundle); err != nil { if apierrors.IsNotFound(err) { // Request object not found, could have been deleted after reconcile request. // Return and don't requeue - log.Println("configmap not found; reconciliation will be skipped", "request", request) + log.Printf("configmap not found; reconciliation will be skipped: request=%v", request) return reconcile.Result{}, nil } // Error reading the object - requeue the request. @@ -251,11 +266,11 @@ func (r *ReconcileProxyConfig) Reconcile(ctx context.Context, request reconcile. if apierrors.IsNotFound(err) { // Request object not found, could have been deleted after reconcile request. // Return and don't requeue - log.Println("proxy not found; reconciliation will be skipped", "request", request) + log.Printf("proxy not found; reconciliation will be skipped: request=%v", request) return reconcile.Result{}, nil } // Error reading the object - requeue the request. - return reconcile.Result{}, fmt.Errorf("failed to get proxy '%s': %v", request.Name, err) + return reconcile.Result{}, fmt.Errorf("failed to get proxy '%s': %v", names.PROXY_CONFIG, err) } // A nil proxy is generated by upgrades and installs not requiring a proxy. @@ -263,7 +278,7 @@ func (r *ReconcileProxyConfig) Reconcile(ctx context.Context, request reconcile. !isSpecHTTPSProxySet(&proxyConfig.Spec) && !isSpecNoProxySet(&proxyConfig.Spec) { log.Printf("httpProxy, httpsProxy and noProxy not defined for proxy '%s'; validation will be skipped", - request.Name) + proxyConfig.Name) validate = false } @@ -312,10 +327,6 @@ func (r *ReconcileProxyConfig) Reconcile(ctx context.Context, request reconcile. } log.Printf("Reconciling additional trust bundle configmap '%s/%s' complete", request.Namespace, request.Name) - default: - // unknown object - log.Println("Ignoring unknown object, reconciliation will be skipped", "request", request) - return reconcile.Result{}, nil } // Make sure the trust bundle configmap is in sync with the api server. diff --git a/pkg/controller/proxyconfig/controller_test.go b/pkg/controller/proxyconfig/controller_test.go new file mode 100644 index 0000000000..23abb828cc --- /dev/null +++ b/pkg/controller/proxyconfig/controller_test.go @@ -0,0 +1,227 @@ +package proxyconfig + +import ( + "context" + "strings" + "testing" + + configv1 "github.com/openshift/api/config/v1" + "github.com/openshift/cluster-network-operator/pkg/client/fake" + "github.com/openshift/cluster-network-operator/pkg/controller/statusmanager" + "github.com/openshift/cluster-network-operator/pkg/names" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/scheme" + + crclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +func init() { + if err := configv1.Install(scheme.Scheme); err != nil { + panic(err) + } +} + +func TestReconcileUpdatesProxyStatusOnNetworkChange(t *testing.T) { + initialCIDR := "10.128.0.0/14" + expandedCIDR := "10.128.0.0/13" + proxy := proxyWithSpec(configv1.ProxySpec{ + HTTPProxy: "http://proxy.example.com:3128", + HTTPSProxy: "http://proxy.example.com:3128", + }) + network := networkWithClusterCIDR(initialCIDR) + client, reconciler := newProxyConfigReconciler(t, proxy, network, infrastructureWithAPIServer("api-int.example.com")) + + reconcileProxyConfig(t, reconciler) + proxyStatus := getProxyStatus(t, client) + if !strings.Contains(proxyStatus.NoProxy, initialCIDR) { + t.Fatalf("expected proxy.Status.NoProxy to contain %s, got: %s", initialCIDR, proxyStatus.NoProxy) + } + + network.Status.ClusterNetwork = []configv1.ClusterNetworkEntry{{CIDR: expandedCIDR}} + if err := client.Update(context.TODO(), network); err != nil { + t.Fatalf("failed to update network: %v", err) + } + reconcileRequest(t, reconciler, reconcile.Request{NamespacedName: types.NamespacedName{Name: "network-event"}}) + + proxyStatus = getProxyStatus(t, client) + if !strings.Contains(proxyStatus.NoProxy, expandedCIDR) { + t.Errorf("expected proxy.Status.NoProxy to contain expanded CIDR %s, got: %s", expandedCIDR, proxyStatus.NoProxy) + } + if strings.Contains(proxyStatus.NoProxy, initialCIDR) { + t.Errorf("proxy.Status.NoProxy still contains old CIDR %s, got: %s", initialCIDR, proxyStatus.NoProxy) + } +} + +func TestReconcileUpdatesProxyStatusOnInfrastructureChange(t *testing.T) { + initialAPIServer := "api-int.initial.example.com" + updatedAPIServer := "api-int.updated.example.com" + proxy := proxyWithSpec(configv1.ProxySpec{ + HTTPProxy: "http://proxy.example.com:3128", + HTTPSProxy: "http://proxy.example.com:3128", + }) + infra := infrastructureWithAPIServer(initialAPIServer) + client, reconciler := newProxyConfigReconciler(t, proxy, networkWithClusterCIDR("10.128.0.0/14"), infra) + + reconcileProxyConfig(t, reconciler) + proxyStatus := getProxyStatus(t, client) + if !strings.Contains(proxyStatus.NoProxy, initialAPIServer) { + t.Fatalf("expected proxy.Status.NoProxy to contain %s, got: %s", initialAPIServer, proxyStatus.NoProxy) + } + + infra.Status.APIServerInternalURL = "https://" + updatedAPIServer + ":6443" + if err := client.Update(context.TODO(), infra); err != nil { + t.Fatalf("failed to update infrastructure: %v", err) + } + reconcileRequest(t, reconciler, reconcile.Request{NamespacedName: types.NamespacedName{Name: "infrastructure-event"}}) + + proxyStatus = getProxyStatus(t, client) + if !strings.Contains(proxyStatus.NoProxy, updatedAPIServer) { + t.Errorf("expected proxy.Status.NoProxy to contain updated API server %s, got: %s", updatedAPIServer, proxyStatus.NoProxy) + } + if strings.Contains(proxyStatus.NoProxy, initialAPIServer) { + t.Errorf("proxy.Status.NoProxy still contains old API server %s, got: %s", initialAPIServer, proxyStatus.NoProxy) + } +} + +func TestReconcileWithNoProxy(t *testing.T) { + client, reconciler := newProxyConfigReconciler( + t, + proxyWithSpec(configv1.ProxySpec{}), + networkWithClusterCIDR("10.128.0.0/14"), + infrastructureWithAPIServer("api-int.example.com"), + ) + + reconcileProxyConfig(t, reconciler) + proxyStatus := getProxyStatus(t, client) + if proxyStatus.NoProxy != "" { + t.Errorf("expected empty proxy.Status.NoProxy when no proxy is configured, got: %s", proxyStatus.NoProxy) + } +} + +func TestReconcileHandlesMissingResources(t *testing.T) { + tests := []struct { + name string + objects []crclient.Object + errorContains string + }{ + { + name: "missing network resource", + objects: []crclient.Object{ + proxyWithSpec(configv1.ProxySpec{HTTPProxy: "http://proxy.example.com:3128"}), + infrastructureWithAPIServer("api-int.example.com"), + }, + errorContains: "not found", + }, + { + name: "missing infrastructure resource", + objects: []crclient.Object{ + proxyWithSpec(configv1.ProxySpec{HTTPProxy: "http://proxy.example.com:3128"}), + networkWithClusterCIDR("10.128.0.0/14"), + }, + errorContains: "not found", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fakeClient := fake.NewFakeClient(tt.objects...) + reconciler := &ReconcileProxyConfig{ + client: fakeClient.Default().CRClient(), + status: statusmanager.New(fakeClient, "network", names.StandAloneClusterName), + } + + _, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: names.Proxy()}) + if err == nil { + t.Fatalf("expected error containing %q, got nil", tt.errorContains) + } + if !strings.Contains(err.Error(), tt.errorContains) { + t.Fatalf("expected error containing %q, got: %v", tt.errorContains, err) + } + }) + } +} + +func newProxyConfigReconciler(t *testing.T, objects ...crclient.Object) (crclient.Client, *ReconcileProxyConfig) { + t.Helper() + objects = append(objects, clusterConfigMap()) + fakeClient := fake.NewFakeClient(objects...) + return fakeClient.Default().CRClient(), &ReconcileProxyConfig{ + client: fakeClient.Default().CRClient(), + status: statusmanager.New(fakeClient, "network", names.StandAloneClusterName), + } +} + +func reconcileProxyConfig(t *testing.T, reconciler *ReconcileProxyConfig) { + t.Helper() + reconcileRequest(t, reconciler, reconcile.Request{NamespacedName: names.Proxy()}) +} + +func reconcileRequest(t *testing.T, reconciler *ReconcileProxyConfig, request reconcile.Request) { + t.Helper() + _, err := reconciler.Reconcile(context.TODO(), request) + if err != nil { + t.Fatalf("reconcile failed: %v", err) + } +} + +func getProxyStatus(t *testing.T, client crclient.Client) configv1.ProxyStatus { + t.Helper() + proxy := &configv1.Proxy{} + if err := client.Get(context.TODO(), names.Proxy(), proxy); err != nil { + t.Fatalf("failed to get proxy: %v", err) + } + return proxy.Status +} + +func proxyWithSpec(spec configv1.ProxySpec) *configv1.Proxy { + return &configv1.Proxy{ + ObjectMeta: metav1.ObjectMeta{Name: names.PROXY_CONFIG}, + Spec: spec, + } +} + +func networkWithClusterCIDR(cidr string) *configv1.Network { + return &configv1.Network{ + ObjectMeta: metav1.ObjectMeta{Name: names.CLUSTER_CONFIG}, + Status: configv1.NetworkStatus{ + ClusterNetwork: []configv1.ClusterNetworkEntry{{CIDR: cidr}}, + ServiceNetwork: []string{"172.30.0.0/16"}, + }, + } +} + +func infrastructureWithAPIServer(host string) *configv1.Infrastructure { + return &configv1.Infrastructure{ + ObjectMeta: metav1.ObjectMeta{Name: names.CLUSTER_CONFIG}, + Status: configv1.InfrastructureStatus{ + APIServerInternalURL: "https://" + host + ":6443", + PlatformStatus: &configv1.PlatformStatus{ + Type: configv1.AWSPlatformType, + AWS: &configv1.AWSPlatformStatus{ + Region: "us-east-1", + }, + }, + }, + } +} + +func clusterConfigMap() *corev1.ConfigMap { + return &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-config-v1", + Namespace: "kube-system", + }, + Data: map[string]string{ + "install-config": ` +controlPlane: + replicas: "3" +networking: + machineCIDR: 10.0.0.0/16 +`, + }, + } +}