GCP-429: Add e2e tests for CNCC GCP credential authentication methods#30832
GCP-429: Add e2e tests for CNCC GCP credential authentication methods#30832apahim wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@apahim: This pull request references GCP-429 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 story to target the "4.22.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: apahim 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 |
|
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:
WalkthroughIntroduces a new end-to-end Go test for CNCC on GCP that snapshots/restores the cloud-credentials secret, detects credential type (WIF/SA), verifies CNCC readiness and restart behavior, exercises credential rotation and invalid-secret handling, tests secret deletion/recovery, and validates EgressIP-driven CloudPrivateIPConfig workflows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
|
@apahim: This pull request references GCP-429 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 story to target the "4.22.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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/networking/cncc_gcp_credentials.go`:
- Line 329: The test fixture embeds a PEM-like private key in the dummySA string
which triggers secret scanners; update the dummySA fixture (and the similar
occurrence at the other spot) to remove any real-looking PEM content and replace
the "private_key" value with a non-secret placeholder (e.g. a short
"<REDACTED_PRIVATE_KEY>" or clearly fake token) or build the JSON
programmatically without PEM text so the variable dummySA no longer contains PEM
markers like "-----BEGIN" / "-----END"; keep the same JSON keys and structure so
tests still parse but do not include PEM-looking secrets.
- Around line 70-87: The restore flow currently treats any error from
clientset.CoreV1().Secrets(cnccNamespace).Get(...) as a deleted secret; change
it to first check if the error is a NotFound (use apierrors.IsNotFound(err)) and
only then create the new secret with cnccSecretName/cnccNamespace; for all other
errors return/fail the test (o.Expect(err).NotTo(o.HaveOccurred(...)) or
propagate the error) instead of attempting Create — keep the existing Update
path for the else case where Get succeeds, and reference the same
clientset.CoreV1().Secrets(...).Get, Create and Update calls and the o.Expect
assertions to place the checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23cc057b-129f-4c25-b816-2e98201af7cc
📒 Files selected for processing (1)
test/extended/networking/cncc_gcp_credentials.go
|
@apahim: This pull request references GCP-429 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 story to target the "4.22.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. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/extended/networking/cncc_gcp_credentials.go (2)
50-57:⚠️ Potential issue | 🟠 MajorReplace PEM-like private key literals with non-secret placeholders.
The
dummySAJSONandinvalidSAJSONconstants contain PEM-formatted private key strings that trigger secret scanners (Gitleaks flagged both). While these are test fixtures, they will cause CI noise and security triage overhead.Replace the
private_keyvalues with clearly fake placeholders:Suggested fix
- dummySAJSON = `{"type":"service_account","project_id":"dummy-project","private_key_id":"dummy","private_key":"-----BEGIN RSA PRIVATE KEY-----\nMIIBogIBAAJBALRo/7FBMHpMx2ls9k5A5feDteQa4ARWV5jNz9/v3mPpM4CPYB/m\nmivnN4g3a3+0kqe5W9USNWQ/lC3qLNjdXnECAwEAAQJAC/wG4sB/YxYILqLywb11\nNpviLqQgeKi7ZXSXT1OBF0gU8bB0OP8OLjkKGfhSW3niqxoM1RRoLvNsL7A0mBp\nwQIhAO0/GNXSdA98JxmNxVdIjPTCSAP6XEoH3R8WFiJ0JBSvAiEAxjn0EDBFKOI7\nhBk1RXlTfMHbDQHrPKFmL7y2GIPBYU8CIGl9a3PJ2QeB2Nz3IlJjSPjQ5U0ASNS\nSN1GMH2eJwYnAiEAoRxE6QUCRzBL7F3l/Dq+IYbwxZkN95Nf8CPm8GjYHD8CIQC8\nuqbJMxbRbhsPNi88e1ZYayVjPhFKEXZfCihqT/YE1Q==\n-----END RSA PRIVATE KEY-----\n","client_email":"dummy@dummy-project.iam.gserviceaccount.com","client_id":"123456789","auth_uri":"https://accounts.google.com/o/oauth2/auth","token_uri":"https://oauth2.googleapis.com/token"}` + dummySAJSON = `{"type":"service_account","project_id":"dummy-project","private_key_id":"dummy","private_key":"not-a-real-key","client_email":"dummy@dummy-project.iam.gserviceaccount.com","client_id":"123456789","auth_uri":"https://accounts.google.com/o/oauth2/auth","token_uri":"https://oauth2.googleapis.com/token"}` - invalidSAJSON = `{"type":"service_account","project_id":"invalid-project","private_key_id":"invalid","private_key":"-----BEGIN RSA PRIVATE KEY-----\nMIIBogIBAAJBALRo/7FBMHpMx2ls9k5A5feDteQa4ARWV5jNz9/v3mPpM4CPYB/m\nmivnN4g3a3+0kqe5W9USNWQ/lC3qLNjdXnECAwEAAQJAC/wG4sB/YxYILqLywb11\nNpviLqQgeKi7ZXSXT1OBF0gU8bB0OP8OLjkKGfhSW3niqxoM1RRoLvNsL7A0mBp\nwQIhAO0/GNXSdA98JxmNxVdIjPTCSAP6XEoH3R8WFiJ0JBSvAiEAxjn0EDBFKOI7\nhBk1RXlTfMHbDQHrPKFmL7y2GIPBYU8CIGl9a3PJ2QeB2Nz3IlJjSPjQ5U0ASNS\nSN1GMH2eJwYnAiEAoRxE6QUCRzBL7F3l/Dq+IYbwxZkN95Nf8CPm8GjYHD8CIQC8\nuqbJMxbRbhsPNi88e1ZYayVjPhFKEXZfCihqT/YE1Q==\n-----END RSA PRIVATE KEY-----\n","client_email":"invalid@invalid-project.iam.gserviceaccount.com","client_id":"000000000","auth_uri":"https://accounts.google.com/o/oauth2/auth","token_uri":"https://oauth2.googleapis.com/token"}` + invalidSAJSON = `{"type":"service_account","project_id":"invalid-project","private_key_id":"invalid","private_key":"not-a-real-key","client_email":"invalid@invalid-project.iam.gserviceaccount.com","client_id":"000000000","auth_uri":"https://accounts.google.com/o/oauth2/auth","token_uri":"https://oauth2.googleapis.com/token"}`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/networking/cncc_gcp_credentials.go` around lines 50 - 57, The dummySAJSON and invalidSAJSON test fixtures include real-looking PEM private_key literals that trigger secret scanners; update the JSON string values for the "private_key" fields in both dummySAJSON and invalidSAJSON to use clearly fake placeholders (e.g., a short non-sensitive token or a clearly marked placeholder like "<PRIVATE_KEY_PLACEHOLDER>" or "-----BEGIN PRIVATE KEY-----\nFAKE-KEY\n-----END PRIVATE KEY-----") so the JSON remains syntactically valid but contains no PEM-like secret material; ensure you only change the private_key content within the dummySAJSON and invalidSAJSON constants.
78-98:⚠️ Potential issue | 🟠 MajorDifferentiate
NotFoundfrom other secret-read errors in restore flow.The code treats any
Get()error as if the secret was deleted (line 79), without distinguishingNotFoundfrom actual failures like permission denied or API server issues. This hides real failures and triggers incorrect remediation.Suggested fix
+import ( + apierrors "k8s.io/apimachinery/pkg/api/errors" +) ... existing, err := clientset.CoreV1().Secrets(cnccNamespace).Get(context.Background(), cnccSecretName, metav1.GetOptions{}) if err != nil { + if !apierrors.IsNotFound(err) { + o.Expect(err).NotTo(o.HaveOccurred(), "failed to read CNCC secret during restore") + return + } // Secret was deleted; recreate it. newSecret := &corev1.Secret{ ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/networking/cncc_gcp_credentials.go` around lines 78 - 98, The restore flow currently treats any error from clientset.CoreV1().Secrets(cnccNamespace).Get(...) as a missing secret; change it to distinguish a NotFound error using apierrors.IsNotFound(err) (from k8s.io/apimachinery/pkg/api/errors) and only recreate the secret (newSecret with Name cnccSecretName, Namespace cnccNamespace, Annotations/Labels/Data/Type from originalSecret) in that case; for any other Get() error, fail the test or surface the error via o.Expect(err).NotTo(o.HaveOccurred(), ...) (or return the error) instead of attempting to recreate, ensuring existing is only used when Get() succeeds and originalSecret/existing are preserved as before.
🧹 Nitpick comments (2)
test/extended/networking/cncc_gcp_credentials.go (2)
447-449: Clarify node label value intention.Line 448 sets
k8s.ovn.org/egress-assignable=with an empty value. While this may work (OVN-Kubernetes typically just checks for label presence), explicitly setting=trueis more conventional and self-documenting.Suggested fix
- _, err = oc.AsAdmin().Run("label").Args("node", workerNodes[0].Name, "k8s.ovn.org/egress-assignable=", "--overwrite").Output() + _, err = oc.AsAdmin().Run("label").Args("node", workerNodes[0].Name, "k8s.ovn.org/egress-assignable=true", "--overwrite").Output()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/networking/cncc_gcp_credentials.go` around lines 447 - 449, The label call that presently sets "k8s.ovn.org/egress-assignable=" with an empty value should explicitly set a boolean value; update the oc.AsAdmin().Run("label").Args invocation that targets workerNodes[0].Name to use "k8s.ovn.org/egress-assignable=true" (and keep --overwrite) so the label value is explicit and self-documenting while preserving the existing error check after Output().
200-219: UsedeferforlogStream.Close()to prevent resource leaks.If
logStream.Read()panics, the stream won't be closed. Usingdeferensures cleanup regardless of how the function exits.Suggested fix
logStream, err := req.Stream(context.Background()) if err != nil { framework.Logf("Failed to get logs for pod %s: %v", pod.Name, err) continue } + defer logStream.Close() buf := new(strings.Builder) _, _ = fmt.Fprintf(buf, "--- Logs from pod %s ---\n", pod.Name) b := make([]byte, 4096) for { n, readErr := logStream.Read(b) if n > 0 { buf.Write(b[:n]) } if readErr != nil { break } } - logStream.Close() allLogs.WriteString(buf.String())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/networking/cncc_gcp_credentials.go` around lines 200 - 219, The log stream opened by req.Stream(...) is closed only at the end and can leak if Read panics or returns early; immediately after acquiring logStream in the block using clientset.CoreV1().Pods(cnccNamespace).GetLogs(...).Stream, check err as you already do and then call defer logStream.Close() (instead of the later explicit logStream.Close()) so the stream is always closed; remove or avoid the later explicit Close call to prevent double-close and ensure buf and allLogs logic remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/extended/networking/cncc_gcp_credentials.go`:
- Around line 50-57: The dummySAJSON and invalidSAJSON test fixtures include
real-looking PEM private_key literals that trigger secret scanners; update the
JSON string values for the "private_key" fields in both dummySAJSON and
invalidSAJSON to use clearly fake placeholders (e.g., a short non-sensitive
token or a clearly marked placeholder like "<PRIVATE_KEY_PLACEHOLDER>" or
"-----BEGIN PRIVATE KEY-----\nFAKE-KEY\n-----END PRIVATE KEY-----") so the JSON
remains syntactically valid but contains no PEM-like secret material; ensure you
only change the private_key content within the dummySAJSON and invalidSAJSON
constants.
- Around line 78-98: The restore flow currently treats any error from
clientset.CoreV1().Secrets(cnccNamespace).Get(...) as a missing secret; change
it to distinguish a NotFound error using apierrors.IsNotFound(err) (from
k8s.io/apimachinery/pkg/api/errors) and only recreate the secret (newSecret with
Name cnccSecretName, Namespace cnccNamespace, Annotations/Labels/Data/Type from
originalSecret) in that case; for any other Get() error, fail the test or
surface the error via o.Expect(err).NotTo(o.HaveOccurred(), ...) (or return the
error) instead of attempting to recreate, ensuring existing is only used when
Get() succeeds and originalSecret/existing are preserved as before.
---
Nitpick comments:
In `@test/extended/networking/cncc_gcp_credentials.go`:
- Around line 447-449: The label call that presently sets
"k8s.ovn.org/egress-assignable=" with an empty value should explicitly set a
boolean value; update the oc.AsAdmin().Run("label").Args invocation that targets
workerNodes[0].Name to use "k8s.ovn.org/egress-assignable=true" (and keep
--overwrite) so the label value is explicit and self-documenting while
preserving the existing error check after Output().
- Around line 200-219: The log stream opened by req.Stream(...) is closed only
at the end and can leak if Read panics or returns early; immediately after
acquiring logStream in the block using
clientset.CoreV1().Pods(cnccNamespace).GetLogs(...).Stream, check err as you
already do and then call defer logStream.Close() (instead of the later explicit
logStream.Close()) so the stream is always closed; remove or avoid the later
explicit Close call to prevent double-close and ensure buf and allLogs logic
remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85ed3122-0f3b-4687-9c74-7eb01c306201
📒 Files selected for processing (1)
test/extended/networking/cncc_gcp_credentials.go
|
@apahim: This pull request references GCP-429 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 story to target the "4.22.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. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/extended/networking/cncc_gcp_credentials.go (2)
205-223: Consider usingdeferfor stream cleanup.The
logStream.Close()call could be missed if a panic occurs between opening and closing. Usingdeferwould ensure proper cleanup.♻️ Suggested improvement
logStream, err := req.Stream(context.Background()) if err != nil { framework.Logf("Failed to get logs for pod %s: %v", pod.Name, err) continue } + defer logStream.Close() buf := new(strings.Builder) _, _ = fmt.Fprintf(buf, "--- Logs from pod %s ---\n", pod.Name) b := make([]byte, 4096) for { n, readErr := logStream.Read(b) if n > 0 { buf.Write(b[:n]) } if readErr != nil { break } } - logStream.Close() allLogs.WriteString(buf.String())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/networking/cncc_gcp_credentials.go` around lines 205 - 223, The log stream opened via req.Stream(...) is closed explicitly later which can be skipped on panic—after successfully acquiring logStream (the variable returned from req.Stream), immediately schedule cleanup with defer logStream.Close() so the stream is always closed; remove the later explicit logStream.Close() call and keep the read loop as-is (references: req.Stream, logStream, and the read loop writing into buf/allLogs).
451-453: Clarify the egress-assignable label value.The label is set with an empty value (
k8s.ovn.org/egress-assignable=). While this works for presence-based checks, consider using an explicit value like"true"for clarity and consistency with common label conventions.♻️ Optional clarification
- _, err = oc.AsAdmin().Run("label").Args("node", workerNodes[0].Name, "k8s.ovn.org/egress-assignable=", "--overwrite").Output() + _, err = oc.AsAdmin().Run("label").Args("node", workerNodes[0].Name, "k8s.ovn.org/egress-assignable=true", "--overwrite").Output()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/networking/cncc_gcp_credentials.go` around lines 451 - 453, The label on the first worker node is being set with an empty value; change the label call that currently sets "k8s.ovn.org/egress-assignable=" to use an explicit value (e.g., "k8s.ovn.org/egress-assignable=true") in the oc.AsAdmin().Run("label") invocation that references workerNodes[0].Name, and update any related presence-based assertions elsewhere in the test to check for the explicit value if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/extended/networking/cncc_gcp_credentials.go`:
- Around line 205-223: The log stream opened via req.Stream(...) is closed
explicitly later which can be skipped on panic—after successfully acquiring
logStream (the variable returned from req.Stream), immediately schedule cleanup
with defer logStream.Close() so the stream is always closed; remove the later
explicit logStream.Close() call and keep the read loop as-is (references:
req.Stream, logStream, and the read loop writing into buf/allLogs).
- Around line 451-453: The label on the first worker node is being set with an
empty value; change the label call that currently sets
"k8s.ovn.org/egress-assignable=" to use an explicit value (e.g.,
"k8s.ovn.org/egress-assignable=true") in the oc.AsAdmin().Run("label")
invocation that references workerNodes[0].Name, and update any related
presence-based assertions elsewhere in the test to check for the explicit value
if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff3f78ea-3aff-4ca8-9452-79cc6b816d38
📒 Files selected for processing (1)
test/extended/networking/cncc_gcp_credentials.go
|
@apahim: This pull request references GCP-429 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 story to target the "4.22.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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/extended/networking/cncc_gcp_credentials.go (1)
97-101: Restore secret metadata on update path too.When the secret exists, only
DataandTypeare restored. Restoring labels/annotations as well keeps “original state” behavior consistent with the recreate path.Proposed fix
} else { // Secret exists; update data back to original. existing.Data = originalSecret.Data existing.Type = originalSecret.Type + existing.Labels = originalSecret.Labels + existing.Annotations = originalSecret.Annotations _, err = clientset.CoreV1().Secrets(cnccNamespace).Update(context.Background(), existing, metav1.UpdateOptions{}) o.Expect(err).NotTo(o.HaveOccurred(), "failed to restore CNCC secret data") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/networking/cncc_gcp_credentials.go` around lines 97 - 101, When restoring an existing secret in the update path, also copy metadata labels and annotations from originalSecret so the secret's original state matches the recreate path; update the block that currently sets existing.Data = originalSecret.Data and existing.Type = originalSecret.Type to also set existing.Labels = originalSecret.Labels and existing.Annotations = originalSecret.Annotations before calling clientset.CoreV1().Secrets(cnccNamespace).Update(...), and keep the same error check using o.Expect(err).NotTo(o.HaveOccurred()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/networking/cncc_gcp_credentials.go`:
- Around line 432-433: The EgressIP setup uses a fixed name (egressIPName) and
then applies an EgressIP object (type EgressIP), which can silently reuse a
stale object from prior runs; change the test to generate a unique name per run
(e.g., append a timestamp/UUID) or explicitly ensure idempotence by deleting any
existing EgressIP with that name before creating/applying it; update all
occurrences around egressIPName and the EgressIP creation blocks (including the
other locations noted) so the test always creates a fresh resource or cleans up
the old one prior to apply.
- Around line 281-284: When reading the cloud-credentials secret (originalSecret
via clientset.CoreV1().Secrets(cnccNamespace).Get(...)) only treat a NotFound
error as a reason to skipper.Skipf; for all other errors (API/auth/transient
failures) fail the test instead of skipping by returning or calling the test
failure helper and include the error details. Use apierrors.IsNotFound(err)
(import "k8s.io/apimachinery/pkg/api/errors" as apierrors) to distinguish
NotFound from other errors and keep the existing skipper.Skipf(cnccNamespace,
cnccSecretName...) branch for the NotFound case.
---
Nitpick comments:
In `@test/extended/networking/cncc_gcp_credentials.go`:
- Around line 97-101: When restoring an existing secret in the update path, also
copy metadata labels and annotations from originalSecret so the secret's
original state matches the recreate path; update the block that currently sets
existing.Data = originalSecret.Data and existing.Type = originalSecret.Type to
also set existing.Labels = originalSecret.Labels and existing.Annotations =
originalSecret.Annotations before calling
clientset.CoreV1().Secrets(cnccNamespace).Update(...), and keep the same error
check using o.Expect(err).NotTo(o.HaveOccurred()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5878b4a7-11d4-4310-98b0-bf338906f908
📒 Files selected for processing (1)
test/extended/networking/cncc_gcp_credentials.go
| originalSecret, err = clientset.CoreV1().Secrets(cnccNamespace).Get(context.Background(), cnccSecretName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| skipper.Skipf("Skipping CNCC credential tests: cloud-credentials secret not found (possible HCP cluster): %v", err) | ||
| } |
There was a problem hiding this comment.
Avoid skipping on non-NotFound secret read failures.
At Line 282, any Get error is treated as “secret not found” and skipped. That can hide real API/auth/transient failures as skipped tests.
Proposed fix
- originalSecret, err = clientset.CoreV1().Secrets(cnccNamespace).Get(context.Background(), cnccSecretName, metav1.GetOptions{})
- if err != nil {
- skipper.Skipf("Skipping CNCC credential tests: cloud-credentials secret not found (possible HCP cluster): %v", err)
- }
+ originalSecret, err = clientset.CoreV1().Secrets(cnccNamespace).Get(context.Background(), cnccSecretName, metav1.GetOptions{})
+ if apierrors.IsNotFound(err) {
+ skipper.Skipf("Skipping CNCC credential tests: cloud-credentials secret not found (possible HCP cluster)")
+ }
+ o.Expect(err).NotTo(o.HaveOccurred(), "failed to read CNCC cloud-credentials secret")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/networking/cncc_gcp_credentials.go` around lines 281 - 284,
When reading the cloud-credentials secret (originalSecret via
clientset.CoreV1().Secrets(cnccNamespace).Get(...)) only treat a NotFound error
as a reason to skipper.Skipf; for all other errors (API/auth/transient failures)
fail the test instead of skipping by returning or calling the test failure
helper and include the error details. Use apierrors.IsNotFound(err) (import
"k8s.io/apimachinery/pkg/api/errors" as apierrors) to distinguish NotFound from
other errors and keep the existing skipper.Skipf(cnccNamespace,
cnccSecretName...) branch for the NotFound case.
| egressIPName := "cncc-cred-test-egressip" | ||
| egressIPObj := &EgressIP{ |
There was a problem hiding this comment.
Prevent stale-object reuse in EgressIP setup.
Using a fixed name plus oc apply can silently reuse an old object from a prior interrupted run, making this test non-deterministic.
Proposed fix
- egressIPName := "cncc-cred-test-egressip"
+ egressIPName := fmt.Sprintf("cncc-cred-test-egressip-%d", time.Now().UnixNano())
...
- err = createEgressIPFromJSON(oc, egressIPJSON)
+ err = createEgressIPFromJSON(oc, egressIPJSON) func createEgressIPFromJSON(oc *exutil.CLI, jsonData []byte) error {
- return oc.AsAdmin().Run("apply").Args("-f", "-").InputString(string(jsonData)).Execute()
+ return oc.AsAdmin().Run("create").Args("-f", "-").InputString(string(jsonData)).Execute()
}Also applies to: 458-459, 507-509
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/networking/cncc_gcp_credentials.go` around lines 432 - 433, The
EgressIP setup uses a fixed name (egressIPName) and then applies an EgressIP
object (type EgressIP), which can silently reuse a stale object from prior runs;
change the test to generate a unique name per run (e.g., append a
timestamp/UUID) or explicitly ensure idempotence by deleting any existing
EgressIP with that name before creating/applying it; update all occurrences
around egressIPName and the EgressIP creation blocks (including the other
locations noted) so the test always creates a fresh resource or cleans up the
old one prior to apply.
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-4.22-e2e-gcp-ovn openshift/cloud-network-config-controller#206 |
|
@apahim: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/67ccb430-1813-11f1-9f44-422dfd494bcd-0 |
|
Scheduling required tests: |
|
The new |
|
/retest |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 6a54273
New tests seen in this PR at sha: 6a54273
|
|
/retest-required |
the issue is that getCnccPodLogs(100) at line 309 doesn't capture the startup log message because the pod has been running for over an hour by the time the smoke test executes. fixing. |
Validates the three-tier GCP credential fallback chain (WIF → SA → GOOGLE_APPLICATION_CREDENTIALS) with six test cases covering smoke verification, credential rotation, WIF priority, invalid credentials, secret deletion/recovery, and IP management persistence across rotation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Amador Pahim <apahim@redhat.com>
|
@apahim: This pull request references GCP-429 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 story to target the "4.22.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. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/extended/networking/cncc_gcp_credentials.go (2)
283-286:⚠️ Potential issue | 🟠 MajorOnly skip on
NotFoundfor cloud-credentials lookup (still open).At Line 284, any
Geterror is treated as “secret not found,” which can hide real API/auth failures as skips.Proposed fix
originalSecret, err = clientset.CoreV1().Secrets(cnccNamespace).Get(context.Background(), cnccSecretName, metav1.GetOptions{}) - if err != nil { - skipper.Skipf("Skipping CNCC credential tests: cloud-credentials secret not found (possible HCP cluster): %v", err) - } + if apierrors.IsNotFound(err) { + skipper.Skipf("Skipping CNCC credential tests: cloud-credentials secret not found (possible HCP cluster)") + } + o.Expect(err).NotTo(o.HaveOccurred(), "failed to read CNCC cloud-credentials secret")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/networking/cncc_gcp_credentials.go` around lines 283 - 286, The current Get call treating any error as "secret not found" hides real API/auth failures; update the error handling after clientset.CoreV1().Secrets(cnccNamespace).Get(...) (using cnccNamespace and cnccSecretName) to check apierrors.IsNotFound(err) and only call skipper.Skipf(...) for NotFound; for any other error return/fail the test (or t.Fatalf/log+fail) so real errors surface instead of being skipped.
434-435:⚠️ Potential issue | 🟠 MajorAvoid stale EgressIP reuse across runs (still open).
Using a fixed name plus
oc applycan silently reuse a leftover object and make this test non-deterministic.Proposed fix
- egressIPName := "cncc-cred-test-egressip" + egressIPName := fmt.Sprintf("cncc-cred-test-egressip-%d", time.Now().UnixNano())func createEgressIPFromJSON(oc *exutil.CLI, jsonData []byte) error { - return oc.AsAdmin().Run("apply").Args("-f", "-").InputString(string(jsonData)).Execute() + return oc.AsAdmin().Run("create").Args("-f", "-").InputString(string(jsonData)).Execute() }Also applies to: 458-461, 509-510
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/networking/cncc_gcp_credentials.go` around lines 434 - 435, The test currently reuses a fixed egressIPName ("cncc-cred-test-egressip") and creates an EgressIP object (egressIPObj), which can silently reuse leftover resources across runs; change the creation to use a unique name per test run (e.g., append a timestamp/UUID/test-run-id or use the test framework's unique suffix) and/or ensure any existing object with that name is deleted before applying; update every occurrence where egressIPName and egressIPObj are used (the blocks around the earlier occurrences noted) so the test creates and cleans up a deterministic, non-colliding EgressIP resource instead of relying on a fixed name.
🧹 Nitpick comments (1)
test/extended/networking/cncc_gcp_credentials.go (1)
365-367: Prefer non-tailed logs for startup-credential assertions.At Line 366, using
getCnccPodLogs(100)can still miss startup messages under noisy logs. Consider full logs (or logs since restart) for this assertion.Suggested change
- logs := getCnccPodLogs(100) + logs := getCnccPodLogs(0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/networking/cncc_gcp_credentials.go` around lines 365 - 367, The test currently uses getCnccPodLogs(100) which tails only recent lines and may miss startup messages; change the assertion to fetch full pod logs (or logs since the pod restart) before checking for the WIF message. Locate the call to getCnccPodLogs(100) and replace it with a function or parameter that returns full logs (or use a variant like getCnccPodLogsSinceRestart or getFullCnccPodLogs) so the subsequent o.Expect(logs).To(o.ContainSubstring("Using GCP Workload Identity Federation credentials from secret")) reliably sees the startup credential message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/networking/cncc_gcp_credentials.go`:
- Around line 295-300: The AfterEach teardown always calls restoreCnccSecret()
and waitForCnccReady() which can dereference uninitialized globals (clientset,
originalSecret) when setup skipped; guard the teardown by tracking setup success
(e.g., add a boolean flag like cnccSetupComplete set true at end of successful
setup in BeforeEach) and only call restoreCnccSecret() and waitForCnccReady()
when that flag is true (or alternatively check clientset != nil and
originalSecret != nil before invoking those functions) to avoid nil pointer
access in waitForCnccReady().
---
Duplicate comments:
In `@test/extended/networking/cncc_gcp_credentials.go`:
- Around line 283-286: The current Get call treating any error as "secret not
found" hides real API/auth failures; update the error handling after
clientset.CoreV1().Secrets(cnccNamespace).Get(...) (using cnccNamespace and
cnccSecretName) to check apierrors.IsNotFound(err) and only call
skipper.Skipf(...) for NotFound; for any other error return/fail the test (or
t.Fatalf/log+fail) so real errors surface instead of being skipped.
- Around line 434-435: The test currently reuses a fixed egressIPName
("cncc-cred-test-egressip") and creates an EgressIP object (egressIPObj), which
can silently reuse leftover resources across runs; change the creation to use a
unique name per test run (e.g., append a timestamp/UUID/test-run-id or use the
test framework's unique suffix) and/or ensure any existing object with that name
is deleted before applying; update every occurrence where egressIPName and
egressIPObj are used (the blocks around the earlier occurrences noted) so the
test creates and cleans up a deterministic, non-colliding EgressIP resource
instead of relying on a fixed name.
---
Nitpick comments:
In `@test/extended/networking/cncc_gcp_credentials.go`:
- Around line 365-367: The test currently uses getCnccPodLogs(100) which tails
only recent lines and may miss startup messages; change the assertion to fetch
full pod logs (or logs since the pod restart) before checking for the WIF
message. Locate the call to getCnccPodLogs(100) and replace it with a function
or parameter that returns full logs (or use a variant like
getCnccPodLogsSinceRestart or getFullCnccPodLogs) so the subsequent
o.Expect(logs).To(o.ContainSubstring("Using GCP Workload Identity Federation
credentials from secret")) reliably sees the startup credential message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 350e41a3-d2d0-4084-a3c2-c324d966c4f5
📒 Files selected for processing (1)
test/extended/networking/cncc_gcp_credentials.go
| g.AfterEach(func() { | ||
| g.By("Restoring CNCC secret to original state") | ||
| restoreCnccSecret() | ||
| g.By("Waiting for CNCC to recover after secret restoration") | ||
| waitForCnccReady() | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n test/extended/networking/cncc_gcp_credentials.go | head -350Repository: openshift/origin
Length of output: 16763
🏁 Script executed:
cat test/extended/networking/cncc_gcp_credentials.go | wc -lRepository: openshift/origin
Length of output: 63
🏁 Script executed:
sed -n '258,293p' test/extended/networking/cncc_gcp_credentials.go | cat -nRepository: openshift/origin
Length of output: 1954
🏁 Script executed:
# Check the exact line numbers of skips in BeforeEach
grep -n "skipper.Skipf\|Skipf" test/extended/networking/cncc_gcp_credentials.go | head -20Repository: openshift/origin
Length of output: 684
🏁 Script executed:
# Let me trace through the variable initialization more carefully
# Check the exact state of variables at each skip point
cat > /tmp/trace.txt << 'EOF'
BeforeEach sequence:
Line 262: cloudType = infra.Spec.PlatformSpec.Type (ALWAYS SET)
Line 265: if cloudType != GCP { Skipf(...) } (SKIP `#1`)
Line 269: clientset = oc.KubeFramework().ClientSet (ONLY IF NOT SKIPPED `#1`)
Line 279: if !CRD { Skipf(...) } (SKIP `#2`, requires clientset from line 269)
Line 283: originalSecret = Get(...) (ONLY IF NOT SKIPPED `#2`)
Line 285: if err != nil { Skipf(...) } (SKIP `#3`, originalSecret could be nil)
Line 290: if credType == Unknown { Skipf(...) } (SKIP `#4`)
When AfterEach runs after each skip scenario:
SKIP `#1` (not GCP): cloudType=SET, clientset=NIL, originalSecret=NIL
SKIP `#2` (no CRD): cloudType=SET, clientset=SET, originalSecret=NIL
SKIP `#3` (no secret): cloudType=SET, clientset=SET, originalSecret=NIL
SKIP `#4` (unknown type): cloudType=SET, clientset=SET, originalSecret=SET
AfterEach guards:
- cloudType != GCPPlatformType: Returns early if skip `#1` occurred
- clientset == nil: Returns early if skip `#1` occurred (covers nil access at line 109)
- originalSecret == nil: Returns early if skip `#1`, `#2`, or `#3` occurred
EOF
cat /tmp/trace.txtRepository: openshift/origin
Length of output: 1122
🏁 Script executed:
# Check if there are any integration tests or documentation about AfterEach behavior
grep -r "AfterEach" test/extended/networking/cncc_gcp_credentials.go -A 5 -B 2Repository: openshift/origin
Length of output: 246
🏁 Script executed:
# Verify waitForCnccReady uses clientset (line 109)
sed -n '105,125p' test/extended/networking/cncc_gcp_credentials.goRepository: openshift/origin
Length of output: 1116
Guard teardown when setup did not fully initialize.
AfterEach always runs waitForCnccReady(). If setup skips early (non-GCP platform, missing CRD, or missing secret), clientset or originalSecret remain uninitialized, causing waitForCnccReady() to fail with a nil pointer access at line 109.
Proposed fix
g.AfterEach(func() {
+ if cloudType != configv1.GCPPlatformType || clientset == nil || originalSecret == nil {
+ return
+ }
g.By("Restoring CNCC secret to original state")
restoreCnccSecret()
g.By("Waiting for CNCC to recover after secret restoration")
waitForCnccReady()
})📝 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.
| g.AfterEach(func() { | |
| g.By("Restoring CNCC secret to original state") | |
| restoreCnccSecret() | |
| g.By("Waiting for CNCC to recover after secret restoration") | |
| waitForCnccReady() | |
| }) | |
| g.AfterEach(func() { | |
| if cloudType != configv1.GCPPlatformType || clientset == nil || originalSecret == nil { | |
| return | |
| } | |
| g.By("Restoring CNCC secret to original state") | |
| restoreCnccSecret() | |
| g.By("Waiting for CNCC to recover after secret restoration") | |
| waitForCnccReady() | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/networking/cncc_gcp_credentials.go` around lines 295 - 300, The
AfterEach teardown always calls restoreCnccSecret() and waitForCnccReady() which
can dereference uninitialized globals (clientset, originalSecret) when setup
skipped; guard the teardown by tracking setup success (e.g., add a boolean flag
like cnccSetupComplete set true at end of successful setup in BeforeEach) and
only call restoreCnccSecret() and waitForCnccReady() when that flag is true (or
alternatively check clientset != nil and originalSecret != nil before invoking
those functions) to avoid nil pointer access in waitForCnccReady().
|
Scheduling required tests: |
|
@apahim: 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. |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: f3423dc
New tests seen in this PR at sha: f3423dc
|
3 participants
Validates the three-tier GCP credential fallback chain (WIF → SA → GOOGLE_APPLICATION_CREDENTIALS) with six test cases covering smoke verification, credential rotation, WIF priority, invalid credentials, secret deletion/recovery, and IP management persistence across rotation.
Summary by CodeRabbit