OSAC-3060: remove kustomize + fix integration tests + migrate to kind-dev - #389
OSAC-3060: remove kustomize + fix integration tests + migrate to kind-dev#389omer-vishlitzky wants to merge 3 commits into
Conversation
|
@omer-vishlitzky: This pull request references OSAC-3060 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: omer-vishlitzky 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 |
WalkthroughThe PR replaces Kustomize-based installation and integration setup with Helm charts, updates CI workflows and log collection, removes legacy console-proxy and Kustomize manifests, and strengthens integration-suite readiness and resource-deletion handling. ChangesIntegration Test Overhaul
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant Ginkgo
participant Helm
participant Kubernetes
CI->>Ginkgo: run integration-tests
Ginkgo->>Helm: install charts
Helm->>Kubernetes: create CRDs and operator
Ginkgo->>Kubernetes: verify pod readiness
Ginkgo->>Kubernetes: clear finalizers and verify deletion
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2e8a501 to
0ce27ea
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with 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.
Inline comments:
In `@go.mod`:
- Around line 22-25: Update the k8s.io/apiserver dependency in go.mod from
v0.36.2 to v0.36.3, keeping it aligned with the existing k8s.io/api,
k8s.io/apimachinery, and k8s.io/client-go versions.
In `@Makefile`:
- Around line 160-163: Update the test-integration-kind and integration-tests
Make targets to invoke Ginkgo through the Go module-pinned runner, using go run
github.com/onsi/ginkgo/v2/ginkgo or an equivalent GINKGO tool target, so
execution matches the v2.32.0 dependency in go.mod instead of relying on a
global PATH binary.
In `@test/integration/console_proxy_test.go`:
- Around line 96-121: Make cleanup for test-ci-no-vm unconditional by
registering DeferCleanup immediately after successful creation. In the cleanup
callback, delete the ComputeInstance, remove its finalizers as needed, and poll
until the resource is confirmed absent; do not discard deletion failures or rely
on reaching the later assertions. Preserve the existing resource name and
operatorNamespace used by the test.
In `@test/integration/integration_suite_test.go`:
- Around line 66-215: Replace the shared context.Background flow in the
integration setup with targeted context.WithTimeout contexts for cluster.Start,
InstallCertManager, InstallTrustManager, InstallCa, each command Execute call,
and LoadArchive. Use operation-appropriate deadlines and ensure each context is
canceled, so hung podman, helm, kubectl, or cluster operations fail promptly
with an attributable timeout while preserving the existing readiness Eventually
timeout.
- Around line 186-215: Adjust the restart validation inside the
controller-manager readiness Eventually block so a single transient historical
restart does not permanently fail setup, while genuine ongoing crash loops still
fail. Track restart counts across polls and fail only when they increase during
the observation window, or apply an equivalent one-restart tolerance; preserve
the existing pod-running, container-ready, and error-reporting checks.
- Around line 152-164: Replace the fixed imageTar path in the operator image
save/load flow with a unique temporary file created via os.CreateTemp, matching
the kubeconfigFile pattern. Close the created file before podman writes the
archive, use its generated path for saveCmd and cluster.LoadArchive, and retain
cleanup with defer.
In `@test/integration/networking_test.go`:
- Around line 242-272: Update the deletion polling closures for the subnet and
VirtualNetwork checks to run kubectl get with --ignore-not-found -o name, return
any unexpected command error, and report success only when the command output is
empty. Do not treat arbitrary kubectl failures as evidence of deletion.
- Around line 34-39: Update removeFinalizers to execute kubectl through
exec.CommandContext with an explicit timeout, and stop discarding the error
returned by utils.Run. Return or propagate that error to removeAllFinalizers so
cleanup can distinguish successful patches, expected missing resources, and
genuine patch failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6fbb6d75-0d45-44e6-b860-be6d85413a16
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
.gitignoreMakefilego.modtest/integration/console_proxy_test.gotest/integration/integration_suite_test.gotest/integration/networking_test.go
| # Run integration tests (creates its own Kind cluster via itenv). | ||
| .PHONY: test-integration-kind integration-tests | ||
| test-integration-kind integration-tests: manifests generate | ||
| ginkgo run --timeout 30m -v test/integration |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does the ginkgo CLI need to match the exact version of github.com/onsi/ginkgo/v2 declared in go.mod?
💡 Result:
Yes, the Ginkgo CLI must match the exact version of the Ginkgo library declared in your go.mod file [1][2][3]. Ginkgo does not maintain a formal compatibility contract between different versions of the CLI and the library [1][4]. Because the CLI and library are packaged together in the same repository, they are intended to be used at identical versions [1][2]. Mismatches between the CLI and the library version can lead to significant issues, such as: * Flag parsing errors, where the CLI passes flags that are not recognized by the version of the Ginkgo library in your project [1][5][6]. * Test execution failures or unexpected behavior in CI/CD pipelines [1][6][7]. To ensure the versions match, it is recommended to invoke the CLI using the following command from within a directory containing your go.mod file [2][6][3]: go run github.com/onsi/ginkgo/v2/ginkgo Alternatively, if you prefer to install the binary, run this command from the same directory [3][8]: go install github.com/onsi/ginkgo/v2/ginkgo The Ginkgo CLI includes a mechanism to detect these version mismatches and will emit a warning or error message if it detects that the installed CLI version does not match the imported library version [2][6].
Citations:
- 1: ginkgo v2.3.0 is failing all CI builds (and local ones as well) onsi/ginkgo#1050
- 2: https://github.com/onsi/ginkgo/blob/master/CHANGELOG.md
- 3: https://onsi.github.io/ginkgo/
- 4: Release Binaries onsi/ginkgo#1300
- 5: v2.3.0: flag provided but not defined: -ginkgo.grace-period onsi/ginkgo#1055
- 6: CI: Version mismatch between Ginkgo CLI and imported Ginkgo package in e2e test apache/apisix-ingress-controller#1799
- 7: feat/fix: pin ginkgo CLI version to go.mod dependency volcano-sh/volcano#5065
- 8: Please update the documentation to include clearer instructions on upgrading the ginkgo cli onsi/ginkgo#1129
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Makefile tool targets around GINKGO/KIND/etc =="
rg -n '^\s*(KIND|KUBECTL|GINKGO|ginkgo|manifests|generate|go run github.com/onsi/ginkgo/v2)' Makefile || true
echo
echo "== Relevant Makefile section =="
sed -n '130,180p' Makefile
echo
echo "== go.mod ginkgo entries =="
if [ -f go.mod ]; then rg -n 'onsi/ginkgo|ginkgo' go.mod go.sum; fiRepository: osac-project/osac-operator
Length of output: 3543
Invoke Ginkgo through an explicitly pinned runner.
ginkgo run depends on whatever global ginkgo CLI is on PATH, while Ginkgo CLI must match the github.com/onsi/ginkgo/v2 version declared as v2.32.0 in go.mod. Run it via go run github.com/onsi/ginkgo/v2/ginkgo or add a $(GINKGO) tool-binary target so local and CI runs use the module-pinned version.
🤖 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 `@Makefile` around lines 160 - 163, Update the test-integration-kind and
integration-tests Make targets to invoke Ginkgo through the Go module-pinned
runner, using go run github.com/onsi/ginkgo/v2/ginkgo or an equivalent GINKGO
tool target, so execution matches the v2.32.0 dependency in go.mod instead of
relying on a global PATH binary.
| cmd.Stdin = createComputeInstanceYAML("test-ci-no-vm", operatorNamespace) | ||
| _, err := utils.Run(cmd) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| By("attempting console access") | ||
| cmd = exec.Command("kubectl", "get", "--raw", | ||
| fmt.Sprintf("/apis/console.osac.openshift.io/v1alpha1/namespaces/%s/computeinstances/test-ci-no-vm/console", | ||
| consoleProxyNamespace), | ||
| operatorNamespace), | ||
| ) | ||
| _, err = utils.Run(cmd) | ||
| Expect(err).To(HaveOccurred()) | ||
|
|
||
| By("attempting VNC access") | ||
| cmd = exec.Command("kubectl", "get", "--raw", | ||
| fmt.Sprintf("/apis/console.osac.openshift.io/v1alpha1/namespaces/%s/computeinstances/test-ci-no-vm/vnc", | ||
| consoleProxyNamespace), | ||
| operatorNamespace), | ||
| ) | ||
| _, err = utils.Run(cmd) | ||
| Expect(err).To(HaveOccurred()) | ||
|
|
||
| By("cleaning up the ComputeInstance") | ||
| removeFinalizers("computeinstance", "test-ci-no-vm", operatorNamespace) | ||
| cmd = exec.Command("kubectl", "delete", "computeinstance", "test-ci-no-vm", | ||
| "-n", consoleProxyNamespace, "--ignore-not-found") | ||
| "-n", operatorNamespace, "--wait=false", "--ignore-not-found") | ||
| _, _ = utils.Run(cmd) | ||
| removeFinalizers("computeinstance", "test-ci-no-vm", operatorNamespace) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'console_proxy_test.go' . || true
echo "== surrounding test context =="
file="$(fd 'console_proxy_test.go' . | head -n1 || true)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '1,180p' "$file" | cat -n
fi
echo "== similar cleanup patterns =="
rg -n "DeferCleanup|removeFinalizers|kubectl delete .*--wait=false|ComputeInstance|computeinstance" test/integration -S || trueRepository: osac-project/osac-operator
Length of output: 7989
Make ComputeInstance cleanup unconditional and verify deletion.
Cleanup is reached only after the subresource assertions succeed, the delete output/error is discarded, and the presence/absence of test-ci-no-vm is never checked. Register DeferCleanup after creation, then delete, remove finalizers, and poll until the ComputeInstance is absent.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 100-103: An argument to exec.Command/exec.CommandContext is built with fmt.Sprintf that interpolates a non-constant value. When the command is run through a shell (e.g. sh -c) this allows command injection. Pass untrusted input as a separate, explicit argument to exec.Command (which does not invoke a shell) instead of formatting it into the command string, and validate or allowlist any values that must be embedded.
Context: exec.Command("kubectl", "get", "--raw",
fmt.Sprintf("/apis/console.osac.openshift.io/v1alpha1/namespaces/%s/computeinstances/test-ci-no-vm/console",
operatorNamespace),
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-exec-sprintf-arg-go)
[error] 108-111: An argument to exec.Command/exec.CommandContext is built with fmt.Sprintf that interpolates a non-constant value. When the command is run through a shell (e.g. sh -c) this allows command injection. Pass untrusted input as a separate, explicit argument to exec.Command (which does not invoke a shell) instead of formatting it into the command string, and validate or allowlist any values that must be embedded.
Context: exec.Command("kubectl", "get", "--raw",
fmt.Sprintf("/apis/console.osac.openshift.io/v1alpha1/namespaces/%s/computeinstances/test-ci-no-vm/vnc",
operatorNamespace),
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-exec-sprintf-arg-go)
🤖 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/integration/console_proxy_test.go` around lines 96 - 121, Make cleanup
for test-ci-no-vm unconditional by registering DeferCleanup immediately after
successful creation. In the cleanup callback, delete the ComputeInstance, remove
its finalizers as needed, and poll until the resource is confirmed absent; do
not discard deletion failures or rely on reaching the later assertions. Preserve
the existing resource name and operatorNamespace used by the test.
Source: Path instructions
| ctx := context.Background() | ||
| logger := slog.New(slog.NewTextHandler(GinkgoWriter, &slog.HandlerOptions{ | ||
| Level: slog.LevelDebug, | ||
| })) | ||
|
|
||
| keepKind, _ := strconv.ParseBool(os.Getenv("IT_KEEP_KIND")) | ||
| root := projectRoot() | ||
|
|
||
| var err error | ||
| cluster, err = itenv.NewKind(). | ||
| SetName("osac-operator-it"). | ||
| SetLogger(logger). | ||
| AddSchemeFunc(osacv1alpha1.AddToScheme). | ||
| Build() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| By("loading the operator image into the kind cluster") | ||
| err = utils.LoadImageToKindClusterWithName(operatorImage) | ||
| existed, err := cluster.Start(ctx) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| By("creating manager namespace") | ||
| cmd = exec.Command("kubectl", "create", "ns", operatorNamespace) | ||
| _, err = utils.Run(cmd) | ||
| if err != nil && !strings.Contains(err.Error(), "AlreadyExists") { | ||
| Fail(fmt.Sprintf("failed to create namespace %s: %v", operatorNamespace, err)) | ||
| } | ||
|
|
||
| By("deploying the controller-manager") | ||
| cmd = exec.Command("kubectl", "apply", "-k", "config/testing/default") | ||
| _, err = utils.Run(cmd) | ||
| kubeconfigFile, err := os.CreateTemp("", "osac-operator-it-*.kubeconfig") | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| By("waiting for controller-manager to be ready") | ||
| verifyControllerUp := func() error { | ||
| cmd := exec.Command("kubectl", "get", | ||
| "pods", "-l", "control-plane=controller-manager", | ||
| "-o", "go-template={{ range .items }}"+ | ||
| "{{ if not .metadata.deletionTimestamp }}"+ | ||
| "{{ .metadata.name }}"+ | ||
| "{{ \"\\n\" }}{{ end }}{{ end }}", | ||
| "-n", operatorNamespace, | ||
| ) | ||
| podOutput, err := utils.Run(cmd) | ||
| if err != nil { | ||
| return err | ||
| _, err = kubeconfigFile.Write(cluster.Kubeconfig()) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(kubeconfigFile.Close()).To(Succeed()) | ||
| kubeconfigPath := kubeconfigFile.Name() | ||
| Expect(os.Setenv("KUBECONFIG", kubeconfigPath)).To(Succeed()) | ||
|
|
||
| if !existed { | ||
| By("installing cert-manager") | ||
| Expect(cluster.InstallCertManager(ctx)).To(Succeed()) | ||
|
|
||
| By("installing trust-manager") | ||
| Expect(cluster.InstallTrustManager(ctx)).To(Succeed()) | ||
|
|
||
| By("installing CA") | ||
| Expect(cluster.InstallCa(ctx)).To(Succeed()) | ||
|
|
||
| By("installing CRDs via Helm") | ||
| installCRDsCmd, err := itenv.NewCommand(). | ||
| SetLogger(logger). | ||
| SetName("helm"). | ||
| SetArgs( | ||
| "install", "osac-operator-crds", | ||
| filepath.Join(root, "charts", "operator-crds"), | ||
| "--kubeconfig", kubeconfigPath, | ||
| "--wait", | ||
| ). | ||
| Build() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(installCRDsCmd.Execute(ctx)).To(Succeed()) | ||
|
|
||
| By("applying fake CRDs for external dependencies") | ||
| fakeCRDs := []string{ | ||
| "hypershift.openshift.io_hostedclusters.yaml", | ||
| "hypershift.openshift.io_hostedcontrolplanes.yaml", | ||
| "hypershift.openshift.io_nodepools.yaml", | ||
| "k8s.ovn.org_userdefinednetworks.yaml", | ||
| "kubevirt.io_virtualmachineinstances.yaml", | ||
| "kubevirt.io_virtualmachines.yaml", | ||
| "osac.openshift.io_baremetalinstances.yaml", | ||
| } | ||
| podNames := utils.GetNonEmptyLines(string(podOutput)) | ||
| if len(podNames) != 1 { | ||
| return fmt.Errorf("expect 1 controller pod running, but got %d", len(podNames)) | ||
| for _, crdFile := range fakeCRDs { | ||
| applyCmd, cmdErr := itenv.NewCommand(). | ||
| SetLogger(logger). | ||
| SetName("kubectl"). | ||
| SetArgs( | ||
| "apply", | ||
| "--kubeconfig", kubeconfigPath, | ||
| "-f", filepath.Join(root, "config", "crd", "fakes", crdFile), | ||
| ). | ||
| Build() | ||
| Expect(cmdErr).NotTo(HaveOccurred()) | ||
| Expect(applyCmd.Execute(ctx)).To(Succeed()) | ||
| } | ||
| } | ||
|
|
||
| cmd = exec.Command("kubectl", "get", | ||
| "pods", podNames[0], "-o", "jsonpath={.status.phase}", | ||
| "-n", operatorNamespace, | ||
| By("building the operator image") | ||
| buildCmd, err := itenv.NewCommand(). | ||
| SetLogger(logger). | ||
| SetName("podman"). | ||
| SetArgs("build", "-t", operatorImage, "."). | ||
| SetDir(root). | ||
| Build() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(buildCmd.Execute(ctx)).To(Succeed()) | ||
|
|
||
| By("saving the operator image to tar archive") | ||
| imageTar := filepath.Join(os.TempDir(), "osac-operator-it.tar") | ||
| saveCmd, err := itenv.NewCommand(). | ||
| SetLogger(logger). | ||
| SetName("podman"). | ||
| SetArgs("save", "--output", imageTar, operatorImage). | ||
| Build() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(saveCmd.Execute(ctx)).To(Succeed()) | ||
| defer os.Remove(imageTar) | ||
|
|
||
| By("loading the operator image into kind") | ||
| Expect(cluster.LoadArchive(ctx, imageTar)).To(Succeed()) | ||
|
|
||
| By("deploying the operator via Helm") | ||
| deployCmd, err := itenv.NewCommand(). | ||
| SetLogger(logger). | ||
| SetName("helm"). | ||
| SetArgs( | ||
| "install", "osac-operator", | ||
| filepath.Join(root, "charts", "operator"), | ||
| "--kubeconfig", kubeconfigPath, | ||
| "--namespace", operatorNamespace, | ||
| "--create-namespace", | ||
| "--set", "image.repository=localhost/osac-operator", | ||
| "--set", "image.tag=latest", | ||
| "--set", "image.pullPolicy=Never", | ||
| "--wait", | ||
| "--timeout", "2m", | ||
| ). | ||
| Build() | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(deployCmd.Execute(ctx)).To(Succeed()) | ||
|
|
||
| By("waiting for controller-manager pod to be running and ready") | ||
| Eventually(func() error { | ||
| pods := &corev1.PodList{} | ||
| listErr := cluster.Client().List(ctx, pods, | ||
| crclient.InNamespace(operatorNamespace), | ||
| crclient.MatchingLabels{"control-plane": "controller-manager"}, | ||
| ) | ||
| status, err := utils.Run(cmd) | ||
| if err != nil { | ||
| return err | ||
| if listErr != nil { | ||
| return fmt.Errorf("failed to list pods: %w", listErr) | ||
| } | ||
| if string(status) != "Running" { | ||
| return fmt.Errorf("controller pod in %s status", status) | ||
| for i := range pods.Items { | ||
| pod := &pods.Items[i] | ||
| if pod.DeletionTimestamp != nil { | ||
| continue | ||
| } | ||
| if pod.Status.Phase != corev1.PodRunning { | ||
| continue | ||
| } | ||
| for _, cs := range pod.Status.ContainerStatuses { | ||
| if cs.RestartCount > 0 { | ||
| return fmt.Errorf("container %s has %d restarts", cs.Name, cs.RestartCount) | ||
| } | ||
| if !cs.Ready { | ||
| return fmt.Errorf("container %s is not ready", cs.Name) | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
| return nil | ||
| } | ||
| Eventually(verifyControllerUp, 2*time.Minute, time.Second).Should(Succeed()) | ||
| }) | ||
| return fmt.Errorf("no running controller-manager pod found") | ||
| }, 2*time.Minute, time.Second).Should(Succeed()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
No per-call timeouts on cluster/install/command operations.
ctx := context.Background() is threaded through cluster.Start, InstallCertManager/TrustManager/Ca, every itenv.NewCommand().Execute(ctx) (helm/kubectl/podman), and LoadArchive, with no context.WithTimeout anywhere. The only safety nets are the 2-minute Eventually for pod readiness and the outer 30-minute ginkgo run --timeout from the Makefile — so a single hung podman build or helm install --wait can silently consume the whole suite budget before failing with a generic timeout, rather than a targeted, attributable error.
As per path instructions for **/*.go, "context.Context for cancellation and timeouts."
🤖 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/integration/integration_suite_test.go` around lines 66 - 215, Replace
the shared context.Background flow in the integration setup with targeted
context.WithTimeout contexts for cluster.Start, InstallCertManager,
InstallTrustManager, InstallCa, each command Execute call, and LoadArchive. Use
operation-appropriate deadlines and ensure each context is canceled, so hung
podman, helm, kubectl, or cluster operations fail promptly with an attributable
timeout while preserving the existing readiness Eventually timeout.
Source: Path instructions
| By("waiting for controller-manager pod to be running and ready") | ||
| Eventually(func() error { | ||
| pods := &corev1.PodList{} | ||
| listErr := cluster.Client().List(ctx, pods, | ||
| crclient.InNamespace(operatorNamespace), | ||
| crclient.MatchingLabels{"control-plane": "controller-manager"}, | ||
| ) | ||
| status, err := utils.Run(cmd) | ||
| if err != nil { | ||
| return err | ||
| if listErr != nil { | ||
| return fmt.Errorf("failed to list pods: %w", listErr) | ||
| } | ||
| if string(status) != "Running" { | ||
| return fmt.Errorf("controller pod in %s status", status) | ||
| for i := range pods.Items { | ||
| pod := &pods.Items[i] | ||
| if pod.DeletionTimestamp != nil { | ||
| continue | ||
| } | ||
| if pod.Status.Phase != corev1.PodRunning { | ||
| continue | ||
| } | ||
| for _, cs := range pod.Status.ContainerStatuses { | ||
| if cs.RestartCount > 0 { | ||
| return fmt.Errorf("container %s has %d restarts", cs.Name, cs.RestartCount) | ||
| } | ||
| if !cs.Ready { | ||
| return fmt.Errorf("container %s is not ready", cs.Name) | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
| return nil | ||
| } | ||
| Eventually(verifyControllerUp, 2*time.Minute, time.Second).Should(Succeed()) | ||
| }) | ||
| return fmt.Errorf("no running controller-manager pod found") | ||
| }, 2*time.Minute, time.Second).Should(Succeed()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restart-count check has no tolerance window; a single historical restart permanently fails this Eventually.
RestartCount never decreases, so if the controller-manager container restarts once early on (e.g., transient leader-election/webhook-cert race) but then stabilizes, this loop will return the "has N restarts" error on every poll for the full 2-minute window and the suite setup will fail even though the pod is actually healthy. This matches the PR's stated intent to "verify zero pod restarts," but consider whether a single early restart should be tolerated (e.g., check restarts only increase during the observation window, or allow one restart) to reduce CI flakiness while still catching genuine crash loops.
🤖 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/integration/integration_suite_test.go` around lines 186 - 215, Adjust
the restart validation inside the controller-manager readiness Eventually block
so a single transient historical restart does not permanently fail setup, while
genuine ongoing crash loops still fail. Track restart counts across polls and
fail only when they increase during the observation window, or apply an
equivalent one-restart tolerance; preserve the existing pod-running,
container-ready, and error-reporting checks.
| func removeFinalizers(kind, name, namespace string) { | ||
| cmd := exec.Command("kubectl", "patch", kind, name, | ||
| "-n", namespace, "--type=merge", | ||
| "-p", `{"metadata":{"finalizers":[]}}`) | ||
| _, _ = utils.Run(cmd) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching networking_test.go:\n'
fd -a 'networking_test\.go$' . || true
printf '\nGit status/stat:\n'
git status --short
git diff --stat || true
printf '\nRelevant file sections:\n'
if [ -f test/integration/networking_test.go ]; then
wc -l test/integration/networking_test.go
sed -n '1,120p' test/integration/networking_test.go | cat -n
fi
printf '\nSearch for Run helper and removeFinalizers usages/defs:\n'
rg -n "func Run|Run\\(|removeFinalizers|CommandContext|Context\\(" . --glob '*.go' --glob '!**/vendor/**' || trueRepository: osac-project/osac-operator
Length of output: 41135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'test/utils/utils.go relevant section:\n'
sed -n '1,180p' test/utils/utils.go | cat -n
printf '\nNetworking afterall/delete/resource sections:\n'
sed -n '200,280p' test/integration/networking_test.go | cat -nRepository: osac-project/osac-operator
Length of output: 10184
Bound and report finalizer-patch failures.
removeFinalizers runs kubectl patch with an unbounded command via exec.Command and discards wrap errors from utils.Run. A stalled kubectl can hang cleanup, and removeAllFinalizers continues as if the patch succeeded. Use exec.CommandContext with a timeout and handle/return the error so cleanup distinguishes real failures from expected missing resources.
🤖 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/integration/networking_test.go` around lines 34 - 39, Update
removeFinalizers to execute kubectl through exec.CommandContext with an explicit
timeout, and stop discarding the error returned by utils.Run. Return or
propagate that error to removeAllFinalizers so cleanup can distinguish
successful patches, expected missing resources, and genuine patch failures.
Source: Path instructions
| Eventually(func() error { | ||
| cmd := exec.Command("kubectl", "get", "subnet", "test-subnet", | ||
| "-n", operatorNamespace) | ||
| _, err := utils.Run(cmd) | ||
| if err == nil { | ||
| return fmt.Errorf("Subnet still exists") | ||
| } | ||
| return nil | ||
| } | ||
| Eventually(verifyDeleted, 60*time.Second, time.Second).Should(Succeed()) | ||
| }, 60*time.Second, time.Second).Should(Succeed()) | ||
| }) | ||
|
|
||
| It("should delete VirtualNetwork successfully", func() { | ||
| By("deleting the VirtualNetwork") | ||
| By("initiating deletion (non-blocking)") | ||
| cmd := exec.Command("kubectl", "delete", "virtualnetwork", "test-vnet", | ||
| "-n", operatorNamespace, "--timeout=60s") | ||
| "-n", operatorNamespace, "--wait=false") | ||
| _, err := utils.Run(cmd) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| By("removing finalizers so deletion can complete without AAP") | ||
| removeFinalizers("virtualnetwork", "test-vnet", operatorNamespace) | ||
|
|
||
| By("verifying VirtualNetwork is deleted") | ||
| verifyDeleted := func() error { | ||
| Eventually(func() error { | ||
| cmd := exec.Command("kubectl", "get", "virtualnetwork", "test-vnet", | ||
| "-n", operatorNamespace) | ||
| _, err := utils.Run(cmd) | ||
| if err == nil { | ||
| return fmt.Errorf("VirtualNetwork still exists") | ||
| } | ||
| return nil | ||
| } | ||
| Eventually(verifyDeleted, 60*time.Second, time.Second).Should(Succeed()) | ||
| }, 60*time.Second, time.Second).Should(Succeed()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Only accept NotFound as successful deletion.
Both polling closures treat any kubectl get error—such as an authentication or cluster-connectivity failure—as proof of deletion. Use --ignore-not-found -o name, return unexpected errors, and succeed only when the command output is empty.
🤖 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/integration/networking_test.go` around lines 242 - 272, Update the
deletion polling closures for the subnet and VirtualNetwork checks to run
kubectl get with --ignore-not-found -o name, return any unexpected command
error, and report success only when the command output is empty. Do not treat
arbitrary kubectl failures as evidence of deletion.
24db463 to
e8593f8
Compare
omer-vishlitzky
changed the title
fecb8dd to
0687eb1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In @.github/workflows/build-image.yaml:
- Around line 120-134: Update the “Generate manifests with helm template” step
to pass steps.sha-tag.outputs.full-sha-tag through the step’s env configuration,
then read that value via the existing SHA_IMG shell variable instead of
interpolating the GitHub expression inside the run script. Preserve the existing
REPO/TAG parsing and Helm commands.
In `@Makefile`:
- Around line 141-143: Align the timeout between the Makefile target
integration-tests and the CI “Run integration tests” step: either raise the
workflow timeout-minutes to at least 30 or lower Ginkgo’s --timeout 30m so it
expires before the CI limit. Ensure Ginkgo’s timeout can trigger and report
before the workflow forcibly terminates the step.
In `@test/integration/integration_suite_test.go`:
- Around line 169-187: Update the Helm command constructed in the operator
deployment block to use the idempotent upgrade-and-install action instead of
plain install. Preserve the existing release name, chart path, namespace, and
deployment options so reused clusters succeed when the osac-operator release
already exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 236c7dd6-8ef1-41ba-a3a8-9652188183e2
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (42)
.github/workflows/build-image.yaml.github/workflows/integration-tests.yml.gitignoreMakefileconfig/console-proxy-kube-system/auth-reader-rolebinding.yamlconfig/console-proxy-kube-system/kustomization.yamlconfig/console-proxy/apiservice.yamlconfig/console-proxy/certificate.yamlconfig/console-proxy/clusterrole-secret-reader.yamlconfig/console-proxy/clusterrole.yamlconfig/console-proxy/clusterrolebinding-auth-delegator.yamlconfig/console-proxy/clusterrolebinding.yamlconfig/console-proxy/deployment.yamlconfig/console-proxy/kustomization.yamlconfig/console-proxy/rolebinding-secret-reader.yamlconfig/console-proxy/service.yamlconfig/console-proxy/serviceaccount.yamlconfig/crd/fakes/kustomization.yamlconfig/crd/kustomization.yamlconfig/crd/kustomizeconfig.yamlconfig/default/kustomization.yamlconfig/default/manager_metrics_patch.yamlconfig/default/metrics_service.yamlconfig/manager/kustomization.yamlconfig/manager/manager.yamlconfig/manifests/kustomization.yamlconfig/network-policy/allow-metrics-traffic.yamlconfig/network-policy/kustomization.yamlconfig/prometheus/kustomization.yamlconfig/prometheus/monitor.yamlconfig/rbac/kustomization.yamlconfig/samples/kustomization.yamlconfig/scorecard/bases/config.yamlconfig/scorecard/kustomization.yamlconfig/scorecard/patches/basic.config.yamlconfig/scorecard/patches/olm.config.yamlconfig/testing/console-proxy/kustomization.yamlconfig/testing/default/kustomization.yamlgo.modtest/integration/console_proxy_test.gotest/integration/integration_suite_test.gotest/integration/networking_test.go
💤 Files with no reviewable changes (34)
- config/console-proxy/clusterrolebinding-auth-delegator.yaml
- config/samples/kustomization.yaml
- config/manifests/kustomization.yaml
- config/prometheus/kustomization.yaml
- config/console-proxy-kube-system/kustomization.yaml
- config/console-proxy/deployment.yaml
- config/console-proxy-kube-system/auth-reader-rolebinding.yaml
- config/network-policy/allow-metrics-traffic.yaml
- config/manager/kustomization.yaml
- config/crd/kustomization.yaml
- config/network-policy/kustomization.yaml
- config/scorecard/kustomization.yaml
- config/crd/kustomizeconfig.yaml
- config/console-proxy/clusterrolebinding.yaml
- config/console-proxy/clusterrole-secret-reader.yaml
- config/console-proxy/serviceaccount.yaml
- config/scorecard/bases/config.yaml
- config/crd/fakes/kustomization.yaml
- config/default/manager_metrics_patch.yaml
- config/console-proxy/clusterrole.yaml
- config/scorecard/patches/basic.config.yaml
- config/testing/console-proxy/kustomization.yaml
- config/scorecard/patches/olm.config.yaml
- config/console-proxy/apiservice.yaml
- config/console-proxy/kustomization.yaml
- config/rbac/kustomization.yaml
- config/testing/default/kustomization.yaml
- config/console-proxy/rolebinding-secret-reader.yaml
- config/console-proxy/service.yaml
- config/console-proxy/certificate.yaml
- config/default/metrics_service.yaml
- config/manager/manager.yaml
- config/prometheus/monitor.yaml
- config/default/kustomization.yaml
| .PHONY: test-integration-kind integration-tests | ||
| test-integration-kind integration-tests: manifests generate | ||
| ginkgo run --timeout 30m -v test/integration |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Ginkgo timeout (30m) is unreachable behind CI's 15m step timeout.
.github/workflows/integration-tests.yml's "Run integration tests" step sets timeout-minutes: 15, so the job is always killed before this --timeout 30m can ever fire. Either raise the CI step timeout to match, or lower this value to give ginkgo's own structured timeout/report a chance to run before the workflow force-kills the job.
🤖 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 `@Makefile` around lines 141 - 143, Align the timeout between the Makefile
target integration-tests and the CI “Run integration tests” step: either raise
the workflow timeout-minutes to at least 30 or lower Ginkgo’s --timeout 30m so
it expires before the CI limit. Ensure Ginkgo’s timeout can trigger and report
before the workflow forcibly terminates the step.
Replace all kustomize-based deployment with Helm charts (charts/operator/ and charts/operator-crds/) as the sole deployment mechanism. Makefile: install/deploy/undeploy/uninstall now use helm upgrade/uninstall. build-installer uses helm template. test-kustomize and test-smoke removed, replaced by helm-lint. integration-tests target added for Ginkgo-based IT. CI: build-image.yaml uses helm lint instead of kustomize validation, and helm template instead of kustomize build for manifest container. Deleted directories (all kustomize-only, superseded by Helm charts): config/console-proxy, config/console-proxy-kube-system, config/default, config/manager, config/manifests, config/network-policy, config/prometheus, config/scorecard, config/testing Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
Three bugs fixed in the existing integration tests: 1. Crash-loop detection: pod.status.phase stays "Running" even during CrashLoopBackOff. The previous check only verified phase=="Running", which always passed. Now checks container ready status AND zero restarts to catch crash-looping controllers. 2. Finalizer handling in deletion tests: with a live controller, deletion blocks because the controller adds finalizers and deprovision (which removes them) needs a real AAP backend. Fix: delete --wait=false first (sets DeletionTimestamp so controller calls handleDelete not handleUpdate), then patch finalizers away. 3. Console proxy tests: previously deployed console proxy separately via kustomize overlays. Now uses the Helm-deployed instance (the operator Helm chart includes the console proxy deployment). Also adds finalizer cleanup for ComputeInstance resources created during tests. The suite now deploys via Helm (install CRDs + fake CRDs + operator), removing the dependency on deleted kustomize overlays. Previously-skipped finalizer tests are re-enabled since the controller is now properly running with all required CRDs (including fakes for HyperShift, KubeVirt, OVN-K). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
Replace the standalone Kind cluster creation with the kind-dev reusable GitHub Action from osac-test-infra. This wraps osac-workspace/kind-dev/ setup.sh as the single source of truth for Kind-based environments. The action runs setup.sh --skip-osac which installs all infrastructure (cert-manager, trust-manager, CA, envoy gateway, postgres, keycloak) without OSAC components. The integration tests then deploy the operator and its CRDs on top. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
0687eb1 to
c66c796
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
test/integration/networking_test.go (2)
34-39: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnbounded, error-discarding finalizer patch (duplicate of prior finding).
removeFinalizersstill runskubectl patchvia plainexec.Commandwith no timeout, and discards the error fromutils.Run. Unchanged from the prior review: a stalledkubectlhangs cleanup indefinitely, and callers (includingremoveAllFinalizers) can't distinguish "already gone" from a genuine patch failure.🤖 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/integration/networking_test.go` around lines 34 - 39, Update removeFinalizers to run the kubectl patch with a bounded timeout and propagate or explicitly handle the error returned by utils.Run. Update callers such as removeAllFinalizers to distinguish successful cleanup, missing resources, and genuine patch failures instead of ignoring the result.
242-250: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAny
kubectl geterror is still treated as proof of deletion (duplicate of prior finding).Both closures return
nil(success) whenevererr != nil, regardless of the actual cause — an auth failure, API-server blip, or context-canceled error would look identical to "resource deleted." This is unchanged from the prior review; use--ignore-not-found -o name, return unexpected errors, and only succeed when output is empty.Also applies to: 264-272
🤖 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/integration/networking_test.go` around lines 242 - 250, Update the Eventually closures around the subnet deletion checks to run kubectl get with --ignore-not-found and -o name, then return unexpected command errors instead of treating every error as deletion. Only return success when the command completes successfully with empty output, preserving retry behavior for resources that still exist.test/integration/console_proxy_test.go (1)
93-122: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCleanup still unreachable on assertion failure (duplicate of prior finding).
Cleanup (
removeFinalizers/delete) is only reached after bothExpect(...).To(HaveOccurred())calls at lines 106 and 114 succeed. Gomega's default fail handler aborts the spec on a failed assertion, so if either console/VNC access unexpectedly doesn't error,test-ci-no-vmnever gets its finalizers cleared or gets deleted — it leaks inoperatorNamespacefor the rest of the suite. Finalizer handling was added, but the underlying structural issue (cleanup gated behind assertions instead of registered unconditionally) from the prior review is unchanged.🛠️ Proposed fix
cmd.Stdin = createComputeInstanceYAML("test-ci-no-vm", operatorNamespace) _, err := utils.Run(cmd) Expect(err).NotTo(HaveOccurred()) + DeferCleanup(func() { + removeFinalizers("computeinstance", "test-ci-no-vm", operatorNamespace) + cmd := exec.Command("kubectl", "delete", "computeinstance", "test-ci-no-vm", + "-n", operatorNamespace, "--wait=false", "--ignore-not-found") + _, _ = utils.Run(cmd) + removeFinalizers("computeinstance", "test-ci-no-vm", operatorNamespace) + }) By("attempting console access") @@ _, err = utils.Run(cmd) Expect(err).To(HaveOccurred()) - - By("cleaning up the ComputeInstance") - removeFinalizers("computeinstance", "test-ci-no-vm", operatorNamespace) - cmd = exec.Command("kubectl", "delete", "computeinstance", "test-ci-no-vm", - "-n", operatorNamespace, "--wait=false", "--ignore-not-found") - _, _ = utils.Run(cmd) - removeFinalizers("computeinstance", "test-ci-no-vm", operatorNamespace) })🤖 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/integration/console_proxy_test.go` around lines 93 - 122, Register ComputeInstance cleanup unconditionally at the start of the test using the test framework’s cleanup/defer mechanism, so removeFinalizers and deletion run even when either console or VNC Expect assertion fails. Keep the existing cleanup operations and test assertions, but move them out of the assertion-gated sequence in “should return an error for a compute instance without VM reference”.
🤖 Prompt for all review comments with 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.
Inline comments:
In @.github/workflows/integration-tests.yml:
- Around line 36-39: Update the “Create Kind cluster with OSAC infrastructure”
step to pin the osac-project/osac-test-infra kind-dev action to the reviewed
full commit SHA instead of the mutable `@main` reference, leaving its mode:
skip-osac configuration unchanged.
In `@Makefile`:
- Line 339: Update the Makefile bundle target to restore generation of manifests
and bundle metadata before validating ./bundle, using the project’s
Helm-compatible generation path. Preserve the existing validation step and
ensure the target’s declared contract is fulfilled so release artifacts are
regenerated after API or deployment changes.
- Line 248: Update the image-reference parsing at Makefile lines 248-248 and
266-266 to preserve registry ports and split only at the final tag separator,
applying the same logic to both Helm commands. Update
.github/workflows/build-image.yaml lines 127-128 to derive REPO and TAG with
this robust parsing so references such as
registry.internal:5000/osac/operator:v1 remain valid.
In `@test/integration/integration_suite_test.go`:
- Around line 71-126: Update the controller-manager readiness check inside the
Eventually closure to tolerate historical transient restarts by tracking restart
counts across polls and requiring them to remain stable for a defined window,
rather than rejecting any count above zero. Also create the kubectl command with
a bounded context or command timeout so a hung exec.Command cannot bypass
Eventually’s five-minute deadline. Preserve the existing phase, readiness,
deletion, and no-running-pods checks.
- Around line 129-137: Extend the AfterSuite cleanup after the Helm uninstall
steps to explicitly remove the fake external CRDs applied from config/crd/fakes
via kubectl, using the same manifest path and non-failing cleanup behavior.
Ensure this also removes their instances and works when resources are already
absent, while preserving the existing Helm release cleanup.
---
Duplicate comments:
In `@test/integration/console_proxy_test.go`:
- Around line 93-122: Register ComputeInstance cleanup unconditionally at the
start of the test using the test framework’s cleanup/defer mechanism, so
removeFinalizers and deletion run even when either console or VNC Expect
assertion fails. Keep the existing cleanup operations and test assertions, but
move them out of the assertion-gated sequence in “should return an error for a
compute instance without VM reference”.
In `@test/integration/networking_test.go`:
- Around line 34-39: Update removeFinalizers to run the kubectl patch with a
bounded timeout and propagate or explicitly handle the error returned by
utils.Run. Update callers such as removeAllFinalizers to distinguish successful
cleanup, missing resources, and genuine patch failures instead of ignoring the
result.
- Around line 242-250: Update the Eventually closures around the subnet deletion
checks to run kubectl get with --ignore-not-found and -o name, then return
unexpected command errors instead of treating every error as deletion. Only
return success when the command completes successfully with empty output,
preserving retry behavior for resources that still exist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8905ec9f-e1d5-4141-856d-416afc0e584f
📒 Files selected for processing (41)
.github/workflows/build-image.yaml.github/workflows/integration-tests.yml.gitignoreMakefileconfig/console-proxy-kube-system/auth-reader-rolebinding.yamlconfig/console-proxy-kube-system/kustomization.yamlconfig/console-proxy/apiservice.yamlconfig/console-proxy/certificate.yamlconfig/console-proxy/clusterrole-secret-reader.yamlconfig/console-proxy/clusterrole.yamlconfig/console-proxy/clusterrolebinding-auth-delegator.yamlconfig/console-proxy/clusterrolebinding.yamlconfig/console-proxy/deployment.yamlconfig/console-proxy/kustomization.yamlconfig/console-proxy/rolebinding-secret-reader.yamlconfig/console-proxy/service.yamlconfig/console-proxy/serviceaccount.yamlconfig/crd/fakes/kustomization.yamlconfig/crd/kustomization.yamlconfig/crd/kustomizeconfig.yamlconfig/default/kustomization.yamlconfig/default/manager_metrics_patch.yamlconfig/default/metrics_service.yamlconfig/manager/kustomization.yamlconfig/manager/manager.yamlconfig/manifests/kustomization.yamlconfig/network-policy/allow-metrics-traffic.yamlconfig/network-policy/kustomization.yamlconfig/prometheus/kustomization.yamlconfig/prometheus/monitor.yamlconfig/rbac/kustomization.yamlconfig/samples/kustomization.yamlconfig/scorecard/bases/config.yamlconfig/scorecard/kustomization.yamlconfig/scorecard/patches/basic.config.yamlconfig/scorecard/patches/olm.config.yamlconfig/testing/console-proxy/kustomization.yamlconfig/testing/default/kustomization.yamltest/integration/console_proxy_test.gotest/integration/integration_suite_test.gotest/integration/networking_test.go
💤 Files with no reviewable changes (34)
- config/console-proxy/clusterrolebinding-auth-delegator.yaml
- config/testing/default/kustomization.yaml
- config/console-proxy-kube-system/kustomization.yaml
- config/manifests/kustomization.yaml
- config/scorecard/bases/config.yaml
- config/crd/fakes/kustomization.yaml
- config/default/kustomization.yaml
- config/scorecard/patches/basic.config.yaml
- config/crd/kustomizeconfig.yaml
- config/console-proxy/service.yaml
- config/network-policy/allow-metrics-traffic.yaml
- config/console-proxy-kube-system/auth-reader-rolebinding.yaml
- config/default/manager_metrics_patch.yaml
- config/console-proxy/rolebinding-secret-reader.yaml
- config/rbac/kustomization.yaml
- config/default/metrics_service.yaml
- config/crd/kustomization.yaml
- config/prometheus/monitor.yaml
- config/scorecard/kustomization.yaml
- config/console-proxy/deployment.yaml
- config/manager/manager.yaml
- config/console-proxy/apiservice.yaml
- config/console-proxy/clusterrole-secret-reader.yaml
- config/prometheus/kustomization.yaml
- config/testing/console-proxy/kustomization.yaml
- config/console-proxy/kustomization.yaml
- config/network-policy/kustomization.yaml
- config/samples/kustomization.yaml
- config/console-proxy/serviceaccount.yaml
- config/scorecard/patches/olm.config.yaml
- config/manager/kustomization.yaml
- config/console-proxy/certificate.yaml
- config/console-proxy/clusterrolebinding.yaml
- config/console-proxy/clusterrole.yaml
| - name: Create Kind cluster with OSAC infrastructure | ||
| uses: osac-project/osac-test-infra/.github/actions/kind-dev@main | ||
| with: | ||
| cluster_name: kind | ||
| mode: skip-osac |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin the Kind action to a full commit SHA.
Line 37 executes a cross-repository action from mutable @main; a later upstream change changes CI behavior without a PR here. Pin the reviewed commit SHA.
🧰 Tools
🪛 zizmor (1.28.0)
[error] 37-37: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/integration-tests.yml around lines 36 - 39, Update the
“Create Kind cluster with OSAC infrastructure” step to pin the
osac-project/osac-test-infra kind-dev action to the reviewed full commit SHA
instead of the mutable `@main` reference, leaving its mode: skip-osac
configuration unchanged.
Sources: Path instructions, Linters/SAST tools
| $(KUSTOMIZE) build config/default > dist/install.yaml | ||
| helm template osac-operator-crds charts/operator-crds/ > dist/install.yaml | ||
| echo "---" >> dist/install.yaml | ||
| helm template osac-operator charts/operator/ --set image.repository=$(firstword $(subst :, ,${IMG})) --set image.tag=$(lastword $(subst :, ,${IMG})) >> dist/install.yaml |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Parse image references without splitting registry ports.
Splitting on the first colon misparses valid references such as registry.internal:5000/osac/operator:v1, producing an invalid repository and tag.
Makefile#L248-L248: replace the colon-token parsing with logic that preserveshost:port/pathand extracts only the final tag separator.Makefile#L266-L266: use the same robust image-reference parsing for Helm deployment overrides..github/workflows/build-image.yaml#L127-L128: preserve registry ports when derivingREPOandTAG.
📍 Affects 2 files
Makefile#L248-L248(this comment)Makefile#L266-L266.github/workflows/build-image.yaml#L127-L128
🤖 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 `@Makefile` at line 248, Update the image-reference parsing at Makefile lines
248-248 and 266-266 to preserve registry ports and split only at the final tag
separator, applying the same logic to both Helm commands. Update
.github/workflows/build-image.yaml lines 127-128 to derive REPO and TAG with
this robust parsing so references such as
registry.internal:5000/osac/operator:v1 remain valid.
| $(OPERATOR_SDK) generate kustomize manifests -q | ||
| cd config/manager && $(KUSTOMIZE) edit set image controller=$(IMG) | ||
| $(KUSTOMIZE) build config/manifests | $(OPERATOR_SDK) generate bundle $(BUNDLE_GEN_FLAGS) | ||
| bundle: manifests operator-sdk ## Generate bundle manifests and metadata, then validate generated files. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Restore bundle generation or make this validate-only target explicit.
bundle now only validates the existing ./bundle; it no longer generates manifests or metadata despite its contract. Release artifacts can remain stale after API or deployment changes. Replace the removed generation path with a Helm-compatible one before validation.
🤖 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 `@Makefile` at line 339, Update the Makefile bundle target to restore
generation of manifests and bundle metadata before validating ./bundle, using
the project’s Helm-compatible generation path. Preserve the existing validation
step and ensure the target’s declared contract is fulfilled so release artifacts
are regenerated after API or deployment changes.
| By("waiting for controller-manager pod to be ready with zero restarts") | ||
| // pod.status.phase stays "Running" even during CrashLoopBackOff — only | ||
| // container status reveals the real state. Check that all containers are | ||
| // ready AND have zero restarts to detect crash-looping controllers early. | ||
| Eventually(func() error { | ||
| cmd := exec.Command("kubectl", "get", "pods", | ||
| "-l", "control-plane=controller-manager", | ||
| "-n", operatorNamespace, | ||
| ) | ||
| podOutput, err := utils.Run(cmd) | ||
| "-o", "json") | ||
| output, err := utils.Run(cmd) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| podNames := utils.GetNonEmptyLines(string(podOutput)) | ||
| if len(podNames) != 1 { | ||
| return fmt.Errorf("expect 1 controller pod running, but got %d", len(podNames)) | ||
|
|
||
| var podList struct { | ||
| Items []struct { | ||
| Metadata struct { | ||
| Name string `json:"name"` | ||
| DeletionTimestamp *time.Time `json:"deletionTimestamp"` | ||
| } `json:"metadata"` | ||
| Status struct { | ||
| Phase string `json:"phase"` | ||
| ContainerStatuses []struct { | ||
| Ready bool `json:"ready"` | ||
| RestartCount int64 `json:"restartCount"` | ||
| } `json:"containerStatuses"` | ||
| } `json:"status"` | ||
| } `json:"items"` | ||
| } | ||
| if err := json.Unmarshal(output, &podList); err != nil { | ||
| return fmt.Errorf("failed to parse pod list: %w", err) | ||
| } | ||
|
|
||
| cmd = exec.Command("kubectl", "get", | ||
| "pods", podNames[0], "-o", "jsonpath={.status.phase}", | ||
| "-n", operatorNamespace, | ||
| ) | ||
| status, err := utils.Run(cmd) | ||
| if err != nil { | ||
| return err | ||
| var running int | ||
| for _, pod := range podList.Items { | ||
| if pod.Metadata.DeletionTimestamp != nil { | ||
| continue | ||
| } | ||
| if pod.Status.Phase != "Running" { | ||
| return fmt.Errorf("pod %s in %s phase", pod.Metadata.Name, pod.Status.Phase) | ||
| } | ||
| for _, cs := range pod.Status.ContainerStatuses { | ||
| if !cs.Ready { | ||
| return fmt.Errorf("pod %s has unready container", pod.Metadata.Name) | ||
| } | ||
| if cs.RestartCount > 0 { | ||
| return fmt.Errorf("pod %s has %d restarts (crash-looping)", pod.Metadata.Name, cs.RestartCount) | ||
| } | ||
| } | ||
| running++ | ||
| } | ||
| if string(status) != "Running" { | ||
| return fmt.Errorf("controller pod in %s status", status) | ||
| if running == 0 { | ||
| return fmt.Errorf("no running controller-manager pods found") | ||
| } | ||
| return nil | ||
| } | ||
| Eventually(verifyControllerUp, 2*time.Minute, time.Second).Should(Succeed()) | ||
| }, 5*time.Minute, 5*time.Second).Should(Succeed()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restart-count check still has no tolerance window (duplicate of prior finding).
cs.RestartCount > 0 fails the Eventually on any historical restart, even a single transient early restart that has since stabilized (e.g., leader-election/webhook-cert races). This will permanently fail suite setup for the full 5‑minute window even though the pod is actually healthy. This is the same restart-tolerance concern raised on a prior commit and is unchanged in substance here.
Additionally, because exec.Command here has no context/timeout, a hung kubectl get pods call blocks the closure from ever returning — which means the outer 5*time.Minute Eventually budget is not actually enforced, since Gomega only checks elapsed time between completed invocations of the polled function.
🤖 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/integration/integration_suite_test.go` around lines 71 - 126, Update the
controller-manager readiness check inside the Eventually closure to tolerate
historical transient restarts by tracking restart counts across polls and
requiring them to remain stable for a defined window, rather than rejecting any
count above zero. Also create the kubectl command with a bounded context or
command timeout so a hung exec.Command cannot bypass Eventually’s five-minute
deadline. Preserve the existing phase, readiness, deletion, and no-running-pods
checks.
| var _ = AfterSuite(func() { | ||
| By("undeploying the controller-manager") | ||
| cmd := exec.Command("kubectl", "delete", "-k", "config/testing/default", "--ignore-not-found") | ||
| By("undeploying the operator") | ||
| cmd := exec.Command("helm", "uninstall", "osac-operator", | ||
| "--namespace", operatorNamespace, "--ignore-not-found") | ||
| _, _ = utils.Run(cmd) | ||
|
|
||
| By("removing manager namespace") | ||
| cmd = exec.Command("kubectl", "delete", "ns", operatorNamespace) | ||
| By("uninstalling CRDs") | ||
| cmd = exec.Command("helm", "uninstall", "osac-operator-crds", "--ignore-not-found") | ||
| _, _ = utils.Run(cmd) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Fake CRDs applied via kubectl apply are never cleaned up.
AfterSuite uninstalls the osac-operator and osac-operator-crds Helm releases, but the fake external CRDs installed directly via kubectl apply --server-side -f config/crd/fakes/ (line 46) aren't removed by either helm uninstall. On IT_KEEP_KIND=true reused clusters, these CRDs (and any instances) persist across runs, unlike the Helm-managed resources, and stale/conflicting fake CRD definitions on a later run risk server-side-apply field-manager conflicts.
🧹 Proposed fix
var _ = AfterSuite(func() {
By("undeploying the operator")
cmd := exec.Command("helm", "uninstall", "osac-operator",
"--namespace", operatorNamespace, "--ignore-not-found")
_, _ = utils.Run(cmd)
By("uninstalling CRDs")
cmd = exec.Command("helm", "uninstall", "osac-operator-crds", "--ignore-not-found")
_, _ = utils.Run(cmd)
+
+ By("removing fake CRDs")
+ cmd = exec.Command("kubectl", "delete", "-f", "config/crd/fakes/", "--ignore-not-found")
+ _, _ = utils.Run(cmd)
})📝 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 _ = AfterSuite(func() { | |
| By("undeploying the controller-manager") | |
| cmd := exec.Command("kubectl", "delete", "-k", "config/testing/default", "--ignore-not-found") | |
| By("undeploying the operator") | |
| cmd := exec.Command("helm", "uninstall", "osac-operator", | |
| "--namespace", operatorNamespace, "--ignore-not-found") | |
| _, _ = utils.Run(cmd) | |
| By("removing manager namespace") | |
| cmd = exec.Command("kubectl", "delete", "ns", operatorNamespace) | |
| By("uninstalling CRDs") | |
| cmd = exec.Command("helm", "uninstall", "osac-operator-crds", "--ignore-not-found") | |
| _, _ = utils.Run(cmd) | |
| var _ = AfterSuite(func() { | |
| By("undeploying the operator") | |
| cmd := exec.Command("helm", "uninstall", "osac-operator", | |
| "--namespace", operatorNamespace, "--ignore-not-found") | |
| _, _ = utils.Run(cmd) | |
| By("uninstalling CRDs") | |
| cmd = exec.Command("helm", "uninstall", "osac-operator-crds", "--ignore-not-found") | |
| _, _ = utils.Run(cmd) | |
| By("removing fake CRDs") | |
| cmd = exec.Command("kubectl", "delete", "-f", "config/crd/fakes/", "--ignore-not-found") | |
| _, _ = utils.Run(cmd) | |
| }) |
🤖 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/integration/integration_suite_test.go` around lines 129 - 137, Extend
the AfterSuite cleanup after the Helm uninstall steps to explicitly remove the
fake external CRDs applied from config/crd/fakes via kubectl, using the same
manifest path and non-failing cleanup behavior. Ensure this also removes their
instances and works when resources are already absent, while preserving the
existing Helm release cleanup.
2 participants
Summary
Three logical commits:
Key fixes
pod.status.phasestaysRunningduring CrashLoopBackOff. Now checkscontainerStatuses[].readyandrestartCount == 0--wait=falsefirst (sets DeletionTimestamp so controller callshandleDelete), then patch finalizers awayCompanion PRs
Test plan
make integration-testspasses locally with kind-dev clusterhelm lintpasses for both charts