Ingress GA, updated KEP, merged review comments

[bowei]

Merging comment feedback.

[bowei]

I will ping when I've merged all of the comments. (I'm about halfway there)

@thockin @liggitt @M00nF1sh @aledbf

[ElvinEfendi]

This PR might not the best place for this, but does the following imply every implementation has to implement healthchecking?

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

It's great you're introducing a separate healthchecking but given not all existing controllers (i.e ingress-nginx) do healthchecking and relies on Readiness probe and this is accepted by users would it not be better to default to whatever the implementation decides to default to?

[ElvinEfendi]

I've few proposals, please let me know if you want me create a separate issue for this.

1. I love the idea of "Alternative backend types", but can we also detach backend definition from rules? That would let us to dynamically decide what backend to use for a given frontend. And would let us implement things like canaring, blue/green deploy easier.

Here's an example manifest:

kind: Ingress
metadata:
  name: example-ingress
spec:
  backends:
    - name: example-backend
      serviceName: service1
      servicePort: 80
    - name: example-backend-canary
      serviceName: service1-canary
      servicePort: 80
  tls:
  - hosts:
    - example.foo.com
    secretName: example-tls
  rules:
    - host: example.foo.com
      http:
        paths:
        - path: /
          backend: example-backend

In the world default backend would be defined using default: true in a backend.

2. For rules introduce another field to explicitly refer to the K8s secret holding TLS certificate. Otherwise currently controllers have to deduce what certificate to use based on host matching which is an extra indirection and limits in certain cases. This field would also enable HTTPs frontend provisioning.

3. rules.http is misleading - people use it to configure i.e a GRPC front-end. Also we have rule inside a rule (referring to the fact that you can define multiple paths in a rule). What if we introduce a new field called protocol in Rule struct and remove http and paths. So example two rules would look like:

- host:  example.foo.com
  path: /
  protocol: http
  backend: example-backend
  secretName: example-tls
- host:  example.foo.com
  path: /foo
  protocol: http
  backend: example-backend-foo
  secretName: example-tls

Next we can extend this further with HTTP verb/method:

- path: /
  protocol: http
  backend: example-backend-foo
  secretName: example-tls
  method: GET

This would configure a front-end that proxies only GET requests to example-backend-foo backend.

When no rule is configured then the Ingress would configure a front-end accepting all HTTP requests and proxying to default backend.

[M00nF1sh]

Adding to @ElvinEfendi
We need to support for verb/header/query based too. It can be supported in Nginx and AWS LB easily.

Also, Since we are adding support for multiple backend(service/external), can we make backend a list instead of single entry? We can add a weight in each entry to achieve weight based routing, which will benefit canary deployment

BTW, I think these parts of istio's API is well designed: https://github.com/istio/api/blob/master/networking/v1alpha3/virtual_service.pb.go#L494

[ElvinEfendi]

We need to support for verb/header/query based too. It can be supported in Nginx and AWS LB easily.

Yes, this would be very useful and I've seen users requesting this in ingress-nginx. If for some reason we want to keep the ingress spec lean, we can also introduce "Alternative front-end types" similar to "Alternative backend types" where the implementation decides what configuration options it provides.

We can add a weight in each entry to achieve weight based routing, which will benefit canary deployment

IMHO backend is a wrong level of abstraction to configure weight. The knowledge of weight is needed by front-end to make routing decision. Therefore I think it'd be more appropriate to configure weight in front-end when configuring backends, for example:

- host:  example.foo.com
  path: /foo
  protocol: http
  backends:
    - name: example-backend-foo
      weight: 30
    - name: example-backend-foo-canary
      weight: 70
  secretName: example-tls

where example-backend-foo and example-backend-foo-canary are the names of backends defined in spec.backends:.

If we decide to support HTTP method/verb, header, request params on top of path and host in a rule we have to define their priorities as well. Maybe we can introduce an optional priority in Rule struct.

[M00nF1sh]

@ElvinEfendi
Yes, that's was what I'm intend to express 😄. when i mentioned backends, it's not reference the spec.backends you proposed.

To be honest, i'm not a fan to predefine backends in spec.backends, it introduced a another name customers need to decide/specify/match. If we do need to do name matching like this, we should add a separate resource like ServiceBackend, but it's more complicated than needed, I prefer to (backend can be inlined too :D)

path: /
   backends:
      -  weight: 50
          backend:
              serviceName: my-svc
              servicePort: 443
      -  weight: 50
          backend:
               resource: my-gke-bucket-crd

If we can make big changes like this, i'd also like to see traffic mirroring. path rewrite get included (though seems only nginx ingress controller can support them now :D)

[ElvinEfendi]

👍 I guess it would not be necessary to predefine backends if we implement the other suggested proposals.

[cmluciano]

Couple of general questions

[cmluciano]

Why default this to ImplementationSpecific in v1beta1 but not in v1?

[cmluciano]

Mentioned in sig-net meeting- this is strictly for backwards-compat for now

[cmluciano]

How do we rename this safely? IIRC we may need to create a new field at this point in the API process.

[liggitt]

fields can be renamed across version boundaries by conversion

[cmluciano]

/lgtm

[k8s-ci-robot]

@bowei: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[vllry]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bowei, cmluciano, vllry
To complete the pull request process, please assign caseydavenport
You can assign the PR to them by writing /assign @caseydavenport in a comment when ready.

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

Needs approval from an approver in each of these files:

• keps/sig-network/OWNERS

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

[bowei]

arghh.. accidentally trashed this PR