Bug 1896977: Enhance API host name validation #167
Conversation
openshift-ci-robot
added
bugzilla/severity-low
|
@candita: This pull request references Bugzilla bug 1896977, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
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. |
|
Companion PR in openshift/router: openshift/router#238 |
| } else { | ||
| errs := kvalidation.IsFullyQualifiedDomainName(specPath.Child("host"), route.Spec.Host) | ||
| if len(errs) != 0 { | ||
| result = append(result, field.Invalid(specPath.Child("host"), route.Spec.Host, fmt.Sprintf("host must conform to DNS 1123 subdomain conventions: %v", errs))) |
There was a problem hiding this comment.
I don't understand why the else branch's IsFullyQualifiedDomainName error gets associated with DNS 1123 subdomain conventions in the error message, while the previous if branch's IsDNS1123Subdomain error gets the generic DNS subdomain conventions conventions. Wouldn't IsDNS1123Subdomain be the one looking for DNS 1123 subdomain violations?
There was a problem hiding this comment.
IsFullyQualifiedDomain calls IsDNS1123Subdomain, IsDNS1123Label, and performs several other checks, making it more closely conform to RFC1123.
| expectedErrors: 1, | ||
| }, | ||
| { | ||
| name: "Non-dns compliant host with override annotation", |
There was a problem hiding this comment.
nit: dns -> DNS here and in other human-oriented strings.
| To: createRouteSpecTo("serviceName", "Service"), | ||
| }, | ||
| }, | ||
| expectedErrors: 1, |
There was a problem hiding this comment.
Somewhat in this space, because it's a bit wiggly without actual error string comparison: do we want explicit checks for "really long subdomain, but overall hostname is short enough" showing we error on that (or not, with the override annotation)?
There was a problem hiding this comment.
| expectedErrors: 1, | ||
| }, | ||
| { | ||
| name: "Generated host name too long", |
There was a problem hiding this comment.
nit: Spec.Host is explicitly set here, so "Generated host name too long" -> "Specified host name too long"? Spec.Host could have been set by default-host-generating logic, or could have been set by whoever created the Route in the first place, and this test-case doesn't need to distinguish between those two cases.
candita
force-pushed
the
BZ-1896977-APIHostnameValidation
branch
from
2c5d8f0
to
d1eff36
Compare
|
/test e2e-aws-upgrade |
|
/hold for openshift/origin#25770 |
openshift-ci-robot
added
the
do-not-merge/hold
| if len(route.Spec.Host) > 0 { | ||
| if len(kvalidation.IsDNS1123Subdomain(route.Spec.Host)) != 0 { | ||
| result = append(result, field.Invalid(specPath.Child("host"), route.Spec.Host, "host must conform to DNS 952 subdomain conventions")) | ||
| if lenient, ok := route.Annotations[oapi.OpenShiftAllowNonDNSCompliantHostAnnotation]; ok && lenient == "true" { |
There was a problem hiding this comment.
you cannot tighten this for updates that are not changing the route.spec.host. That means you'll need to find a way to split up the update validation.
There was a problem hiding this comment.
Thanks @deads2k , that sounds reasonable. I have made a change and updated some unit tests. PTAL when you get a chance.
|
as long as we don't break route REST updates and the only impact is to
I think this reasonable. |
candita
force-pushed
the
BZ-1896977-APIHostnameValidation
branch
from
d1eff36
to
11bf5e1
Compare
|
/unhold |
openshift-ci-robot
removed
the
do-not-merge/hold
|
@candita: The following test failed, say
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. |
|
/test e2e-aws |
|
/test e2e-cmd |
1 similar comment
|
/test e2e-cmd |
|
/retest |
|
/test e2e-cmd |
|
https://bugzilla.redhat.com/show_bug.cgi?id=1895107 describes the issue with the failing e2e-cmd test. |
|
/test e2e-cmd |
|
/hold for openshift/library-go#975 |
|
/assign @mfojtik |
| func ValidateRouteUpdate(route *routeapi.Route, older *routeapi.Route) field.ErrorList { | ||
| allErrs := validation.ValidateObjectMetaUpdate(&route.ObjectMeta, &older.ObjectMeta, field.NewPath("metadata")) | ||
| allErrs = append(allErrs, validation.ValidateImmutableField(route.Spec.WildcardPolicy, older.Spec.WildcardPolicy, field.NewPath("spec", "wildcardPolicy"))...) | ||
| if route.Spec.Host != older.Spec.Host { | ||
| allErrs = append(allErrs, ValidateHost(route)...) | ||
| } | ||
| allErrs = append(allErrs, ValidateRoute(route)...) |
There was a problem hiding this comment.
This looks improper based on our previous conversation. The rules for updating are different than the rules for creating. This is a long-standing pattern in kube where we have three distinct methods with different rules
- validateObj - this validates objects for create semantics
- validateObjUpdate - this validates differently than validateObj because create calls can be tighter than update calls. Also, this should ignore status.
- validateObjectStatusUpdate - this validates differently than validateObjUpdate because status updates should never fail on valid specs.
Your validateRoute function should fully validate a route for the purposes of create. Because the update semantics don't match the create semantics, you cannot call one from the other. Making your validateRoute function not fully validate a route isn't a good idea.
There was a problem hiding this comment.
I have addressed this, and Miciah's similar comment #167 (comment), in this PR instead of a followup PR. @deads2k can you PTAL?
candita
force-pushed
the
BZ-1896977-APIHostnameValidation
branch
from
132f77c
to
4ddf12e
Compare
candita
force-pushed
the
BZ-1896977-APIHostnameValidation
branch
from
4ddf12e
to
402c0fd
Compare
|
/test e2e-aws |
2 similar comments
|
/test e2e-aws |
|
/test e2e-aws |
|
/test e2e-aws-upgrade |
|
/test e2e-aws-serial |
|
/test e2e-cmd |
|
I don't think I'd personally express it this way, but the public API is acceptable. /approve |
openshift-ci-robot
added
the
approved
|
/lgtm |
|
@candita: you cannot LGTM your own PR. 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: candita, deads2k, Miciah 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:
Approvers can indicate their approval by writing |
|
@candita: All pull requests linked via external trackers have merged:
Bugzilla bug 1896977 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. |
|
Hey @candita So we recently started seeing errors in our openshift samples testing (stuff that has been around since 3.x of openshift) where creation of simple routes where the host is not specified can fail. Based on the timing and when this merged, and the nature of this change that I can gleam, I suspect it stems from these changes. Here is the key snippet in the test logs: It definitely looks like we are getting the default hostname generation from https://github.com/openshift/openshift-apiserver/blob/master/pkg/route/apiserver/simplerouteallocation/plugin.go#L50-L55 where it takes the route name, namespace, and In this test case, the name is My question is (and I did try to see if this was captured in prior PR discussion, but my skimming did not find anything - apologies if you have to regurgitate old discussion) should we not expect our generation logic to be cognizant of the host field restrictions. ... i.e. start stripping characters from the name and/or namespace etc. to get things under 63? Or is the recommendation that is going to come out from all this is that users are going to have to start explicitly configuring the host field or something of that ilk? @adambkaplan @bparees FYI |
Yes, the generation logic will not do the name-shrinking for you. The workaround is to set a custom host name or use the new annotation which allows invalid host names (which make invalid routes). The solution we decided on was to make the route creation fail faster, without introducing host names that departed from the known formula. If your test here is creating routes, they would have been invalid routes because the generated host name label is too long, and that would not have been discovered at the route-creation validation, but at the route-admission validation. If you require a valid route, you will need to use a custom host name. If you do not require a valid route, you can use a new route annotation to override the strict validation: |
|
@candita: Bugzilla bug 1896977 is in an unrecognized state (VERIFIED) and will not be 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. |
|
On Mon, Feb 8, 2021 at 6:08 PM Candace Holman ***@***.***> wrote:
...should we not expect our generation logic to be cognizant of the host
field restrictions. ... i.e. start stripping characters from the name
and/or namespace etc. to get things under 63?
Or is the recommendation that is going to come out from all this is that
users are going to have to start explicitly configuring the host field or
something of that ilk?
Yes, the generation logic will not do the name-shrinking for you. The
workaround is to set a custom host name or use the new annotation which
allows invalid host names (which make invalid routes).
The solution we decided on was to make the route creation fail faster,
without introducing host names that departed from the known formula. If
your test here is creating routes, they would have been invalid routes
because the generated host name label is too long, and that would not have
been discovered at the route-creation validation, but at the
route-admission validation.
If you require a valid route, you will need to use a custom host name. If
you do not require a valid route, you can use a new route annotation to
override the strict validation:
route.openshift.io/allow-non-dns-compliant-host, described in
https://github.com/openshift/api/blob/670ac3fc997c2f1d19b8c29ef04f70d6e3d4a59e/route/v1/types.go#L287
Gotcha. Thanks for the pointers.
…
<https://github.com/openshift/api/blob/670ac3fc997c2f1d19b8c29ef04f70d6e3d4a59e/route/v1/types.go#L287>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#167 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA3NU5EBJIWIN44A5SRILHDS6BVH3ANCNFSM4U3IRYSQ>
.
|
Change route validation to apply the stricter validation rules from IsFullyQualifiedDomainName on host name when OpenShiftAllowNonDNSCompliantHostAnnotation is false or missing: host name must have at least two labels, with each label less than 64 chars, no disallowed characters, total length less than 254 chars, and trailing dots are allowed. If OpenShiftAllowNonDNSCompliantHostAnnotation is true then use the former lenient validation of just IsDNS1123Subdomain: total length less than 254, and no disallowed characters.
Add new annotation: OpenShiftAllowNonDNSCompliantHostAnnotation = "route.openshift.io/allow-non-dns-compliant-host" to allow configuations that suppress the stricter validation. This may validate routes that cannot be admitted (same behavior as before).
Add/update unit tests.