Skip to content

Commit

Permalink
feat(transl) disable automatic SNI handling
Browse files Browse the repository at this point in the history
Disable automatic SNI criteria addition by commenting out the code that
adds it. This is a useful convenience feature, but would cause breakage
because of Kong/kong#6425

Once that issue is fixed, we should re-enable this functionality, but
gate it with a flag, to avoid issues with older Kong versions.
  • Loading branch information
Travis Raines committed Oct 22, 2020
1 parent be66b5d commit e0523a8
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 34 deletions.
10 changes: 6 additions & 4 deletions internal/ingress/controller/parser/parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2529,10 +2529,12 @@ func TestParserSNI(t *testing.T) {
StripPath: kong.Bool(false),
RegexPriority: kong.Int(0),
Hosts: kong.StringSlice("example.com"),
SNIs: kong.StringSlice("example.com"),
PreserveHost: kong.Bool(true),
Paths: kong.StringSlice("/"),
Protocols: kong.StringSlice("http", "https"),
// TODO disabled pending resolution of https://github.com/Kong/kong/issues/6425
// this relies on automatic addition of SNI criteria
//SNIs: kong.StringSlice("example.com"),
PreserveHost: kong.Bool(true),
Paths: kong.StringSlice("/"),
Protocols: kong.StringSlice("http", "https"),
}, state.Services[0].Routes[0].Route)
assert.Equal(kong.Route{
Name: kong.String("default.foo.10"),
Expand Down
55 changes: 25 additions & 30 deletions internal/ingress/controller/parser/translate.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,16 +45,16 @@ func fromIngressV1beta1(log logrus.FieldLogger, ingressList []*networkingv1beta1
// someone has created an Ingress whose rule hostname set is a proper superset of the Ingress's TLS hostname
// set, ignoring some complications introduced by wildcards. maybe.
result.SecretNameToSNIs.addFromIngressV1beta1TLS(ingressSpec.TLS, ingress.Namespace)
//var routeSNIs []*string
hasSNI := false
for i := range ingressSpec.TLS {
if len(ingressSpec.TLS[i].Hosts) > 0 {
hasSNI = true
}
// for _, hostname := range ingressSpec.TLS[i].Hosts {
// routeSNIs = append(routeSNIs, &hostname)
// }
}
// TODO currently disabled because of https://github.com/Kong/kong/issues/6425
//hasSNI := false
//for i := range ingressSpec.TLS {
// if len(ingressSpec.TLS[i].Hosts) > 0 {
// hasSNI = true
// }
// // for _, hostname := range ingressSpec.TLS[i].Hosts {
// // routeSNIs = append(routeSNIs, &hostname)
// // }
//}

for i, rule := range ingressSpec.Rules {
host := rule.Host
Expand Down Expand Up @@ -93,26 +93,21 @@ func fromIngressV1beta1(log logrus.FieldLogger, ingressList []*networkingv1beta1
if host != "" {
hosts := kong.StringSlice(host)
r.Hosts = hosts
// TODO maybe. this forcibly adds SNI match criteria for the TLS hostnames in an Ingress rule
// to the Kong route. That criteria arguably should exist for any Ingress rules with a hostname,
// and adding it automatically is useful for the current common (only?) use of this criteria in the
// Kong proxy, indicating when the proxy should request an mTLS client cert. It may create issues
// if users require a different SNI match criteria (unlikely) or support clients without SNI
// support (less common over time, but still a reality in regions with a large number of older
// devices with EOL OSes). A vendor-specific override can address either case, though may need to
// consider future changes to SNI matching in the Kong proxy core. Wildcard hostnames present a
// challenge, because you might reasonably want to add them (and the Ingress spec doesn't care
// about SNI, so it allows them by virtue of allowing wildcard hostnames), but the proxy doesn't
// let you configure them.
if hasSNI {
var snis []*string
for _, hostname := range hosts {
if !strings.Contains(*hostname, "*") {
snis = append(snis, hostname)
}
}
r.SNIs = snis
}
// TODO this would SNI criteria to routes with certificates automatically based on their hostnames
// It's currently disabled because of https://github.com/Kong/kong/issues/6425. Adding them as-is
// would prevent those routes from handling HTTP traffic or HTTP to HTTPS redirects at all.
// It should be safe to enable with that fixed, though older Kong versions will remain a concern.
// It's also a concern if the route needs to support SNI-incapable clients. As such, this behavior
// should probably have a flag.
//if hasSNI {
// var snis []*string
// for _, hostname := range hosts {
// if !strings.Contains(*hostname, "*") {
// snis = append(snis, hostname)
// }
// }
// r.SNIs = snis
//}
}

serviceName := ingress.Namespace + "." +
Expand Down

0 comments on commit e0523a8

Please sign in to comment.