Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions test/e2e/upgrade/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ func AllTests() []upgrades.Test {
&prometheus.MetricsAvailableAfterUpgradeTest{},
&dns.UpgradeTest{},
&router.GatewayAPIUpgradeTest{},
&router.HAProxyVersionUpgradeTest{Pinned: false},
&router.HAProxyVersionUpgradeTest{Pinned: true},
}
}

Expand Down
191 changes: 191 additions & 0 deletions test/extended/router/haproxyversion_upgrade.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
package router

import (
"context"
"fmt"
"time"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/kubernetes/test/e2e/framework"
"k8s.io/kubernetes/test/e2e/upgrades"

g "github.com/onsi/ginkgo/v2"
o "github.com/onsi/gomega"
operatorv1 "github.com/openshift/api/operator/v1"
operatorv1client "github.com/openshift/client-go/operator/clientset/versioned"
exutil "github.com/openshift/origin/test/extended/util"
)

// HAProxyVersionUpgradeTest verifies if HAProxy version selection behaves
// as expected during upgrades.
// Pinned is a test parameter that should define whether the test uses a
// pinned version during upgrades, or it should upgrade leaving version unset.
type HAProxyVersionUpgradeTest struct {
Pinned bool
//
oc *exutil.CLI
operatorClient operatorv1client.Interface
controllers *ingressControllers
precheckErr error
ic types.NamespacedName
pinnedVersion operatorv1.HAProxyVersion // only used if Pinned is true
}

func (h *HAProxyVersionUpgradeTest) Name() string {
if h.Pinned {
return "haproxy-pinned-version-upgrade"
}
return "haproxy-unset-version-upgrade"
}

func (h *HAProxyVersionUpgradeTest) DisplayName() string {
if h.Pinned {
return "[sig-network-edge][Feature:Router][apigroup:route.openshift.io] Verify HAProxy pinned version state during upgrade"
}
return "[sig-network-edge][Feature:Router][apigroup:route.openshift.io] Verify HAProxy unset version state during upgrade"
}

// Skip defines if the test should be skipped. HAProxy version test is skipped
// if the HAProxy version field cannot be found in the API.
func (h *HAProxyVersionUpgradeTest) Skip(_ upgrades.UpgradeContext) bool {
oc := exutil.NewCLIForMonitorTest(h.Name() + "-skip").AsAdmin()
hasField, err := apiHasHAProxyVersionField(context.Background(), oc)
if err != nil {
h.precheckErr = fmt.Errorf("error checking for HAProxy version API: %w", err)
return false
}

h.precheckErr = nil
return !hasField
}

// Setup configures all the test attributes and creates an IngressController
// resource that should be verified after the upgrade.
func (h *HAProxyVersionUpgradeTest) Setup(ctx context.Context, f *framework.Framework) {
o.Expect(h.precheckErr).NotTo(o.HaveOccurred(), "Skip() precheck failed: could not determine if HAProxy version upgrade test should run")

g.By("Setting up HAProxy version test")

h.oc = exutil.NewCLIWithFramework(f).AsAdmin()
h.operatorClient = h.oc.AdminOperatorClient()
h.controllers = &ingressControllers{}

var customIngress func(*operatorv1.IngressController)
versions, err := getHAProxyVersionParams(ctx, h.oc)
o.Expect(err).NotTo(o.HaveOccurred(), "error getting HAProxy versions")
if h.Pinned {
customIngress = func(ic *operatorv1.IngressController) {
ic.Spec.HAProxyVersion = versions.defaultVersion
}
h.pinnedVersion = versions.defaultVersion
} else {
h.pinnedVersion = ""
}

g.By("Creating the IngressController resource")

const createControllerTimeout = 2 * time.Minute
ic, err := h.controllers.createIngressController(ctx, h.oc, createControllerTimeout, customIngress)
o.Expect(err).NotTo(o.HaveOccurred(), "error creating IngressController resource")
h.ic = types.NamespacedName{
Namespace: ic.Namespace,
Name: ic.Name,
}

g.By("Checking HAProxy version for Ingress " + ic.Name)

err = waitForHAProxyVersion(ctx, h.oc, ic.Name, versions.defaultVersion)
o.Expect(err).NotTo(o.HaveOccurred(), "error getting HAProxy version from runtime API")
}

