Skip to content

[WIP] OCPBUGS-55192: Add IngressController .spec.domain validation #2308

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

grzpiotrowski
Copy link

This PR fixes OCPBUGS-55192.

Add ratcheting validation of the .spec.domain field of ingress controller.
Domain must consist of lowercase alphanumeric characters '-' or '.', and each label must start and end with an alphanumeric character.

Previously, the user could configure the .spec.domain field incorrectly which could result in the router pods entering a CrashLoopBackOff state immediately upon creation with error: invalid canonical hostname: [...].

Opted not to add the max length validation // +kubebuilder:validation:MaxLength=253 as it could be slightly misleading, because even if .spec.domain does not exceed that 253 characters limit, we could end up with an invalid canonical hostname error as this is constructed from the router name and the IC domain, which length could not be predicted here.

  • operator/v1/types_ingress.go (IngressControllerSpec): Add ratcheting validation of the Domain field.
  • operator/v1/tests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml Add test cases for the ingress controller .spec.domain field validation

Copy link
Contributor

openshift-ci bot commented May 2, 2025

Hello @grzpiotrowski! 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/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 2, 2025
@openshift-ci openshift-ci bot requested review from deads2k and everettraven May 2, 2025 15:03
Copy link
Contributor

openshift-ci bot commented May 2, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

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

@grzpiotrowski grzpiotrowski changed the title Add IngressController .spec.domain validation OCPBUGS-55192: Add IngressController .spec.domain validation May 2, 2025
@openshift-ci-robot openshift-ci-robot added jira/severity-critical Referenced Jira bug's severity is critical for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. labels May 2, 2025
@openshift-ci-robot
Copy link

@grzpiotrowski: This pull request references Jira Issue OCPBUGS-55192, which is invalid:

  • expected the bug to target the "4.20.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

This PR fixes OCPBUGS-55192.

Add ratcheting validation of the .spec.domain field of ingress controller.
Domain must consist of lowercase alphanumeric characters '-' or '.', and each label must start and end with an alphanumeric character.

Previously, the user could configure the .spec.domain field incorrectly which could result in the router pods entering a CrashLoopBackOff state immediately upon creation with error: invalid canonical hostname: [...].

Opted not to add the max length validation // +kubebuilder:validation:MaxLength=253 as it could be slightly misleading, because even if .spec.domain does not exceed that 253 characters limit, we could end up with an invalid canonical hostname error as this is constructed from the router name and the IC domain, which length could not be predicted here.

  • operator/v1/types_ingress.go (IngressControllerSpec): Add ratcheting validation of the Domain field.
  • operator/v1/tests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml Add test cases for the ingress controller .spec.domain field validation

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-robot openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label May 2, 2025
@grzpiotrowski grzpiotrowski force-pushed the OCPBUGS-55192-ic-spec-domain-validation branch from 2e10452 to a538d6c Compare May 2, 2025 15:34
@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 2, 2025
This commit fixes OCPBUGS-55192.

https://issues.redhat.com/browse/OCPBUGS-55192

Add ratcheting validation of the .spec.domain field of ingress controller.
Domain must consist of lowercase alphanumeric characters '-' or '.',
and each label must start and end with an alphanumeric character.

* operator/v1/types_ingress.go
(IngressControllerSpec): Add ratcheting validation of the Domain field.

* operator/v1/tests/ingresscontrollers.operator.openshift.io/AAA_ungated.yaml
Add test cases for the ingress controller .spec.domain field validation
@grzpiotrowski grzpiotrowski force-pushed the OCPBUGS-55192-ic-spec-domain-validation branch from a538d6c to d5e7974 Compare May 2, 2025 15:44
@openshift-ci openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 2, 2025
@grzpiotrowski grzpiotrowski changed the title OCPBUGS-55192: Add IngressController .spec.domain validation [WIP] OCPBUGS-55192: Add IngressController .spec.domain validation May 2, 2025
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 2, 2025
@grzpiotrowski grzpiotrowski marked this pull request as draft May 2, 2025 16:01
Copy link
Contributor

openshift-ci bot commented May 2, 2025

@grzpiotrowski: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/verify d5e7974 link true /test verify

Full PR test history. Your PR dashboard.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. jira/severity-critical Referenced Jira bug's severity is critical for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants