CORS-4420, CORS-4421, CORS-4422, CORS-4449, CORS-4514, CORS-4515: Add GCP Germany Sovereign Cloud support#10630
CORS-4420, CORS-4421, CORS-4422, CORS-4449, CORS-4514, CORS-4515: Add GCP Germany Sovereign Cloud support#10630barbacbd wants to merge 4 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@barbacbd: This pull request references CORS-4420 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 "5.0.0" version, but no target version was set. This pull request references CORS-4421 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 "5.0.0" version, but no target version was set. This pull request references CORS-4422 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 "5.0.0" version, but no target version was set. This pull request references CORS-4449 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 "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. |
barbacbd
force-pushed
the
installer_config_updates_gcd
branch
from
d5d735c to
4d29509
Compare
|
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:
📝 WalkthroughWalkthroughAdds Germany sovereign cloud support for GCP. A new ChangesGCP Germany Sovereign Cloud Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 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/types/gcp/validation/platform.go`:
- Around line 225-227: The error message in the field.Invalid call for the
projectID validation is providing incorrect guidance by telling users to set
cloudEnvironment, which is not user-configurable in the install-config. Update
the error message to instead inform users about the supported projectID format
requirements, removing the reference to cloudEnvironment and providing clear,
actionable guidance on what projectID format is expected for public GCP.
- Around line 260-264: The variable errMsg is assigned values on lines 260-263
but is never used, causing a compilation error. Remove the unused errMsg
assignment and construction logic, or alternatively, update the
field.NotSupported() call to use the errMsg variable as the error message
parameter instead of validValues, ensuring the custom error message is actually
included in the validation error that gets appended to allErrs.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 34256dc5-08cf-48df-ae74-27ee08ebaf8e
📒 Files selected for processing (3)
pkg/asset/manifests/gcp/cluster.gopkg/types/gcp/platform.gopkg/types/gcp/validation/platform.go
barbacbd
force-pushed
the
installer_config_updates_gcd
branch
3 times, most recently
from
fa4e9e0 to
99d9bed
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
barbacbd
force-pushed
the
installer_config_updates_gcd
branch
from
99d9bed to
92064ad
Compare
barbacbd
changed the title
|
@barbacbd: This pull request references CORS-4420 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 "5.0.0" version, but no target version was set. This pull request references CORS-4421 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 "5.0.0" version, but no target version was set. This pull request references CORS-4422 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 "5.0.0" version, but no target version was set. This pull request references CORS-4449 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 "5.0.0" version, but no target version was set. This pull request references CORS-4514 which is a valid jira issue. This pull request references CORS-4515 which is a valid jira issue. 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: 4
🤖 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.go`:
- Around line 215-216: The code references the package qualifier `gcptypes` on
lines 215-216 in the calls to `GetCloudEnvironment()` and the constant
`CloudEnvironmentGermanySovereign`, but the GCP types package is imported with
the alias `gcp` at line 22. Replace all instances of `gcptypes` with the correct
package alias `gcp` to match the import declaration and resolve the compilation
error.
In `@pkg/types/gcp/machinepools.go`:
- Around line 383-414: Add unit tests for the functions
`DefaultDiskTypeForInstanceAndProjectID` and `GetDiskTypes` in the machinepools
package. The tests should cover disk type selection logic for both public GCP
and sovereign cloud environments, including edge cases such as a3-ultra
instances and fallback behavior when specific disk types are not available.
Create test cases that verify correct disk type assignments for different
instance types and project configurations.
In `@pkg/types/gcp/validation/platform.go`:
- Around line 20-24: The sovereignCloudRegions map for
gcp.CloudEnvironmentGermanySovereign currently only allows u-germany-northeast1,
but the PR documentation specifies that supported Germany sovereign regions are
europe-west3 and europe-west4. Update the sovereignCloudRegions map entry for
gcp.CloudEnvironmentGermanySovereign to replace the u-germany-northeast1 region
with both europe-west3 and europe-west4, each with appropriate display name
mappings.
- Around line 285-286: The logrus.Warnf call that logs the universe domain
mismatch is exposing sensitive internal hostnames/domain information by
including the raw universeDomain and envUniverseDomain values in the log
message. Modify the warning log to remove or sanitize these actual values,
instead logging a generic message that indicates a mismatch exists without
revealing the specific domain details. This prevents leaking internal
infrastructure information into shared logs while still alerting to the
configuration inconsistency.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 98fc7c9a-eca8-44d5-843b-ddcfc0d69d79
📒 Files selected for processing (10)
pkg/asset/cluster/gcp/gcp.gopkg/asset/installconfig/gcp/validation.gopkg/asset/machines/worker.gopkg/asset/manifests/gcp/cluster.gopkg/types/gcp/defaults/machinepool.gopkg/types/gcp/defaults/platform.gopkg/types/gcp/machinepools.gopkg/types/gcp/metadata.gopkg/types/gcp/platform.gopkg/types/gcp/validation/platform.go
✅ Files skipped from review due to trivial changes (1)
- pkg/types/gcp/defaults/machinepool.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/asset/manifests/gcp/cluster.go
| func DefaultDiskTypeForInstanceAndProjectID(instanceType, projectID string) string { | ||
| defaultDiskType := PDSSD | ||
| diskTypes, ok := GetDiskTypes(instanceType) | ||
| if ok { | ||
| supportedDiskTypes := sets.New(diskTypes...) | ||
| switch { | ||
| case supportedDiskTypes.Has(PDSSD): | ||
| defaultDiskType = PDSSD | ||
| case supportedDiskTypes.Has(HyperDiskBalanced): | ||
| defaultDiskType = HyperDiskBalanced | ||
| default: | ||
| // this shouldn't happen because all supported instance types | ||
| // have either pd-ssd or hyperdisk balanced | ||
| defaultDiskType = diskTypes[0] | ||
| cloudEnv := GetCloudEnvironment(projectID) | ||
|
|
||
| // Sovereign cloud prefers hyperdisk-balanced | ||
| if cloudEnv == CloudEnvironmentGermanySovereign { | ||
| switch { | ||
| case supportedDiskTypes.Has(HyperDiskBalanced): | ||
| defaultDiskType = HyperDiskBalanced | ||
| case supportedDiskTypes.Has(PDSSD): | ||
| defaultDiskType = PDSSD | ||
| default: | ||
| defaultDiskType = diskTypes[0] | ||
| } | ||
| } else { | ||
| // Public GCP prefers pd-ssd | ||
| switch { | ||
| case supportedDiskTypes.Has(PDSSD): | ||
| defaultDiskType = PDSSD | ||
| case supportedDiskTypes.Has(HyperDiskBalanced): | ||
| defaultDiskType = HyperDiskBalanced | ||
| default: | ||
| // this shouldn't happen because all supported instance types | ||
| // have either pd-ssd or hyperdisk balanced | ||
| defaultDiskType = diskTypes[0] | ||
| } | ||
| } | ||
| } | ||
| return defaultDiskType |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if GetDiskTypes can return empty slice with ok=true
ast-grep --pattern $'func GetDiskTypes($$$) ($_, $_) {
$$$
}'Repository: openshift/installer
Length of output: 1012
🏁 Script executed:
#!/bin/bash
# Read the actual code at lines 383-414
sed -n '383,414p' pkg/types/gcp/machinepools.goRepository: openshift/installer
Length of output: 1053
🏁 Script executed:
#!/bin/bash
# Find the definition of InstanceTypeToDiskTypeMap
rg 'InstanceTypeToDiskTypeMap' pkg/types/gcp/machinepools.go -B 2 -A 10Repository: openshift/installer
Length of output: 815
🏁 Script executed:
#!/bin/bash
# Check for unit tests for the new function
fd -t f '*_test.go' pkg/types/gcp/ -exec grep -l 'DefaultDiskTypeForInstanceAndProjectID\|GetDiskTypes' {} \;Repository: openshift/installer
Length of output: 599
🏁 Script executed:
#!/bin/bash
# Find test files in pkg/types/gcp
find pkg/types/gcp -name '*_test.go' -type fRepository: openshift/installer
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Search for tests related to the new function or GetDiskTypes
rg 'DefaultDiskTypeForInstanceAndProjectID|GetDiskTypes' pkg/types/gcp/ --type goRepository: openshift/installer
Length of output: 843
🏁 Script executed:
#!/bin/bash
# Check the test files to see what's tested
cat pkg/types/gcp/validation/machinepool_test.go | head -100Repository: openshift/installer
Length of output: 2530
🏁 Script executed:
#!/bin/bash
# Look for any test related to disk type selection or the new function
rg 'DefaultDiskType|DiskType' pkg/types/gcp/validation/ -B 2 -A 5Repository: openshift/installer
Length of output: 5516
🏁 Script executed:
#!/bin/bash
# Check if there are any tests in pkg/types/gcp/machinepools.go or related files
find pkg/types/gcp -name '*machinepools*test*.go' -o -name '*test*machinepools*.go'Repository: openshift/installer
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Search more broadly for tests that test DefaultDiskType or GetDiskTypes
rg 'func.*Test.*Disk|func.*Test.*Default' pkg/types/gcp/ --type goRepository: openshift/installer
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Verify the guideline requirement for unit tests
# Check if there's any test file for machinepools
ls -la pkg/types/gcp/ | grep -i testRepository: openshift/installer
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check the defaults directory where the function is called from
cat pkg/types/gcp/defaults/machinepool.go | head -50Repository: openshift/installer
Length of output: 1014
Add unit tests for DefaultDiskTypeForInstanceAndProjectID and GetDiskTypes
Code in pkg/ requires unit tests per project guidelines. These functions currently lack test coverage. Add tests to verify disk type selection logic for both public GCP and sovereign cloud environments, including edge cases like a3-ultra instances and fallback behavior.
🤖 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 `@pkg/types/gcp/machinepools.go` around lines 383 - 414, Add unit tests for the
functions `DefaultDiskTypeForInstanceAndProjectID` and `GetDiskTypes` in the
machinepools package. The tests should cover disk type selection logic for both
public GCP and sovereign cloud environments, including edge cases such as
a3-ultra instances and fallback behavior when specific disk types are not
available. Create test cases that verify correct disk type assignments for
different instance types and project configurations.
92064ad to
6add22b
Compare
There was a problem hiding this comment.
There's a lot of good stuff here, but we need to edit down somewhat ruthlessly to focus just on the core type stuff, like passing in the universe domain and setting it.
gcd sovereign clouds will extend to many more regions, all of the others would most likely be outside germany, so let's drop the specifics about german cloud
If we need to switch on gcd, let's make an isGCD function that determines we're running on gcd. Right now you're making that on the eu0 prefix, but I think we might see more prefixes. Perhaps we do it solely based on the :? I don't know if there are any other indicating factors, especially if we make universe domain optional.
I do wonder whether we even need an isGCD function? The main need I'm aware of is to switch on instance type, and maybe also disk type? There is an API that would get us at least available instance types.
barbacbd
force-pushed
the
installer_config_updates_gcd
branch
2 times, most recently
from
1a3a9b5 to
30f1139
Compare
barbacbd
force-pushed
the
installer_config_updates_gcd
branch
from
30f1139 to
e0ba5ca
Compare
rochacbruno
left a comment
rochacbruno
left a comment
There was a problem hiding this comment.
Added some comments and also noted that pkg/destroy/gcp/gcp.go must also have GOOGLE_CLOUD_UNIVERSE_DOMAIN otherwise will break sovereign cloud cluster terdown.
barbacbd
force-pushed
the
installer_config_updates_gcd
branch
4 times, most recently
from
154b684 to
0e41475
Compare
| "github.com/openshift/installer/pkg/types" | ||
| dnstypes "github.com/openshift/installer/pkg/types/dns" | ||
| "github.com/openshift/installer/pkg/types/gcp" | ||
| gcptypes "github.com/openshift/installer/pkg/types/gcp" |
There was a problem hiding this comment.
This change doesn't seem necessary and adds a bunch of noise to the pr
barbacbd
force-pushed
the
installer_config_updates_gcd
branch
from
0e41475 to
5a0c6e4
Compare
| // validateSovereignCloudRegion validates that sovereign cloud projects use appropriate region formats. | ||
| func validateSovereignCloudRegion(projectID, region, cloudEnvironment string, fldPath *field.Path) field.ErrorList { | ||
| allErrs := field.ErrorList{} | ||
|
|
||
| if cloudEnvironment == gcp.CloudEnvironmentSovereign { | ||
| // Sovereign cloud regions should use the u- prefix format | ||
| if region != "" && !strings.HasPrefix(region, "u-") { | ||
| allErrs = append(allErrs, field.Invalid(fldPath, region, | ||
| "sovereign cloud projects should use regions with 'u-' prefix (e.g., u-germany-northeast1)")) | ||
| } | ||
| } | ||
|
|
||
| return allErrs |
There was a problem hiding this comment.
can we just depend on the gcp sdk/api to validate the user-provided region? what happens if users input an invalid region without this validation? if they get back a useful error message, I think we're fine without this.
barbacbd
force-pushed
the
installer_config_updates_gcd
branch
from
5a0c6e4 to
d422ca0
Compare
| // GetCloudEnvironmentWithRegion determines the cloud environment using project ID and region. | ||
| // A known sovereign cloud prefix in the project ID is definitive. | ||
| // This prevents misclassifying organization-scoped public GCP projects (orgname:project-id). | ||
| func GetCloudEnvironmentWithRegion(projectID, region string) string { |
There was a problem hiding this comment.
Looks like parameter region is not being used as we can rely the project prefix, right? Maybe we can simplify the code as:
var (
// sovereignCloudProjectPrefixes contains known project ID prefixes for sovereign clouds.
// Project IDs in sovereign clouds use the format: <prefix>:<project-id>
// This list helps distinguish from organization-scoped public GCP projects (orgname:project-id).
sovereignCloudProjectPrefixes = []string{
"eu0:", // European sovereign cloud (Germany)
}
)
// GetCloudEnvironment determines the cloud environment from the project ID format.
// Returns CloudEnvironmentSovereign for sovereign cloud environments, empty string for public GCP.
// Uses known sovereign cloud project ID prefixes to distinguish from organization-scoped
// public GCP projects (orgname:project-id).
func GetCloudEnvironment(projectID string) string {
// Check if project ID has a known sovereign cloud prefix
for _, prefix := range sovereignCloudProjectPrefixes {
if strings.HasPrefix(projectID, prefix) {
return CloudEnvironmentSovereign
}
}
return ""
}| // For non-sovereign colon-formatted projects (e.g., orgname:project-id), | ||
| // strip the prefix for backward compatibility | ||
| projectID = parts[1] |
There was a problem hiding this comment.
I'm curious if we should strip the prefix here, because, previously, we just pass in the projectID directly 🤔
If we were incorrect before, then wouldn't the email be in format @<project-id>.<org-prefix>.iam.gserviceaccount.com too? See kubeflow/kubeflow#2244
| // For public GCP, this should be left empty (defaults to googleapis.com). | ||
| // For sovereign clouds like GCD, set this to the appropriate domain (e.g., apis-berlin-build0.goog). | ||
| // +optional | ||
| UniverseDomain string `json:"universeDomain,omitempty"` |
There was a problem hiding this comment.
Looks like you need another go generate ./pkg/types/installconfig.go :D
barbacbd
force-pushed
the
installer_config_updates_gcd
branch
6 times, most recently
from
e36735e to
26b2e0d
Compare
Add UniverseDomain field to the GCP platform configuration to support sovereign cloud environments that use different API endpoints. The universe domain specifies which GCP universe the cluster operates in. Public GCP uses googleapis.com (the default), while sovereign clouds use their own universe domains (e.g., apis-berlin-build0.goog for the German sovereign cloud). This field is validated to ensure it's only set when using sovereign cloud project IDs (those with a colon prefix like eu0:project-id).
Update credential loading to use CredentialsFromJSONWithParams and FindDefaultCredentialsWithParams, which automatically extract and apply the universe domain from the credentials JSON. Explicitly set the universe domain on all GCP service clients when it differs from the default googleapis.com. This ensures all GCP API calls use the correct endpoints for sovereign cloud environments. Without this, API calls would attempt to use googleapis.com endpoints even when credentials specify a different universe domain, resulting in authentication failures.
Configure default instance and disk types for sovereign cloud regions. Sovereign clouds have limited machine type availability compared to public GCP. Use c3-standard-4 instances and hyperdisk-balanced disks as defaults for sovereign cloud regions (those with 'u-' prefix), as these types are available in sovereign clouds while the standard defaults (n2-standard-4 and pd-ssd) are not.
barbacbd
force-pushed
the
installer_config_updates_gcd
branch
from
1e18f7a to
218a44f
Compare
| func (f *contentLoader) Load(ctx context.Context) (*googleoauth.Credentials, error) { | ||
| return googleoauth.CredentialsFromJSON(ctx, []byte(f.content), compute.CloudPlatformScope) | ||
| // Use CredentialsFromJSONWithParams to ensure universe domain from credentials is applied | ||
| return googleoauth.CredentialsFromJSONWithParams(ctx, []byte(f.content), googleoauth.CredentialsParams{ |
There was a problem hiding this comment.
I think CredentialsFromJSON internally calls CredentialsFromJSONWithParams already, right?
|
|
||
| // Explicitly set universe domain from credentials if present | ||
| // This is required for sovereign clouds which use a different universe domain | ||
| universeDomain := ssn.Credentials.UniverseDomain() |
There was a problem hiding this comment.
UniverseDomain func is deprecated. We should call GetUniverseDomain instead:
ud, err := ssn.Credentials.GetUniverseDomain()
if err != nil {
return nil, fmt.Errorf("failed to get universe domain: %w", err)
}
barbacbd
force-pushed
the
installer_config_updates_gcd
branch
2 times, most recently
from
18fe01a to
b887781
Compare
The GCP Cluster API provider needs the GOOGLE_CLOUD_UNIVERSE_DOMAIN
environment variable set to use the correct API endpoints in sovereign
cloud environments.
Without this, the CAPI provider attempts to use the default googleapis.com
endpoints even when credentials contain a different universe domain,
resulting in errors like:
the configured universe domain ("googleapis.com") does not match
the universe domain found in the credentials ("apis-berlin-build0.goog")
Extract the universe domain from the GCP session credentials and set it
as an environment variable for the CAPG infrastructure controller when it
differs from the default googleapis.com.
Also update universe domain validation to make it optional - users can
provide it explicitly in install-config, or let the GCP SDK extract it
from credentials automatically.
barbacbd
force-pushed
the
installer_config_updates_gcd
branch
from
b887781 to
7ac111d
Compare
|
@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. |
5 participants
Summary
Adds support for GCP Germany Sovereign Cloud (GCD) to the OpenShift installer with automatic cloud environment detection, validation, and CAPI
provider configuration.
Changes
Automatic Cloud Environment Detection
GetCloudEnvironment()helper that detects sovereign cloud from project ID formateu0:prefix → germany-sovereign cloudProject ID Validation (CORS-4422)
eu0:<project-name>format:characterRegion Validation (CORS-4420, CORS-4449)
sovereignCloudRegionsmap with cloud-specific region listseurope-west3,europe-west4getValidRegions()andvalidateRegion()functionsCAPI Provider Configuration (CORS-4421)
ComputeServiceEndpoint,IAMServiceEndpoint, andResourceManagerServiceEndpointcloud-environment: germany-sovereignlabel to GCPCluster for trackingGetCloudEnvironment()from project IDTesting
Validation can be tested with:
eu0:my-projectu-germany-northeast1Summary by CodeRabbit
Release Notes
New Features
cloud-environment=germany-sovereignlabel.universeDomainon GCP metadata, including project- and sovereign-aware handling.Improvements