kubectl create ingress - Opinions and best approaches

[Ricardo Katz]

Hello SIG,

I've been working in the PR of issue #93267
(https://github.com/kubernetes/kubernetes/issues/93267) in
https://github.com/kubernetes/kubernetes/pull/94327 and would like
some opinions. First of all, sorry for the long email and please feel
free to put your thoughts, ideas, criticisms and make suggestions. I
know the code isn't actually pretty (yet) and the syntax isn't neither
:) Also once I got your thoughts about this, I can propose the PR to
sig-cli because maybe this command structure might be against some
guideline.

The main problem of this command is that Ingress is a much more
complex object than the other ones. So you can have one to any hosts,
you can have no hosts but backends defined (the catch all ingress),
you can have different Path Prefixes, each for a path that can be
inside a host.

So Tim has "proposed" (in the way of 'this would work') something like:

kubectl create ingress myingress \
--host=myhost.com --pathtype=prefix --path=/foo
--service-name=foo-svc --service-port=80 \
--host=yourhost.com --pathtype=prefix --path=/bar
--service-name=bar-svc --service-port=80

1) The first problem starts with a case where a host has multiple
paths. The idea above is something 'positional', so my idea right now
is that the 'main' argument here is the 'path', but for each path you
need to re-declare a host. As an example, a host "bla.com" with paths
"/foo" and "/bar" would have duplicate declarations:
# host[0] ptype[0] path[0]
svcname[0] svcport[0]
--host=bla.com --pathtype=prefix --path=/foo --service-name=foo-svc
--service-port=80 \
# host[1] ptype[1] path[1]
svcname[1] svcport[1]
--host=bla.com --pathtype=prefix --path=/bar --service-name=bar-svc
--service-port=80

Assuming the usage of 'cobra' library, and that I couldn't find any
'grouping' argument, how does this sound to you? Anyone got a better
idea? The validation case in the kubectl command will be "the number
of host arguments must be equal to the number of pathtype arguments
that must be equal to path argument" and so goes on.

The same above is valid as:

kubectl create ingress myingress --host=myhost.com,yourhost.com
--pathtype=prefix,prefix --path=/foo,/bar etc etc (Cobra
StringSliceVar being used here). It's not pretty, but it "works"

2) One can have a path without a host declaration. Because every path
needs its host, I'm looking for the "_" (underscore) character as a
wildcard/catchall. Like --host="_" represents no host. The creation of
an ingress with char "_" is forbidden, so does this sounds good,
parsing hosts with a single "_" and turning this into a 'no host rule
spec'?

3) Opposite to other kubectl create commands, and because of historic
Ingress object needs, I'm adding a -a/--annotation flag so users can
provide their annotations. This is not functional yet in the PR, but
is this something that causes a good or a bad feeling?

4) TLS rules suffer the same problem as the ingress rules. One might
have a rule with a single secretName and no hosts, no secretName and
any hosts, and a secretName with multiple hosts.

So here I'm looking for something like
--tls=secretName,host1.com,host2.com --tls=_,host3.com,host4.com.
Thoughts?

5) I'm considering in this first go backends of type
IngressServiceBackend (for both default-backend and backends inside
ingress rules) and disregarding the v1.TypedLocalObjectReference as a
possible ingress. IMO the command is already "ugly" enough, don't know
how much use cases we would have for this once the command is
available.


Suggestion/opinions are always welcome, and have a nice weekend you all.

[Sandor Szuecs]

Hi!

From my insides to our users the 99% case are host rules to the same service with 1-N hostnames. The multiple service backends are possible but I wouldn’t add the complexity of N hostnames to M backends where M > 1.

For the catchall rule with _ I am fine (dns disallows _ as part of domain names in A record as far as I remember) and this might be handy for rpi clusters at home or some e2e tests. Path and pathtype seems to be simple enough to just add it, too.

So I would recommend to have it simple opinionated and not implement all possible combinations.

Best, sandor 

— 

--

