host2regex: doesn't take in consideration *
#3296
Conversation
Signed-off-by: Mustafa Abdelrahman <[email protected]>
Signed-off-by: Mustafa Abdelrahman <[email protected]>
Signed-off-by: Mustafa Abdelrahman <[email protected]>
|
maybe it's not a good idea to have this at all |
| }, | ||
| } { | ||
| t.Run(ti.msg, func(t *testing.T) { | ||
| regex := createHostRx(ti.host) |
There was a problem hiding this comment.
another idea is to update validation webhook to say if the host regex is valid
MustafaSaber
added
do-not-merge
major
| @@ -0,0 +1,3 @@ | |||
| kube_foo__qux____example_org_____qux: | |||
| Host("^([a-z0-9]+(-[a-z0-9]+)?[.]example[.]org[.]?(:[0-9]+)?)$") && PathSubtree("/") | |||
There was a problem hiding this comment.
I think you want
^([a-z0-9][a-z0-9-]*)?[.]example....
Better also to write down in the issue what kind of match or regexp you want to create.
There was a problem hiding this comment.
I want to match a valid hostname, it can't end or start with - I think this ([a-z0-9][a-z0-9-]*) can end with -
There was a problem hiding this comment.
What about foo-bar-qux.example , that your regexp doesn't match?
maybe:
^[a-z0-9]([a-z0-9-]*[a-z0-9])?[.]example....
Not sure if we need the capture group for the full first part of the hostname.
There was a problem hiding this comment.
updated the PR to match it, the new regex [a-z0-9]+((-[a-z0-9]+)?)*
Signed-off-by: Mustafa Abdelrahman <[email protected]>
|
See #3297 (comment) |
|
A problem with the current implementation is that a request can flap between 2 or more routes request to |
| @@ -15,6 +15,9 @@ func createHostRx(hosts ...string) string { | |||
|
|
|||
| hrx := make([]string, len(hosts)) | |||
| for i, host := range hosts { | |||
| if strings.HasPrefix(host, "*.") { | |||
| host = strings.Replace(host, "*", "[a-z0-9]+((-[a-z0-9]+)?)*", 1) | |||
There was a problem hiding this comment.
We need to verify this matches behaviour descibed at https://kubernetes.io/docs/concepts/services-networking/ingress/#hostname-wildcards
Makes sense to add this link as a comment.
Subdomain regexp looks strange with two capture groups using ? followed by *.
I guess subdomain shoud start and end with alphanumeric (not dash). Lets check this https://stackoverflow.com/questions/7930751/regexp-for-subdomain and similar answers and reuse/adopt regexp from there.
There was a problem hiding this comment.
Sounds like #3296 (comment) would be the one 😀
Limiting the size makes also sense as you linked and non ascii domain support is also great to have!
There was a problem hiding this comment.
I think this one already works for all cases.
https://regex101.com/r/ziI6Sv/1
There was a problem hiding this comment.
Sounds like #3296 (comment) would be the one
yes this also works https://regex101.com/r/dNnhqk/1
|
Can this advance, e.g. with a feature toggle to support it? I would say potential conflicts can also be ruled user error and not skipper error. What would be needed if you wanted to elevate the support? Weights? Warnings? |
converting host from Ingress/RouteGroup for
Hostpredicate doesn't take into consideration that Ingress support wildcard hostnames (see https://kubernetes.io/docs/concepts/services-networking/ingress/#hostname-wildcards) and we produce invalid regexThis PR adds testcases to reproduce the error, requires fix before merging.
following ingress produce
Error:
fixes #3297