Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.11 router] dynamic configuration manager support for custom configuration templates #21170

Closed
alexcern opened this issue Oct 4, 2018 · 9 comments
Assignees
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. sig/network-edge

Comments

@alexcern
Copy link
Contributor

alexcern commented Oct 4, 2018

The router dynamic configuration manager introduced by #19073 for the upcoming 3.11 release is a very welcome addition, many thanks @ramr !

We would be interested to make it work with customized router templates when the customization involves new, custom annotations.

Version

3.11 alpha

Steps To Reproduce
  1. Enable dynamic configuration manager
  2. Set up a customized haproxy router template involving custom route annotations. For instance add something like this to backend definitions:
  {{ if isTrue (index $cfg.Annotations "my-custom-annotation")  }}
    acl my_condition src -m ip 1.2.3.4/32
    tcp-request content reject unless my_condition
  {{ end }}
  1. Create a route with annotation my-custom-annotation=true
Current Result

my-custom-annotation is not taken into account when matching the new annotated route to a blueprint. Thus a blueprint without the annotation is used, and the custom configuration associated with my-custom-annotation is not applied.

Expected Result

A blueprint matching the custom annotation is selected, or no blueprint with that annotation is found and router configuration is reloaded completely to enforce the custom configuration.

Additional Information

It seems to me this there could be two ways to support a customized router template with the dynamic manager:

  • allow an empty pool of blueprints so every route added or changed causes a full reload, this may be as simple as allowing 0 for the minimum value of ROUTER_BLUEPRINT_ROUTE_POOL_SIZE in
    flag.IntVar(&o.BlueprintRoutePoolSize, "blueprint-route-pool-size", int(util.EnvInt("ROUTER_BLUEPRINT_ROUTE_POOL_SIZE", 10, 1)), "Specifies the size of the pre-allocated pool for each route blueprint managed by the router specific dynamic configuration manager. This can be overriden by an annotation router.openshift.io/pool-size on an individual route.")
    - the dynamic manager remains interesting for dynamic management of endpoints
  • add a new router configuration option where one can specify a list of additional custom annotations that need to be taken into account when matching blueprints. This would be merged with the default annotation list in
    func modAnnotationsList(termination routev1.TLSTerminationType) []string {
    annotations := []string{
    "haproxy.router.openshift.io/balance",
    "haproxy.router.openshift.io/ip_whitelist",
    "haproxy.router.openshift.io/timeout",
    "haproxy.router.openshift.io/rate-limit-connections",
    "haproxy.router.openshift.io/rate-limit-connections.concurrent-tcp",
    "haproxy.router.openshift.io/rate-limit-connections.rate-tcp",
    "haproxy.router.openshift.io/rate-limit-connections.rate-http",
    "haproxy.router.openshift.io/pod-concurrent-connections",
    "router.openshift.io/haproxy.health.check.interval",
    }
    if termination == routev1.TLSTerminationPassthrough {
    return annotations
    }
    annotations = append(annotations, "haproxy.router.openshift.io/disable_cookies")
    annotations = append(annotations, "router.openshift.io/cookie_name")
    annotations = append(annotations, "haproxy.router.openshift.io/hsts_header")
    return annotations
    }

I would be happy to submit a PR for either option.

@alexcern alexcern changed the title Router dynamic configuration manager support for custom configuration templates [3.11 router] dynamic configuration manager support for custom configuration templates Oct 4, 2018
@ramr
Copy link
Contributor

ramr commented Oct 4, 2018

@alexcern cool. Yeah, adding support for custom templates/annotations was something that we had punted and would love to get a PR for this.

Thinking of this from a general configuration standpoint, something that encompasses both of your options could be useful.

  1. To make a route not use a blueprint aka cause a router to just flush out the config and reload.
    One idea might be to use a well-known annotation
    (e.g. haproxy.router.openshift.io/skip-route-blueprint-match)
    and if a route contains that annotation return no matching blueprint for the route
    in the configmanager code for findMatchingBlueprint.

  2. A mechanism to add to or update the list of annotations that modify the haproxy config.
    We could do that statically via a environment-variable/command line option or maybe another
    alternative would be to read it dynamically aka have some config object that we watch and update
    the info from dynamically. Example a configmap (could be in the blueprints namespace).