You received this message because you are subscribed to the Google Groups "kubernetes-sig-network" group.

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-sig-ne...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-network/CAG3fPQ73aF9jGw%3DqftbwvNYQzM1pSme7eUoNWhgjE%2BjvUj4L5w%40mail.gmail.com.

--

Sandor Szücs | 418 I'm a teapot

[jay vyas]

So, yeah, seems like the minimal implementation of an ingress (1 host, multipath) is good enough ?  imo this command would be useful.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-network/CAHNrm7LLABvmArPG7i4tAiL0PWsAMuFVTwjQCq3FEkOrHo854g%40mail.gmail.com.

--

jay vyas

[Tim Hockin]

I also encourage simpler-is-better. The CLI will *never* be as rich as the API.

I don't think we have much precedent for this, but some other ideas:

# Use {} to control groups, any "fiellds" not specified in a group
might be defaults
k create ingress \
{ --host=foo.com --pathtype=prefix --path=/foo
--service-name=foo-svc --service-port=80 } \
{ --host=bar.com --pathtype=prefix --path=/bar
--service-name=bar-svc --service-port=80 }

# Use stringly-typed pseudo-structure
k create ingress \
--rule=host=foo.com/foo,pathtype=prefix,service=foo-svc:80 \
--rule=bar.com/bar,pathtype=prefix,service=bar-svc:80

The objective should be to make simple, normal things simple. I think
"one service on one hostname at /" is going to be very common. At
some point, the CLI breaks down and you just have to switch to API
(and this is true with every 'create' command. The CLI "structure"
does not have to exactly match the API.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-network/CACVCA%3DeCy5d2Uxp4R0BUoQeD7-%2BsX6zz%2Bn7NS%3Dtvkzt0ZZ9qMw%40mail.gmail.com.

[Bowei Du]

+1 to simpler is better. Once people have to break out and try to understand the manpage, the CLI has decreasing value as opposed to simply writing a YAML.

Bowei

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-network/CAO_RewaanoDrBY7ftcBD67-9nPp1CD7zGYLp-9M4MFo1t8rnew%40mail.gmail.com.

[Ricardo Katz]

Hello all, and thank you for the answers!

So yeah, we all agree that simple is better :D IMO kubectl create
should be used in some 'quick start' and 'how to quick do this' but
also covering all cases is really hard.

From your insights, one thing that seems a good approach IMO (adds
value without too much complexity) is to have a kubectl create ingress
with a single host (--host) and multiple paths (--path, --path). This
could cover cases as 'deploy this multi service guest book demo,
pointing a path to a service and another path to another service'.

An example of what it could look like:

kubectl create ingress --host=bla.com --ingress-class=nginx \
--path=location=/,type=prefix,service=foo-svc:80 \
--path=location=/admin,type=exact,service=admin-svc:8080 \
--tls-secret=secretName \ #This will add automatically the
tls.* spec into the ingress
--default-backend=defaultsvc:12345

How does it look? I think this is 'simple enough' to cover the
majority of cases (1 host / 1 path, 1 host / n paths, tls usage in the
host, default backend, ingress class) without adding too much
complexity.

One caveat here is that this syntax 'drops' the multi host support in
favor of keeping things simple. A possibility that came to my mind is
a '--append' flag to the kubectl create ingress that could merge the
rules (instead of giving an error message that the ingress already
exists) but I honestly don't know the implications of this (would need
to test) and also make some cases as "what if I try to append with a
different ingress class, or with default backend" or even "how to
merge also the tls in case of equal secrets and different hosts", etc
etc.

@Tim about grouping, it was something that I've been looking for in
cobra (exactly as grouping arguments) but it seems there's actually no
support. IMO leaving the arguments logic to something external to the
flags lib might lead to undesired behaviours and need some more
implementation effort (like getting all args, checking if this is "{"
or "}", etc). But I might be missing something here.

@Tim @Bowei @Jay and @Sandor thank you again for your time and opinions.

[Tim Hockin]

On Tue, Sep 22, 2020 at 4:48 AM Ricardo Katz <ricard...@gmail.com> wrote:
>
> Hello all, and thank you for the answers!
>
> So yeah, we all agree that simple is better :D IMO kubectl create
> should be used in some 'quick start' and 'how to quick do this' but
> also covering all cases is really hard.
>
> From your insights, one thing that seems a good approach IMO (adds
> value without too much complexity) is to have a kubectl create ingress
> with a single host (--host) and multiple paths (--path, --path). This
> could cover cases as 'deploy this multi service guest book demo,
> pointing a path to a service and another path to another service'.
>
> An example of what it could look like:
>
> kubectl create ingress --host=bla.com --ingress-class=nginx \
> --path=location=/,type=prefix,service=foo-svc:80 \
> --path=location=/admin,type=exact,service=admin-svc:8080 \
> --tls-secret=secretName \ #This will add automatically the
> tls.* spec into the ingress
> --default-backend=defaultsvc:12345

That seems pretty approachable, especially to start. Does cobra
handle repeated args well? Since we're parsing strings, maybe eve
simpler:

kubectl create ingress --host=bla.com --class=nginx \
--path=/*=foo-svc:80 \
--path=/admin=admin-svc:8080 \
--default-backend=defaultsvc:12345

Does that go too far? It assumes we can parse /* as "type=prefix".
If we need to parse other types, it is less easy. It also assumes
"path=service:port" and no other fields. IF you want to carry that
forward:

kubectl create ingress --class=nginx \
--rule=bla.com/*=foo-svc:80 \
--rule=bla.com/admin=admin-svc:8080 \
--default-backend=defaultsvc:12345

And now you CAN do multi-host, but maybe that makes TLS harder?

> How does it look? I think this is 'simple enough' to cover the
> majority of cases (1 host / 1 path, 1 host / n paths, tls usage in the
> host, default backend, ingress class) without adding too much
> complexity.
>
> One caveat here is that this syntax 'drops' the multi host support in
> favor of keeping things simple. A possibility that came to my mind is
> a '--append' flag to the kubectl create ingress that could merge the
> rules (instead of giving an error message that the ingress already
> exists) but I honestly don't know the implications of this (would need
> to test) and also make some cases as "what if I try to append with a
> different ingress class, or with default backend" or even "how to
> merge also the tls in case of equal secrets and different hosts", etc
> etc.

I'd skip that until/unless we KNOW we need it. I find 'create' is a
handy way to start a resource, but after that I take over manually

[Ricardo Katz]

> Does cobra handle repeated args well?

Yeap, you can create a StringSliceVar (and it separates all commas) or
a StringArrayVar and it creates an array with each of the repeated
arguments :)

> kubectl create ingress --class=nginx \
> --rule=bla.com/*=foo-svc:80 \
> --rule=bla.com/admin=admin-svc:8080 \
> --default-backend=defaultsvc:12345

I do not oppose myself about this, just thinking about the 'user
experience' trying to understand what exactly the command does :) I
think we have a good balance here between the 'compressed format'
you've proposed and the 'descriptive format' that I've proposed. Just
wondering here (thinking loud) about the cases spliting the host from
the path with a '/' separator and the best approach to this.

Can we assume that the default path-type will be 'prefix' and anyone
wanting the other implementations should create the yaml object? This
way the path does not need a "*", but understands the path is prefixed
by whatever is there. We can solve the TLS with a slight syntax change
(for the sake of reading it) and adding it to each rule, like:

kubectl create ingress --class=nginx \
--rule=bla.com/admin=admin-svc:8080,tls \
--rule=bla.com/=foo-svc:80,tls=secret1 \
--rule=bla.com/img=img-svc:9999 \
--default-backend=defaultsvc:12345

If the value of the key 'tls' in each entry is empty, no problem. The
entry might still be created without any secret (using the default
from ingress). If it's duplicated one might assume that the most
'complete' one will be used. So in the case above, for bla.com the
secret "secret1" will be used even if the other rules does not
specifies a secret, or a tls entry at all.

I'm starting to implement in both ways and will give it a try here
about both behaviors.

Thanks!

[Tim Hockin]

On Thu, Sep 24, 2020 at 7:46 AM Ricardo Katz <ricard...@gmail.com> wrote:
>
> > Does cobra handle repeated args well?
>
> Yeap, you can create a StringSliceVar (and it separates all commas) or
> a StringArrayVar and it creates an array with each of the repeated
> arguments :)
>
> > kubectl create ingress --class=nginx \
> > --rule=bla.com/*=foo-svc:80 \
> > --rule=bla.com/admin=admin-svc:8080 \
> > --default-backend=defaultsvc:12345
>
> I do not oppose myself about this, just thinking about the 'user
> experience' trying to understand what exactly the command does :) I
> think we have a good balance here between the 'compressed format'
> you've proposed and the 'descriptive format' that I've proposed. Just
> wondering here (thinking loud) about the cases spliting the host from
> the path with a '/' separator and the best approach to this.

The definition of a URL uses the first / to indicate the "path" or
"category", so parsing is unambiguous unless a literal * is allowed,
which I think it is not (without encoding).

It is a little unclear whether "foo.com/foo*" and "foo.com/foo/*" mean
the same thing - I think they do.

If we think we need to support more path types than exact and prefix,
more structure is probably better (with good defaults) - we should not
invent wild new syntax.

If we need to support more than "host/path=svc:port", more structure
is probably better.

But the common path seems to be just this, so keeping it as simple as
possible is ideal.

> Can we assume that the default path-type will be 'prefix' and anyone
> wanting the other implementations should create the yaml object? This
> way the path does not need a "*", but understands the path is prefixed

I think we need to support prefix and exact, which all impls support (I think).

> by whatever is there. We can solve the TLS with a slight syntax change
> (for the sake of reading it) and adding it to each rule, like:
>
> kubectl create ingress --class=nginx \
> --rule=bla.com/admin=admin-svc:8080,tls \
> --rule=bla.com/=foo-svc:80,tls=secret1 \
> --rule=bla.com/img=img-svc:9999 \
> --default-backend=defaultsvc:12345
>
> If the value of the key 'tls' in each entry is empty, no problem. The
> entry might still be created without any secret (using the default
> from ingress). If it's duplicated one might assume that the most
> 'complete' one will be used. So in the case above, for bla.com the
> secret "secret1" will be used even if the other rules does not
> specifies a secret, or a tls entry at all.

Something like that, yeah. A tight definition of what is supported
and what all possible syntaxes mean goes a long way.

[Ricardo Katz]

Ok, so re-reading the docs:

https://kubernetes.io/docs/concepts/services-networking/ingress/#examples

I can assume when we have no "*" the path type might be Exact, and
when have "*" the PathType might be of type Prefix.

Per doc also, when using Exact, foo.com/bar and foo.com/bar/ are
different matches (and the opposite too), but /foo* (pathtype=prefix
and path=/foo) match both /foo and /foo/ (and /foobar). Probably going
further this might get things complicated (also, there's support for
regex in nginx ingress implementation, as example) but then I think
also that it would be better for one willing to use more "advanced"
options to go and use the API or yaml object directly.

Thanks for the feedback :)

[Ricardo Katz]

Hello everybody, but specially Ingress implementators :)

"kubectl create ingress" has shipped into the upstream code
(https://github.com/kubernetes/kubernetes/pull/94327) using the
proposed syntax, and everything is described in the PR.

I would like to strongly suggest anyone that implements ingress to
test the scenarios and please please please, provide some feedback if
some bug, some issue or some undesired behavior happens. So we might
have enough time to correct before v1.20 :)

Thank you everyone for the previous feedback and suggestions provided
to the command syntax.

[Tim Hockin]

Yay!

[jay vyas]

nice one ricardo!   tested it w/ contour on a single node cluster and it worked nicely. thanks ! 

--

jay vyas

[Manuel Zapf]

Well done! 

I’ll give it some rundowns tomorrow as well. Thanks for working on that. 

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-network/CACVCA%3Dd1RPMOffc1DdpCBX21rwxda2JOEzqtJYh9oAu45Wupxw%40mail.gmail.com.