Host wildcard policy for supporting wildcard routes #11550

ramr · 2016-10-25T05:42:18Z

As per discussions in #11201 added a new api field for wildcard policy and fixed issues relating to top-level subdomains.

default wildcardPolicy to None
add wildcardPolicy to RouteIngress in status (defaulting to None)
record admitted wildcardPolicy when recording route status in haproxy
reject wildcard routes from haproxy when wildcards are disabled
make route.spec.wildcardPolicy immutable (in update validation)
log details about conflicting routes, but don't leak info to the user
change error back to "HostAlreadyClaimed"
ensure we only record a rejection once (either inside addRoute() or with the returned error, but not both places)

ramr · 2016-10-25T05:43:01Z

@smarterclayton / @liggitt this is ready code wise and unit test wise + this addressses the problem with top level subdomains. I wanted to run some tests but am unable to do that. Have been struggling since this evening - where am running into an issue where the persisted object in etcd is missing the WildcardPolicy type. Do I have do anything specific for that for an existing object - I thought not?

Just as a background, I did change the types.go and did a make update (so all the completions/conversions/etc are generated and then did a rebuild).
https://github.com/ramr/origin/blob/94d3128a55685353a8a1a4ebdb7bfc53c6c9e695/pkg/route/api/types.go#L47

and assumed that should be the only change needed.

I can create routes and it rejects routes with an invalid WildcardPolicy ala:

$ oc create -f bad-wildcard-unsecured-route.json   
The Route "header-test-wildcard-bad" is invalid: spec.wildcardPolicy: Invalid value: "badinput": invalid value for WildcardPolicy option, acceptable values are Subdomain, None, or empty

And so the code validating the route is doing its work, but creating "good" routes using this example:
https://gist.github.com/ramr/6ec490c1fb026bd46c4bd81b4e648f02
gives me this output:
https://gist.github.com/ramr/fc2022d0806bd9ddf9d176e23a34dc52

The wildcardPolicy is missing from the stored version. Commands I ran were:

$ oc create -f https://gist.githubusercontent.com/ramr/6ec490c1fb026bd46c4bd81b4e648f02/raw/934fdb88092cdd6693ebef2537bb24cceedddef9/example-wildcard-policy-route.json 
route "header-test-wildcard-path-http-redirect-to-https" created  
$ oc get route header-test-wildcard-path-http-redirect-to-https  -o yaml | grep -i ildcardPolicy   
$

Any clues/ideas on what's missing to persist it into etcd - versioning or something else?
Thanks a ton.

ramr · 2016-10-25T06:54:34Z

travis failed due to missing journalctl looks like.
[test]

liggitt · 2016-10-25T06:56:29Z

pkg/route/api/v1/types.go

@@ -82,6 +82,9 @@ type RouteSpec struct {

 	// The tls field provides the ability to configure certificates and termination for the route.
 	TLS *TLSConfig `json:"tls,omitempty" protobuf:"bytes,6,opt,name=tls"`
+
+	// Wildcard policy if any for the route.
+	WildcardPolicy WildcardPolicyType `json:"wildcardPolicy,omitempty" protobuf:"bytes,7,opt,name=wildcardPolicy"`


set this to None in defaults.go if empty

Thanks did that and it now shows up fine. Took a while to get this all working - missed a protoc version message 3.0.x

liggitt · 2016-10-25T06:57:07Z

pkg/route/api/validation/validation.go

+// validateWildcardPolicy tests that the wildcard policy is either empty or one of the supported types.
+func validateWildcardPolicy(policy routeapi.WildcardPolicyType, fldPath *field.Path) *field.Error {
+	// Check policy value if specified (empty is ok).
+	if len(policy) == 0 {


should never happen if defaulting is working

It still happens - if the tests create a Route object without specifying that field value wildcardPolicy. Maybe the code in defaults.go is not right - dunno. I kept this as the validation tests fail otherwise.

ramr · 2016-10-25T12:11:46Z

@liggitt / @smarterclayton made the changes - PTAL. Thx
/cc @knobunc @eparis @marun @rajatchopra

Tests pass and can create wildcard routes, so this is good for review now. There's some pending stuff vis-a-vis the defaults - see comments above.

ramr · 2016-10-25T12:13:20Z

Travis error is to do with journalctl not being found. /home/travis/gopath/src/github.com/openshift/origin/hack/util.sh: line 372: journalctl: command not found

knobunc · 2016-10-25T13:03:10Z

docs/man/man1/openshift-infra-f5-router.1

+    Allow wildcard host names for routes
+
+.PP
+\fB\-\-allowed\-domains\fP=[]


Do we need some kind of depth restriction for wildcards to prevent someone from trying to claim *.com and other TLDs? Not as a fixed number, but as an env with a default?

This is what the router admin would setup - allowed domains for this router instance.
E.g. acme.auto, acme.test, acme.com.
The routes that users create would need to be in that domain set ala www.dev.acme.test.

knobunc · 2016-10-25T13:04:32Z

pkg/route/api/types.go

+
+	// WildcardPolicySubdomain indicates the host needs wildcard support for the subdomain.
+	// Example: For host = "www.acme.test", this indicates that the
+	//          router should support requests for *.acme.test


Can you clarify that acme.test does not match *.acme.test

acme.test would not match *.acme.test - yeah will update the comments.

knobunc · 2016-10-25T13:06:39Z

pkg/route/api/v1/defaults.go

+	case WildcardPolicyType("Subdomain"):
+		obj.WildcardPolicy = WildcardPolicySubdomain
+	}
+}


Do we want to do something on the case where something else is encountered? Log an error and default to None? Log and ignore the route (with appropriate status)? Or is that definitely impossible by some other mechanism I haven't spotted.

I don't know too much about this part - AFAICT, it was just setting defaults if no values were set on fields in an object. And there's no errors thrown back/logging here in similar defaults code. @liggitt may be able to tell more.

not in conversion/defaulting, only in validation

knobunc · 2016-10-25T13:07:58Z

pkg/route/api/v1/swagger_doc.go

@@ -67,6 +67,7 @@ var map_RouteSpec = map[string]string{
 	"alternateBackends": "alternateBackends is an extension of the 'to' field. If more than one service needs to be pointed to, then use this field. Use the weight field in RouteTargetReference object to specify relative preference. If the weight field is zero, the backend is ignored.",
 	"port":              "If specified, the port to be used by the router. Most routers will use all endpoints exposed by the service by default - set this value to instruct routers which port to use.",
 	"tls":               "The tls field provides the ability to configure certificates and termination for the route.",
+	"wildcardPolicy":    "Wildcard policy if any for the route.",


Should we tell the user what policies they can set? And say what the default is?

Will update the comments where the docs get generated from.

knobunc · 2016-10-25T13:50:06Z

pkg/router/controller/host_admitter.go

+		case watch.Added, watch.Modified:
+			if err := p.addRoute(route); err != nil {
+				glog.Errorf("Route %s not admitted: %s", routeNameKey(route), err.Error())
+				p.recorder.RecordRouteRejection(route, "SubdomainAlreadyClaimed", err.Error())


Didn't we already do that in addRoute() ?

Recording rejections? No we don't pass the recorder/status plugins down. Its a chain of plugins - similar to unique_host.go this logs the info. Or were you talking about the log statement?

I'd probably switch this "HostAlreadyClaimed" or something, since it's not necessarily dealing with subdomains

and yes, p.recorder.RecordRouteRejection is called inside p.addRoute()... audit to make sure we only record rejection once for a given route admission, and don't leak info about the route in the other namespace in rejection cases (I think you had different messages that were logged than were shown to the user... we probably still want that)

ok - will do.

liggitt · 2016-10-25T07:00:50Z

pkg/route/api/validation/validation.go

@@ -277,3 +281,24 @@ func validateCertificatePEM(certPEM string, options *x509.VerifyOptions) (*x509.

 	return cert, nil
 }
+


var ( allowedWildcardPolicies = []string{string(routeapi.WildcardPolicyTypeNone), string(routeapi.WildcardPolicyTypeSubdomain)} allowedWildcardPoliciesSet = sets.NewString(allowedWildcardPolicies...) )

liggitt · 2016-10-25T07:01:03Z

pkg/route/api/validation/validation.go

+		return nil
+	}
+
+	// Check if policy is one of None, Subdomain.


if !allowedWildcardPoliciesSet.Has(string(policy)) { return field.NotSupported(fldPath, policy, allowedWildcardPolicies) }

liggitt · 2016-10-25T07:08:40Z

pkg/route/api/helper.go

+// This handles top-level domain names as well.
+// Example: acme.test and www.acme.test both return acme.test
+//          and www1.edge.acme.test returns edge.acme.test
+func GetSubdomainForHost(host string) string {


for purposes of determining ownership, don't special case single-segment hosts. stripping the first segment to determine conflicts between hostnames isn't correct (see #11496... the test referenced there still fails against this branch).

If we don't do that then acme.test and foo.test would belong to the same domain for ownership purpose - we do need to handle TLDs separately. And I did add similar test to the host_admitter_test.go and they pass. Will try your specific test as well but it might not be valid because of the *.foo.com host names - its a wee bit different now in terms of what the host field is set to.

I'm saying we don't want to strip the first segment for claimedHosts, only for claimedWildcards/blockedWildcards

liggitt · 2016-10-25T07:27:32Z

pkg/router/controller/host_admitter.go

+
+	// subdomainToRoute contains all routes associated with a subdomain
+	// (includes fully qualified and wildcard routes).
+	subdomainToRoute SubdomainToRouteMap


I don't think this data structure will work... it incorrectly identifies foo.example.com and bar.example.com as conflicting with each other.

I think we need two differently keyed maps, one that maps explicit hostnames to routes, and one that maps wildcard specs to a list of blocker and claimant routes for that wildcard spec:

// hostToRoutes lists the routes for a given hostname (e.g. foo.example.com) hostToRoutes map[hostname][]Route // wildcardToParticipants lists the routes that block or claim a given wildcard spec (e.g. *.example.com) wildcardToParticipants map[wildcardSpec]wildcardParticipants type hostname string type wildcardSpec string // participants holds routes that are related to the wildcardSpec (e.g. *.example.com) type wildcardParticipants struct { // blockers holds routes that block the wildcard from being claimed (e.g. foo.example.com, bar.example.com) blockers []Route // claimants holds routes that are claiming the wildcard (e.g. www.example.com+wildcardPolicy=Subdomain) claimants []Route }

when we're admitting a route, we need to determine two things:

does another route prevent us from getting the host we request

what routes (if any) need to be removed?

A function like this would let us gather that data (warning: hastily written pseudo-code ahead):

func wildcardSpec(hostname) wildcardSpec { return hostname with first segment replaced with * } // displacedRoutes returns the existing routes (if any) this new route displaces, // or an error if an existing route has priority over this route func displacedRoutes(route, hostToRoutes, wildcardToParticipants) ([]route, error) { for hostToRoutes[route.host] { if in a different namespace, and is older, return error otherwise add to displaced list } for wildcardToParticipants[wildcardSpec(route.host)].claimants { if in a different namespace, and is older, return error otherwise add to displaced list } if route is a wildcard route { for wildcardToParticipants[wildcardSpec(route.host)].blockers { if in a different namespace, and is older, return error otherwise add to displaced list } } return displaced list, nil }

to add a route to the structures:

insert at the right place in hostToRoutes[route.host]

if route is not a wildcard route, insert at the right place in wildcardToParticipants[wildcardSpec(route.host)].blockers

if route is a wildcard route, insert at the right place in wildcardToParticipants[wildcardSpec(route.host)].claimants

all the wildcardParticipants uses could be gated on whether wildcards were enabled in the router... if they aren't, there's no need to use that structure at all

So this plugin works in conjunction with the unique_host.go plugin which does checks for host ownership. It was only for handling wildcard routes - aah, I can see now that changed because the host no longer has that wildcard spec on it/is no longer returning that boolean indicating it was a wildcard. I'll rework this a bit - we probably just need a map for indicating that - the structure above sounds good. Thx

I took a stab at it in https://github.com/liggitt/origin/commits/ramr-host-wildcard-policy if you want to take a look (just a proof of concept). I actually set up three lists - claimedHosts, claimedWildcards, blockedWildcards, and made the key of the wildcard maps the parent domain to avoid allocating new strings

I had something as well - but this looks nicer and the tests look more thorough as well. I'll pull it in and modify/use. Thx a ton.

…o be reported as the older route based on name/namespace checks. Ensure that we have a stable ordering based on the age of a route.

@marun

o Allow wildcard (currently only *.) routes to be created and add tests. o Add a host admission controller and allow/deny list of domains and control the admission/blockage of wildcard routes. o Fix test cases and expection. o Add helper to generate valid wildcard regular expressions. o Add wildcard domain map + regex based rules and use the rules for wildcard routes. o Bug fixes and add tests. o Add generated completions and docs. o Changes as per @marun, @rajatchopra, @smarterclayton review comments o Rework as per api changes to use wildcard policy with routes o Add defaults and update generated files.

and tests and add missing generated files.

ramr · 2016-10-26T01:23:49Z

[test]

ramr · 2016-10-26T01:36:33Z

@liggitt i just used what you had with some minor changes. I did have one concern on this:

if a route specifies a TLD test and a wildcard policy of the subdomain, it works fine from the host admission standpoint but it would block other TLDs org, com etc. On the haproxy side, its going to match every host - so I did put in a check on the template generation side to not allow that if the domain is empty.

Granted its contrived edge case for testing but something to be aware off + its a possible attack vector - the haproxy check prevents that for now. imho, I liked restricting the way we handle the domain to a set that doesn't do anything with TLDs - that would have been cleaner from a functional standpoint. In either case, need to document this behavior. If you are not averse, I'd like to revert that function to not doing anything for TLDs and restricting it to subdomains.

Either ways, can you PTAL. Thx

liggitt · 2016-10-26T01:51:39Z

On the haproxy side, its going to match every host

haproxy is only matching wildcards one level deep, right?

By TLD, I assume you mean public suffix (like com, net, org, etc). Problem is, these can be multi-segment (like co.uk)… I'm not sure what special-casing a single segment wildcard gets us.

ramr · 2016-10-26T02:02:45Z

Yes, haproxy just one level deep for wildcards. That's true re: co.<country-code> etc. Anyway ... let me know if you want any changes here. Thx

Updated

ramr · 2016-10-26T17:36:00Z

Hmm, tests failed because the generated protobuf file is different - is it got something to do with the protoc version (3.0.0) am using? File: pkg/route/api/v1/generated.pb.go

liggitt

I didn't look closely at the haproxy template changes... did much change there?

liggitt · 2016-10-26T21:59:52Z

pkg/cmd/infra/router/router.go

+			return err, false
+		}
+
+		if len(route.Spec.WildcardPolicy) > 0 && route.Spec.WildcardPolicy != routeapi.WildcardPolicyNone && !o.AllowWildcardRoutes {


why is this here and not in AdmissionCheck()?

conversion should default an empty wildcardpolicy to None even if we read it from an old api server, so you don't need to handle ""

change to a switch that allows None, conditionally allows Subdomain if the router enables it, and rejects other wildcardPolicy as unknown

Well it could be but it felt better to separate them out as the admission check is an outright rejection - in/not in black/white lists. But a wildcard policy check in here (even if the router policy doesn't allow it), we still want to handle the host ... this would make it behave similar to previous/olders routers (where wildcards are not allowed - well would ignore the api field if the api server was updated but not the router and the router would process the route host). I'll make the other changes.

I thought @smarterclayton expected rejection if a wildcard-aware router with wildcards disabled was given a wildcard-requesting route

confirmed with clayton that a new wildcard-aware router that has explicitly turned off wildcard support should reject wildcard-requesting routes, not admit them host-only

So a user adds a wildcard route to something non-wildcard aware. It 'partially' works. The admin updates the router image. The 'partially' working route stops working. I believe that's called a regression....

the old router didn't know about wildcards. the new one does, and is disabling them.

no different than a pod with initContainers running without init containers in 1.3 (because the server doesn't know about them), and being rejected in 1.4 if the initContainers are disallowed. the 1.4 server doesn't just skip the initContainers and run the main containers.

Agree with the analogy. Just that is a regression, No? Pod ran yesterday. Update cluster. Pod no longer runs.

Production up yesterday.
Production down today.

I don't honestly think it's big deal here (or there). I'm discussing the ideology of regressions. We seem, ummm, schizo at best...

liggitt · 2016-10-26T22:01:52Z

pkg/route/api/helper.go

@@ -20,11 +22,26 @@ func RouteLessThan(route1, route2 *Route) bool {
 	if route1.CreationTimestamp.Before(route2.CreationTimestamp) {
 		return true
 	}


simpler:

if route1.CreationTimestamp.Before(route2.CreationTimestamp) { return true } if route2.CreationTimestamp.Before(route1.CreationTimestamp) { return false } return route1.UID < route2.UID

liggitt · 2016-10-26T22:03:06Z

pkg/route/api/types.go

+
+	// Wildcard policy if any for the route.
+	// Currently only 'Subdomain' or 'None' is allowed.
+	WildcardPolicy WildcardPolicyType


I would also expect wildcardpolicy on RouteIngress so clients can know what wildcard policy the router is using. @smarterclayton, that match what you would expect?

liggitt · 2016-10-26T22:03:49Z

pkg/route/api/v1/defaults.go

@@ -2,6 +2,18 @@ package v1

 import "k8s.io/kubernetes/pkg/runtime"

+func SetDefaults_RouteSpec(obj *RouteSpec) {
+	if len(obj.WildcardPolicy) == 0 {
+		obj.WildcardPolicy = "None"


use the constant defined in types.go

liggitt · 2016-10-26T22:04:09Z

pkg/route/api/v1/defaults.go

+	if len(obj.WildcardPolicy) == 0 {
+		obj.WildcardPolicy = "None"
+	}
+	switch obj.WildcardPolicy {


this is a no-op, remove the switch

Made this change. btw, I followed what was already in here for TLSConfig.

liggitt · 2016-10-26T22:07:31Z

pkg/router/controller/host_admitter.go

+		case watch.Added, watch.Modified:
+			if err := p.addRoute(route); err != nil {
+				glog.Errorf("Route %s not admitted: %s", routeNameKey(route), err.Error())
+				p.recorder.RecordRouteRejection(route, "SubdomainAlreadyClaimed", err.Error())


I'd probably switch this "HostAlreadyClaimed" or something, since it's not necessarily dealing with subdomains

liggitt · 2016-10-26T22:09:17Z

pkg/router/controller/host_admitter.go

+		case watch.Added, watch.Modified:
+			if err := p.addRoute(route); err != nil {
+				glog.Errorf("Route %s not admitted: %s", routeNameKey(route), err.Error())
+				p.recorder.RecordRouteRejection(route, "SubdomainAlreadyClaimed", err.Error())


and yes, p.recorder.RecordRouteRejection is called inside p.addRoute()... audit to make sure we only record rejection once for a given route admission, and don't leak info about the route in the other namespace in rejection cases (I think you had different messages that were logged than were shown to the user... we probably still want that)

liggitt · 2016-10-26T22:10:01Z

pkg/router/controller/host_admitter.go

+	// Find displaced routes (or error if an existing route displaces us)
+	displacedRoutes, err := p.displacedRoutes(route)
+	if err != nil {
+		err := fmt.Errorf("a route in another namespace holds host %s (%v)", route.Spec.Host, err)


log the returned error (which has details about the other route), but don't report it to the user

liggitt · 2016-10-26T22:11:02Z

pkg/router/controller/host_admitter.go

+		p.claimedWildcards.RemoveRoute(wildcardKey, displacedRoute)
+
+		msg := fmt.Sprintf("a route in another namespace holds host %s", displacedRoute.Spec.Host)
+		p.recorder.RecordRouteRejection(displacedRoute, "SubdomainAlreadyClaimed", msg)


HostAlreadyClaimed?

liggitt · 2016-10-26T22:12:26Z

pkg/router/controller/host_admitter.go

+		// ensure the route doesn't exist as a claimed host or blocked wildcard
+		p.claimedHosts.RemoveRoute(route.Spec.Host, route)
+		p.blockedWildcards.RemoveRoute(wildcardKey, route)
+	}


add a default case and reject as unrecognized wildcard policy?

ramr · 2016-10-26T22:51:42Z

@liggitt no changes in the haproxy template in this version - there were older changes for adding to the wildcard map + matching on the regexp + its generation. Will make the other fixes.

On another note, the 2 protoc versions I used - 3.0.0 and 3.0.2 generate the same pkg/route/api/v1/generated.pb.go file that's committed with the changes in this PR but with jenkins there's a different one that gets generated.
Not sure what's out of whack in my environment if any or its something in jenkins? Any ideas? Thx

liggitt · 2016-10-26T23:39:41Z

Let's also start out with wildcardPolicy being immutable on update (enforced in apiserver validation), since swapping from none to subdomain can let you displace newer routes

liggitt · 2016-10-27T07:12:13Z

added tasks for remaining items to the description

would like someone more familiar with haproxy config to look at the template change, checking:

behavior when wildcards are disabled
precedence (specific hosts win over wildcards)
level-matching (wildcards only match a single level)
ensure the cert-list format we just added works with wildcard hosts

smarterclayton · 2016-10-27T15:40:41Z

You need to be on Go 1.6.

On Wed, Oct 26, 2016 at 1:36 PM, Ram Ranganathan [email protected]
wrote:

Hmm, tests failed because the generated protobuf file is different - is it
got something to do with the protoc version (3.0.0) am using? File:
pkg/route/api/v1/generated.pb.go

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11550 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p4fQy3rqTRVF-MrYY0nQzMqDm7Imks5q34-CgaJpZM4KflVE
.

o Fix generated route protobuf file o Default Route.Spec.WildcardPolicy to None o Add WildcardPolicy to RouteIngress in RouteStatus (defaulting to None) o Record admitted wildcard policy when recording route status o Reject wildcard routes when wildcards are disabled o Change admission functions to just return errors (no bool) and update checks for policy to use a switch in lieu of a if statement o Simpilify RouteLessThan api helper function o Make Route.Spec.WildcardPolicy immutable (in update validation) o Log details about conflicting routes, but don't leak info to the user o Change error to "HostAlreadyClaimed" o Add checks for defaults on Spec and Ingress WildcardPolicy in conversion tests.

ramr · 2016-10-28T04:49:22Z

@smarterclayton thanks - that was probably it - was using golang 1.7
@liggitt addressed all the tasks/changes - took a while to get the build going (vm needs 6 odd GB to build w/ it!!). PTAL Thx

ramr · 2016-10-28T04:49:26Z

[test]

…e rejection.

liggitt · 2016-10-28T05:30:43Z

pkg/router/controller/host_admitter.go

+// recordRejection records why the route was rejected.
+func (p *HostAdmitter) recordRejection(route *routeapi.Route, reason, message string) {
+	oldPolicy := route.Spec.WildcardPolicy
+	route.Spec.WildcardPolicy = routeapi.WildcardPolicyNone


why mess with the wildcardPolicy here? If I request host foo.com with wildcardPolicy=Subdomain, and the router rejects it, it would make sense to me to see the rejection also have host foo.com and wildcardPolicy=Subdomain

Its to set it on the status/RouteIngress field - its a lot of change to the other plugins if we want to pass it down as a parameter.

and its sort of similar to how the host allocation sets the route.spec.host for status updates.

We should reject it with the same wildcardPolicy that was requested

liggitt · 2016-10-28T05:31:59Z

pkg/router/controller/host_admitter.go

 		p.plugin.HandleRoute(watch.Deleted, displacedRoute)
 	}

+	if len(route.Spec.WildcardPolicy) == 0 {


this shouldn't be necessary, were you seeing empty wildcard policy fields?

Some of the tests fail if this is removed - we need to explicitly set it otherwise the default is empty string in the tests. AFAICT, the default code only kicks when it does a conversion to/from the store. The unit tests don't really do that - they just instantiate a new object and use it.

ramr · 2016-10-30T23:53:49Z

@liggitt made the change - can't figure out why the test in pkg/api/serialization_test is failing -
something missing in conversion (needs to be done)? The check is failing for "" vs "None" - so something more apart from the defaulting is needed? Thx
FAIL: TestTypes

Ref: https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_check/7852/console

Updated added link to log/console output.

liggitt · 2016-10-30T23:55:32Z

Might need a custom fuzzer that does the default fuzzing, then sets to "None" if the fuzzer left the field empty

liggitt · 2016-10-31T00:20:23Z

API and admission bits LGTM

would like sign-off from someone more familiar with haproxy config, checking:

behavior when wildcards are disabled
precedence (specific hosts win over wildcards)
level-matching (wildcards only match a single level)
ensuring the cert-list format we just added works with wildcard hosts

/cc @knobunc @eparis @marun @rajatchopra

knobunc · 2016-10-31T13:07:49Z

Flaked on #11057 [test]

Failure log https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_check/7900/consoleText

liggitt · 2016-10-31T13:21:43Z

I think those are red herrings. I see:

FAIL    github.com/openshift/origin/pkg/api 17.719s
In suite "github.com/openshift/origin/pkg/api", test case "TestTypes" failed:
=== RUN   TestTypes
--- FAIL: TestTypes (17.55s)
    serialization_test.go:530: 1: Route: diff: 
        object.Spec.WildcardPolicy:
          a: ""
          b: "None"
        Codec: &{0xc82016c400 0xc8201744c0 {0xc82016c3c0} 0xc82016c3c0 0xc82016c3c0 0xc82016c3c0 [{ v1}] {}}
        Data: {"kind":"Route","apiVersion":"v1","metadata":{"name":"粿ùÙ芽!齰|朑Óȿ.膖È","generateName":"4ą湮ç豭Č","selfLink":"ĔƴJ麯\u0026啭鐐攥ǫ郰Kʃ姬网T","uid":"顚栄馁|=獕?Oö'","resourceVersion":"9988799481717431531","creationTimestamp":null},"spec":{"host":"@餭坤\u003eP","path":"滭Ȗ呸嚂Ȣ媿T婎辮","to":{"kind":"Service","name":"ɞ镸榩ǯgÝǎHw2覯魝ɀ!Q","weight":100},"alternateBackends":[{"kind":"Service","name":"Į ªȳ6c","weight":100}],"port":{"targetPort":854974194},"tls":{"termination":"穙\"Țð","certificate":"iħt痨)","key":"傱cl斍ʡ;飖焅}Ď¾əǜ横)甘","caCertificate":"烿'c躡Ơ攎+ǫ愈碓ȏË丆pN","destinationCACertificate":"ƶ{幙fVVAo碈(Ƿ腨o,癫ǮƌȺ","insecureEdgeTerminationPolicy":"ʏ朲憖Ǥ鎫顒N齻瀳[ɫɆ"}},"status":{"ingress":null}}
    serialization_test.go:530: 1: Route: diff: 
        object.Spec.WildcardPolicy:
          a: ""
          b: "None"
        Codec: &{0xc82052f4a0 0xc82052f4a0 {0xc82016c3c0} 0xc82016c3c0 0xc82016c3c0 0xc82016c3c0 [{apps v1alpha1} {policy v1alpha1} {federation v1beta1} {certificates.k8s.io v1alpha1} {storage.k8s.io v1beta1} {imagepolicy.k8s.io v1alpha1} { v1} {extensions v1beta1} {autoscaling v1} {batch v1} {rbac.authorization.k8s.io v1alpha1}] {}}
        Data:

liggitt · 2016-10-31T13:24:25Z

I think you need something like this in serialization_test.go:

        func(j *route.RouteSpec, c fuzz.Continue) {
            c.FuzzNoCustom(j)
            if len(j.WildcardPolicy) == 0 {
                j.WildcardPolicy = route.WildcardPolicyNone
            }
        },
        func(j *route.RouteIngress, c fuzz.Continue) {
            c.FuzzNoCustom(j)
            if len(j.WildcardPolicy) == 0 {
                j.WildcardPolicy = route.WildcardPolicyNone
            }
        },

knobunc · 2016-10-31T13:31:26Z

Regarding @liggitt 's list:

behavior when wildcards are disabled
LGTM, the haproxy conf is using the "base" hostname in the wildcard route when they are disabled
precedence (specific hosts win over wildcards)
LGTM, in haproxy.conf we check the specific host before we strip the hostname and try it as a wildcard match. And in go the precedence in the claims looks right.
level-matching (wildcards only match a single level)
LGTM, the generated regex is: [^.]*. prepended to the quoted domain name. That can not match more than one level (and the
ensuring the cert-list format we just added works with wildcard hosts
Not sure about this. I'm not familiar with that code yet. @rajatchopra PTAL?

wildcard policy.

ramr · 2016-11-01T18:09:19Z

[test]

knobunc · 2016-11-01T19:01:41Z

@liggitt assuming this test passes, are you ok with a merge?

liggitt · 2016-11-01T19:04:15Z

yes

knobunc · 2016-11-01T19:55:14Z

[test] flaked on #11661

openshift-bot · 2016-11-01T20:06:03Z

Evaluated for origin test up to 482a044

openshift-bot · 2016-11-01T21:30:44Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10955/) (Base Commit: 16306a6)

smarterclayton · 2016-11-02T04:14:04Z

[merge]

On Tue, Nov 1, 2016 at 5:30 PM, OpenShift Bot [email protected]
wrote:

continuous-integration/openshift-jenkins/test SUCCESS (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10955/) (Base
Commit: 16306a6
16306a6
)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11550 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_pwHiX5NA0PgrhLT0DZ3yep_Gu_2oks5q56-HgaJpZM4KflVE
.

openshift-bot · 2016-11-02T04:22:03Z

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

openshift-bot · 2016-11-02T04:22:03Z

Evaluated for origin merge up to 482a044

ramr mentioned this pull request Oct 25, 2016

Add router support for wildcard domains (*.foo.com) #11201

Merged

liggitt reviewed Oct 25, 2016

View reviewed changes

ramr force-pushed the host-wildcard-policy branch from 94d3128 to abbd711 Compare October 25, 2016 12:00

ramr changed the title ~~[WIP] Host wildcard policy~~ Host wildcard policy for supporting wildcard routes Oct 25, 2016

knobunc reviewed Oct 25, 2016

View reviewed changes

liggitt reviewed Oct 25, 2016

View reviewed changes

ramr force-pushed the host-wildcard-policy branch from abbd711 to ad4e77e Compare October 26, 2016 01:22

ramr and others added 4 commits October 25, 2016 18:23

Fix an issue with route ordering: it was possible for a newer route t…

3117d54

…o be reported as the older route based on name/namespace checks. Ensure that we have a stable ordering based on the age of a route.

Data structure rework

1b98d1c

Change to use helper function and fix up helper function expectations

629921f

and tests and add missing generated files.

ramr force-pushed the host-wildcard-policy branch from ad4e77e to 629921f Compare October 26, 2016 01:23

ramr mentioned this pull request Oct 26, 2016

Route hostnames are not admitted correctly #11496

Closed

liggitt self-assigned this Oct 26, 2016

liggitt added this to the 1.4.0 milestone Oct 26, 2016

liggitt reviewed Oct 26, 2016

View reviewed changes

Fix as per @liggitt review comments - write wildcard spec as is in th…

438b1df

…e rejection.

liggitt reviewed Oct 28, 2016

View reviewed changes

Fix the failing serialization test - need a custom fuzzer to default the

482a044

wildcard policy.

openshift-bot merged commit eebbc14 into openshift:master Nov 2, 2016

ramr deleted the host-wildcard-policy branch February 3, 2017 00:08

		@@ -277,3 +281,24 @@ func validateCertificatePEM(certPEM string, options x509.VerifyOptions) (x509.

		return cert, nil
		}

Host wildcard policy for supporting wildcard routes #11550

Host wildcard policy for supporting wildcard routes #11550

Conversation

ramr commented Oct 25, 2016 • edited Loading

ramr commented Oct 25, 2016 • edited Loading

ramr commented Oct 25, 2016

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

ramr commented Oct 25, 2016

ramr commented Oct 25, 2016 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Oct 25, 2016 • edited by stevekuznetsov 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

liggitt Oct 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Oct 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramr commented Oct 26, 2016

ramr commented Oct 26, 2016

liggitt commented Oct 26, 2016

ramr commented Oct 26, 2016 • edited Loading

ramr commented Oct 26, 2016

liggitt 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Oct 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Oct 26, 2016 • edited by stevekuznetsov 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

ramr commented Oct 26, 2016

liggitt commented Oct 26, 2016

liggitt commented Oct 27, 2016

smarterclayton commented Oct 27, 2016

ramr commented Oct 28, 2016

ramr commented Oct 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramr Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramr commented Oct 25, 2016 •

edited

Loading

ramr commented Oct 25, 2016 •

edited

Loading

ramr commented Oct 25, 2016 •

edited

Loading

liggitt Oct 25, 2016 •

edited by stevekuznetsov

Loading

liggitt Oct 25, 2016 •

edited

Loading

liggitt Oct 25, 2016 •

edited

Loading

ramr commented Oct 26, 2016 •

edited

Loading

liggitt Oct 27, 2016 •

edited

Loading

liggitt Oct 26, 2016 •

edited by stevekuznetsov

Loading

ramr Oct 28, 2016 •

edited

Loading

ramr commented Oct 30, 2016 •

edited

Loading

knobunc commented Oct 31, 2016 •

edited

Loading