Fix haproxy router config manager issue where sanitize pems cause a blueprint route to not be selected #20646
Conversation
openshift-ci-robot
added
sig/network-edge
size/M
| @@ -915,8 +928,23 @@ func (entry *routeBackendEntry) BuildMapAssociations(route *routeapi.Route) { | |||
| } | |||
| } | |||
|
|
|||
| // validateBlueprint runs extended validation on a blueprint route. | |||
| func validateBlueprintRoute(route *routeapi.Route) error { | |||
There was a problem hiding this comment.
The function name in the comment and the actual function name differ.
| @@ -915,8 +928,23 @@ func (entry *routeBackendEntry) BuildMapAssociations(route *routeapi.Route) { | |||
| } | |||
| } | |||
|
|
|||
| // validateBlueprint runs extended validation on a blueprint route. | |||
| func validateBlueprintRoute(route *routeapi.Route) error { | |||
| errs := validation.ExtendedValidateRoute(route) | |||
There was a problem hiding this comment.
Check out this package for easier aggregated error handling.
| @@ -199,6 +204,14 @@ func (cm *haproxyConfigManager) AddBlueprint(route *routeapi.Route) { | |||
| newRoute.Namespace = blueprintRoutePoolNamespace | |||
| newRoute.Spec.Host = "" | |||
|
|
|||
| if cm.extendedValidation { | |||
| if err := validateBlueprintRoute(newRoute); err != nil { | |||
| glog.Errorf("Skipping blueprint route %s/%s due to invalid configuration: %v", | |||
There was a problem hiding this comment.
If invalid routes are present, what's the impact to the cluster, and what action must the admin take? What's the likelihood the admin would ever see this error? Are invalid blueprint route errors surfaced in some other way (e.g. via status)?
There was a problem hiding this comment.
So in the normal case if a route is invalid, the route is not admitted by the router and the status is updated (if router has perms) with the invalidation errors.
In the case of blueprint routes, we aren't really updating the status - it may well end up happening (explained below) but we can only log here that there was an error and ignore that blueprint.
Now its possible the blueprint route is in a namespace that is also being served by the router - in which case the router will update the status of the blueprint route (well as far as the router is concerned in this case its just another route) with the errors.
There was a problem hiding this comment.
Will the error continue to happen every resync interval as long as the naughty route exists? Would an event be more useful?
There was a problem hiding this comment.
Hmm, that's true - yeah, it would happen every resync interval.
There was a problem hiding this comment.
@ironcladlou looked at event generation fixes here - it might end up being a wee bit too invasive here for a bug fix as we don't have the client here, etc.
I'd rather do that separately in a follow on PR - which you can decide whether or not to put in this release. That sound good?
There was a problem hiding this comment.
Had a chance to actually read some of the controller code.... looks like the admin controls the namespace containing the route templates, and so probably also controls the routes in the blueprint namespace. So, if there's an invalid route there which was causing an issue of any kind, the route could presumably be modified to be valid or be removed. Does that sound accurate? Thanks for being patient with explanations!
There was a problem hiding this comment.
Yes that is correct - the admin does control the namespace.
So generating an event might be a wee bit lower in priority but I think generating an event will still be better (after logging an initial error).
…ch when extended validation is enabled (causes a reload where none is needed). fixes bugz #1615802 o address review comments.
ramr
force-pushed
the
fix-extended-validation-diffs
branch
from
fc23518
to
8773471
Compare
| @@ -915,8 +928,18 @@ func (entry *routeBackendEntry) BuildMapAssociations(route *routeapi.Route) { | |||
| } | |||
| } | |||
|
|
|||
| // validateBlueprintRoute runs extended validation on a blueprint route. | |||
| func validateBlueprintRoute(route *routeapi.Route) error { | |||
There was a problem hiding this comment.
I think you can actually delete this entire function and replace all usages with:
if err := validation.ExtendedValidateRoute(newRoute).ToAggregate(); err != nil {
glog.Errorf("Skipping blueprint route %s/%s due to invalid configuration: %s", route.Namespace, route.Name, err)
}There was a problem hiding this comment.
Will do it in a followup. Thx
|
@ramr Just a small nit with some potentially redundant code (#20646 (comment)), no reason to block on it. Can refactor in a tiny followup if you agree. Thanks for the fix! /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ironcladlou, ramr 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 |
openshift-ci-robot
added
the
approved
Fix haproxy router config manager issue where sanitize pems don't match when
extended validation is enabled (causes a reload where none is needed).
fixes bugz #1615802
(Sorta depends on #20630 to merge to test dynamic blueprint changes).
/cc @openshift/sig-network-edge