Skip to content

MAO: Validate GCP disk license URLs at admission - #1553

Open
redhat-chai-bot wants to merge 1 commit into
openshift:mainfrom
redhat-chai-bot:mao-gcp-disk-licenses
Open

redhat-chai-bot wants to merge 1 commit into
openshift:mainfrom
redhat-chai-bot:mao-gcp-disk-licenses

Conversation

@redhat-chai-bot

@redhat-chai-bot redhat-chai-bot commented Sep 14, 2026

Copy link
Copy Markdown

Summary

Extend the existing Machine validating webhook to validate GCPDisk.Licenses for GCP provider specs.

The validation accepts both the full Google Compute license URI and the short self-link form, while preserving nil/empty backward compatibility. Invalid host, path, project ID, and license resource-name values are rejected at the indexed provider-spec field path.

Implementation

  • Reuses the existing validateGCPDisks webhook path; no new webhook server is introduced.
  • Matches the schema validation added in openshift/api#2980.
  • Updates the API dependency through a reproducible bot-fork replacement and refreshes vendored files.

Validation

  • make vendor
  • make crds-sync
  • GOFLAGS=-mod=vendor go test ./pkg/webhooks -run '^TestValidateGCPProviderSpec$' -count=1
  • make unit
  • make lint
  • make vet
  • git diff --check
  • make check

All completed successfully.

Related


AI-generated. Review for accuracy.

@damdo requested in Slack thread

Warning

Content scanning did not attest this change.
The scanner could not produce a verdict for the pushed content (never_attempted), so it has not been checked for credentials or malware.
Content scanning currently fails open, so this did not hold the change request for review — please review the diff with that in mind.

Summary by CodeRabbit

  • Bug Fixes

    • Added validation for GCP disk license URLs.
    • Invalid license formats now produce a clear validation error, while supported full URLs and short self-links are accepted.
  • Tests

    • Expanded coverage for valid and invalid GCP disk license values, including malformed URLs, unsupported hosts, and invalid names.

@openshift-ci

openshift-ci Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo 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. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 0cb6b4bb-a9ad-4e45-99d4-b183a143285a

📥 Commits

Reviewing files that changed from the base of the PR and between a8e64d1 and 355aeda.

⛔ Files ignored due to path filters (54)
  • go.sum is excluded by !**/*.sum
  • vendor/github.com/openshift/api/config/v1/types_authentication.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/types_infrastructure.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/types_ingress.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/types_kmsencryption.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-Default.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-OKD.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-Hypershift.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-SelfManagedHA.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-CustomNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-DevPreviewNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Hypershift-TechPreviewNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-CustomNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-SelfManagedHA-DevPreviewNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-CustomNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-DevPreviewNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses-TechPreviewNoUpgrade.crd.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yaml is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/config/v1alpha1/zz_generated.model_name.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/envtest-releases.yaml is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/features.md is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/features/features.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/features/legacyfeaturegates.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/machine/v1beta1/types_gcpprovider.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/machine/v1beta1/zz_generated.deepcopy.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/operator/v1/types.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1/types_kmsencryption.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1/zz_generated.deepcopy.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/operator/v1/zz_generated.model_name.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/operator/v1alpha1/register.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1alpha1/types_ingress.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.featuregated-crd-manifests.yaml is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.model_name.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.go is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/github.com/openshift/api/route/v1/generated.proto is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/route/v1/types.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/api/route/v1/zz_generated.featuregated-crd-manifests.yaml is excluded by !**/vendor/**, !vendor/**, !**/zz_generated*
  • vendor/modules.txt is excluded by !**/vendor/**, !vendor/**
📒 Files selected for processing (3)
  • go.mod
  • pkg/webhooks/machine_webhook.go
  • pkg/webhooks/machine_webhook_test.go

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.


Walkthrough

The webhook now validates GCP disk license URLs against supported formats. Tests cover valid and invalid license values. The Go module replace block now maps github.com/openshift/api to a specified fork and version.

Changes

GCP license validation

Layer / File(s) Summary
Validate GCP disk licenses
pkg/webhooks/machine_webhook.go, pkg/webhooks/machine_webhook_test.go
The webhook validates each disk license against supported full URI and short self-link formats. Tests cover accepted and rejected values.

OTE-tests module replacement

Layer / File(s) Summary
Update API module replacement
go.mod
The OTE-tests replace block maps github.com/openshift/api to github.com/redhat-chai-bot/api at the specified version.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Feature

Suggested reviewers: vr4manta

Merge Risk: ⚪ Minimal · up to 355ae

The validation accepts the license forms supported by the selected API and rejects invalid forms as intended. No actionable merge risk remains.

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (14 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: admission-time validation of GCP disk license URLs in MAO.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed The pull request adds 13 static Go subtest names such as "with a valid full disk license URI" and "with a disk license invalid host". The tests pass each tc.testCase string directly to t.Run; none…
Test Structure And Quality ✅ Passed PASS. The pull request adds table-driven t.Run cases to TestValidateGCPProviderSpec; it does not add Ginkgo It blocks. Each new case covers one license behavior and uses the existing runner with…
Microshift Test Compatibility ✅ Passed PASS: The pull request adds no new Ginkgo e2e tests. The changed test code extends the existing Go TestValidateGCPProviderSpec unit test with table cases. The added code contains no It, Describe
Single Node Openshift (Sno) Test Compatibility ✅ Passed PASS: The pull request adds no Ginkgo e2e tests. The new cases are table-driven Go unit tests in TestValidateGCPProviderSpec(t *testing.T) and only validate license strings. The changed files contai…
Topology-Aware Scheduling Compatibility ✅ Passed The pull request does not add or modify deployment manifests, operator scheduling logic, or controllers. The only application-code change adds GCP disk license validation in validateGCPDisks. The re…
Ote Binary Stdout Contract ✅ Passed No OTE stdout-contract violation is introduced. The PR changes only GCP validation, test cases, dependency metadata, and vendored API code. The new package-level initializer only calls `regexp.MustCom…
Ipv6 And Disconnected Network Test Compatibility ✅ Passed PASS: The pull request adds standard Go table cases inside TestValidateGCPProviderSpec, not new Ginkgo e2e tests. The changed test inventory contains no It, Describe, Context, or When declarations, no…
No-Weak-Crypto ✅ Passed PASS. The pull request adds URL regular-expression validation and indexed field errors for GCP disk licenses. The changed implementation does not add cryptographic operations, custom crypto, secret/to…
Container-Privileges ✅ Passed PASS. The pull request changes Go validation, dependency metadata, vendored API code, and generated CRDs. No added lines set privileged, hostPID, hostNetwork, hostIPC, SYS_ADMIN, or allowPrivilegeEsca…
No-Sensitive-Data-In-Logs ✅ Passed No logging of sensitive data was introduced. The added production code only validates GCP license strings and returns a field validation error; it adds no klog, logger, zap, or print call. Existing lo…
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Some tools did not complete. Review the errors below.

🔧 golangci-lint (2.13.2)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions


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

@openshift-ci

openshift-ci Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign racheljpg for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

@redhat-chai-bot: 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.

@redhat-chai-bot

Copy link
Copy Markdown
Author

/hold

Holding this PR until QE testing has been performed on the implementation to prove it works.


AI-generated. Review for accuracy.

@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 Sep 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant