Skip to content

CORS-4281: Remove the GCP Service Endpoints#2576

Merged
openshift-merge-bot[bot] merged 1 commit intoopenshift:masterfrom
barbacbd:CORS-4281
Nov 26, 2025
Merged

CORS-4281: Remove the GCP Service Endpoints#2576
openshift-merge-bot[bot] merged 1 commit intoopenshift:masterfrom
barbacbd:CORS-4281

Conversation

@barbacbd
Copy link
Copy Markdown
Contributor

Removing the GCP Service Endpoints in favor of a new solution. The new solution will create a private hosted zone that will route traffic to the googleapi endpoints via an ip address created during the initialization of the private serivce connect endpoint. The cluster components no longer need the endpoint overrides, so the service endpoints can and should be removed to avoid confusion.

@openshift-ci-robot
Copy link
Copy Markdown

Pipeline controller notification
This repository is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. Review these jobs and use /test <job> to manually trigger optional jobs most likely to be impacted by the proposed changes.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 10, 2025
@openshift-ci-robot
Copy link
Copy Markdown

openshift-ci-robot commented Nov 10, 2025

@barbacbd: This pull request references CORS-4281 which is a valid jira issue.

Details

In response to this:

Removing the GCP Service Endpoints in favor of a new solution. The new solution will create a private hosted zone that will route traffic to the googleapi endpoints via an ip address created during the initialization of the private serivce connect endpoint. The cluster components no longer need the endpoint overrides, so the service endpoints can and should be removed to avoid confusion.

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.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Nov 10, 2025

Hello @barbacbd! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

@openshift-ci openshift-ci Bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 10, 2025
Copy link
Copy Markdown
Member

@damdo damdo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Thanks

/assign @everettraven @JoelSpeed

@damdo
Copy link
Copy Markdown
Member

damdo commented Nov 20, 2025

@barbacbd verify jobs are failing, might be worth rebasing and checking what's wrong

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2025
@openshift-ci-robot
Copy link
Copy Markdown

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-aws-ovn
/test e2e-aws-ovn-hypershift
/test e2e-aws-ovn-hypershift-conformance
/test e2e-aws-ovn-techpreview
/test e2e-aws-serial-1of2
/test e2e-aws-serial-2of2
/test e2e-aws-serial-techpreview-1of2
/test e2e-aws-serial-techpreview-2of2
/test e2e-azure
/test e2e-gcp
/test e2e-upgrade
/test e2e-upgrade-out-of-change

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Nov 20, 2025

Walkthrough

This pull request removes GCP custom API endpoints functionality from infrastructure and machine configuration APIs, including type definitions, CRD schema fields, test suites, and generated supporting code across multiple API versions and deployment modes.

Changes

Cohort / File(s) Summary
Test files deletion
config/v1/tests/infrastructures.config.openshift.io/GCPCustomAPIEndpoints.yaml, config/v1/tests/infrastructures.config.openshift.io/GCPCustomAPIEndpointsInstall.yaml, machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/GCPCustomAPIEndpoints.yaml, machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/GCPCustomAPIEndpointsInstall.yaml
Deleted test suites covering GCP endpoint override scenarios, validations, and error cases.
Go type definitions
config/v1/types_infrastructure.go
Removed GCPServiceEndpointName type, GCPServiceEndpoint struct, all related constants (GCPServiceEndpointNameCompute, GCPServiceEndpointNameContainer, etc.), and ServiceEndpoints field from GCPPlatformStatus. Added tombstone markers around removed sections.
Infrastructure CRD manifests
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-*.crd.yaml, payload-manifests/crds/0000_10_config-operator_01_infrastructures-*.crd.yaml
Removed serviceEndpoints field and related schemas from AWS, GCP, IBMCloud, and PowerVS provider sections across CustomNoUpgrade, DevPreviewNoUpgrade, and TechPreviewNoUpgrade variants.
Machine configuration CRD manifests
machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-*.crd.yaml, payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-*.crd.yaml
Removed serviceEndpoints field from AWS, GCP, IBMCloud, and PowerVS provider sections across CustomNoUpgrade, DevPreviewNoUpgrade, and TechPreviewNoUpgrade variants.
Generated code files
config/v1/zz_generated.deepcopy.go, config/v1/zz_generated.swagger_doc_generated.go, openapi/generated_openapi/zz_generated.openapi.go
Removed DeepCopyInto/DeepCopy methods for GCPServiceEndpoint, deleted swagger documentation entries, and removed OpenAPI schema definitions.
Feature gate references
config/v1/zz_generated.featuregated-crd-manifests.yaml, machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yaml
Removed GCPCustomAPIEndpointsInstall feature gate entries.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Changes are straightforward deletions with no complex logic modifications
  • Highly repetitive pattern: same serviceEndpoints field removed from multiple CRD manifest variants across providers
  • Generated code updates are mechanical consequences of type definition removal
  • No new functionality or behavioral changes introduced
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between b9b4604 and 6718b87.

📒 Files selected for processing (22)
  • config/v1/tests/infrastructures.config.openshift.io/GCPCustomAPIEndpoints.yaml (0 hunks)
  • config/v1/tests/infrastructures.config.openshift.io/GCPCustomAPIEndpointsInstall.yaml (0 hunks)
  • config/v1/types_infrastructure.go (2 hunks)
  • config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yaml (0 hunks)
  • config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yaml (0 hunks)
  • config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml (0 hunks)
  • config/v1/zz_generated.deepcopy.go (0 hunks)
  • config/v1/zz_generated.featuregated-crd-manifests.yaml (0 hunks)
  • config/v1/zz_generated.swagger_doc_generated.go (0 hunks)
  • machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/GCPCustomAPIEndpoints.yaml (0 hunks)
  • machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/GCPCustomAPIEndpointsInstall.yaml (0 hunks)
  • machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml (0 hunks)
  • machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml (0 hunks)
  • machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml (0 hunks)
  • machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yaml (0 hunks)
  • openapi/generated_openapi/zz_generated.openapi.go (1 hunks)
  • payload-manifests/crds/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yaml (0 hunks)
  • payload-manifests/crds/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yaml (0 hunks)
  • payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml (0 hunks)
  • payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml (0 hunks)
  • payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml (0 hunks)
  • payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml (0 hunks)
💤 Files with no reviewable changes (20)
  • machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/GCPCustomAPIEndpoints.yaml
  • config/v1/tests/infrastructures.config.openshift.io/GCPCustomAPIEndpoints.yaml
  • config/v1/zz_generated.deepcopy.go
  • payload-manifests/crds/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yaml
  • config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yaml
  • payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml
  • machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml
  • config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yaml
  • config/v1/zz_generated.swagger_doc_generated.go
  • machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/GCPCustomAPIEndpointsInstall.yaml
  • machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml
  • payload-manifests/crds/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yaml
  • config/v1/tests/infrastructures.config.openshift.io/GCPCustomAPIEndpointsInstall.yaml
  • machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yaml
  • machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml
  • config/v1/zz_generated.featuregated-crd-manifests.yaml
  • payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml
  • payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml
  • config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml
  • payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
  • openapi/generated_openapi/zz_generated.openapi.go
🧰 Additional context used
📓 Path-based instructions (1)
**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

  • config/v1/types_infrastructure.go
🔇 Additional comments (2)
config/v1/types_infrastructure.go (2)

702-738: Verify stakeholder alignment before proceeding with removal.

The PR comments indicate active disagreement about whether this functionality should be removed:

  • JoelSpeed added /hold citing a prior agreement that endpoints should remain for CCM configuration patterns
  • patrickdillon countered that CCM configuration isn't needed for this feature

The tombstoning approach itself is technically correct and follows best practices by preserving the complete type structure in comments. However, given the unresolved disagreement among maintainers, ensure this removal has been properly coordinated and the architectural concerns have been addressed before merging.

Note: The tombstoning implementation is sound—it preserves the full API structure to prevent future naming conflicts, which is the correct approach for removing public API types.


794-808: Tombstoning approach is well-executed.

The comments clearly document that this field was tech preview functionality (confirmed by the GCPCustomAPIEndpointsInstall feature gate reference), and the tombstoning properly prevents future naming conflicts. The explanatory comments provide good context for future maintainers.

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: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented
The command is terminated due to an error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented


Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2025
@damdo
Copy link
Copy Markdown
Member

damdo commented Nov 21, 2025

/lgtm

@everettraven are the CRD verifications failures expected? TY

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Nov 21, 2025
@openshift-ci-robot
Copy link
Copy Markdown

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-aws-ovn
/test e2e-aws-ovn-hypershift
/test e2e-aws-ovn-hypershift-conformance
/test e2e-aws-ovn-techpreview
/test e2e-aws-serial-1of2
/test e2e-aws-serial-2of2
/test e2e-aws-serial-techpreview-1of2
/test e2e-aws-serial-techpreview-2of2
/test e2e-azure
/test e2e-gcp
/test e2e-upgrade
/test e2e-upgrade-out-of-change

