Bug 2039256: kubebuilder validation format hostname is not RFC compliant #1120

candita · 2022-02-11T14:43:04Z

We use the go-openapi/strfmt library for the "hostname" kubebuilder validation format in componentRoutes. This format does not accept numbers or hyphens in the TLD as noted in https://bugzilla.redhat.com/show_bug.cgi?id=2039256#c3.

I checked with the openshift/api team and they recommended replacing the hostname format with my own regex, but I have to be careful with downgrades. Therefore I changed the regex pattern to be looser.

The old pattern that came from go-openapi/strfmt:
^([a-zA-Z0-9\p{S}\p{L}]((-?[a-zA-Z0-9\p{S}\p{L}]{0,62})?)|([a-zA-Z0-9\p{S}\p{L}](([a-zA-Z0-9-\p{S}\p{L}]{0,61}[a-zA-Z0-9\p{S}\p{L}])?)(\.)){1,}([a-zA-Z\p{L}]){2,63})$

The new pattern is simply an | of the old pattern with the correct, tighter, pattern (formatted here for clarity):
^([a-zA-Z0-9\p{S}\p{L}]((-?[a-zA-Z0-9\p{S}\p{L}]{0,62})?)|([a-zA-Z0-9\p{S}\p{L}](([a-zA-Z0-9-\p{S}\p{L}]{0,61}[a-zA-Z0-9\p{S}\p{L}])?)(\.)){1,}([a-zA-Z\p{L}]){2,63})$ | ^(([a-z0-9][-a-z0-9]{0,61}[a-z0-9]|[a-z0-9]{1,63})[\.]){0,}([a-z0-9][-a-z0-9]{0,61}[a-z0-9]|[a-z0-9]{1,63})$

Based on previous code review, the tighter side of the | removes the uppercase A-Z provision, removes \p{S} (symbols), and \p{L} (international language letters). Also enforces labels must start/end in alphanumeric characters, and one-word hostnames accepted.

I also checked with go-openapi owners in go-openapi/strfmt#94 about changing the library regex, but the solution they prefer doesn't fit for a solution for us.

Also fixes duplicate bug (with higher priority) https://bugzilla.redhat.com/show_bug.cgi?id=2049473

openshift-ci · 2022-02-11T14:43:11Z

