CORENET-6822: OTE framework for Ingress Node Firewall with LEVEL0 and 7 more test cases - #694
CORENET-6822: OTE framework for Ingress Node Firewall with LEVEL0 and 7 more test cases#694anuragthehatter wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds OpenShift extended e2e tests: dependency pins in go.mod, a test build system and Makefile targets, Dockerfile packaging of the gzipped test binary, a Cobra-based OTE test entrypoint, OCClient and kubeconfig utilities, and an initial Ginkgo operator installation test. ChangesExtended tests (OTE) integration
Sequence DiagramsequenceDiagram
participant DockerBuilder
participant MakeTest
participant GoCompiler
participant gzip
participant RuntimeImage
DockerBuilder->>MakeTest: build e2e tests
MakeTest->>GoCompiler: compile main.go
GoCompiler->>gzip: gzip binary
DockerBuilder->>RuntimeImage: copy gzipped binary to /usr/bin/
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ 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.
|
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 |
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.
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 :( |
anuragthehatter
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
50de070 to
f519c43
Compare
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
openshift-ci
Bot
removed
the
lifecycle/stale
ea3b966 to
31a22b5
Compare
anuragthehatter
changed the title
openshift-ci
Bot
added
the
needs-rebase
31a22b5 to
fb43da2
Compare
openshift-ci
Bot
removed
the
needs-rebase
fb43da2 to
cac59ff
Compare
|
@anuragthehatter should the folder name be "ote" instead of "otp" as decided in CNO PR? |
df006c8 to
e4452a5
Compare
@asood-rh Can we get +1 on this if no moe comments. Also there is no way to validate these cases via rehearsal unless this PR merges first and then validate CI jobs at openshift/release#79937 Current status on these tests locally ran on OTE framework ` ┌─────┬─────────┬─────────────────────────────────────────────┬────────┬──────────┐ Suite: openshift/ingress-node-firewall/aws (6 of 8 tests — 2 Baremetal-labeled tests excluded) |
|
|
||
| g.BeforeEach(func() { | ||
| ctx, cancel = context.WithTimeout(context.Background(), 10*time.Minute) | ||
| oc = e2e.NewOCClient("") |
There was a problem hiding this comment.
@anuragthehatter Is there reason of using oc instead of exutil from github.com/openshift/origin/test/extended/util?
Using oc as it is in BeforeEach the kubeconfig is set up before every test. It is fragile for various reasons.
There was a problem hiding this comment.
@asood-rh Good observation, in ovn-kubernetes PRs we do use exutil.NewCLI from openshift/origin/test/extended/util since ovn-kubernetes already imports openshift/origin in its test module. However, ingress-node-firewall doesn't have openshift/origin as a dependency, and pulling it in just for exutil.CLI would significantly bloat the dependency tree (openshift/origin brings in k8s.io/kubernetes and hundreds of transitive deps).
The e2e.OCClient is a lightweight alternative that wraps the oc binary and reads KUBECONFIG from the environment, same as how CI sets it up via the shared directory. Happy to add origin as a dep if you feel strongly, but wanted to keep the footprint minimal for a non-payload operator repo.
Or we can keep this thread open to get advise on this going. cc @jcaamano @tssurya any thoughts on this?
06fe30f to
5e93ac9
Compare
|
/lgtm |
5e93ac9 to
3b9f0ba
Compare
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anuragthehatter, asood-rh 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 |
8a62a5f to
dd78908
Compare
…ss-node-firewall Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dd78908 to
87dba4e
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. |
5 participants
Summary
[OTP][LEVEL0]tag andLifecycleBlockingDockerfile.openshift(gzipped to/usr/bin/ingress-node-firewall-tests.gz)go mod vendorruns in Dockerfile before test binary buildFiles Changed
test/cmd/main.go— OTE entry point with 4 suites (parallel, serial, slow, all),LifecycleBlockingfor all specstest/e2e/operator/operator.go— LEVEL0 test: validates operator namespace, CRDs, and deployment readinesstest/e2e/cli.go/test/e2e/util.go— OC client helper and utilitiestest/Makefile— Buildsingress-node-firewall-testsbinaryMakefile— Addsbuild-e2e-teststarget delegating totest/MakefileDockerfile.openshift— Builds and gzips test binary into the operator imagemanifests/stable/image-references— Addstestextension.redhat.io/componentandtestextension.redhat.io/binaryannotations for non-payload OTE discoverygo.mod/go.sum— OTE and ginkgo dependenciesNext Steps
Test Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests