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 +`, + }, + } +}