OCPBUGS-105456: fix KMS service agent domain for sovereign clouds - #10752
OCPBUGS-105456: fix KMS service agent domain for sovereign clouds#10752barbacbd wants to merge 5 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@barbacbd: This pull request references Jira Issue OCPBUGS-105456, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
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:
📝 WalkthroughWalkthroughThe GCP Cluster API provider module now targets Go 1.26.0, uses a newer CAPG pseudo-version, removes CAPG replacement directives, and refreshes indirect dependencies. ChangesGCP module refresh
Estimated code review effort: 2 (Simple) | ~10 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/asset/installconfig/gcp/validation_test.go`:
- Around line 1998-1999: Add a regression case in the validation tests using a
valid non-sovereign domain-scoped project where gcp.GetCloudEnvironment does not
return gcp.CloudEnvironmentSovereign. Assert that the Compute Engine and Cloud
Storage service-agent identities use unsuffixed domains, while retaining the
existing eu0:openshift sovereign coverage.
🪄 Autofix
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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8df0ef5d-cece-4334-bcf6-4d8d18af03a2
📒 Files selected for processing (2)
pkg/asset/installconfig/gcp/validation.gopkg/asset/installconfig/gcp/validation_test.go
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@barbacbd: This pull request references Jira Issue OCPBUGS-105456, which is valid. 3 validation(s) were run on this bug
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. |
2781e7a to
e46d26e
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
openshift-ci
Bot
added
the
needs-rebase
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Caution CodeRabbit couldn't update its existing comment. The review summary may be out of date. Error details |
| sovereignComputeAgent := "serviceAccount:service-123456789@compute-system.eu0-system.iam.gserviceaccount.com" | ||
| sovereignGCSAgent := "serviceAccount:service-123456789@gs-project-accounts.eu0-system.iam.gserviceaccount.com" | ||
| nonSovereignComputeAgent := "serviceAccount:service-123456789@compute-system.iam.gserviceaccount.com" | ||
| nonSovereignGCSAgent := "serviceAccount:service-123456789@gs-project-accounts.iam.gserviceaccount.com" |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cluster-api/providers/gcp/go.mod (1)
7-112: 🔒 Security & Privacy | 🔵 TrivialComplete the supply-chain checks for the refreshed graph.
The file pins versions and the supplied
go.sumcontext records CAPG checksums. It does not establish license compatibility, absence of new OSV findings, vendor synchronization, SBOM/provenance attestations, or Sigstore/cosign signatures. Compare the old and newgo.modandgo.sum, and scope vulnerability findings to dependencies changed by this PR.As per coding guidelines, use
go get,go mod tidy, andgo mod vendor, with vendored code committed separately. As per path instructions, cross-reference changed dependencies withosv.devand verify SBOM/provenance and artifact signing. Based on learnings, runmake precommitbefore considering this change complete.🤖 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 `@cluster-api/providers/gcp/go.mod` around lines 7 - 112, Complete supply-chain validation for the refreshed dependency graph in go.mod and go.sum: compare old and new dependency changes, check license compatibility, and scope osv.dev findings to changed dependencies. Run go get, go mod tidy, and go mod vendor; verify vendor synchronization, SBOM/provenance attestations, and Sigstore/cosign signatures, committing vendored updates separately. Run make precommit before completion.Sources: Coding guidelines, Path instructions, Learnings
🤖 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 `@cluster-api/providers/gcp/go.mod`:
- Line 5: Replace the unreleased sigs.k8s.io/cluster-api-provider-gcp dependency
in go.mod with a released CAPG tag that contains the KMS validation fix; do not
retain the pseudo-version unless an approved production-security exception
includes provenance and signing evidence.
---
Nitpick comments:
In `@cluster-api/providers/gcp/go.mod`:
- Around line 7-112: Complete supply-chain validation for the refreshed
dependency graph in go.mod and go.sum: compare old and new dependency changes,
check license compatibility, and scope osv.dev findings to changed dependencies.
Run go get, go mod tidy, and go mod vendor; verify vendor synchronization,
SBOM/provenance attestations, and Sigstore/cosign signatures, committing
vendored updates separately. Run make precommit before completion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| ) | ||
|
|
||
| require sigs.k8s.io/cluster-api-provider-gcp v1.10.1-0.20251030112604-5c2bfd5aadb3 | ||
| require sigs.k8s.io/cluster-api-provider-gcp v1.13.1-0.20260810150012-23049b61863d |
There was a problem hiding this comment.
Let's also sync with top-level go.mod?
Line 129 in 1b579f1
There was a problem hiding this comment.
Nevermind, Github didn't show the affected top-level go mod file because the commit is way too large.
91e2dbc to
c162cc6
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Caution CodeRabbit couldn't update its existing comment. The review summary may be out of date. Error details |
c162cc6 to
0b2df01
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Caution CodeRabbit couldn't update its existing comment. The review summary may be out of date. Error details |
0b2df01 to
3b43e51
Compare
openshift-ci
Bot
added
the
needs-rebase
Domain-scoped project IDs were unconditionally applying the domain prefix to service agent email addresses, but sovereign cloud service agents use a "-system" suffix (e.g. "eu0-system") rather than just the bare domain prefix. Restrict the domain-scoped logic to sovereign cloud environments using GetCloudEnvironment and append the correct suffix.
* Update the component CRD for CAPG to allow/validate the service account(s) and project ID for sovereign clouds. The Project ID can now include a colon and the service accounts can include a period in places other than the expected suffix.
3b43e51 to
f0e8d17
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/test unit /lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon 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
| go.podman.io/image/v5 v5.37.0 | ||
| go.uber.org/mock v0.6.0 | ||
| golang.org/x/crypto v0.53.0 | ||
| golang.org/x/crypto v0.54.0 |
There was a problem hiding this comment.
I suspect golang.org/x/crypto v0.54.0 bump causes the unit test to fail:
The commit of interest: 7626c50 ssh: verify declared key type matches decoded key in authorized_keys by Nicola Murino · 10 weeks ago
That said, we just need to mark the case below as valid: false:
installer/pkg/validate/validate_test.go
Lines 616 to 620 in 3a117ed
The test for invalid format should have been failing and the valid was set to true. The new crypto package may have caused this to finally come up. setting the validity to false to make sure the test is correct.
|
Scheduling tests matching the |
|
woah, that's a massive list of jobs to run. Yolo why not 😅 |
|
/test e2e-gcp-kms-encryption |
|
@barbacbd: 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. |
4 participants
Domain-scoped project IDs were unconditionally applying the domain prefix to service agent email addresses, but sovereign cloud service agents use a "-system" suffix (e.g. "eu0-system") rather than just the bare domain prefix. Restrict the domain-scoped logic to sovereign cloud environments using GetCloudEnvironment and append the correct suffix.
Summary by CodeRabbit