Skip to content

OCPBUGS-36465: Delete and recreate canary route to clear spec.host #1095

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
Jul 10, 2024
Merged

OCPBUGS-36465: Delete and recreate canary route to clear spec.host #1095

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions pkg/operator/controller/canary/route.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Copy link
Contributor

@candita candita Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we also need to fix the function canaryRouteChanged to return true if the Spec.Host changed, otherwise we never get here.

/hold

Copy link
Contributor

@candita candita Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: I was mistaken on this.
/unhold

Copy link
Contributor

@candita candita Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/openshift/cluster-ingress-operator/pull/965/files hasn't been fully backported yet and I was looking in the wrong branch.

// 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
Copy link
Contributor

@candita candita Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DeletePropagationForeground keeps the key-value in the store until all the dependencies are also deleted (making sure there are no orphans). It's not perfectly clear to me why this is needed, but I don't expect it to cause any harm.

Copy link
Contributor Author

@Miciah Miciah Jul 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The godoc for the DeletePropagationForeground value is getting into the implementation details, but the godoc for the PropagationPolicy field is more to the point:

	// Whether and how garbage collection will be performed.
	// Either this field or OrphanDependents may be set, but not both.
	// The default policy is decided by the existing finalizer set in the
	// metadata.finalizers and the resource-specific default policy.
	// Acceptable values are: 'Orphan' - orphan the dependents; 'Background' -
	// allow the garbage collector to delete the dependents in the background;
	// 'Foreground' - a cascading policy that deletes all dependents in the
	// foreground.

The important thing is that deletion happens in the foreground so that the operator can immediately recreate the route.

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 {
Expand All @@ -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)
}

Expand Down
1 change: 1 addition & 0 deletions test/e2e/all_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ func TestAll(t *testing.T) {
t.Run("TestUpdateDefaultIngressControllerSecret", TestUpdateDefaultIngressControllerSecret)
t.Run("TestCanaryRoute", TestCanaryRoute)
t.Run("TestCanaryWithMTLS", TestCanaryWithMTLS)
t.Run("TestCanaryRouteClearsSpecHost", TestCanaryRouteClearsSpecHost)
t.Run("TestRouteHTTP2EnableAndDisableIngressConfig", TestRouteHTTP2EnableAndDisableIngressConfig)
t.Run("TestRouteHardStopAfterEnableOnIngressConfig", TestRouteHardStopAfterEnableOnIngressConfig)
t.Run("TestRouteHardStopAfterEnableOnIngressControllerHasPriorityOverIngressConfig", TestRouteHardStopAfterEnableOnIngressControllerHasPriorityOverIngressConfig)
Expand Down
58 changes: 58 additions & 0 deletions test/e2e/canary_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,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)
}
Copy link
Contributor

@candita candita Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to specifically call the function updateCanaryRoute to test this. In unit test. Can be completed in a followup.

Copy link
Contributor Author

@Miciah Miciah Jul 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an end-to-end test, not a unit test. All it needs to do is the following:

  1. Get the canary route.
  2. Update its spec.host field.
  3. Verify that the operator clears the field.

The fact that clearing the field requires deleting and recreating the route is irrelevant for this test. If the operator succeeds in clearing spec.host, it's good. Is there some case that this test fails to cover?


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 {
Expand Down