// Test verifies if the expected HAProxy version is found after the upgrade.
// Current version is read from the IngressController status and from the
// HAProxy's runtime API.
func (h *HAProxyVersionUpgradeTest) Test(ctx context.Context, f *framework.Framework, done <-chan struct{}, upgrade upgrades.UpgradeType) {
g.By("Waiting for upgrade to complete")
<-done

g.By("Validating HAProxy version after upgrade")

var expectedVersion operatorv1.HAProxyVersion
if h.Pinned {
expectedVersion = h.pinnedVersion
} else {
versions, err := getHAProxyVersionParams(ctx, h.oc)
o.Expect(err).NotTo(o.HaveOccurred(), "error getting HAProxy versions")
expectedVersion = versions.defaultVersion
}

const rollingOutTimeout = 15 * time.Minute
err := wait.PollUntilContextTimeout(ctx, time.Second, rollingOutTimeout, true, func(ctx context.Context) (ready bool, err error) {
ic, err := h.operatorClient.OperatorV1().IngressControllers(h.ic.Namespace).Get(ctx, h.ic.Name, metav1.GetOptions{})
if err != nil {
framework.Logf("error getting IngressController resource: %s", err.Error())
return false, nil
}
if ic.Status.EffectiveHAProxyVersion != expectedVersion {
framework.Logf("HAProxy version from IngressResource status %q does not match expected value %q", ic.Status.EffectiveHAProxyVersion, expectedVersion)
return false, nil
}
return true, nil
})
o.Expect(err).NotTo(o.HaveOccurred(), "timed out waiting for EffectiveHAProxyVersion to match expected version")

g.By("Validating HAProxy version from runtime API")

err = waitForHAProxyVersion(ctx, h.oc, h.ic.Name, expectedVersion)
o.Expect(err).NotTo(o.HaveOccurred(), "error getting HAProxy version from runtime API")
}

// Teardown removes the configured IngressController after the test runs.
func (h *HAProxyVersionUpgradeTest) Teardown(ctx context.Context, f *framework.Framework) {
if h.operatorClient == nil {
framework.Logf("Skipping cleanup because setup did not initialize test resources")
return
}
if err := h.controllers.deleteAll(ctx, h.operatorClient); err != nil {
framework.Logf("error deleting IngressController resource: %s", err.Error())
}
}
Comment on lines +143 to +151

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Verify that the declared toolchain supports context.WithoutCancel.
rg -n '^(go|toolchain) ' go.mod

# Inspect both cleanup call sites.
rg -n -C 5 'deleteAll\(' test/extended/router/multi-haproxy.go test/extended/router/haproxyversion_upgrade.go

Repository: openshift/origin

Length of output: 2816


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect imports and surrounding cleanup context usage in both test files.
for f in test/extended/router/multi-haproxy.go test/extended/router/haproxyversion_upgrade.go; do
  echo "===== $f ====="
  sed -n '1,90p' "$f"
  echo
done

# Search for IngressController deletion helpers and context use around cleanup.
rg -n -C 4 'deleteAll|i\.Delete\(|OperatorV1\(\)\.IngressControllers|context\.With|context\.Background|context\.Timeout|context\.WithoutCancel' test/extended/router test -g '*.go' | head -n 200

Repository: openshift/origin

Length of output: 21383


Use bounded cleanup contexts.

A canceled test context can make deleteAll fail before it sends deletion requests. Use an uncancelable cleanup base context with a timeout before calling deleteAll in both cleanup paths.

