Bug 1837251: Add unsupported http/2 kill switch #401
Conversation
openshift-ci-robot
added
the
approved
frobware
changed the title
openshift-ci-robot
added
bugzilla/severity-urgent
|
@frobware: This pull request references Bugzilla bug 1837251, 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
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. |
This is to support the kill functionality added by: openshift/cluster-ingress-operator#401
This is to support the kill functionality added by: openshift/cluster-ingress-operator#401
|
@frobware: This pull request references Bugzilla bug 1837251, which is valid. 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. |
|
The failures might be genuine to this PR.... /retest |
|
The operator's log output from the CI run repeats the following error: The operator has permission to get the resource: cluster-ingress-operator/manifests/00-cluster-role.yaml Lines 75 to 84 in 8a8beb1 However I think you need permission to list and watch: - apiGroups:
- config.openshift.io
resources:
- ingresses
verbs:
- list
- watch |
| @@ -498,6 +514,11 @@ func desiredRouterDeployment(ci *operatorv1.IngressController, ingressController | |||
| env = append(env, corev1.EnvVar{Name: WildcardRouteAdmissionPolicy, Value: "false"}) | |||
| } | |||
|
|
|||
| if HTTP2IsDisabledByAnnotation(ci.Annotations) || HTTP2IsDisabledByAnnotation(ingressConfig.Annotations) { | |||
There was a problem hiding this comment.
What if the cluster ingress config has ingress.operator.openshift.io/unsupported-disable-http2=true and the ingresscontroller has ingress.operator.openshift.io/unsupported-disable-http2=false? Seems like the ingresscontroller's annotation should override the ingress config's. Granted, this is an unsupported annotation, so I wouldn't worry too much about this if it makes the logic hairy.
There was a problem hiding this comment.
Seems like the ingresscontroller's annotation should override the ingress config's.
Interesting. I interpreted the ingress config to have the higher priority. Rather than annotating all the ingresscontrollers you could just add it to the ingress config to disable it everywhere.
There was a problem hiding this comment.
My reasoning is that if the user explicitly added an annotation to both the ingress config as well as to an individual ingresscontroller, then the user is expressing a general preference with the former annotation and an overriding preference for the specific ingresscontroller with the latter annotation.
Adding: ingress.operator.openshift.io/unsupported-disable-http2: "true" to either an ingresscontroller or the ingress configuration will disable HTTP/2 support in the router deployment. The corresponding change for the router: openshift/router#133 Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1837251
|
@Miciah I addressed all your feedback with the exception of changing the logic for who wins when the config is "true/false" vis-a-vis when the ingresscontroller disables with "true/false". How much complexity do we want here? If either is set, the ingress controller will have http/2 disabled. I also moved the new e2e test into its own file and factored out the functions from the core test logic. PTAL |
|
I did some manual testing with openshift/router#133 and using the http/2 tests in origin I was able to see that backend would report "HTTP 1.1" when the kill switch was in effect. And testing against the grpc-interop-reencrypt route also failed. |
|
You got a test failure: You probably need to do a |
Going to do both. Admittedly I've only been testing the single http/2 e2e locally, relying on CI for the full gamut. |
|
/retest |
The controller now needs to watch for ingress objects so that we can detect if the http/2 disable annotation has been added. We watch the ingress config resource and trigger reconcile for all related ingresscontrollers.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: frobware, 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 |
|
@frobware: Some pull requests linked via external trackers have merged: openshift/cluster-ingress-operator#401. The following pull requests linked via external trackers have not merged:
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. |
Adding:
ingress.operator.openshift.io/unsupported-disable-http2: "true"
to either an ingresscontroller or the ingress configuration will
disable HTTP/2 support in the router deployment.
Corresponding router change: openshift/router#133