Ingress GA, updated KEP, merged review comments (recovered version of #1036) #1113

bowei · 2019-06-24T18:49:43Z

This is a recovered version of #1036, which I closed by accident.

cmluciano

minor nit but overall /lgtm

keps/sig-network/20190125-ingress-api-group.md

bowei · 2019-07-24T19:59:37Z

@aledbf @thockin @cmluciano

bowei · 2019-07-24T20:06:03Z

/assign @thockin @cmluciano

bowei · 2019-07-24T20:06:38Z

cc: @liggitt

keps/sig-network/20190125-ingress-api-group.md

liggitt · 2019-07-30T13:05:18Z

keps/sig-network/20190125-ingress-api-group.md

@@ -425,9 +436,10 @@ type IngressBackend struct {
 }


The API server will automatically populate the Resource field below.
...
If both ServiceName and Resource are specified, then they must reference the same Service object.

this sounds like cross-field defaulting, which got us in trouble with workload label selectors. consider the following scenario:

user creates ingress not knowing about the new fields, populating service name with "foo"

the apiserver populates resource with type: Service, name: foo, apiGroup: ""

the user modifies the ingress to change service name to "bar"

apiserver fails validation because service name and resource.name no longer match

Agree. Can we call this a one-of without a discriminator (we have high confidence it will only be 2 options)? Do we need to structure it as below?

BackendType string Service ServicBackend // {name string, port int or string} Resource v1.TypedLocalObjectReference

Or can we leave it like this get away with it?

@apelisse

I'd also like to consider not using IntOrString and instead doing something like:

Port BackendPort

type BackendPort { Number int Name string }

Do we need a discriminator if we know it is A or B and there will never be a C ?

Probably not, as all clients know they have to reset the other value.

As a side note, Name is usually used as a key for associative lists though, so that's slightly confusing outside the context.

@thockin I would even like to have just an int32, but I think this kind of change is bigger than beta -> GA

I've changed the proposal to have a one-of union between ServiceBackend, Resource. If someone wants to reference a Service using the Resource, that should still be ok, just more verbose for them.

@thockin -- is IntOrString considered something we wish to move away from?

IntOrString is generally seen as a mistake at this point.

If I interpret @apelisse correctly, he wants a discriminator field. Is that right?

I think they can get away without the discriminator since there are no other non-union members in that structure and it's not embedded anywhere.

My "Probably not" was an answer to:

Do we need a discriminator if we know it is A or B and there will never be a C ?

keps/sig-network/20190125-ingress-api-group.md

thockin · 2019-08-15T00:04:47Z

keps/sig-network/20190125-ingress-api-group.md

+  // Prefix - matches based on a URL path prefix split
+  //   by '/'. This is not a substring match. For example:
+  //   "/foo/bar" will match "/foo/bar", "/foo/bar/abc", but
+  //   will not match "/foo/barzzz". A Prefix Path of "/" will


Is this the most common semantic? It's different from other things I have seen, e.g. Istio and Envoy (I think) would match /foo/barzzz and require 2 rules to express /foo/bar (exact) and /foo/bar/* (prefix). I though nginx did too but I can't tell from docs...

@aledbf

Envoy has a few options to determine the route that it is matching on.

RouteMatch prefix validates that the prefix specified matches the beginning of the :path header

RouteMatch path is similar to the Exact case that Tim mentioned below. It validates that the path matches the :path header exactly after the query string is removed.

RouteMatch regex utilizes the C++ std::regex semantics to match the entire path specified in the :path header (minus the query bits). This does not work if only a subset of the :path header matches.

There is also a new safe_regex that will utilize the Google RE2 library. This new option is under development and is a response to some unsafe (normally user provided) regular expressions that could cause Envoy to crash.

Nginx locations also have similar options but rely on several key symbols for exact vs more fuzzy matching

I'm not sure if this helps much but figured it would at least add some more context in comparison to a few open source implementations.

/foo/bar (exact) and /foo/bar/* (prefix).

Same for nginx http://nginx.org/en/docs/http/ngx_http_core_module.html#location

location = /foo/bar {} -> exact match
location /foo/bar {} -> /foo/bar/*

Just in case, ingress-nginx does not provide "exact match" locations.

location /foo/bar {} -> /foo/bar/*

I'm not sure if this is the case.
Nginx matches /foo/barzzz in this case. It is a string prefix match and not a path prefix.

Add clarification of the behavior

GCE will implement as "/foo/bar/*".
Nginx will implement as location "/foo/bar/" and location "=/foo/bar". (prefix + equality)
Envoy will use path match.

It does not seem like a good idea to define this as string-based matching "/foobar", as that has fewer use cases and somewhat unexpected behavior.

keps/sig-network/20190125-ingress-api-group.md

thockin · 2019-08-15T00:17:59Z

keps/sig-network/20190125-ingress-api-group.md

@@ -425,9 +436,10 @@ type IngressBackend struct {
 }


Agree. Can we call this a one-of without a discriminator (we have high confidence it will only be 2 options)? Do we need to structure it as below?

BackendType string Service ServicBackend // {name string, port int or string} Resource v1.TypedLocalObjectReference

Or can we leave it like this get away with it?

@apelisse

thockin · 2019-08-15T00:19:43Z

keps/sig-network/20190125-ingress-api-group.md

@@ -425,9 +436,10 @@ type IngressBackend struct {
 }


I'd also like to consider not using IntOrString and instead doing something like:

Port BackendPort

type BackendPort { Number int Name string }

Do we need a discriminator if we know it is A or B and there will never be a C ?

keps/sig-network/20190125-ingress-api-group.md

szuecs

I have concerns about API changes in ingress, please check the details in my other comments.
Thanks for attention your heavy production user (>70 production clusters with >10000 ingress).

keps/sig-network/20190125-ingress-api-group.md

szuecs · 2019-10-14T09:09:28Z

keps/sig-network/20190125-ingress-api-group.md

@@ -425,9 +436,10 @@ type IngressBackend struct {
 }


@thockin I would even like to have just an int32, but I think this kind of change is bigger than beta -> GA

keps/sig-network/20190125-ingress-api-group.md

bowei · 2019-10-15T01:12:05Z

I've tried to address many of the comments above, but I'm fairly certain that there are some that are missed.

Please take a look.

We might just want to merge the update into the repo to cleaner diff and better discussion.

bowei · 2019-10-15T07:07:47Z

@thockin probably should merge this and keep accumulating comments on a new PR.

thockin

lots of open comments

keps/sig-network/20190125-ingress-api-group.md

thockin · 2019-10-15T22:58:56Z

keps/sig-network/20190125-ingress-api-group.md

+Interoperability between versions will be as follows:
+
+1. v1beta1 will default to `ImplementationSpecific`.
+1. v1 will default to `Prefix`.


Problem.

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md

...says "Adding a new field with a default value: the default values must be semantically equivalent in all currently supported API versions."

I am struggling to recall the exact reason for this rule. @liggitt is better at this game than me.

Defaulting to a different value in a new version is perfectly fine. The emphasis on the rule is on "currently supported versions", so that the current implicit behavior is maintained.

That said, would it be better to make v1 writers specify a PathType (e.g. make it required with no default in v1)? The downside is it makes every consumer be explicit in v1, which makes the manifests more verbose. The upside is it makes every consumer be explicit in v1, which reduces surprise.

That is a handy doc -- is there any plan to merge the content into the API conventions doc? The current "defaults" section is pretty short.

Changing to require being explicit, let's see how we feel about the (slight) increase in verbosity.

Actually -- wouldn't defaulting to ImplementationSpecific be more behavior preserving?

keps/sig-network/20190125-ingress-api-group.md

thockin · 2019-10-15T23:10:11Z

keps/sig-network/20190125-ingress-api-group.md

+  // Service is the name of the referenced service. The service must exist in
+  // the same namespace as the Ingress object.
+  // +optional
+  Service string


shouldnt this be called "Name"

This was in response to @apelisse 's comment about the use of the "Name" field as reserved for key-like.
It is more natural as "Name".

thockin · 2019-10-15T23:14:06Z

keps/sig-network/20190125-ingress-api-group.md

@@ -425,9 +436,10 @@ type IngressBackend struct {
 }


IntOrString is generally seen as a mistake at this point.

If I interpret @apelisse correctly, he wants a discriminator field. Is that right?

bowei · 2019-10-16T01:07:07Z

Note: I'm clicking on "resolve conversation" as a way to keep track of what has been merged into the text and to make the github review page more readable.

Does not mean discussion has been resolved, please keep commenting.

keps/sig-network/20190125-ingress-api-group.md

bowei · 2019-10-23T22:07:49Z

There were are some comments around the newly introduced union types; we will resolve them as part of the API review process, I think on the general direction there is consensus.

@thockin

thockin · 2019-10-24T20:16:46Z

Let's merge and iterate. This is close.

/lgtm
/approve

bowei · 2019-10-25T23:40:35Z

@thockin -- needs relgtm due to ToC verify issue

thockin · 2019-10-28T21:39:20Z

Thanks!

/lgtm

k8s-ci-robot · 2019-10-28T21:39:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~keps/sig-network/OWNERS~~ [thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bowei changed the title ~~Ingress GA, updated KEP, merged review comments~~ Ingress GA, updated KEP, merged review comments (recovered version of #1036) Jun 24, 2019

k8s-ci-robot requested review from dcbw and thockin June 24, 2019 18:50

cmluciano approved these changes Jul 8, 2019

View reviewed changes

keps/sig-network/20190125-ingress-api-group.md Outdated Show resolved Hide resolved

bowei force-pushed the ingress-ga-recovered branch from f8bba27 to 3994b90 Compare July 24, 2019 19:58

k8s-ci-robot assigned cmluciano and thockin Jul 24, 2019

bowei mentioned this pull request Jul 29, 2019

Use GCE load balancer controller with backend buckets kubernetes/ingress-gce#33

Closed

liggitt reviewed Jul 30, 2019

View reviewed changes

aledbf reviewed Jul 30, 2019

View reviewed changes

keps/sig-network/20190125-ingress-api-group.md Show resolved Hide resolved

thockin reviewed Aug 15, 2019

View reviewed changes

cmluciano mentioned this pull request Aug 23, 2019

Move Ingress to the networking.v1beta1 API group #758

Closed

1 task

smarterclayton suggested changes Sep 3, 2019

View reviewed changes

keps/sig-network/20190125-ingress-api-group.md Show resolved Hide resolved

keps/sig-network/20190125-ingress-api-group.md Outdated Show resolved Hide resolved

keps/sig-network/20190125-ingress-api-group.md Show resolved Hide resolved

rikatz reviewed Sep 26, 2019

View reviewed changes

keps/sig-network/20190125-ingress-api-group.md Show resolved Hide resolved

hbagdi reviewed Oct 7, 2019

View reviewed changes

szuecs reviewed Oct 11, 2019

View reviewed changes

keps/sig-network/20190125-ingress-api-group.md Outdated Show resolved Hide resolved

szuecs reviewed Oct 14, 2019

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 15, 2019

thockin reviewed Oct 15, 2019

View reviewed changes

hbagdi reviewed Oct 16, 2019

View reviewed changes

keps/sig-network/20190125-ingress-api-group.md Show resolved Hide resolved

keps/sig-network/20190125-ingress-api-group.md Outdated Show resolved Hide resolved

keps/sig-network/20190125-ingress-api-group.md Show resolved Hide resolved

cmluciano mentioned this pull request Oct 17, 2019

New branch for Ingress v1 API kubernetes/kubernetes#84026

Closed

bowei force-pushed the ingress-ga-recovered branch from 95b5da5 to 91c87b7 Compare October 22, 2019 22:35

Updates to the Ingress V1 GA

2951204

bowei force-pushed the ingress-ga-recovered branch from 91c87b7 to 2951204 Compare October 22, 2019 22:40

hbagdi reviewed Oct 22, 2019

View reviewed changes

keps/sig-network/20190125-ingress-api-group.md Show resolved Hide resolved

Fix typo

3f792fc

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 24, 2019

Fix TOC

7a7b8b3

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 28, 2019

k8s-ci-robot merged commit 37c1f02 into kubernetes:master Oct 28, 2019

k8s-ci-robot added this to the v1.17 milestone Oct 28, 2019

bowei deleted the ingress-ga-recovered branch May 14, 2021 18:14

Ingress GA, updated KEP, merged review comments (recovered version of #1036) #1113

Ingress GA, updated KEP, merged review comments (recovered version of #1036) #1113

Conversation

bowei commented Jun 24, 2019

cmluciano left a comment

Choose a reason for hiding this comment

bowei commented Jul 24, 2019

bowei commented Jul 24, 2019

bowei commented Jul 24, 2019

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

bowei Oct 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szuecs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowei commented Oct 15, 2019

bowei commented Oct 15, 2019

thockin 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowei commented Oct 16, 2019

bowei commented Oct 23, 2019

thockin commented Oct 24, 2019

bowei commented Oct 25, 2019

thockin commented Oct 28, 2019

k8s-ci-robot commented Oct 28, 2019

bowei Oct 15, 2019 •

edited

Loading