@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 21, 2025
@everettraven
Copy link
Copy Markdown
Contributor

@damdo @barbacbd Both verify-crd-schema and verify-crdify are failing in an anticipated way. They aren't aware of the nuance of TPNU vs GA states so these can be overridden.

That being said, verify is failing for a legitimate reason - it looks like the payload manifests were not updated appropriately. Looks like something needs to be regenerated so running make update and including the payload manifest changes in your commit should resolve that.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
config/v1/types_infrastructure.go (1)

756-770: Tombstoned serviceEndpoints looks good; consider disabling the remaining kubebuilder/feature-gate markers

The approach of commenting out the ServiceEndpoints field and adding a tombstone comment is consistent with the earlier ClusterHostedDNS tombstone and clearly reserves the JSON field name.

However, all of the schema/feature-gate markers immediately below (the +listType, +listMapKey, +kubebuilder:validation:*, and +openshift:enable:FeatureGate=GCPCustomAPIEndpointsInstall lines) are still active markers even though there is no longer a field. That can:

  • Mislead future readers into thinking serviceEndpoints is still part of the active schema.
  • Potentially confuse code-generation/validation tooling, since these markers are intended to be field-scoped.

I'd recommend either deleting those marker lines or “hard-commenting” them so they are no longer parsed as markers (e.g. prefix with an extra /), while keeping the explanatory tombstone comments and the commented-out field name.

For example:

-// +listType=map
-// +listMapKey=name
-// +kubebuilder:validation:MaxItems=11
-// +kubebuilder:validation:XValidation:rule="self.all(x, self.exists_one(y, x.name == y.name))",message="only 1 endpoint override is permitted per GCP service name"
-// +optional
-// +openshift:enable:FeatureGate=GCPCustomAPIEndpointsInstall
-// ServiceEndpoints []GCPServiceEndpoint `json:"serviceEndpoints,omitempty"`
+//// +listType=map
+//// +listMapKey=name
+//// +kubebuilder:validation:MaxItems=11
+//// +kubebuilder:validation:XValidation:rule="self.all(x, self.exists_one(y, x.name == y.name))",message="only 1 endpoint override is permitted per GCP service name"
+//// +optional
+//// +openshift:enable:FeatureGate=GCPCustomAPIEndpointsInstall
+//// ServiceEndpoints []GCPServiceEndpoint `json:"serviceEndpoints,omitempty"`

This keeps the historical context while ensuring no stray markers affect CRD/OpenAPI/feature-gate generation.

Please re-run make update and the verify-crd-schema / verify-crdify / verify jobs after such a change to confirm all generators still pass cleanly.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 013c645 and 7e003d3.

📒 Files selected for processing (22)
  • config/v1/tests/infrastructures.config.openshift.io/GCPCustomAPIEndpoints.yaml (0 hunks)
  • config/v1/tests/infrastructures.config.openshift.io/GCPCustomAPIEndpointsInstall.yaml (0 hunks)
  • config/v1/types_infrastructure.go (1 hunks)
  • config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yaml (0 hunks)
  • config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yaml (0 hunks)
  • config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml (0 hunks)
  • config/v1/zz_generated.deepcopy.go (0 hunks)
  • config/v1/zz_generated.featuregated-crd-manifests.yaml (0 hunks)
  • config/v1/zz_generated.swagger_doc_generated.go (0 hunks)
  • machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/GCPCustomAPIEndpoints.yaml (0 hunks)
  • machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/GCPCustomAPIEndpointsInstall.yaml (0 hunks)
  • machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml (0 hunks)
  • machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml (0 hunks)
  • machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml (0 hunks)
  • machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yaml (0 hunks)
  • openapi/generated_openapi/zz_generated.openapi.go (1 hunks)
  • payload-manifests/crds/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yaml (0 hunks)
  • payload-manifests/crds/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yaml (0 hunks)
  • payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml (0 hunks)
  • payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml (0 hunks)
  • payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml (0 hunks)
  • payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml (0 hunks)
