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 3 commits into
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. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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)
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
|
/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. |
|
/retest-required |
|
The e2e-gcp-ovn test is failing because it needs the CNCC PR merged: It passes here, with a CNCC build including the PR: |
|
/retest-required |
|
@ricky-rav the e2e-aws-ovn-microshift seems to be broken since Mar 5: The new tests here are passing when using a build of the CNCC PR; |
|
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
|
ricky-rav
left a comment
ricky-rav
left a comment
There was a problem hiding this comment.
Overall looks very good, I've left two minor comments
- Consolidate dummySAJSON and invalidSAJSON into single fakeSAJSON constant - Differentiate NotFound from other errors when reading cloud-credentials secret - Guard AfterEach teardown against nil clientset/originalSecret on skipped tests - Use unique EgressIP names per run to prevent stale-object reuse - Switch from oc apply to oc create for EgressIP creation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/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/b4819320-1e00-11f1-9f43-e9ee7a909854-0 |
|
Scheduling required tests: |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: d0f8466
New tests seen in this PR at sha: d0f8466
|
1 similar comment
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: d0f8466
New tests seen in this PR at sha: d0f8466
|
| for k, v := range dep.Spec.Selector.MatchLabels { | ||
| parts = append(parts, fmt.Sprintf("%s=%s", k, v)) | ||
| } | ||
| return strings.Join(parts, ",") |
There was a problem hiding this comment.
return metav1.FormatLabelSelector(dep.Spec.Selector) ?
| case credentialTypeWIF: | ||
| o.Expect(logs).To(o.ContainSubstring("Using GCP Workload Identity Federation credentials from secret")) | ||
| case credentialTypeSA: | ||
| o.Expect(logs).To(o.ContainSubstring("Using GCP service account JSON credentials from secret")) |
There was a problem hiding this comment.
We shouldn't be asserting on exact strings logged by another component.
(In general, it seems like this test ought to be using that new openshift testing thing where we build the tests in the repo of the component that they're testing, and then this would be fine. But I don't know the details of how that all works...)
There was a problem hiding this comment.
CNCC doen't use addopt the OTE framework yet.
Ideally we would rely on CNCC conditions here, but we don't have any status field that exposes which credential type was selected.
If you're ok with it, we can remove the log assertions. The test already proves WIF priority indirectly: it adds a fake SA alongside the real WIF config. If CNCC picked the fake SA, it would crash.
| // Test 1: Smoke — current credential type works | ||
| g.It("should have a working CNCC deployment with valid GCP credentials", func() { | ||
| g.By("Verifying CNCC deployment is ready") | ||
| waitForCnccReady() |
There was a problem hiding this comment.
If CNCC isn't ready then CNO should report not ready, in which case the installer should report not ready, in which case the tests should not be running yet. So is this really needed?
If it is needed, isn't it needed in the other tests too? (The tests aren't guaranteed to be run in the order they appear in. Note that getCnccPodRestartCount() in the other tests will just return 0 if CNCC isn't running, rather than waiting for it to be running.)
There was a problem hiding this comment.
Good point, removing then waitForCnccReady().
- Rename Cncc to CNCC in function names per Go acronym convention - Use metav1.FormatLabelSelector instead of manual label construction - Remove waitForCNCCReady from smoke test (CNO guarantees readiness) - Remove log string assertions (avoid coupling to another component's log output); rely on functional verification instead - Remove unused getCNCCPodLogs function and strings import Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@apahim: The following test 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. |
|
/lgtm it would be cool if there was some TRT dashboard that could notice if some e2e test got skipped everywhere but for now, 🤷♂️ |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: apahim, danwinship, ricky-rav The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/retest-required |
1 similar comment
|
/retest-required |
|
@apahim this is apparently waiting for a |
|
/verified by @apahim |
openshift-ci-robot
added
the
verified
|
@apahim: This PR has been marked as verified by 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. |
4 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