From a1d393425777434879ca56ffb254b5c5fa880414 Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Wed, 24 Apr 2024 12:50:46 -0400 Subject: [PATCH 1/2] Allow operator to update Route spec.subdomain Before this commit, cluster-ingress-operator did not have permission to update spec.host or spec.subdomain on an existing route as the operator's serviceaccount did not have the necessary "routes/custom-host" permission. A previous change to the operator had added logic to clear spec.host and instead set spec.subdomain, but without the required permission, the update would fail with the following error message: ERROR operator.init controller/controller.go:265 Reconciler error {"controller": "canary_controller", "object": {"name":"default","namespace":"openshift-ingress-operator"}, "namespace": "openshift-ingress-operator", "name": "default", "reconcileID": "463061e3-93a1-4067-802e-03e3f1f8cdd0", "error": "failed to ensure canary route: failed to update canary route openshift-ingress-canary/canary: Route.route.openshift.io \"canary\" is invalid: spec.subdomain: Invalid value: \"canary-openshift-ingress-canary\": field is immutable"} This commit adds the needed permission to the clusterrole for the operator's serviceaccount so that the update can succeed. This commit fixes OCPBUGS-32887. https://issues.redhat.com/browse/OCPBUGS-32887 Follow-up to commit 530d326013c9169d1efee77387f33c9ec13108d4. * manifests/00-cluster-role.yaml: Add permission for routes/custom-host. --- manifests/00-cluster-role.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/manifests/00-cluster-role.yaml b/manifests/00-cluster-role.yaml index 41a0596bf9..a476d17671 100644 --- a/manifests/00-cluster-role.yaml +++ b/manifests/00-cluster-role.yaml @@ -217,6 +217,7 @@ rules: - route.openshift.io resources: - routes + - routes/custom-host verbs: - '*' From 357c1a7942e5a194687febab169cce3624f35ddc Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Wed, 26 Jun 2024 23:15:43 -0400 Subject: [PATCH 2/2] Delete & recreate canary route to clear spec.host Fix the update logic for the canary route to handle clearing spec.host. Attempts to clear spec.host using a simple update may be ignored[1]. Therefore it is necessary to delete and recreate the route. 1. https://github.com/openshift/origin/commit/54c072c122b48c5d7074fe8904d493d0534733e2 Before this commit, the operator would set spec.subdomain, but it did not actually clear spec.host, and so setting spec.subdomain had no effect. After this commit, the operator should clear spec.host, and spec.subdomain should be in effect. Follow-up to commit 77c61bab7abd7be9c7804d66149a8786f4544152. This commit is related to OCPBUGS-36465. https://issues.redhat.com/browse/OCPBUGS-36465 * pkg/operator/controller/canary/route.go (updateCanaryRoute): Delete and recreate the route in order to clear spec.host. (deleteCanaryRoute): Add an options parameter for updateCanaryRoute to use. * test/e2e/canary_test.go (TestCanaryRouteClearsSpecHost): New test. Verify that the operator clears spec.host if it is set on the canary route. * test/e2e/all_test.go (TestAll): Add TestCanaryRouteClearsSpecHost as a serial test. --- pkg/operator/controller/canary/route.go | 21 ++++++++- test/e2e/all_test.go | 1 + test/e2e/canary_test.go | 59 +++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 2 deletions(-) diff --git a/pkg/operator/controller/canary/route.go b/pkg/operator/controller/canary/route.go index da587e3037..e1bf40e99d 100644 --- a/pkg/operator/controller/canary/route.go +++ b/pkg/operator/controller/canary/route.go @@ -14,6 +14,8 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + crclient "sigs.k8s.io/controller-runtime/pkg/client" ) // ensureCanaryRoute ensures the canary route exists @@ -75,6 +77,21 @@ func (r *reconciler) updateCanaryRoute(current, desired *routev1.Route) (bool, e return false, nil } + if len(updated.Spec.Host) == 0 && len(current.Spec.Host) != 0 { + // Attempts to clear spec.host may be ignored. Thus, to clear + // spec.host, it is necessary to delete and recreate the route. + log.Info("deleting and recreating the canary route to clear spec.host", "namespace", current.Namespace, "name", current.Name, "old spec.host", current.Spec.Host) + foreground := metav1.DeletePropagationForeground + deleteOptions := crclient.DeleteOptions{PropagationPolicy: &foreground} + if _, err := r.deleteCanaryRoute(current, &deleteOptions); err != nil { + return false, err + } + if err := r.createCanaryRoute(desired); err != nil { + return false, err + } + return true, nil + } + // Diff before updating because the client may mutate the object. diff := cmp.Diff(current, updated, cmpopts.EquateEmpty()) if err := r.client.Update(context.TODO(), updated); err != nil { @@ -85,9 +102,9 @@ func (r *reconciler) updateCanaryRoute(current, desired *routev1.Route) (bool, e } // deleteCanaryRoute deletes a given route -func (r *reconciler) deleteCanaryRoute(route *routev1.Route) (bool, error) { +func (r *reconciler) deleteCanaryRoute(route *routev1.Route, options *crclient.DeleteOptions) (bool, error) { - if err := r.client.Delete(context.TODO(), route); err != nil { + if err := r.client.Delete(context.TODO(), route, options); err != nil { return false, fmt.Errorf("failed to delete canary route %s/%s: %v", route.Namespace, route.Name, err) } diff --git a/test/e2e/all_test.go b/test/e2e/all_test.go index 4f6f87d53e..2c578804d0 100644 --- a/test/e2e/all_test.go +++ b/test/e2e/all_test.go @@ -110,6 +110,7 @@ func TestAll(t *testing.T) { t.Run("TestRouterCompressionOperation", TestRouterCompressionOperation) t.Run("TestUpdateDefaultIngressControllerSecret", TestUpdateDefaultIngressControllerSecret) t.Run("TestCanaryRoute", TestCanaryRoute) + t.Run("TestCanaryRouteClearsSpecHost", TestCanaryRouteClearsSpecHost) t.Run("TestRouteHTTP2EnableAndDisableIngressConfig", TestRouteHTTP2EnableAndDisableIngressConfig) t.Run("TestRouteHardStopAfterEnableOnIngressConfig", TestRouteHardStopAfterEnableOnIngressConfig) t.Run("TestRouteHardStopAfterEnableOnIngressControllerHasPriorityOverIngressConfig", TestRouteHardStopAfterEnableOnIngressControllerHasPriorityOverIngressConfig) diff --git a/test/e2e/canary_test.go b/test/e2e/canary_test.go index ae9160577b..00a46ca863 100644 --- a/test/e2e/canary_test.go +++ b/test/e2e/canary_test.go @@ -6,6 +6,7 @@ package e2e import ( "bufio" "context" + "fmt" "strings" "testing" "time" @@ -124,6 +125,64 @@ func TestCanaryRoute(t *testing.T) { } } +// TestCanaryRouteClearsSpecHost verifies that the operator clears the canary +// route's spec.host field (which requires deleting and recreating the route) if +// spec.host gets set. +// +// This is a serial test because it modifies the canary route. +func TestCanaryRouteClearsSpecHost(t *testing.T) { + t.Log("Waiting for the default IngressController to be available...") + if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, defaultName, defaultAvailableConditions...); err != nil { + t.Fatal(err) + } + + var canaryRoute routev1.Route + canaryRouteName := controller.CanaryRouteName() + t.Log("Getting the canary route...") + if err := wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) { + if err := kclient.Get(context.TODO(), canaryRouteName, &canaryRoute); err != nil { + t.Log(err) + return false, nil + } + + return true, nil + }); err != nil { + t.Fatal(err) + } + + if v := canaryRoute.Spec.Host; len(v) != 0 { + t.Fatalf("Expected canary route to have empty spec.host, found %q.", v) + } + + const bogusHost = "foo.bar" + canaryRoute.Spec.Host = bogusHost + t.Log("Setting a bogus spec.host...") + if err := kclient.Update(context.TODO(), &canaryRoute); err != nil { + t.Fatal(err) + } + + t.Log("Waiting for the operator to clear spec.host...") + if err := wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) { + if err := kclient.Get(context.TODO(), canaryRouteName, &canaryRoute); err != nil { + t.Log(err) + return false, nil + } + + switch v := canaryRoute.Spec.Host; v { + case bogusHost: + t.Log("The operator has not yet cleared spec.host.") + return false, nil + case "": + t.Log("The operator has cleared spec.host.") + return true, nil + default: + return true, fmt.Errorf("found unexpected spec.host: %q", v) + } + }); err != nil { + t.Fatal(err) + } +} + // buildCanaryCurlPod returns a pod definition for a pod with the given name and image // and in the given namespace that curls the specified route via the route's hostname. func buildCanaryCurlPod(name, namespace, image, host string) *corev1.Pod {