UPSTREAM: <carry>: add validation for ingress.config.openshift.io component routes #1216
Conversation
openshift-ci-robot
added
the
backports/unvalidated-commits
openshift-ci
bot
added
the
do-not-merge/hold
|
@deads2k: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
openshift-ci
bot
added
the
approved
|
@deads2k: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
openshift-ci
bot
removed
the
do-not-merge/hold
deads2k
removed
the
backports/unvalidated-commits
|
/retest |
| t.Fatal(tc.expectedErr) | ||
| case len(actual) != 0 && len(tc.expectedErr) == 0: | ||
| t.Fatal(actual) | ||
| case len(actual) != 0 && len(tc.expectedErr) != 0: | ||
| found := false | ||
| for _, actualErr := range actual { | ||
| found = found || strings.Contains(actualErr.Error(), tc.expectedErr) | ||
| } | ||
| if !found { | ||
| t.Fatal(actual) |
There was a problem hiding this comment.
If a test fails, the user needs to look closely for brackets or line numbers to figure out whether the printed error is the expected but missing error or an unexpected error.
| t.Fatal(tc.expectedErr) | |
| case len(actual) != 0 && len(tc.expectedErr) == 0: | |
| t.Fatal(actual) | |
| case len(actual) != 0 && len(tc.expectedErr) != 0: | |
| found := false | |
| for _, actualErr := range actual { | |
| found = found || strings.Contains(actualErr.Error(), tc.expectedErr) | |
| } | |
| if !found { | |
| t.Fatal(actual) | |
| t.Fatalf("didn't get expected error: %v", tc.expectedErr) | |
| case len(actual) != 0 && len(tc.expectedErr) == 0: | |
| t.Fatalf("unexpected error: %v", actual) | |
| case len(actual) != 0 && len(tc.expectedErr) != 0: | |
| found := false | |
| for _, actualErr := range actual { | |
| found = found || strings.Contains(actualErr.Error(), tc.expectedErr) | |
| } | |
| if !found { | |
| t.Fatalf("got %q, expected %q", actual tc.expectedErr) |
| t.Fatal(tc.expectedErr) | ||
| case len(actual) != 0 && len(tc.expectedErr) == 0: | ||
| t.Fatal(actual) | ||
| case len(actual) != 0 && len(tc.expectedErr) != 0: | ||
| found := false | ||
| for _, actualErr := range actual { | ||
| found = found || strings.Contains(actualErr.Error(), tc.expectedErr) | ||
| } | ||
| if !found { | ||
| t.Fatal(actual) |
There was a problem hiding this comment.
| t.Fatal(tc.expectedErr) | |
| case len(actual) != 0 && len(tc.expectedErr) == 0: | |
| t.Fatal(actual) | |
| case len(actual) != 0 && len(tc.expectedErr) != 0: | |
| found := false | |
| for _, actualErr := range actual { | |
| found = found || strings.Contains(actualErr.Error(), tc.expectedErr) | |
| } | |
| if !found { | |
| t.Fatal(actual) | |
| t.Fatalf("didn't get expected error: %v", tc.expectedErr) | |
| case len(actual) != 0 && len(tc.expectedErr) == 0: | |
| t.Fatalf("unexpected error: %v", actual) | |
| case len(actual) != 0 && len(tc.expectedErr) != 0: | |
| found := false | |
| for _, actualErr := range actual { | |
| found = found || strings.Contains(actualErr.Error(), tc.expectedErr) | |
| } | |
| if !found { | |
| t.Fatalf("got %q, expected %q", actual tc.expectedErr) |
| plugins.Register(PluginName, func(config io.Reader) (admission.Interface, error) { | ||
| return customresourcevalidation.NewValidator( | ||
| map[schema.GroupResource]bool{ | ||
| configv1.Resource("Ingresses"): true, |
There was a problem hiding this comment.
Maybe it doesn't matter, but shouldn't the resource name be lower-case?
| configv1.Resource("Ingresses"): true, | |
| configv1.Resource("ingresses"): true, |
| return obj, nil | ||
| } | ||
|
|
||
| type featureGateV1 struct { |
There was a problem hiding this comment.
Copypasta, or is that name intentional?
| var knownFeatureSets = sets.NewString( | ||
| "", | ||
| string(configv1.TechPreviewNoUpgrade), | ||
| string(configv1.CustomNoUpgrade), | ||
| string(configv1.IPv6DualStackNoUpgrade), | ||
| string(configv1.LatencySensitive), | ||
| ) |
| if errs := routeapihelpers.ValidateHost( | ||
| string(currRoute.Hostname), | ||
| "", | ||
| field.NewPath("spec.componentRoutes").Index(i).Child("hostname"), | ||
| ); len(errs) > 0 { | ||
| allErrs = append(allErrs, errs...) | ||
| } |
There was a problem hiding this comment.
The length check is superfluous.
| if errs := routeapihelpers.ValidateHost( | |
| string(currRoute.Hostname), | |
| "", | |
| field.NewPath("spec.componentRoutes").Index(i).Child("hostname"), | |
| ); len(errs) > 0 { | |
| allErrs = append(allErrs, errs...) | |
| } | |
| allErrs = append(allErrs, routeapihelpers.ValidateHost( | |
| string(currRoute.Hostname), | |
| "", | |
| field.NewPath("spec.componentRoutes").Index(i).Child("hostname"), | |
| )...) |
| previousRouteHostName := configv1.Hostname("") | ||
| for _, oldRoute := range oldSpec.ComponentRoutes { | ||
| if oldRoute.Name == currRoute.Name && oldRoute.Namespace == currRoute.Namespace { | ||
| previousRouteHostName = oldRoute.Hostname | ||
| break | ||
| } | ||
| } | ||
| // we don't enforce new validation rules if the hostname has not changed | ||
| if previousRouteHostName == currRoute.Hostname { | ||
| continue | ||
| } |
There was a problem hiding this comment.
This logic allows the following test case to pass; is the test case valid, or should it expect an error?
{
name: "no previous value to empty",
hostname: "",
oldHostname: "nohost",
noPrevious: true,
expectedErr: ``,
},
openshift-ci-robot
added
the
backports/unvalidated-commits
|
@deads2k: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
/lgtm |
|
@deads2k: The following tests 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. |
deads2k
removed
the
backports/unvalidated-commits
|
Admission validation carry patches are accepted for types that must logically exist before their operators are present. |
openshift-ci-robot
added
the
backports/unvalidated-commits
|
@deads2k: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
diff of diffs is identical. relabeling. |
deads2k
removed
the
backports/unvalidated-commits
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
|
/refresh |
| { | ||
| name: "new validation fails", | ||
| hostname: "host", | ||
| expectedErr: `spec.componentRoutes[0].hostname: Invalid value: "host": host must conform to DNS 1123 naming conventions: [spec.componentRoutes[0].hostname: Invalid value: "host": should be a domain with at least two segments separated by dots]`, |
There was a problem hiding this comment.
A single label hostname is a valid hostname and should not fail validation
| { | ||
| name: "change from invalid to valid", | ||
| hostname: "host.com", | ||
| oldHostname: "host", |
There was a problem hiding this comment.
A single label hostname is a valid hostname and should not fail validation. Here and in other parts of these tests.
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
We have to hold this until we can work out whether #1216 (comment) is desired behavior or not. I don't actually know. cc @Miciah |
|
openshift/api PR was updated to loosen only. /close |
/hold
just a POC, I don't plan to finish it out, but everything is stubbed for network edge.
/assign @candita