WIP: OTE framework for Ingress Node Firewall and migrating a LEVEL0 usecase #694
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anuragthehatter The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughThis pull request introduces end-to-end testing infrastructure for Ingress Node Firewall. Changes include Dockerfile modifications to package test binaries, new Makefile targets for building and cleaning tests, expanded Go module dependencies (Ginkgo v2, OpenShift tests extension, Cobra), a test build system, OTE-based test registry entrypoint, Kubernetes E2E utilities and oc command wrapper, and an initial Ginkgo-based operator test suite. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@test/e2e/operator/operator.go`:
- Around line 15-16: The defer g.GinkgoRecover() call is misplaced inside the
g.Describe callback; remove it from the Describe block and either delete it
entirely or relocate it to the setup of any goroutine-starting tests (e.g.,
inside BeforeEach/It where goroutines are spawned) so that GinkgoRecover() is
deferred in the same function that starts those goroutines; search for
g.Describe and GinkgoRecover to find and update the placement accordingly.
In `@test/e2e/util.go`:
- Around line 91-92: Check for nil before dereferencing
deployment.Spec.Replicas: compute an int32 desiredReplicas := int32(1) and if
deployment.Spec.Replicas != nil set desiredReplicas = *deployment.Spec.Replicas,
then compare deployment.Status.ReadyReplicas == desiredReplicas &&
deployment.Status.UpdatedReplicas == desiredReplicas instead of directly
dereferencing deployment.Spec.Replicas; update the conditional that currently
uses deployment.Spec.Replicas to use this safe desiredReplicas value.
- Around line 52-59: CreateNamespace currently fails if the namespace already
exists; update CreateNamespace to call client.CoreV1().Namespaces().Create and,
if it returns an error, check kubernetes API error using
apierrors.IsAlreadyExists(err) and in that case return the existing namespace
via client.CoreV1().Namespaces().Get(ctx, name, metav1.GetOptions{}) with a nil
error; otherwise propagate the original error. Use the apierrors.IsAlreadyExists
helper and the CreateNamespace function name to locate where to add this
handling.
🧹 Nitpick comments (4)
test/e2e/util.go (1)
70-72: Consider tolerating transient errors during polling.Returning
false, erron Get failure stops polling immediately. ForNotFounderrors (pod not yet created), this may be premature. Consider returningfalse, nilfor transient/expected errors to allow polling to continue.Proposed approach
pod, err := client.CoreV1().Pods(namespace).Get(ctx, podName, metav1.GetOptions{}) if err != nil { + if k8serrors.IsNotFound(err) { + return false, nil // Pod not yet created, keep polling + } return false, err }test/e2e/operator/operator.go (1)
55-58: UseContainSubstringmatcher directly for cleaner assertions.More idiomatic Gomega
for _, crd := range expectedCRDs { - o.Expect(strings.Contains(crdOutput, crd)).To(o.BeTrue(), - "CRD %s should be installed", crd) + o.Expect(crdOutput).To(o.ContainSubstring(crd), + "CRD %s should be installed", crd) }test/extension/registry.go (1)
9-14: Consider if mutex is necessary for the current usage pattern.The
RWMutexprovides thread-safety, but based on the usage intest/cmd/main.go, the registry is created and populated once during startup before any concurrent access. If concurrent registration isn't a requirement, the mutex adds unnecessary complexity. However, this is fine to keep if you anticipate future concurrent usage.test/extension/cmd/commands.go (1)
43-50: Hardcoded test list will become stale.The test names are hardcoded, which means this list must be manually updated whenever tests are added or removed. Consider generating this dynamically from Ginkgo's spec tree, or documenting that this list requires manual maintenance.
test/e2e/operator/operator.go
Outdated
| var _ = g.Describe("[sig-network] INFW", func() { | ||
| defer g.GinkgoRecover() |
There was a problem hiding this comment.
Misplaced GinkgoRecover() - remove or relocate.
defer g.GinkgoRecover() inside the Describe callback executes when the setup function returns, not during test execution. It's intended for goroutines spawned during tests. This is likely ineffective here.
Proposed fix
var _ = g.Describe("[sig-network] INFW", func() {
- defer g.GinkgoRecover()
-
var (📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var _ = g.Describe("[sig-network] INFW", func() { | |
| defer g.GinkgoRecover() | |
| var _ = g.Describe("[sig-network] INFW", func() { | |
| var ( |
🤖 Prompt for AI Agents
In `@test/e2e/operator/operator.go` around lines 15 - 16, The defer
g.GinkgoRecover() call is misplaced inside the g.Describe callback; remove it
from the Describe block and either delete it entirely or relocate it to the
setup of any goroutine-starting tests (e.g., inside BeforeEach/It where
goroutines are spawned) so that GinkgoRecover() is deferred in the same function
that starts those goroutines; search for g.Describe and GinkgoRecover to find
and update the placement accordingly.
test/e2e/util.go
Outdated
| func CreateNamespace(ctx context.Context, client kubernetes.Interface, name string) (*corev1.Namespace, error) { | ||
| ns := &corev1.Namespace{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: name, | ||
| }, | ||
| } | ||
| return client.CoreV1().Namespaces().Create(ctx, ns, metav1.CreateOptions{}) | ||
| } |
There was a problem hiding this comment.
CreateNamespace should handle AlreadyExists for idempotency.
Unlike the existing namespaces.Create in test/e2e/namespaces/namespaces.go (lines 32-42), this function returns an error if the namespace already exists. This will cause test failures on reruns or when the namespace pre-exists.
Proposed fix
+import k8serrors "k8s.io/apimachinery/pkg/api/errors"
+
// CreateNamespace creates a namespace with the given name
func CreateNamespace(ctx context.Context, client kubernetes.Interface, name string) (*corev1.Namespace, error) {
ns := &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: name,
},
}
- return client.CoreV1().Namespaces().Create(ctx, ns, metav1.CreateOptions{})
+ created, err := client.CoreV1().Namespaces().Create(ctx, ns, metav1.CreateOptions{})
+ if k8serrors.IsAlreadyExists(err) {
+ return client.CoreV1().Namespaces().Get(ctx, name, metav1.GetOptions{})
+ }
+ return created, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func CreateNamespace(ctx context.Context, client kubernetes.Interface, name string) (*corev1.Namespace, error) { | |
| ns := &corev1.Namespace{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: name, | |
| }, | |
| } | |
| return client.CoreV1().Namespaces().Create(ctx, ns, metav1.CreateOptions{}) | |
| } | |
| func CreateNamespace(ctx context.Context, client kubernetes.Interface, name string) (*corev1.Namespace, error) { | |
| ns := &corev1.Namespace{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: name, | |
| }, | |
| } | |
| created, err := client.CoreV1().Namespaces().Create(ctx, ns, metav1.CreateOptions{}) | |
| if k8serrors.IsAlreadyExists(err) { | |
| return client.CoreV1().Namespaces().Get(ctx, name, metav1.GetOptions{}) | |
| } | |
| return created, err | |
| } |
🤖 Prompt for AI Agents
In `@test/e2e/util.go` around lines 52 - 59, CreateNamespace currently fails if
the namespace already exists; update CreateNamespace to call
client.CoreV1().Namespaces().Create and, if it returns an error, check
kubernetes API error using apierrors.IsAlreadyExists(err) and in that case
return the existing namespace via client.CoreV1().Namespaces().Get(ctx, name,
metav1.GetOptions{}) with a nil error; otherwise propagate the original error.
Use the apierrors.IsAlreadyExists helper and the CreateNamespace function name
to locate where to add this handling.
test/e2e/util.go
Outdated
| if deployment.Status.ReadyReplicas == *deployment.Spec.Replicas && | ||
| deployment.Status.UpdatedReplicas == *deployment.Spec.Replicas { |
There was a problem hiding this comment.
Potential nil pointer dereference on Spec.Replicas.
deployment.Spec.Replicas can be nil (defaults to 1 when unset). Dereferencing without a nil check will panic.
Proposed fix
+func int32PtrDerefOr(ptr *int32, def int32) int32 {
+ if ptr != nil {
+ return *ptr
+ }
+ return def
+}
+
// WaitForDeploymentReady waits for a deployment to be ready
func WaitForDeploymentReady(ctx context.Context, client kubernetes.Interface, namespace, deploymentName string) error {
return wait.PollUntilContextTimeout(ctx, DefaultInterval, DefaultTimeout, true, func(ctx context.Context) (bool, error) {
deployment, err := client.AppsV1().Deployments(namespace).Get(ctx, deploymentName, metav1.GetOptions{})
if err != nil {
return false, err
}
- if deployment.Status.ReadyReplicas == *deployment.Spec.Replicas &&
- deployment.Status.UpdatedReplicas == *deployment.Spec.Replicas {
+ replicas := int32PtrDerefOr(deployment.Spec.Replicas, 1)
+ if deployment.Status.ReadyReplicas == replicas &&
+ deployment.Status.UpdatedReplicas == replicas {
return true, nil
}
return false, nil
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if deployment.Status.ReadyReplicas == *deployment.Spec.Replicas && | |
| deployment.Status.UpdatedReplicas == *deployment.Spec.Replicas { | |
| func int32PtrDerefOr(ptr *int32, def int32) int32 { | |
| if ptr != nil { | |
| return *ptr | |
| } | |
| return def | |
| } | |
| // WaitForDeploymentReady waits for a deployment to be ready | |
| func WaitForDeploymentReady(ctx context.Context, client kubernetes.Interface, namespace, deploymentName string) error { | |
| return wait.PollUntilContextTimeout(ctx, DefaultInterval, DefaultTimeout, true, func(ctx context.Context) (bool, error) { | |
| deployment, err := client.AppsV1().Deployments(namespace).Get(ctx, deploymentName, metav1.GetOptions{}) | |
| if err != nil { | |
| return false, err | |
| } | |
| replicas := int32PtrDerefOr(deployment.Spec.Replicas, 1) | |
| if deployment.Status.ReadyReplicas == replicas && | |
| deployment.Status.UpdatedReplicas == replicas { | |
| return true, nil | |
| } | |
| return false, nil | |
| }) | |
| } |
🤖 Prompt for AI Agents
In `@test/e2e/util.go` around lines 91 - 92, Check for nil before dereferencing
deployment.Spec.Replicas: compute an int32 desiredReplicas := int32(1) and if
deployment.Spec.Replicas != nil set desiredReplicas = *deployment.Spec.Replicas,
then compare deployment.Status.ReadyReplicas == desiredReplicas &&
deployment.Status.UpdatedReplicas == desiredReplicas instead of directly
dereferencing deployment.Spec.Replicas; update the conditional that currently
uses deployment.Spec.Replicas to use this safe desiredReplicas value.
|
You are not using OTE framework ? but title named 'OTE' . and you are building the binary with owned options. |
|
Could you make the go vendor and go.sum .etc in one separate commit thus we can review others changes easily? thanks |
anuragthehatter
force-pushed
the
infw_ote
branch
from
c5e16b5 to
50de070
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@go.mod`:
- Line 94: Upgrade the indirect dependency golang.org/x/oauth2 in go.mod from
v0.25.0 to v0.27.0 (or newer) to remediate CVE-2025-22868; update the version
string for golang.org/x/oauth2, run go mod tidy to refresh go.sum, and re-run
your build/tests to ensure no dependency breakage (look for the
golang.org/x/oauth2 entry in go.mod and the resulting changes in go.sum).
🧹 Nitpick comments (2)
test/e2e/cli.go (1)
49-57: Consider adding--ignore-not-foundflag for cleanup resilience.The
Deletemethod may fail if the resource doesn't exist, which can cause issues during test cleanup or idempotent operations.♻️ Proposed enhancement
// Delete deletes a resource -func (c *OCClient) Delete(ctx context.Context, resourceType, name, namespace string) error { - args := []string{"delete", resourceType, name} +func (c *OCClient) Delete(ctx context.Context, resourceType, name, namespace string, ignoreNotFound bool) error { + args := []string{"delete", resourceType, name} + if ignoreNotFound { + args = append(args, "--ignore-not-found") + } if namespace != "" { args = append(args, "-n", namespace) }test/e2e/operator/operator.go (1)
71-72: Useg.By()orGinkgoWriterinstead offmt.Println.
fmt.Printlnoutput may not be captured properly by Ginkgo's test output handling. For consistency with the rest of the test, useg.By()for step logging.♻️ Proposed fix
g.By("SUCCESS - Ingress Node Firewall operator and CRDs installed") - fmt.Println("Operator install and CRDs check successful!")The
g.By()call on line 71 already logs the success message, making thefmt.Printlnredundant.
go.mod
Outdated
| golang.org/x/crypto v0.36.0 // indirect | ||
| golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 // indirect | ||
| golang.org/x/net v0.37.0 // indirect | ||
| golang.org/x/oauth2 v0.25.0 // indirect |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
golang.org/x/oauth2 v0.25.0 GO-2025-3488 security vulnerability
💡 Result:
golang.org/x/oauth2 v0.25.0 is vulnerable to GO-2025-3488 (aliases: CVE-2025-22868, GHSA-6v2p-p543-phr9). The issue is unexpected/unbounded memory consumption while parsing a malformed token, which can be triggered remotely and can lead to denial of service (availability impact). Affected code is in golang.org/x/oauth2/jws (notably Verify). [1][2][3]
Affected versions: all versions before v0.27.0 (so v0.25.0 is affected). [1][2][3]
Fixed version: v0.27.0. [1][2][3]
Remediation: upgrade:
go get golang.org/x/oauth2@v0.27.0(or later) [2][3]
🌐 Web query:
GHSA-6v2p-p543-phr9 golang.org/x/oauth2 vulnerability
💡 Result:
GHSA-6v2p-p543-phr9 is CVE-2025-22868 affecting golang.org/x/oauth2. It allows an attacker to send a malformed token that triggers unexpected / excessive memory consumption during token parsing, potentially causing denial of service (DoS). [1][2]
- Affected:
golang.org/x/oauth2versions < v0.27.0 (specifically thegolang.org/x/oauth2/jwspackage,Verifyfunction). [1][3] - Fixed: v0.27.0. [1][3]
- Remediation: upgrade dependency, e.g.
go get golang.org/x/oauth2@v0.27.0(or newer). [1][3]
Sources: [1] OSV (GO-2025-3488) [2] OSV (GHSA entry) [3] Go vuln report (pkg.go.dev/vuln/GO-2025-3488
🏁 Script executed:
# Check golang.org/x/oauth2 security advisories from OSV database
curl -s "https://api.osv.dev/v1/query" \
-H "Content-Type: application/json" \
-d '{"package": {"name": "golang.org/x/oauth2", "ecosystem": "Go"}, "version": "0.25.0"}' | jq '.'Repository: openshift/ingress-node-firewall
Length of output: 4520
Upgrade golang.org/x/oauth2 to v0.27.0 or later to address a HIGH severity denial-of-service vulnerability.
golang.org/x/oauth2 v0.25.0 is affected by GO-2025-3488 (CVE-2025-22868), which allows an attacker to trigger unexpected memory consumption during token parsing in the jws package, causing denial of service. All versions before v0.27.0 are affected. Upgrade to v0.27.0 or later.
🧰 Tools
🪛 OSV Scanner (2.3.2)
[HIGH] 94-94: golang.org/x/oauth2 0.25.0: Unexpected memory consumption during token parsing in golang.org/x/oauth2
(GO-2025-3488)
[HIGH] 94-94: golang.org/x/oauth2 0.25.0: golang.org/x/oauth2 Improper Validation of Syntactic Correctness of Input vulnerability
🤖 Prompt for AI Agents
In `@go.mod` at line 94, Upgrade the indirect dependency golang.org/x/oauth2 in
go.mod from v0.25.0 to v0.27.0 (or newer) to remediate CVE-2025-22868; update
the version string for golang.org/x/oauth2, run go mod tidy to refresh go.sum,
and re-run your build/tests to ensure no dependency breakage (look for the
golang.org/x/oauth2 entry in go.mod and the resulting changes in go.sum).
You're right. This is fixed. Thanks for reviewing that. Re-ran the usecase. It was an experiment and seems like real cimmit was missed :( |
test/e2e/cli.go
Outdated
| } | ||
|
|
||
| // Apply applies a resource from a file | ||
| func (c *OCClient) Apply(ctx context.Context, file string) error { |
anuragthehatter
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Add vendor dependencies required for the OpenShift Tests Extension (OTE) framework integration, including: - github.com/openshift-eng/openshift-tests-extension - github.com/onsi/ginkgo/v2 (OpenShift fork) - github.com/spf13/cobra for CLI support - google.golang.org/cel-go and related CEL dependencies - Additional supporting libraries Also upgrade golang.org/x/oauth2 from v0.25.0 to v0.27.0 to address CVE-2025-22868 (GO-2025-3488, GHSA-6v2p-p543-phr9), a HIGH severity denial-of-service vulnerability that allows attackers to trigger unexpected memory consumption during token parsing. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add comprehensive test infrastructure using the OpenShift Tests Extension (OTE) framework to enable standardized testing and integration with the OpenShift testing ecosystem. Changes include: - Add test/cmd/main.go as the entry point for OTE-based tests - Add test/e2e/cli.go with OCClient for executing oc commands - Add test/e2e/util.go with GetKubeconfig helper - Add test/e2e/operator/operator.go with OCP-61481 test case for operator installation verification - Add test/Makefile with targets for building and running OTE tests - Update Dockerfile.openshift to include test binary in the image - Update Makefile with test-related targets Code improvements and cleanup: - Remove misplaced defer GinkgoRecover() from Describe block - Remove unused OCClient methods (Apply, Delete) - Remove apigroup annotation from test case OCP-61481 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
anuragthehatter
force-pushed
the
infw_ote
branch
from
50de070 to
f519c43
Compare
|
@anuragthehatter: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
OTE framework for Ingress Node Firewall and migrating a LEVEL0 usecase
Once this PR merge, next steps would be
Ran a test with operator installed on a cluster