💤 Files with no reviewable changes (20)
  • config/v1/zz_generated.swagger_doc_generated.go
  • config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yaml
  • config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml
  • machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/GCPCustomAPIEndpointsInstall.yaml
  • machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/GCPCustomAPIEndpoints.yaml
  • machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yaml
  • payload-manifests/crds/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yaml
  • config/v1/zz_generated.deepcopy.go
  • config/v1/tests/infrastructures.config.openshift.io/GCPCustomAPIEndpointsInstall.yaml
  • payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml
  • machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml
  • payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml
  • payload-manifests/crds/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yaml
  • config/v1/zz_generated.featuregated-crd-manifests.yaml
  • machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml
  • payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml
  • payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml
  • config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yaml
  • config/v1/tests/infrastructures.config.openshift.io/GCPCustomAPIEndpoints.yaml
  • machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

  • openapi/generated_openapi/zz_generated.openapi.go
  • config/v1/types_infrastructure.go
🔇 Additional comments (1)
openapi/generated_openapi/zz_generated.openapi.go (1)

12930-12940: LGTM - Generated code correctly reflects GCPServiceEndpoint removal.

The Dependencies array correctly omits the removed GCPServiceEndpoint type, leaving only the three remaining GCP-related types. Since this is generated code (zz_generated prefix), ensure the generators have been run completely via make update as mentioned in the PR discussion.

@JoelSpeed
Copy link
Copy Markdown
Contributor

/hold

@barbacbd and I had agreed that these should stay to follow our newer patterns for populating CCM configuration. Would like to catch up on what's changed before we move forward here

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 21, 2025
@patrickdillon
Copy link
Copy Markdown
Contributor

Would like to catch up on what's changed before we move forward here

We determined that CCM configuration is not needed for this feature. All GCP API resolution in the cluster is redirected to the private endpoint by installer-created DNS records. Fun fact, the DNS solution is actually the only way this will work for OpenShift, we learned, because overriding the endpoints does not support certain APIs (iam, sts).

Nothing in OpenShift should be using the API defined here.

@barbacbd and I had agreed that these should stay to follow our newer patterns for populating CCM configuration

I gathered some loose details on the CCM configuration pattern; it sounded like the installer should not be laying down CCM configuration, which was new to me. We should discuss to better establish expectations and motivations.

