OCPBUGS-27741: Reject Invalid Route Paths with Rewrite-Target Annotation #1791
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
jira/severity-critical
|
@gcs278: This pull request references Jira Issue OCPBUGS-27741, which is invalid:
Comment The bug has been updated to 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gcs278 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 |
gcs278
force-pushed
the
OCPBUGS-27741-reject-invalid-path-with-rewrite-target
branch
from
f0754de
to
707a5f2
Compare
| @@ -212,6 +214,42 @@ func validateRoute(ctx context.Context, route *routev1.Route, checkHostname bool | |||
| result = append(result, errs...) | |||
| } | |||
|
|
|||
| if errs := validatePathWithRewriteTargetAnnotation(route, specPath.Child("path")); len(errs) != 0 { | |||
There was a problem hiding this comment.
It appears you only have an instant view of the object here, and don't have the prior view of it, is that correct?
Typically in webhooks you can compare state with the old state. If you can do that, you can make this validation ratchet by allowing the persistence of existing bad configuration, and only validate when there is actually a change to the path/annotation.
That would be safer here, as, presently, in theory, someone could upgrade and this would then prevent any writes to the object until the validation issue has been resolved.
If the validation here fails, I understand that HAProxy will be in a crash loop right? So in theory, the cluster would not be upgradeable anyway, and so the argument is that this doesn't need to ratchet?
There was a problem hiding this comment.
Correct. Let me clarify this: what happens with this current non-ratcheting validation when someone updates with a newly invalid route?
I think the answer is: nothing...until the user changes any field on the invalid route. It's not going to cause a K8S cluster melt-down right? Will ALL updates, including status updates made by the openshift-router be rejected too? (that might be okay, but something for me think about...)
On the other hand, ratcheting validation allows the user to still update other fields route while our spec.path still be invalid (but we won't reject yet), correct?
I think if i understand how non-ratcheting validation compares to ratcheting (e.g. non-ratcheting rejects ALL updates to a route), yes, I think we should be okay since the validation I'm adding results in a HAProxy crash loop, which means the user is already broken.
I'd like to confirm if this is a valid use case for doing the sledgehammer of non-ratcheting validation, since, yes, HAProxy will be crashlooping in any of these situations.
There was a problem hiding this comment.
I think the answer is: nothing...until the user changes any field on the invalid route. It's not going to cause a K8S cluster melt-down right? Will ALL updates, including status updates made by the openshift-router be rejected too? (that might be okay, but something for me think about...)
Test this, but, I believe it will prevent ALL writes, including status.
On the other hand, ratcheting validation allows the user to still update other fields route while our spec.path still be invalid (but we won't reject yet), correct?
Correct, they would only be presented with errors when they change something specific to this validation, the spec path or the annotation in this particular case.
I think if i understand how non-ratcheting validation compares to ratcheting (e.g. non-ratcheting rejects ALL updates to a route), yes, I think we should be okay since the validation I'm adding results in a HAProxy crash loop, which means the user is already broken.
How would this manifest in the cluster? Would it cause an upgradeable condition to go false on the cluster operator? If it does, then the user must fix the issue before they upgrade into a version containing this validation, right?
There was a problem hiding this comment.
Test this, but, I believe it will prevent ALL writes, including status.
Hmm looks like if I try to edit the status via oc edit route... it fails, but the router status update appears to be successful despite the non-ratcheting validation.
How would this manifest in the cluster? Would it cause an upgradeable condition to go false on the cluster operator? If it does, then the user must fix the issue before they upgrade into a version containing this validation, right?
No upgradeable condition currently. The problem with doing upgradeable, is that we want to backport. In theory, I think we could do upgradeable=false on z streams, but previously the upgrade team didn't like the idea of a single route preventing an entire cluster from upgrading (route admin vs. cluster admin personas), so we used Admin-Acks so the cluster admin could override, which only works for minor level upgrades. Upgradeable=false is also going to be more painful to implement.
The real argument against upgradeable=false, is that users that are already in this state (have invalid config), are already broken. Upgradeable doesn't feel useful when routers are already broken.
So, users have a broken HAProxy if they enter this invalid state we are adding validation for. Ratcheting validation would allow them to modify other config fields, but what's the point if they are totally broken? Arguably they should fix the invalid state ASAP. However, the only argument for ratcheting validation is: it is somewhat of a safety mechanism in case we miss an edge case where a newly invalid state WAS actually valid (didn't cause failure). However, it's still ratcheting validation, so eventually if they recreate the route, or try to change spec.path or the rewrite-target annotation, the validation will catch up with them. And this scenario would be a bug we should fix anyways.
So, I don't see any strong reasons to do ratcheting validation. @JoelSpeed Do you agree, or have any thoughts otherwise?
There was a problem hiding this comment.
Hmm looks like if I try to edit the status via oc edit route... it fails, but the router status update appears to be successful despite the non-ratcheting validation.
Did you use the --subresource status flag when trying oc edit? You'll need to if the object has a status subresource
The real argument against upgradeable=false, is that users that are already in this state (have invalid config), are already broken. Upgradeable doesn't feel useful when routers are already broken.
I wasn't suggesting to implement an upgradeable false, sorry, I meant, if the routers are already broken, how does the cluster operator look? Does an invalid config today (causing restarting HAProxy) already cause an upgradeable false or some other note in the cluster that would prevent the user upgrading into this while already broken?
Reject routes with the rewrite-target annotation set and a spec.path that would cause HAProxy startup failure. This solution maintains compatability for the rewrite-target annotation while only rejecting routes with problematic configuration. Additionally, add warnings to indicate to users that their route configuration with rewrite-target may produce unexpected results.
gcs278
force-pushed
the
OCPBUGS-27741-reject-invalid-path-with-rewrite-target
branch
from
707a5f2
to
5de0e83
Compare
Reject routes with the rewrite-target annotation set and a spec.path that would cause HAProxy startup failure. This solution maintains compatability for the rewrite-target annotation while only rejecting routes with problematic configuration. Additionally, add warnings to indicate to users that their route configuration with rewrite-target may produce unexpected results.