Add support for ingress-configured router #12416

marun · 2017-01-08T04:40:49Z

The controller generates route events from ingress and secret events
ingresses are cached by the controller to enable:
- route deletion when an ingress rule is removed
- tracking of which secrets to cache
- generation of routes in response to secrets events
secrets are cached by the controller to minimize the lookups required to generate routes from an ingress
only secrets referenced by an ingress seen by the router will be cached

TODO

ensure changes to the targeted namespace(s) apply to cached ingresses and secrets
make ingress support configurable and disable by default
fix integration testing of tls route
unit testing

cc: @openshift/networking @smarterclayton

marun · 2017-01-13T23:08:57Z

This is ready for review.

cc: @knobunc @JacobTanenbaum @ramr

marun · 2017-01-13T23:12:47Z

re-[test]

marun · 2017-01-14T01:19:41Z

flake #10773

https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_conformance/10144/consoleFull#-69186922956cbb9a5e4b02b88ae8c2f77

marun · 2017-01-14T14:57:37Z

pkg/router/controller/controller.go

@@ -63,6 +77,8 @@ func (c *RouterController) Run() {
 	if c.WatchNodes {
 		go utilwait.Forever(c.HandleNode, 0)
 	}
+	go utilwait.Forever(c.HandleIngress, 0)


Need to make starting these goroutines conditional on EnableIngress

marun · 2017-01-14T15:04:17Z

~~Also need a unit test for hostname generation to validate that the same hostname will be generated for an ingress and route sharing a namespace and name.~~

Done

marun · 2017-01-16T15:07:11Z

flake #11563 #10773 re-[test]

https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_conformance/10145/consoleFull

marun · 2017-01-16T16:41:30Z

~~It probably makes sense to ensure integration coverage of namespace filtering.~~

Done

knobunc

Nice work Maru. @JacobTanenbaum @rajatchopra @ramr PTAL

knobunc · 2017-01-16T21:01:33Z

pkg/cmd/infra/router/f5.go

@@ -225,7 +225,8 @@ func (o *F5RouterOptions) Run() error {

 	factory := o.RouterSelection.NewFactory(oc, kc)
 	watchNodes := (len(o.InternalAddress) != 0 && len(o.VxlanGateway) != 0)
-	controller := factory.Create(plugin, watchNodes)
+	// TODO should ingress be configurable for f5?


Can the existing plugin support it? We already set the --enable-ingress on the router, may as well use it here if the router is configured for f5.

Ok, I'll enable it. The f5 and template router define their flags separately, so f5 will use --f5-enable-ingress

knobunc · 2017-01-16T21:06:28Z

pkg/router/controller/ingress.go

+	for _, tls := range ingressTLS {
+		// TODO what if a host is matched by more than one IngressTLS instance?
+
+		// TODO what if IngressTLS doesn't specify Hosts?  There's mention of defaulting to a controller-specific


We should do the same as when a route has no host. i.e. use the run-time set one, or the default if that's not specified.

By 'the default', do you mean the generated host name? Currently the host name is generated by the unique_host plugin, so its not currently available when the ingress events are being generated. Should the ingress translator be generating the host name?

I think this is problematic either way. If an ingress is created with rules that lack a host name and tls that requires a specific host name to be applied, the hostname template for a given router could mean the difference between tls being applied or not being applied. Not sure there's any way to solve this, maybe the best that can be done is logging when ingress paths will not have tls applied because no host matches.

Re-reading the code comment, my concern was that the code in its present form can't apply tls that doesn't specify hosts. If an ingress defined a single tls secret without hosts, maybe that tls should be applied to all routes generated from the ingress?

I think that is a good solution. And only one blank host should be allowed? Or the first or last in the file is taken?

Done. The first blank host is chosen.

knobunc · 2017-01-16T21:06:52Z

pkg/router/controller/ingress.go

+// tlsForHost attempts to retrieve ingress tls configuration for the given host
+func (it *IngressTranslator) tlsForHost(ingressTLS []extensions.IngressTLS, namespace, host string) *cachedTLS {
+	for _, tls := range ingressTLS {
+		// TODO what if a host is matched by more than one IngressTLS instance?


Don't we claim it for a namespace?

Host claims are the same as for routes. The point here is that there is nothing stopping an ingress from defining tls entries with overlapping hosts:

apiVersion: extensions/v1beta1 kind: Ingress metadata: name: foo spec: tls: - apiVersion: extensions/v1beta1 kind: Ingress metadata: name: no-rules-map spec: tls: - secretName: secret1 hosts: - foo - secretName: secret2 hosts: - foo

In a case like this, which tls configuration should be applied to a generated route with host foo?

knobunc · 2017-01-16T21:07:56Z

pkg/router/controller/ingress.go

+	// name submitted to the api, but generated routes are only intended to
+	// be used in the context of the router controller.
+	//
+	// TODO should the path be hashed or otherwise encoded for compatibility?


Compatibility with what?

The name of the route (with - and / replaced with _) is used in haproxy configuration. I'm assuming there are other characters that would be legal in a path that would not be legal in haproxy configuration (e.g. ? & and other characters related to a path with query parameters).

@knobunc This should be addressed before merge.

Should we map all but legal characters to _ and then append a hash of the whole thing to ensure uniqueness?

marun · 2017-01-17T16:45:37Z

~~The failing unit test is expecting consistent ordering when iterating over a map. Will fix.~~

Done

marun · 2017-01-17T21:09:12Z

flake #9414

https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_conformance/10265/

marun · 2017-01-17T21:09:18Z

re-[test]

knobunc

LGTM, thanks a lot @marun

JacobTanenbaum

LGTM

openshift-bot · 2017-01-19T21:37:28Z

Evaluated for origin test up to 0e2fa74

openshift-bot · 2017-01-19T22:58:08Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13050/) (Base Commit: 9e77cfe)

knobunc · 2017-01-19T23:07:52Z

[merge]

openshift-bot · 2017-01-19T23:13:26Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13050/) (Image: devenv-rhel7_5730)

openshift-bot · 2017-01-19T23:13:26Z

Evaluated for origin merge up to 0e2fa74

DanyC97 · 2017-01-22T17:22:28Z

@marun will be good if we have few notes which describes how it can be used etc

marun · 2017-01-25T21:33:11Z

@DanyC97 I'm not sure ingress in openshift is worth using over routes in the near term, since there are functional gaps. This is just the first step in reconciling the two. If you'd like to experiment anyway, you'll need to build a router from master (since it isn't yet released) and:

create ingress resources as per the kube docs: https://kubernetes.io/docs/user-guide/ingress/
deploy a router and enable ingress support on the dc (e.g. oc env dc/router ROUTER_ENABLE_INGRESS=true)
Ensure the router service account can read the cluster's secrets. Ingress relies on secrets to store tls configuration, so this only required for secure connections. This is problematic from a security perspective, though, and is not recommended for production clusters.
- oadm policy add-cluster-role-to-user cluster-reader system:serviceaccount:default:router
- oadm policy add-cluster-role-to-user system:service-serving-cert-controller system:serviceaccount:default:router

marun added the component/networking label Jan 8, 2017

marun force-pushed the ingress-configured-router branch 15 times, most recently from 83bd1d9 to 54811f8 Compare January 13, 2017 21:18

marun changed the title ~~WIP Add support for router configured by ingress~~ Add support for ingress-configured router Jan 13, 2017

marun force-pushed the ingress-configured-router branch 4 times, most recently from fc7b3bd to 4e34553 Compare January 13, 2017 23:08

marun force-pushed the ingress-configured-router branch from 4e34553 to 277c406 Compare January 13, 2017 23:11

marun commented Jan 14, 2017

View reviewed changes

marun force-pushed the ingress-configured-router branch 2 times, most recently from 0cf1aec to 7e79991 Compare January 16, 2017 16:34

marun force-pushed the ingress-configured-router branch 2 times, most recently from 68cd11d to 35e12fd Compare January 16, 2017 17:01

knobunc requested review from rajatchopra and JacobTanenbaum January 16, 2017 20:59

knobunc reviewed Jan 16, 2017

View reviewed changes

knobunc requested a review from ramr January 16, 2017 21:09

marun force-pushed the ingress-configured-router branch 3 times, most recently from 3cd9f76 to 6a99d30 Compare January 17, 2017 03:19

marun force-pushed the ingress-configured-router branch from 37c978f to cee94b5 Compare January 17, 2017 19:35

knobunc approved these changes Jan 18, 2017

View reviewed changes

JacobTanenbaum approved these changes Jan 19, 2017

View reviewed changes

marun force-pushed the ingress-configured-router branch from cee94b5 to aec6e82 Compare January 19, 2017 21:14

marun added 2 commits January 19, 2017 13:33

router: add support for configuration by ingress

0ce8a32

router: validate ingress compatibility with namespace filtering

0e2fa74

marun force-pushed the ingress-configured-router branch from aec6e82 to 0e2fa74 Compare January 19, 2017 21:33

openshift-bot merged commit 1ccbb6c into openshift:master Jan 20, 2017

lulf mentioned this pull request Mar 30, 2017

Use kubernetes ingress feature EnMasseProject/enmasse#16

Closed

Add support for ingress-configured router #12416

Add support for ingress-configured router #12416

Conversation

marun commented Jan 8, 2017 • edited Loading

marun commented Jan 13, 2017

marun commented Jan 13, 2017

marun commented Jan 14, 2017

Choose a reason for hiding this comment

marun commented Jan 14, 2017 • edited Loading

marun commented Jan 16, 2017

marun commented Jan 16, 2017 • edited Loading

knobunc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marun Jan 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marun commented Jan 17, 2017 • edited Loading

marun commented Jan 17, 2017

marun commented Jan 17, 2017

knobunc left a comment

Choose a reason for hiding this comment

JacobTanenbaum left a comment

Choose a reason for hiding this comment

openshift-bot commented Jan 19, 2017

openshift-bot commented Jan 19, 2017

knobunc commented Jan 19, 2017

openshift-bot commented Jan 19, 2017 • edited Loading

openshift-bot commented Jan 19, 2017

DanyC97 commented Jan 22, 2017

marun commented Jan 25, 2017 • edited Loading

marun commented Jan 8, 2017 •

edited

Loading

marun commented Jan 14, 2017 •

edited

Loading

marun commented Jan 16, 2017 •

edited

Loading

marun Jan 16, 2017 •

edited

Loading

marun commented Jan 17, 2017 •

edited

Loading

openshift-bot commented Jan 19, 2017 •

edited

Loading

marun commented Jan 25, 2017 •

edited

Loading