Comment on lines -741 to -770
// GCPServiceEndpoint store the configuration of a custom url to
// override existing defaults of GCP Services.
type GCPServiceEndpoint struct {
// name is the name of the GCP service whose endpoint is being overridden.
// This must be provided and cannot be empty.
//
// Allowed values are Compute, Container, CloudResourceManager, DNS, File, IAM, ServiceUsage,
// Storage, and TagManager.
//
// As an example, when setting the name to Compute all requests made by the caller to the GCP Compute
// Service will be directed to the endpoint specified in the url field.
//
// +required
Name GCPServiceEndpointName `json:"name"`

// url is a fully qualified URI that overrides the default endpoint for a client using the GCP service specified
// in the name field.
// url is required, must use the scheme https, must not be more than 253 characters in length,
// and must be a valid URL according to Go's net/url package (https://pkg.go.dev/net/url#URL)
//
// An example of a valid endpoint that overrides the Compute Service: "https://compute-myendpoint1.p.googleapis.com"
//
// +required
// +kubebuilder:validation:MaxLength=253
// +kubebuilder:validation:XValidation:rule="isURL(self)",message="must be a valid URL"
// +kubebuilder:validation:XValidation:rule="isURL(self) ? (url(self).getScheme() == \"https\") : true",message="scheme must be https"
// +kubebuilder:validation:XValidation:rule="url(self).getEscapedPath() == \"\" || url(self).getEscapedPath() == \"/\"",message="url must consist only of a scheme and domain. The url path must be empty."
URL string `json:"url"`
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to know what this looked like to have the complete tombstone, so this should be commented rather than removed, same for the string alias, I'm happy to remove the constants though

Copy link
Copy Markdown
Contributor

@everettraven everettraven Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this as a learning opportunity for tombstoning:

Does it matter what the underlying field structure looked like if we can never add a field with the parent serialized name again?

If I can never create a field with name tombstonedField, I inherently cannot create a new field with name tombstonedField.subField so why does having the complete view of what this looked like matter?

The only value I see that adding is for future readers of the tombstone - which if that is the reason, fine by me.

EDIT: I also now see value in keeping just the struct name tombstoned so we don't accidentally re-implement a same named type, but my question still seems relevant for removing the fields of the child type.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yaml (1)

498-511: Type mismatch in GCP platform spec validations — validation rules won't execute as intended.

The gcp object is defined as type: object (line 501) with no properties, but the added CEL validation rules (lines 502-511) apply URL validation functions (isURL(), url().getScheme(), url().getEscapedPath()) to self. These functions expect a string, not an object, causing a semantic type mismatch. The validations will fail or behave unexpectedly at runtime.

Clarify the intended design: either restructure gcp to include a URL property and validate that property, change gcp to type: string, or remove these validations if GCP no longer requires configuration.

payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml (1)

498-511: Remove orphaned x-kubernetes-validations from empty GCP object.

The x-kubernetes-validations block at lines 502–511 applies URL-validation rules (checking scheme, path, and format) to the GCP platform spec object itself. However, the GCP section has type: object with no properties defined (the serviceEndpoints field was removed). These validations are now orphaned and meaningless—they attempt to validate an empty object as if it were a URL string.

This will either fail CRD validation or provide no practical effect. Remove this block or ensure GCP properties are re-added if validation is necessary.

         gcp:
           description: gcp contains settings specific to the Google
             Cloud Platform infrastructure provider.
           type: object
-          x-kubernetes-validations:
-          - message: must be a valid URL
-            rule: isURL(self)
-          - message: scheme must be https
-            rule: 'isURL(self) ? (url(self).getScheme() == "https")
-              : true'
-          - message: url must consist only of a scheme and domain.
-              The url path must be empty.
-            rule: url(self).getEscapedPath() == "" || url(self).getEscapedPath()
-              == "/"
payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml (1)

498-511: Remove x-kubernetes-validations from the gcp object—these URL validation rules cannot execute on type: object and will fail at runtime.

The gcp field (line 498) is defined as type: object with no nested properties, yet the x-kubernetes-validations (lines 502–511) attempt to apply string/URL operations (isURL(self), url(self).getScheme(), url(self).getEscapedPath()) to self. Since self refers to the entire gcp object (not a string), these CEL rules will fail.

The git history shows these rules were added in commit b9b4604 when serviceEndpoints was removed. The validations appear to have been incorrectly relocated from the nested url string field to the parent gcp object level. Either remove the validations or restructure gcp to include a URL string property if endpoint configuration is still needed.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 7e003d3 and b9b4604.

📒 Files selected for processing (23)
  • config/v1/tests/infrastructures.config.openshift.io/GCPCustomAPIEndpoints.yaml (0 hunks)
  • config/v1/tests/infrastructures.config.openshift.io/GCPCustomAPIEndpointsInstall.yaml (0 hunks)
  • config/v1/types_infrastructure.go (2 hunks)
  • config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yaml (0 hunks)
  • config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yaml (0 hunks)
  • config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml (0 hunks)
  • config/v1/zz_generated.deepcopy.go (0 hunks)
  • config/v1/zz_generated.featuregated-crd-manifests.yaml (0 hunks)
  • config/v1/zz_generated.swagger_doc_generated.go (0 hunks)
  • machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/GCPCustomAPIEndpoints.yaml (0 hunks)
  • machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/GCPCustomAPIEndpointsInstall.yaml (0 hunks)
  • machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml (0 hunks)
  • machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml (0 hunks)
  • machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml (0 hunks)
  • machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yaml (0 hunks)
  • openapi/generated_openapi/zz_generated.openapi.go (1 hunks)
  • payload-manifests/crds/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yaml (0 hunks)
  • payload-manifests/crds/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yaml (0 hunks)
  • payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml (0 hunks)
  • payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml (1 hunks)
  • payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yaml (1 hunks)
  • payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml (1 hunks)
  • payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml (1 hunks)
💤 Files with no reviewable changes (17)
  • config/v1/zz_generated.deepcopy.go
  • config/v1/tests/infrastructures.config.openshift.io/GCPCustomAPIEndpoints.yaml
  • payload-manifests/crds/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yaml
  • machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml
  • machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yaml
  • machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml
  • machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml
  • payload-manifests/crds/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yaml
  • config/v1/zz_generated.featuregated-crd-manifests.yaml
  • config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml
  • config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yaml
  • config/v1/zz_generated.swagger_doc_generated.go
  • config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yaml
  • machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/GCPCustomAPIEndpointsInstall.yaml
  • config/v1/tests/infrastructures.config.openshift.io/GCPCustomAPIEndpointsInstall.yaml
  • payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml
  • machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/GCPCustomAPIEndpoints.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

  • payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml
  • openapi/generated_openapi/zz_generated.openapi.go
  • payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yaml
  • config/v1/types_infrastructure.go
  • payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml
  • payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml
🔇 Additional comments (3)
openapi/generated_openapi/zz_generated.openapi.go (1)

12937-12939: LGTM! Generated OpenAPI dependencies correctly updated.

The Dependencies list has been properly updated to remove the GCPServiceEndpoint reference, which aligns with the PR objective of removing GCP Service Endpoints. The generated code is consistent with the type and CRD removals throughout the PR.

config/v1/types_infrastructure.go (2)

702-738: LGTM - Tombstone implementation follows previous review guidance.

The GCP service endpoint types are properly commented out rather than removed, preserving the complete API history as requested in the previous review. The tombstone markers are clear and all documentation is preserved.

Based on past review comments, this correctly addresses the requirement to comment rather than remove the types.


794-808: LGTM - ServiceEndpoints field properly tombstoned.

The ServiceEndpoints field is correctly tombstoned with preserved documentation and a clear warning that the field name cannot be reused. This follows the same pattern as the existing ClusterHostedDNS tombstone above it (lines 775-778), maintaining consistency.

    Removing the GCP Service Endpoints in favor of a new solution. The new solution will create a
    private hosted zone that will route traffic to the googleapi endpoints via an ip address created
    during the initialization of the private serivce connect endpoint. The cluster components no longer
    need the endpoint overrides, so the service endpoints can and should be removed to avoid confusion.
@JoelSpeed
Copy link
Copy Markdown
Contributor

/lgtm
/override ci/prow/verify-crdify ci/prow/verify-crd-schema

@JoelSpeed
Copy link
Copy Markdown
Contributor

/hold cancel

Discussed with @barbacbd out of band

@openshift-ci openshift-ci Bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 25, 2025
@openshift-ci-robot
Copy link
Copy Markdown

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-aws-ovn
/test e2e-aws-ovn-hypershift
/test e2e-aws-ovn-hypershift-conformance
/test e2e-aws-ovn-techpreview
/test e2e-aws-serial-1of2
/test e2e-aws-serial-2of2
/test e2e-aws-serial-techpreview-1of2
/test e2e-aws-serial-techpreview-2of2
/test e2e-azure
/test e2e-gcp
/test e2e-upgrade
/test e2e-upgrade-out-of-change

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Nov 25, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damdo, JoelSpeed

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 25, 2025
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Nov 25, 2025

@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-crd-schema, ci/prow/verify-crdify

Details

In response to this:

/lgtm
/override ci/prow/verify-crdify ci/prow/verify-crd-schema

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 kubernetes-sigs/prow repository.

@barbacbd
Copy link
Copy Markdown
Contributor Author

/test e2e-aws-ovn-techpreview

@barbacbd
Copy link
Copy Markdown
Contributor Author

/verified by integration

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Nov 25, 2025
@openshift-ci-robot
Copy link
Copy Markdown

@barbacbd: This PR has been marked as verified by integration.

Details

In response to this:

/verified by integration

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
Copy link
Copy Markdown
Contributor Author

/override ci/prow/verify-crdify ci/prow/verify-crd-schema

Copying from Joel above, these are expected to fail here.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Nov 25, 2025

@barbacbd: barbacbd unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:openshift: openshift-release-oversight openshift-staff-engineers openshift-sustaining-engineers.

Details

In response to this:

/override ci/prow/verify-crdify ci/prow/verify-crd-schema

Copying from Joel above, these are expected to fail here.

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 kubernetes-sigs/prow repository.

@openshift-ci-robot
Copy link
Copy Markdown

/retest-required

Remaining retests: 0 against base HEAD 5cf710f and 2 for PR HEAD 6718b87 in total

@JoelSpeed
Copy link
Copy Markdown
Contributor

/override ci/prow/verify-crdify ci/prow/verify-crd-schema

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Nov 26, 2025

@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-crd-schema, ci/prow/verify-crdify

Details

In response to this:

/override ci/prow/verify-crdify ci/prow/verify-crd-schema

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 kubernetes-sigs/prow repository.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Nov 26, 2025

@barbacbd: all tests passed!

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot Bot merged commit ef9f994 into openshift:master Nov 26, 2025
28 checks passed
@vincentdephily
Copy link
Copy Markdown

This seems to have broken https://github.com/openshift/client-go/blob/master/config/applyconfigurations/config/v1/gcpserviceendpoint.go which still references configv1.GCPServiceEndpointName. Do you know if there is a PR planed for o/client-go, or do you have advice for a fix ?

@patrickdillon
Copy link
Copy Markdown
Contributor

patrickdillon commented Dec 2, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants