Implement support for multiple filter chains, SNI with multiple certs by ZackButcher · Pull Request #4975 · istio/istio

ZackButcher · 2018-04-16T18:31:21Z

This wound up being a bit larger than anticipated, plumbing support for multiple filter chains everywhere required a decent bit of work.

I'm writing up unit tests and an explicit SNI e2e test right now, but as-is all pilotv2 tests pass against this branch locally, so I think review can start while I get more testing written.

…gateways; this is required because we resolve gateway names now after merging gateways

istio-testing · 2018-04-16T18:31:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: costinm

Assign the PR to them by writing /assign @costinm in a comment when ready.

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

pilot/OWNERS
tests/OWNERS

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

rshriram · 2018-04-16T18:41:15Z

pilot/pkg/model/config.go

 		} else {
-			for _, ruleGateway := range rule.Gateways {
-				if gatewayRequested[ruleGateway] {
+			log.Infof("service %v has gateways %v, comparing against %v", config.Name, rule.Gateways, gateways)


I assume you are going to get rid of these printfs?

Ah, missed that one. Tried to move them all to Debugf at least.

rshriram · 2018-04-16T18:45:03Z

pilot/pkg/networking/core/v1alpha3/gateway.go

 	return listeners, nil
 }

+func createHTTPFilterChainOpts(env model.Environment, servers []*networking.Server, gatewayNames map[string]bool) []*filterChainOpts {


createGatewayHTTPFilterChainOpts (qualify with gateway)

rshriram · 2018-04-16T18:45:21Z

pilot/pkg/networking/core/v1alpha3/gateway.go

 	}
 }

+func createTCPFilterChainOpts(env model.Environment, servers []*networking.Server, gatewayNames map[string]bool) []*filterChainOpts {


createGatewayTCPFilterChainOpts...

codecov · 2018-04-16T18:59:59Z

Codecov Report

Merging #4975 into master will decrease coverage by 1%.
The diff coverage is 0%.

@@           Coverage Diff           @@
##           master   #4975    +/-   ##
=======================================
- Coverage      74%     74%   -<1%     
=======================================
  Files         307     307            
  Lines       25588   25914   +326     
=======================================
+ Hits        18724   18937   +213     
- Misses       6115    6225   +110     
- Partials      749     752     +3

Impacted Files	Coverage Δ
pilot/pkg/model/gateway.go	`0% <0%> (-100%)`	⬇️
pilot/pkg/networking/core/v1alpha3/listener.go	`0% <0%> (ø)`	⬆️
...ilot/pkg/networking/plugin/authn/authentication.go	`68% <0%> (ø)`	⬇️
pilot/pkg/networking/core/v1alpha3/gateway.go	`0% <0%> (ø)`	⬆️
pilot/pkg/networking/core/v1alpha3/httproute.go	`28% <0%> (ø)`	⬇️
pilot/pkg/model/config.go	`51% <0%> (ø)`	⬇️
...olarwinds/internal/papertrail/papertrail_logger.go	`59% <0%> (-21%)`	⬇️
mixer/adapter/prometheus/server.go	`97% <0%> (-3%)`	⬇️
mixer/adapter/kubernetesenv/kubernetesenv.go	`68% <0%> (-2%)`	⬇️
mixer/adapter/servicecontrol/testhelper.go	`72% <0%> (-1%)`	⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e373621...31bfa70. Read the comment docs.

ZackButcher · 2018-04-16T19:08:46Z

/retest

ostromart · 2018-04-16T18:36:48Z

pilot/pkg/model/gateway.go

+		log.Debugf("MergeGateways: merging gateway %q into %v:\n%v", name, names, gateway)
+		for _, s := range gateway.Servers {
+			log.Debugf("MergeGateways: gateway %q processing server %v", name, s.Hosts)
+			if ss, exists := servers[s.Port.Number]; exists {


nit: for maps of ptrs you can just use if servers[x] != nil. otherwise it's idiomatic to always use "ok" when testing for presence (rather than exists etc).

This isn't actually a map of pointers, it's a map of arrays of pointers. By construction the key will either not exist in the map or it will exist and the value array will have a least one element in it (the else case below).

Either way, included a comment denoting why this is safe.

ostromart · 2018-04-16T18:38:57Z

pilot/pkg/model/gateway.go

+			log.Debugf("MergeGateways: gateway %q processing server %v", name, s.Hosts)
+			if ss, exists := servers[s.Port.Number]; exists {
+				// TODO: remove this check when Envoy supports filter chain matching so we can expose multiple protocols on the same physical port
+				if ss[0].Port.Protocol != s.Port.Protocol {


it's not clear that ss couldn't exist and be empty here. suggest comment or better just check for len and return error.

See above; included a comment too.

ostromart · 2018-04-16T18:47:23Z

pilot/pkg/networking/core/v1alpha3/gateway.go

-		newListener := buildListener(opts)

-		var httpFilters []*http_conn.HttpFilter
+		listenerType := plugin.ModelProtocolToListenerType(protocol)


👍
could we also simplify the long case lists with a similar helper e.g. case isL7Proto(protocol): or something like that? ModelProtocolToListenerType could be written in terms of that base function.

We already have that method on model.Protocol. In writing this PR I'm not convinced that ListenerType should exist at all, I think we can replace it with model.Protocol just fine. I just didn't want to include that change in this PR too.

ostromart · 2018-04-16T18:53:37Z

pilot/pkg/networking/core/v1alpha3/gateway.go

+		l := buildListener(opts)
+		mutable := &plugin.MutableObjects{
+			Listener:     l,
+			FilterChains: make([]plugin.FilterChain, len(l.FilterChains)),


this is not obvious and could use a comment somewhere e.g. buildListener sets the number of FilterChains but does not create any filters.

Done. I think, given the current usage, buildListener should return a plugin.MutableObjects that's constructed correctly. See my comment in listeners.go about wanting to combine listener, mutable objects, and opts; I think consuming opts to produce a mutable objects (with listener inline) is probably what we want. But I didn't want to include that change in this PR, especially since it'd likely include moving mutable objects from pkg plugin to pkg core.

makes sense

ostromart · 2018-04-16T18:58:30Z

pilot/pkg/networking/core/v1alpha3/gateway.go

+		return []*filterChainOpts{}
+	}
+
+	nameToServiceMap := make(map[string]*model.Service)


optional: make(map[string]*model.Service, len(services))

ostromart · 2018-04-16T19:16:55Z

pilot/pkg/networking/core/v1alpha3/listener.go

 			l := buildListener(listenerOpts)
-			if err := marshalFilters(l, listenerOpts, buildInboundNetworkFilters(instance), nil); err != nil {
-				log.Warn(err.Error())
+			// TODO: should we call plugins for the admin port listeners too? We do everywhere else we contruct listeners.


yes, we should. maybe set listener type to ListenerTypeADMIN to make life easy for plugins? i can take this in a followup PR.

ostromart · 2018-04-16T19:19:40Z

pilot/pkg/networking/core/v1alpha3/listener.go

+		&http_conn.HttpFilter{Name: xdsutil.Router},
+		// TODO: need alphav3 fault filters.
+		//buildFaultFilters(opts.config, opts.env, opts.proxy)...
+	)


ostromart · 2018-04-16T19:20:56Z

pilot/pkg/networking/core/v1alpha3/listener.go

+		protocolPrefix = "http"
 	} else {
-		protoPrefix = "tcp"
+		protocolPrefix = "tcp"


put this block in helper for better readability?

ostromart · 2018-04-16T19:27:16Z

pilot/pkg/networking/core/v1alpha3/route/route.go

 	"github.com/envoyproxy/go-control-plane/envoy/api/v2/core"
 	"github.com/envoyproxy/go-control-plane/envoy/api/v2/route"
 	"github.com/gogo/protobuf/types"
+	"istio.io/istio/pkg/log"


ostromart · 2018-04-16T19:31:28Z

pilot/pkg/networking/plugin/plugin.go


+// FilterChain describes a set of filters (HTTP or TCP) with a shared TLS context.
+type FilterChain struct {
+	HTTP []*http_conn.HttpFilter


lint (comments)

ostromart · 2018-04-16T19:33:50Z

could you pls check visually that mixer config still looks reasonable since we still don't have a test for this.

ostromart · 2018-04-16T22:17:29Z

pilot/pkg/networking/core/v1alpha3/gateway.go

+		l := buildListener(opts)
+		mutable := &plugin.MutableObjects{
+			Listener:     l,
+			FilterChains: make([]plugin.FilterChain, len(l.FilterChains)),


makes sense

ostromart · 2018-04-16T22:17:35Z

pilot/pkg/networking/core/v1alpha3/gateway.go

-		newListener := buildListener(opts)

-		var httpFilters []*http_conn.HttpFilter
+		listenerType := plugin.ModelProtocolToListenerType(protocol)


ZackButcher · 2018-04-16T23:58:27Z

@costinm can you provide LGTM for tests/ dir.

ZackButcher added 4 commits April 16, 2018 11:27

first pass at SNI, untested

449933e

treat unknown protocols as TCP rather than skipping entirely

e43eda6

resolve gateway names when seeing if virtual services match a set of …

1ac1daa

…gateways; this is required because we resolve gateway names now after merging gateways

structure vhosts correctly for envoy http config

6e07a0c

ZackButcher requested review from costinm, ijsnellf, ostromart and rshriram April 16, 2018 18:31

googlebot added the cla: yes label Apr 16, 2018

model.ConvertCaseInsensitiveStringToProtocol -> model.ParseProtocol

5924558

rshriram requested review from GregHanson and zachgersh April 16, 2018 18:40

rshriram reviewed Apr 16, 2018

View reviewed changes

remove log line

0694faa

rshriram reviewed Apr 16, 2018

View reviewed changes

Qualify names with 'Gateway'; t.Fatal -> t.Skip; bin/fmt.sh

2ccc8fa

ostromart reviewed Apr 16, 2018

View reviewed changes

ZackButcher added 2 commits April 16, 2018 14:52

address review comments

1784754

Comment about mutable

a4c7986

ZackButcher changed the title ~~Implement support for multiple filter chains, SNI~~ Implement support for multiple filter chains, SNI with multiple certs Apr 16, 2018

fix linter complaint about format arg

31bfa70

ostromart approved these changes Apr 16, 2018

View reviewed changes

allocate correct size for array backing service map

7beccbc

rshriram approved these changes Apr 16, 2018

View reviewed changes

ostromart mentioned this pull request Apr 16, 2018

Change plugin MutableObjects use pointers to data so plugin can mutate it #4986

Closed

rshriram merged commit 7b26e4a into istio:master Apr 17, 2018

ostromart mentioned this pull request Apr 19, 2018

Mixer filter fixes #5012

Merged

Conversation

ZackButcher commented Apr 16, 2018

Uh oh!

istio-testing commented Apr 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ZackButcher commented Apr 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZackButcher Apr 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ostromart commented Apr 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZackButcher commented Apr 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Apr 16, 2018 •

edited

Loading

ZackButcher Apr 16, 2018 •

edited

Loading