Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

I signed the CLA

CLAs look good, thanks!

Labelling this PR as size/S

Indeed:

    // SecretName is the name of the secret used to terminate SSL traffic on 443.
    // Field is left optional to allow SSL routing based on SNI hostname alone.
    // If the SNI host in a listener conflicts with the "Host" header field used
    // by an IngressRule, the SNI host is used for termination and value of the
    // Host header is used for routing.
    SecretName string `json:"secretName,omitempty"`

As the comment you're deleting says:

-   // Currently the Ingress only supports HTTP(S), so a secretName is required.
 -  // This will not be the case if we support SSL routing at L4 via SNI.

The reason it is "omit-empty" or optional, is because it's the most logical way to support SNI. And when we do support SNI in the various ingress backends, it will be truly optional.

The reason we validate against it is because there currently isn't an ingress backend that does it (and by isn't, I mean, isn't in a kubernetes repo that I can point to in the docs). So the risk here is that someone will see these docs: https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/extensions/types.go#L658, create a secret-less Ingress on AWS, and wait around hoping for SNI.

@tam7t can you describe how you're doing SNI? Are you creating multiple Ingresses each with a different host, going to a backend? Another way to express SNI is:
#19333 (comment)

I'm also not against this change, if you can produce an example that trumps the one in #19333 (comment)

As discussed in that issue, SNI through Ingress will become more natural when we've resolved: #23291

@bprashanth I'm currently using:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: myapplication
spec:
  tls:
  - hosts:
    - foo.bar.com
    secretName: fake-secret
  rules:
  - host: foo.bar.com
    http:
      paths:
      - backend:
          serviceName: myapplication
          servicePort: 443
        path: /
  - host: baz.bar.com
    http:
      paths:
      - backend:
          serviceName: myapplication
          servicePort: 80
        path: /

Creating an ingress-per-service that may include a mix of both HTTP and TLS hosts.

And using a (non-public) fork of https://github.com/macb/hing with SNI routing based on haproxy's req.ssl_sni -m beg -i foo.bar.com TCP match rule. I thought about using annotations but decided to go the IngressTLS route when I read the documentation, only to be tricked 😛 and have to add the fake-secret as a workaround.

@tam7t so you want to express a client advertising a different host in clientHello than in Host header? (otherwise you can just mux on rules.host right)

Right, I have a single IP address available for ingress listening on 443 and am routing connections to appropriate backends based on TLS client hello's.

alright, so you're not decrypting the connection since you don't have the certs.
so you get foo.bar.com in clientHello
you send it to myapplication
you get baz.bar.com in clientHello
what do you do?

your SNI switch is only on foo.bar.com, but ideally you'd want to route it to myapplication, right?
I'm confused why you need foo.bar.com in 2 places. Can you clarify?

Or are you just using the tls section as an effective "boolean"?

ah, ok i missed it. i didn't read you yaml carefully.

Yeah, I'm essentially using the tls.hosts array as a boolean

This makes sense if you're also listening on :80, because then you can recive plain http traffic and figure out what baz.bar.com is, but if you get stuff on :443 for one of the specified hostnames, you send it via SNI. Is this what you meant?

Regardless if that's what you're doing or not, I think that establishes precedence.

@tam7t if you can please add oneline to the end of the comments in https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/extensions/types.go#L658
https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/extensions/v1beta1/types.go
to say something like:

// .. the SNI TLS extension, if the 
// ingress controller fulfilling the ingress supports SNI. 

I'll LGTM this.

@bprashanth I've updated the comment (and ran hack/update-generated-swagger-docs.sh)

CLAs look good, thanks!

Labelling this PR as size/M

GCE e2e build/test passed for commit e61e524783c55794b1247b3b44454f3cd2fc271a.

• Build Log
• Test Artifacts
• Internal Jenkins Results

GCE e2e build/test passed for commit 113b0a9eaad548576bb0b038814cbe3218550ad6.

• Build Log
• Test Artifacts
• Internal Jenkins Results

@tam7t I think the unittest failure is related:

ok      k8s.io/kubernetes/pkg/registry/ingress  1.169s
=== RUN   TestCreate
--- PASS: TestCreate (0.72s)
=== RUN   TestUpdate
--- FAIL: TestUpdate (0.69s)
    resttest.go:472: expected nil object and no creation for object: &{{ } {foo4  test    1 0001-01-01 00:00:00 +0000 UTC <nil> <nil> map[] map[]} {0xc8209085d0 [{[foo.bar.com *.bar.com] fooSecret} {[foo.bar.com] }] [{foo.bar.com {0xc820900c00}}]} {{[{127.0.0.1 }]}}}
    resttest.go:475: expected invalid or bad request error, got <nil>
=== RUN   TestDelete
--- PASS: TestDelete (0.

Please fix this test: https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/ingress/etcd/etcd_test.go#L167

GCE e2e build/test passed for commit 4d22c2f.

• Build Log
• Test Artifacts
• Internal Jenkins Results

@tam7t LGTM, thanks

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

GCE e2e build/test passed for commit 4d22c2f.

• Build Log
• Test Artifacts
• Internal Jenkins Results

Automatic merge from submit-queue

Commit found in the "release-1.2" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.