@candita: This pull request references Bugzilla bug 2039256, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.11.0) matches configured target release for branch (4.11.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @quarterpin

In 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 kubernetes/test-infra repository.

openshift-ci · 2022-02-11T14:43:42Z

@candita: This pull request references Bugzilla bug 2039256, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.11.0) matches configured target release for branch (4.11.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @quarterpin

In 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 kubernetes/test-infra repository.

openshift-ci · 2022-02-14T18:42:46Z

@candita: This pull request references Bugzilla bug 2039256, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.11.0) matches configured target release for branch (4.11.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @quarterpin

In 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 kubernetes/test-infra repository.

openshift-ci · 2022-02-14T18:45:47Z

@candita: This pull request references Bugzilla bug 2039256, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.11.0) matches configured target release for branch (4.11.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @quarterpin

In 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 kubernetes/test-infra repository.

openshift-ci · 2022-02-14T18:47:24Z

@candita: This pull request references Bugzilla bug 2039256, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.11.0) matches configured target release for branch (4.11.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @quarterpin

In 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 kubernetes/test-infra repository.

openshift-ci · 2022-02-14T19:12:36Z

@candita: This pull request references Bugzilla bug 2039256, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.11.0) matches configured target release for branch (4.11.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @quarterpin

In 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 kubernetes/test-infra repository.

openshift-ci · 2022-02-14T19:16:04Z

@candita: This pull request references Bugzilla bug 2039256, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.11.0) matches configured target release for branch (4.11.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @quarterpin

In 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 kubernetes/test-infra repository.

openshift-ci · 2022-02-17T20:35:27Z

@candita: This pull request references Bugzilla bug 2039256, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.11.0) matches configured target release for branch (4.11.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @quarterpin

In 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 kubernetes/test-infra repository.

candita · 2022-02-17T20:43:20Z

/retest

candita · 2022-02-17T22:20:58Z

/retest

config/v1/types_ingress.go

Miciah · 2022-02-28T18:53:06Z

/lgtm

openshift-ci · 2022-02-28T20:30:54Z

@candita: This pull request references Bugzilla bug 2039256, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.11.0) matches configured target release for branch (4.11.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @quarterpin

In 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 kubernetes/test-infra repository.

tkashem · 2022-02-28T21:52:20Z

/assign @benluddy

benluddy · 2022-03-01T22:57:46Z

Is it necessary to change the regular expression so that it no longer matches strings that might already exist in the wild (e.g. with uppercase letters or symbols)? Any update to an existing configv1.Ingress containing a bad hostname would be rejected.

Would it be reasonable to loosen this validation, then expect a bad hostname (or, more likely, errors resulting from using the bad hostname) to be surfaced by an operator in ComponentRouteStatus?

candita · 2022-03-02T00:57:05Z

Hi @benluddy,

It's a tradeoff. I considered this when I began the process, got some feedback on it, and decided it was better to match the route hostname validation. Eventually a componentRoute becomes a route and the hostname validation for route matches this new regex. See https://github.com/openshift/openshift-apiserver/blob/db22c038b9184b4ae823bba3f57b669526999a2b/pkg/route/apis/route/validation/validation.go#L25
So it's unlikely that a bad hostname would exist, but if it did, it's unlikely that the componentRoute would have been admitted.

I considered this too, and consulted with the developer of ComponentRoute about why there is no validation performed at this point by the operator. By design, all validation is supposed to be performed by the components that consume ComponentRoutes. This was also discussed in openshift/cluster-ingress-operator#552 (comment)

If there's some scenario where we would want to allow ComponentRoutes that could not be admitted as Routes, then it would be reasonable to loosen the validation, but for now, people with valid hostnames can't make use of the feature, which has been available relatively recently in 4.8.

benluddy · 2022-03-02T17:46:00Z

I think that's what I'm proposing, too. Aren't these the same components that would be encountering validation errors during Route updates? For example, https://github.com/openshift/cluster-authentication-operator/blob/65cd3f0c3801462ac5bd4f3839a92f021d75f1ce/pkg/controllers/customroute/custom_route_controller.go#L100? I'd expect this controller to set "Degraded: true" on the appropriate entry of configv1.Ingress status in a case where the update payload is invalid.

Couldn't there be cases where some, but not all, of the componentRoutes contain hostnames containing uppercase letters (for example)? If this regex is tightened, none of the components consuming componentRoutes that are valid will no longer be able to keep their respective componentRouteStatus up-to-date. How will users discover that their cluster is in this state?

candita · 2022-03-02T21:05:34Z

At one point in the design, there was no hostname validation, then there was a trusted kubebuilder format that was actually broken, so I am trying to fix that without introducing a complicated regex that is hard to maintain. To fix this bug, all I really need to do is make it allow dashes and numbers in the TLD. But there are so many other potential bugs with the former regex, that it's worth fixing this now (it doesn't check for total length, allows symbols and upper case, doesn't allow one-word hostnames, allow start/end of labels with non-alphanumeric)

There is one componentRouteStatus per componentRoute, stored in configv1.IngressStatus, so each has its own state.

api/config/v1/types_ingress.go

Line 101 in a615696

ComponentRoutes []ComponentRouteStatus `json:"componentRoutes,omitempty"`

Nothing in IngressOperator uses or updates that IngressStatus, so it must be there for the components. Did I answer your question?

tkashem · 2022-03-03T12:50:01Z

if I understand it correctly, the components (other operators than ingress operator) will update the status of the singleton cluster object, right? If so, post upgrade, update operation to the status field of the cluster object will fail if there exists a hostname that was previously valid but is no longer valid due to the change.

we want to understand how big of a problem that is.

candita · 2022-03-21T18:54:01Z

The verify test fails due to using a invalid construct for RE2. (?=.{1,253}$), as lookahead, only works in PCRE. I removed it.

openshift-ci · 2022-03-21T19:19:06Z

@candita: This pull request references Bugzilla bug 2039256, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.11.0) matches configured target release for branch (4.11.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @quarterpin

In 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 kubernetes/test-infra repository.

config/v1/types_ingress.go

candita · 2022-03-23T20:07:59Z

/retest

candita · 2022-03-23T20:48:50Z

@Miciah, I fixed that last typo. Could you PTAL and label?

Miciah · 2022-03-24T15:17:58Z

Thanks!
/lgtm

openshift-ci · 2022-03-24T15:25:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: candita, deads2k, Miciah, tkashem

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [deads2k]

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

openshift-ci · 2022-03-24T15:31:19Z

@candita: all tests passed!

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/test-infra repository. I understand the commands that are listed here.

openshift-ci · 2022-03-24T15:31:53Z

@candita: This pull request references Bugzilla bug 2039256. The bug has been updated to no longer refer to the pull request using the external bug tracker.

In 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 kubernetes/test-infra repository.

candita · 2022-03-24T22:22:44Z

Would this fix the missing bugzilla link?
/bugzilla refresh

openshift-ci · 2022-03-24T22:22:57Z

@candita: All pull requests linked via external trackers have merged:

Bugzilla bug 2039256 has been moved to the MODIFIED state.

In 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 kubernetes/test-infra repository.

candita · 2022-04-06T21:34:32Z

/cherry-pick release-4.10

openshift-cherrypick-robot · 2022-04-06T21:35:13Z

@candita: new pull request created: #1162

In 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 kubernetes/test-infra repository.

candita · 2022-05-03T18:20:03Z

/cherry-pick release-4.8

openshift-cherrypick-robot · 2022-05-03T18:20:44Z

@candita: #1120 failed to apply on top of branch "release-4.8":

Applying: Bug 2039256: kubebuilder validation format hostname is not RFC compliant
Using index info to reconstruct a base tree...
M	config/v1/0000_10_config-operator_01_ingress.crd.yaml
M	config/v1/types_ingress.go
Falling back to patching base and 3-way merge...
Auto-merging config/v1/types_ingress.go
Auto-merging config/v1/0000_10_config-operator_01_ingress.crd.yaml
CONFLICT (content): Merge conflict in config/v1/0000_10_config-operator_01_ingress.crd.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Bug 2039256: kubebuilder validation format hostname is not RFC compliant
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In 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 kubernetes/test-infra repository.

candita · 2022-05-03T18:25:30Z

@candita: #1120 failed to apply on top of branch "release-4.8":

Applying: Bug 2039256: kubebuilder validation format hostname is not RFC compliant
Using index info to reconstruct a base tree...
M	config/v1/0000_10_config-operator_01_ingress.crd.yaml
M	config/v1/types_ingress.go
Falling back to patching base and 3-way merge...
Auto-merging config/v1/types_ingress.go
Auto-merging config/v1/0000_10_config-operator_01_ingress.crd.yaml
CONFLICT (content): Merge conflict in config/v1/0000_10_config-operator_01_ingress.crd.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Bug 2039256: kubebuilder validation format hostname is not RFC compliant
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Maybe not.

…validation format hostname is not RFC compliant

Bug 2081457: kubebuilder validation format hostname is not RFC compliant

openshift-ci bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Feb 11, 2022

openshift-ci bot requested review from quarterpin, knobunc and sjenning February 11, 2022 14:43

candita force-pushed the BZ-2039256-componentRoutesHostnameRegex branch from cbaf1ae to d6c4fd5 Compare February 11, 2022 14:56

candita force-pushed the BZ-2039256-componentRoutesHostnameRegex branch from d6c4fd5 to 06635b6 Compare February 17, 2022 20:30

Miciah reviewed Feb 21, 2022

View reviewed changes

config/v1/types_ingress.go Outdated Show resolved Hide resolved

candita force-pushed the BZ-2039256-componentRoutesHostnameRegex branch from 06635b6 to 9ae3162 Compare February 28, 2022 16:45

openshift-ci bot assigned Miciah Feb 28, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2022

openshift-ci bot assigned benluddy Feb 28, 2022

candita force-pushed the BZ-2039256-componentRoutesHostnameRegex branch from e204de0 to ec20d3a Compare March 21, 2022 19:36

Miciah reviewed Mar 23, 2022

View reviewed changes

config/v1/types_ingress.go Outdated Show resolved Hide resolved

Bug 2039256: kubebuilder validation format hostname is not RFC compliant

1fec415

candita force-pushed the BZ-2039256-componentRoutesHostnameRegex branch from ec20d3a to 1fec415 Compare March 23, 2022 16:51

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2022

openshift-merge-robot merged commit 0012fcd into openshift:master Mar 24, 2022

candita mentioned this pull request Mar 24, 2022

Bug 2039256: Bump API for ComponentRoutes fix openshift/cluster-config-operator#239

Closed

openshift-cherrypick-robot mentioned this pull request Apr 6, 2022

[release-4.10] Bug 2072739: kubebuilder validation format hostname is not RFC compliant #1162

Merged

This was referenced Apr 8, 2022

[release-4.10] Bug 2072739: kubebuilder validation format hostname is not RFC compliant #1167

Closed

[release-4.10] Bug 2072739: kubebuilder validation format hostname is not RFC compliant #1168

Closed

candita added a commit to candita/api that referenced this pull request May 5, 2022

Bug 2081457: Cherry-pick openshift#1120 to release 4.8 - kubebuilder …

421fd6d

…validation format hostname is not RFC compliant

candita mentioned this pull request May 5, 2022

Bug 2081457: kubebuilder validation format hostname is not RFC compliant #1190

Closed

candita added a commit to candita/api that referenced this pull request May 5, 2022

Cherry-pick openshift#1120 to [release-4.8]

3db5ed2

Bug 2081457: kubebuilder validation format hostname is not RFC compliant

candita mentioned this pull request May 5, 2022

Bug 2081457: kubebuilder validation format hostname is not RFC compliant #1191

Merged

Miciah mentioned this pull request Mar 19, 2024

OCPBUGS-31058: config/ingress: Make Hostname godoc user-friendly #1819

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 2039256: kubebuilder validation format hostname is not RFC compliant #1120

Bug 2039256: kubebuilder validation format hostname is not RFC compliant #1120

candita commented Feb 11, 2022 •

edited

openshift-ci bot commented Feb 11, 2022

openshift-ci bot commented Feb 11, 2022

openshift-ci bot commented Feb 14, 2022

openshift-ci bot commented Feb 14, 2022

openshift-ci bot commented Feb 14, 2022

openshift-ci bot commented Feb 14, 2022

openshift-ci bot commented Feb 14, 2022

openshift-ci bot commented Feb 17, 2022

candita commented Feb 17, 2022

candita commented Feb 17, 2022

Miciah commented Feb 28, 2022

openshift-ci bot commented Feb 28, 2022

tkashem commented Feb 28, 2022

benluddy commented Mar 1, 2022

candita commented Mar 2, 2022 •

edited

benluddy commented Mar 2, 2022

candita commented Mar 2, 2022 •

edited

tkashem commented Mar 3, 2022

candita commented Mar 21, 2022

openshift-ci bot commented Mar 21, 2022

candita commented Mar 23, 2022

candita commented Mar 23, 2022

Miciah commented Mar 24, 2022

openshift-ci bot commented Mar 24, 2022

openshift-ci bot commented Mar 24, 2022

openshift-ci bot commented Mar 24, 2022

candita commented Mar 24, 2022

openshift-ci bot commented Mar 24, 2022

candita commented Apr 6, 2022

openshift-cherrypick-robot commented Apr 6, 2022

candita commented May 3, 2022 •

edited

openshift-cherrypick-robot commented May 3, 2022

candita commented May 3, 2022

Bug 2039256: kubebuilder validation format hostname is not RFC compliant #1120

Bug 2039256: kubebuilder validation format hostname is not RFC compliant #1120

Conversation

candita commented Feb 11, 2022 • edited

openshift-ci bot commented Feb 11, 2022

openshift-ci bot commented Feb 11, 2022

openshift-ci bot commented Feb 14, 2022

openshift-ci bot commented Feb 14, 2022

openshift-ci bot commented Feb 14, 2022

openshift-ci bot commented Feb 14, 2022

openshift-ci bot commented Feb 14, 2022

openshift-ci bot commented Feb 17, 2022

candita commented Feb 17, 2022

candita commented Feb 17, 2022

Miciah commented Feb 28, 2022

openshift-ci bot commented Feb 28, 2022

tkashem commented Feb 28, 2022

benluddy commented Mar 1, 2022

candita commented Mar 2, 2022 • edited

benluddy commented Mar 2, 2022

candita commented Mar 2, 2022 • edited

tkashem commented Mar 3, 2022

candita commented Mar 21, 2022

openshift-ci bot commented Mar 21, 2022

candita commented Mar 23, 2022

candita commented Mar 23, 2022

Miciah commented Mar 24, 2022

openshift-ci bot commented Mar 24, 2022

openshift-ci bot commented Mar 24, 2022

openshift-ci bot commented Mar 24, 2022

candita commented Mar 24, 2022

openshift-ci bot commented Mar 24, 2022

candita commented Apr 6, 2022

openshift-cherrypick-robot commented Apr 6, 2022

candita commented May 3, 2022 • edited

openshift-cherrypick-robot commented May 3, 2022

candita commented May 3, 2022

candita commented Feb 11, 2022 •

edited

candita commented Mar 2, 2022 •

edited

candita commented Mar 2, 2022 •

edited

candita commented May 3, 2022 •

edited