Add Ingress GA details #1025

bowei · 2019-04-30T18:32:13Z

Fleshes out some details regarding the issues we wish to tackle in the 1.15+ releases.
Implementation and details are WIP.
This is an initial pass.

@thockin @liggitt @aledbf

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

ElvinEfendi · 2019-04-30T19:47:39Z

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

+```
+
+If `Healthcheck` is nil, then the implementation default healthcheck will be
+configured, healthchecking the root `/` path. If `Healthcheck` is specfied,


Alternative default behaviour would be to not configure any healthchecking and rely on service controller to keep the list of endpoints up-to-date based on Readiness probe and consider all the endpoints healthy.

some APIs require a healthcheck be defined :(

I agree with @ElvinEfendi .
We'd better push the service team to remove the mandatory healthCheck requirement.
e.g. For aws ALB, the healthCheck from AWS is mandatory, and i have been pushing the internal team to remove such requirement.

aledbf · 2019-04-30T19:49:37Z

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

+  // populated to match the Resource. If both ServiceName and Resource are
+  // specified, then they must reference the same Service object.
+  // +optional
+  Resource v1.TypedLocalObjectReference


Can we use the same approach in HTTPIngressPath to allow a custom configuration? This would allow exposing features available in the particular implementation removing the ambiguity of annotations.

Do you really want a CRD for path strings?

Also, that doesn't help with the round-trip requirement -- we have to be able to go from v1beta1 -> v1 -> v1beta1 and not lose information

Do you really want a CRD for path strings?

No, I want to remove annotations to configure "things" in the path.

we have to be able to go from v1beta1 -> v1 -> v1beta1 and not lose information

Good point.

aledbf · 2019-04-30T19:54:59Z

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

+| `(abc)`    | grouping                |
+
+Maintaining a regular expression subset is likely not worth the complexity and
+is likely impossible across the [many implementations][regex-survey].


Just for reference kubernetes/ingress-nginx#555

aledbf · 2019-04-30T19:57:50Z

@bowei what about referencing secrets from a different namespace?
This is a common complaint of users with a wildcard SSL certificate that must be manually copied when the ingress for different hostnames is located in different namespaces (cert-manager)

liggitt · 2019-04-30T20:06:02Z

@bowei what about referencing secrets from a different namespace?
This is a common complaint of users with a wildcard SSL certificate that must be manually copied when the ingress for different hostnames is located in different namespaces (cert-manager)

Cross-namespace object references are a non-goal (not just for ingress, but broadly). Future enhancements like named certificates (satisfied by controller-specific methods, such as resolving secrets in an ingress-controller-defined namespace) could address that usability issue.

aledbf · 2019-04-30T20:14:07Z

Cross-namespace object references are a non-goal

Can we put that in the spec?

thockin

A bunch of comments on the details of the API, but the KEP seems fine.

Thanks!

/lgtm
/approve

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

thockin · 2019-04-30T21:42:08Z

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

+> If `Host` is a wildcard, then request matches the rule if the http host header
+> is to equal to the suffix (removing the first label) of the wildcard rule.
+> E.g. wildcard "*.foo.com" matches "bar.foo.com" because they share an equal
+> suffix "foo.com".


Also make clear that wildcards a re whole-word replacements and must be the first segment.

thockin · 2019-04-30T21:44:28Z

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

+
+type IngressBackendHealthcheck struct {
+  // HTTP defines healthchecks using the HTTP protocol.
+  HTTP *IngressBackendHTTPHealthcheck


Do we need this indirection? We're already inside the HTTP block, and I don't think we'll realistically support anything but HTTP

thockin · 2019-04-30T21:49:47Z

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

+  // Class references an IngressClass resource in kube-system. This is used
+  // by the cluster Ingress controllers to determine which controller operates
+  // on this resource.
+  Class string


As with all other *Class, we now need to define default behavior if not specified.

We could do like Storage and add an admission controller which sets this to the name of the class that carries a label.

We could ignore it and let the controllers fight it out (e.g. on GCP we could hard-configure gce-ingress as the default via flags, and users could not change that)

Hi, I think it's better to change Class to IngressClassName
Which aligns with

StorageClassName

PriorityClassName

thockin · 2019-04-30T21:51:12Z

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

+}
+
+type IngressClassSpec struct {
+  // This is currently empty.


Will have to carry the identifier for the controller which implements the class. E.g. Controller string -- and document it as namespaced just like storage provisioners.

maybe just provisioner string? so we can get consistent naming with StorageClass.
From my pov, 'provisioner' and 'controller' have the same meaning in this context.
Also, is it possible to include Parameters map[string]string as well, so we can have controller specific settings expressed as fields.

One use case could be explicit denote the sharing LB among Ingresses.
e.g. users can provision separate IngressClass for different LB like:

kind: StorageClass apiVersion: storage.k8s.io/v1 metadata: name: alb-lb-a provisioner: kubernetes.io/aws-alb parameters: lbName: lb-a

all ingresses(across namespace) using this IngressClass with be backed by a single LB called lb-a.

thockin · 2019-04-30T21:56:05Z

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

+Add a field to the `IngressBackend` struct with an object reference:
+
+```golang
+type IngressBackend struct {


I think we'll want to restructure this as something like:

// +union type IngressBackend struct { // +union-discriminator Type string Service IngressBackendService External v1.TypedObjectReference } type IngressBackendService struct { ServiceName string ServicePort intstr.IntOrString }

k8s-ci-robot · 2019-04-30T22:24:00Z

[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

thockin · 2019-05-01T17:44:07Z

This already merged -- do you want to open a followup PR?

…

On Wed, May 1, 2019 at 10:39 AM Bowei Du ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In keps/sig-network/20190125-ingress-api-group.md <#1025 (comment)> : > +> is to equal to the suffix (removing the first label) of the wildcard rule. +> E.g. wildcard "*.foo.com" matches "bar.foo.com" because they share an equal +> suffix "foo.com". + +### Healthchecks + +The current spec does not have any provisions to customize appropriate +healtchecks for referenced backends. Many users already have a healthcheck +URLthat is lightweight and different from the HTTP root (e.g. `/`). + +One option that has been explored is to infer the healthcheck URL from the +Readiness probes on the Pods of the Service. This method has proven to be quite +unstable: Every Pod in a Service can have a different Readiness probe definition +and it's not clear which one should be used. Furthermore, the behavior is quite +implicit and creates action-at-a-distance relationship between the Ingress and +Pod resources. updated text — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#1025 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABKWAVDUH7U3P3KMZ4BBFI3PTHIVHANCNFSM4HJPDYDA> .

bowei · 2019-05-01T18:05:50Z

Comment feedback is being integrated into https://github.com/bowei/enhancements/blob/ingress-ga/keps/sig-network/20190125-ingress-api-group.md

If you notice your comment has been resolved, check the link above.

bowei · 2019-05-01T18:06:14Z

Yes -- opening a new PR once I sweep through this set of comments.

M00nF1sh · 2019-05-02T02:41:55Z

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

+```golang
+type IngressSpec struct {
+  ...
+  // Class references an IngressClass resource in kube-system. This is used


I think this IngressClass should be a cluster scope resource instead of namespaced resource in kube-system

Agree, it should be like storage class

Update the Ingress KEP with GA items

e2bd9bd

k8s-ci-robot requested review from dcbw and thockin April 30, 2019 18:32

ElvinEfendi reviewed Apr 30, 2019

View reviewed changes

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

aledbf reviewed Apr 30, 2019

View reviewed changes

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

ElvinEfendi reviewed Apr 30, 2019

View reviewed changes

aledbf reviewed Apr 30, 2019

View reviewed changes

thockin reviewed Apr 30, 2019

View reviewed changes

k8s-ci-robot assigned thockin Apr 30, 2019

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 30, 2019

bowei changed the title ~~WIP: Update the Ingress KEP with GA items~~ Update the Ingress KEP with GA items May 1, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 1, 2019

bowei changed the title ~~Update the Ingress KEP with GA items~~ WIP: Update the Ingress KEP with GA items May 1, 2019

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 1, 2019

bowei changed the title ~~WIP: Update the Ingress KEP with GA items~~ WIP: May 1, 2019

bowei changed the title ~~WIP:~~ Add Ingress GA details May 1, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 1, 2019

k8s-ci-robot merged commit 5c54c75 into kubernetes:master May 1, 2019

M00nF1sh reviewed May 2, 2019

View reviewed changes

aledbf mentioned this pull request May 25, 2019

Standardise or document Ingress path expressions across implementations kubernetes/ingress-nginx#555

Closed

sftim mentioned this pull request Dec 23, 2019

Document Ingress for GA #1420

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Ingress GA details #1025

Add Ingress GA details #1025

bowei commented Apr 30, 2019

ElvinEfendi Apr 30, 2019

thockin Apr 30, 2019

M00nF1sh May 2, 2019 •

edited

Loading

aledbf Apr 30, 2019 •

edited

Loading

thockin Apr 30, 2019

aledbf Apr 30, 2019

aledbf Apr 30, 2019

aledbf commented Apr 30, 2019

liggitt commented Apr 30, 2019

aledbf commented Apr 30, 2019

thockin left a comment

thockin Apr 30, 2019

thockin Apr 30, 2019

thockin Apr 30, 2019

M00nF1sh May 2, 2019

thockin Apr 30, 2019

M00nF1sh May 2, 2019 •

edited

Loading

thockin Apr 30, 2019

k8s-ci-robot commented Apr 30, 2019

thockin commented May 1, 2019 via email

bowei commented May 1, 2019

bowei commented May 1, 2019

M00nF1sh May 2, 2019

thockin May 7, 2019

Add Ingress GA details #1025

Add Ingress GA details #1025

Conversation

bowei commented Apr 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M00nF1sh May 2, 2019 • edited Loading

Choose a reason for hiding this comment

aledbf Apr 30, 2019 • 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

aledbf commented Apr 30, 2019

liggitt commented Apr 30, 2019

aledbf commented Apr 30, 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

M00nF1sh May 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 30, 2019

thockin commented May 1, 2019 via email

bowei commented May 1, 2019

bowei commented May 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M00nF1sh May 2, 2019 •

edited

Loading

aledbf Apr 30, 2019 •

edited

Loading

M00nF1sh May 2, 2019 •

edited

Loading