📍 Affects 2 files
  • test/extended/router/haproxyversion_upgrade.go#L143-L151 (this comment)
  • test/extended/router/multi-haproxy.go#L55-L58
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/extended/router/haproxyversion_upgrade.go` around lines 143 - 151, The
Teardown cleanup in test/extended/router/haproxyversion_upgrade.go:143-151 and
the corresponding cleanup in test/extended/router/multi-haproxy.go:55-58 must
use an uncancelable base context with a bounded timeout when calling
controllers.deleteAll, rather than the canceled test context; preserve the
existing resource checks and error logging.

Sources: Path instructions, Learnings


// haproxyVersionParams has HAProxy version parameters from the Ingress operator.
type haproxyVersionParams struct {
defaultVersion operatorv1.HAProxyVersion
}

// getHAProxyVersionParams parses the current Ingress operator configuration
// and extracts HAProxy version parameters.
func getHAProxyVersionParams(ctx context.Context, oc *exutil.CLI) (haproxyVersionParams, error) {
operatorNamespace := "openshift-ingress-operator"
operatorName := "ingress-operator"
deploy, err := oc.AdminKubeClient().AppsV1().Deployments(operatorNamespace).Get(ctx, operatorName, metav1.GetOptions{})
if err != nil {
return haproxyVersionParams{}, err
}

containers := deploy.Spec.Template.Spec.Containers
if len(containers) < 1 {
return haproxyVersionParams{}, fmt.Errorf("ingress-operator deployment is missing the operator container")
}

operator := containers[0]
if operator.Name != "ingress-operator" {
return haproxyVersionParams{}, fmt.Errorf("ingress-operator deployment has an unexpected container name: %s", operator.Name)
}

defaultVersion := func() string {
for _, env := range operator.Env {
if env.Name == "DEFAULT_HAPROXY_VERSION" {
return env.Value
}
}
// DEFAULT_HAPROXY_VERSION envvar not found, so this is pre 4.23/5.0, assume "2.8"
return "2.8"
}()

return haproxyVersionParams{
defaultVersion: operatorv1.HAProxyVersion(defaultVersion),
}, nil
}
58 changes: 36 additions & 22 deletions test/extended/router/multi-haproxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
g "github.com/onsi/ginkgo/v2"
o "github.com/onsi/gomega"
corev1 "k8s.io/api/core/v1"
apiextensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/types"
Expand All @@ -19,7 +20,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"

operatorv1 "github.com/openshift/api/operator/v1"
apiextensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
operatorv1client "github.com/openshift/client-go/operator/clientset/versioned"

"github.com/openshift/origin/test/extended/router/shard"
exutil "github.com/openshift/origin/test/extended/util"
Expand Down Expand Up @@ -51,32 +52,14 @@ var _ = g.Describe("[sig-network-edge][Feature:Router][apigroup:route.openshift.
exutil.DumpPodLogsStartingWithInNamespace(ic.controller.Name, ic.controller.Namespace, oc)
}
}
var errs []error
for _, ic := range controllers.items {
err := operatorClient.OperatorV1().IngressControllers(ic.controller.Namespace).Delete(ctx, ic.controller.Name, *metav1.NewDeleteOptions(1))
errs = append(errs, client.IgnoreNotFound(err))
}
o.Expect(errors.Join(errs...)).NotTo(o.HaveOccurred())
err := controllers.deleteAll(ctx, operatorClient)
o.Expect(err).NotTo(o.HaveOccurred())
controllers.items = nil
})

g.BeforeEach(func() {

apiExtClient, err := apiextensionsclient.NewForConfig(oc.AdminConfig())
o.Expect(err).NotTo(o.HaveOccurred())

crd, err := apiExtClient.ApiextensionsV1().CustomResourceDefinitions().Get(ctx, "ingresscontrollers.operator.openshift.io", metav1.GetOptions{})
hasField, err := apiHasHAProxyVersionField(ctx, oc)
o.Expect(err).NotTo(o.HaveOccurred())

// Check if haproxyVersion field exists in the CRD schema
hasField := false
for _, v := range crd.Spec.Versions {
if v.Name == "v1" && v.Schema != nil && v.Schema.OpenAPIV3Schema != nil {
if _, ok := v.Schema.OpenAPIV3Schema.Properties["spec"].Properties["haproxyVersion"]; ok {
hasField = true
}
}
}
if !hasField {
g.Skip("IngressController CRD does not have haproxyVersion field — operator not yet updated")
}
Expand Down Expand Up @@ -288,6 +271,15 @@ func (i *ingressControllers) createIngressController(ctx context.Context, oc *ex
return ingress, nil
}

func (i *ingressControllers) deleteAll(ctx context.Context, operatorClient operatorv1client.Interface) error {
var errs []error
for _, ic := range i.items {
err := operatorClient.OperatorV1().IngressControllers(ic.controller.Namespace).Delete(ctx, ic.controller.Name, *metav1.NewDeleteOptions(1))
errs = append(errs, client.IgnoreNotFound(err))
}
return errors.Join(errs...)
}

// poll the router pods HAProxy Container to check that the version is correctly asserted
func waitForHAProxyVersion(ctx context.Context, oc *exutil.CLI, ingressName string, desiredVersion operatorv1.HAProxyVersion) error {
if desiredVersion == "" {
Expand All @@ -308,3 +300,25 @@ func waitForHAProxyVersion(ctx context.Context, oc *exutil.CLI, ingressName stri
})
return err
}

func apiHasHAProxyVersionField(ctx context.Context, oc *exutil.CLI) (bool, error) {
apiExtClient, err := apiextensionsclient.NewForConfig(oc.AdminConfig())
if err != nil {
return false, err
}

crd, err := apiExtClient.ApiextensionsV1().CustomResourceDefinitions().Get(ctx, "ingresscontrollers.operator.openshift.io", metav1.GetOptions{})
if err != nil {
return false, err
}

// Check if haproxyVersion field exists in the CRD schema
for _, v := range crd.Spec.Versions {
if v.Name == "v1" && v.Schema != nil && v.Schema.OpenAPIV3Schema != nil {
if _, ok := v.Schema.OpenAPIV3Schema.Properties["spec"].Properties["haproxyVersion"]; ok {
return true, nil
}
}
}
return false, nil
}