/cc @ironcladlou

Aside: The configmap idea from above got me thinking it might be worth looking at having some of the router options be dynamically update-able. Some of the options like max connections, dynamic servers/blueprint pool sizes, resync/commit/reload intervals (probably even the template and reload-script although there is a breakage possibility to consider therein) could potentially be updated at run time without requiring a router re-deployment - a router reload would suffice. Just a thought.

@jwforres
Copy link
Member

@openshift/sig-network-edge

@alexcern
Copy link
Contributor Author

Thanks @ramr

The skip-route-blueprint-match annotation makes a lot of sense. I will prepare a PR with this and an environment variable to override the list of annotations for blueprint match.

Ideally we would set this together with the template itself, since the list of annotations to match is exactly the list of annotations used by the template to vary the configuration. But I could not think of a fine way to integrate this in the template definition.

isantospardo added a commit to isantospardo/origin that referenced this issue Nov 22, 2018
…nshift#21170

Adds a possibility to configure a list of annotations that need to vary
the HAProxy configuration, and adds a route annotation that disables
matching routes to existing "blueprints"
isantospardo added a commit to isantospardo/origin that referenced this issue Nov 22, 2018
…nshift#21170

Adds a possibility to configure a list of annotations that need to vary
the HAProxy configuration, and adds a route annotation that disables
matching routes to existing "blueprints"
isantospardo added a commit to isantospardo/origin that referenced this issue Nov 22, 2018
…nshift#21170

Adds a possibility to configure a list of annotations that need to vary
the HAProxy configuration, and adds a route annotation that disables
matching routes to existing "blueprints"
isantospardo added a commit to isantospardo/origin that referenced this issue Nov 23, 2018
…nshift#21170

Adds a possibility to configure a list of annotations that need to vary
the HAProxy configuration, and adds a route annotation that disables
matching routes to existing "blueprints"
isantospardo added a commit to isantospardo/origin that referenced this issue Nov 29, 2018
…nshift#21170

Adds a possibility to configure a list of annotations that need to vary
the HAProxy configuration, and adds a route annotation that disables
matching routes to existing "blueprints"
@isantospardo
Copy link
Contributor

@ramr Ready for a review when possible :)

@ramr
Copy link
Contributor

ramr commented Dec 11, 2018

@isantospardo thanks for the PR ... sorry for the delay in reviewing - have been out of commission for a while. I left some comments/requested some changes on the PR. Thanks again.

isantospardo added a commit to isantospardo/origin that referenced this issue Dec 11, 2018
…nshift#21170

Adds a possibility to configure a list of annotations that need to vary
the HAProxy configuration, and adds a route annotation that disables
matching routes to existing "blueprints"
isantospardo added a commit to isantospardo/origin that referenced this issue Dec 12, 2018
…nshift#21170

Adds a possibility to configure a list of annotations that need to vary
the HAProxy configuration, and adds a route annotation that disables
matching routes to existing "blueprints"
isantospardo added a commit to isantospardo/origin that referenced this issue Dec 13, 2018
…nshift#21170

Adds a possibility to configure a list of annotations that need to vary
the HAProxy configuration, and adds a route annotation that disables
matching routes to existing "blueprints"
isantospardo added a commit to isantospardo/origin that referenced this issue Jan 9, 2019
…nshift#21170

Adds a possibility to configure a list of annotations that need to vary
the HAProxy configuration, and adds a route annotation that disables
matching routes to existing "blueprints"
isantospardo added a commit to isantospardo/origin that referenced this issue Jan 9, 2019
…nshift#21170

Adds a possibility to configure a list of annotations that need to vary
the HAProxy configuration, and adds a route annotation that disables
matching routes to existing "blueprints"
isantospardo added a commit to isantospardo/origin that referenced this issue Jan 9, 2019
…nshift#21170

Adds a possibility to configure a list of annotations that need to vary
the HAProxy configuration, and adds a route annotation that disables
matching routes to existing "blueprints"
@openshift-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci-robot openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 11, 2019
@openshift-bot
Copy link
Contributor

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci-robot openshift-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 10, 2019
@openshift-bot
Copy link
Contributor

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

@openshift-ci-robot
Copy link

@openshift-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. sig/network-edge
Projects
None yet
7 participants