Initial code for NetworkScope - filter outbound listener/routes #9361

costinm · 2018-10-16T18:45:24Z

Enable namespace isolation - the services used in generating outbound listeners and routes will
include only local namespace and explicitly defined imports.

This seems the simplest approach for the current code - we can optimize it further.

Separate PRs will add the import, once the API is finalized - but this setting can already be
used on larger clusters if most namespaces only receive requests from gateways.
For Clusters/EDS we'll build a list based on Listeners/Routes - in a separate PR.

codecov · 2018-10-25T22:06:01Z

Codecov Report

Merging #9361 into release-1.1 will increase coverage by 7%.
The diff coverage is 69%.

@@              Coverage Diff               @@
##           release-1.1   #9361      +/-   ##
==============================================
+ Coverage           63%     70%      +7%     
==============================================
  Files              559     436     -123     
  Lines            51528   39652   -11876     
==============================================
- Hits             32384   27492    -4892     
+ Misses           17285   10911    -6374     
+ Partials          1859    1249     -610

Impacted Files	Coverage Δ
pkg/mcp/creds/watcher.go	`89% <ø> (+1%)`	⬆️
pilot/pkg/proxy/envoy/v2/eds.go	`79% <ø> (-3%)`	⬇️
pilot/pkg/networking/core/v1alpha3/listener.go	`82% <ø> (+7%)`	⬆️
pilot/pkg/proxy/envoy/v2/discovery.go	`72% <ø> (-7%)`	⬇️
pilot/pkg/proxy/envoy/v2/ep_filters.go	`83% <ø> (-2%)`	⬇️
pilot/pkg/proxy/envoy/v2/lds.go	`43% <ø> (-14%)`	⬇️
pilot/pkg/kube/inject/webhook.go	`83% <0%> (+1%)`	⬆️
pilot/pkg/networking/core/v1alpha3/httproute.go	`92% <100%> (+1%)`	⬆️
pilot/pkg/kube/inject/inject.go	`79% <50%> (-1%)`	⬇️
pilot/pkg/model/context.go	`68% <65%> (-1%)`	⬇️
... and 232 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 a363120...4d8dab5. Read the comment docs.

rshriram · 2018-12-04T01:36:35Z

install/kubernetes/helm/istio/templates/configmap.yaml

+    # example: '{"start_time": "%START_TIME%", "req_method": "%REQ(:METHOD)%"}'
+    # Leave empty to use default log format
+    accessLogFormat: '{{ .Values.global.proxy.accessLogFormat }}'
+


more bad merge?

hzxuzhonghu · 2018-12-04T01:41:31Z

pilot/pkg/model/context.go

+	// The list will be populated either from explicit declarations or using 'on-demand'
+	// feature, before generation takes place. Each node may have a different list, based on
+	// the requests handled by envoy.
+	serviceDependencies []*Service


A question: how this field can be updated? I can not find.

hzxuzhonghu · 2018-12-04T01:43:43Z

pkg/features/pilot/pilot.go

+	// egress and other mesh services to only hosts defined in same namespace or
+	// 'admin' namespaces. Using services from any other namespaces will require the new NetworkScope
+	// config. In most cases 'istio-system' should be included.
+	NetworkScopes = os.Getenv("DEFAULT_NAMESPACE_DEPENDENCIES")


Have more comments like format "ns1,ns2,ns3"

Ok ( this is a short-lived env - will be replaced by values.yaml before release - but should have comments)

hzxuzhonghu · 2018-12-04T01:45:54Z

pkg/features/pilot/pilot.go

+	// NodeMetadataNetwork defines the network the node belongs to. It is an optional metadata,
+	// set at injection time. When set, the Endpoints returned to a note and not on same network
+	// will be replaced with the gateway defined in the settings.
+	NodeMetadataNetwork = "NETWORK"


I planed to do this kind of work 👍

Sharing work is good :-)

hzxuzhonghu · 2018-12-04T01:47:16Z

pilot/pkg/serviceregistry/kube/controller.go

@@ -734,6 +734,8 @@ func (c *Controller) AppendInstanceHandler(f func(*model.ServiceInstance, model.
 			return nil
 		}
 		c.updateEDS(ep)
+
+		log.Debugf("Handle endpoint %s in namespace %s", ep.Name, ep.Namespace)


In c.updateEDS there is already a log of this.

remove that looks good

Ah, it moved. Ok.

hzxuzhonghu · 2018-12-04T01:50:35Z

pilot/pkg/proxy/envoy/v2/ads.go

 						// Not logging full request, can be very long.
 						adsLog.Debugf("ADS:RDS: ACK %s %s (%s) %s %s", peerAddr, con.ConID, con.modelNode.ID, discReq.VersionInfo, discReq.ResponseNonce)
 						// Already got a list of routes to watch and has same length as the request, this is an ack
-						if discReq.ResponseNonce != "" {
+						if discReq.ErrorDetail == nil && discReq.ResponseNonce != "" {


no need this

hzxuzhonghu · 2018-12-04T01:50:43Z

pilot/pkg/proxy/envoy/v2/ads.go

@@ -511,11 +496,11 @@ func (s *DiscoveryServer) StreamAggregatedResources(stream ads.AggregatedDiscove
 					sort.Strings(routes)
 					sort.Strings(con.Routes)

-					if reflect.DeepEqual(con.Routes, routes) {
+					if reflect.DeepEqual(con.Routes, routes) || len(routes) == 0 {


no need too

The 'ack' handling is pretty strange and may change with 'on-demand' - better to be safe.
My understanding is that 'on-demand' will use an ack that has only status code, doesn't include full routes. But I can move this to separate PR.

I see it was moved up ( had to resolve some conflicts when merging - I think that's the old code.)

hzxuzhonghu · 2018-12-05T01:27:08Z

pilot/pkg/model/push_context.go

+				res = append(res, s)
+			}
+		}
+		proxy.serviceDependencies = res


Since OnConnect is only called once per proxy, IIUC, serviceDependencies is set once and never updated.

Then this is not accurate.

Like, we create a ServiceEntry, how can this updated?

Ack, will fix in separate PR ( I have a TODO in pushConnection )

costinm · 2018-12-06T03:44:07Z

/test istio-pilot-e2e-envoyv2-v1alpha3

rshriram · 2018-12-06T21:18:54Z

/lgtm

istio-testing · 2018-12-06T21:19:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: costinm, rshriram

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:

~~pilot/OWNERS~~ [costinm,rshriram]
~~pkg/OWNERS~~ [costinm,rshriram]

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

istio-release-robot · 2018-12-07T00:04:53Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Review the full test history for this PR.
/lgtm cancel will stop retrying.

hzxuzhonghu · 2018-12-07T01:23:43Z

/hold

initConnectionNode--> OnConnect --> serviceDependencies set, but when no new proxy connect, then serviceDependencies can not be updated.

hzxuzhonghu · 2018-12-07T01:33:23Z

/hold cancel

notice this https://github.com/istio/istio/pull/9361/files#diff-ee39cd332931ab6f8c10713ce3446d09R333

hzxuzhonghu · 2018-12-07T01:33:49Z

/test e2e-dashboard

istio-testing · 2018-12-07T02:27:22Z

@costinm: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/e2e-simpleTests-cni.sh	`69a64fe`	link	`/test e2e-simpleTests-cni`
prow/build-tests.sh	`69a64fe`	link	`/test build-tests`
prow/istio-integ-k8s-tests.sh	`99b914b`	link	`/test istio-integ-k8s-tests`
prow/e2e-mixer-no_auth-mcp.sh	`d753538`	link	`/test e2e-mixer-no_auth-mcp`
prow/istio-pilot-multicluster-e2e.sh	`4d8dab5`	link	`/test istio-pilot-multicluster-e2e`

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. I understand the commands that are listed here.

Initial code to filter outbound listener

0534aff

costinm requested review from rshriram and louiscryan October 16, 2018 18:45

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Oct 16, 2018

istio-testing requested review from andraxylia and ZackButcher October 16, 2018 18:45

googlebot added the cla: yes label Oct 16, 2018

costinm added 2 commits October 17, 2018 11:36

Move the outbound list to Proxy

574605e

Merge branch 'master' of github.com:istio/istio into 11-graph

e374aef

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Oct 21, 2018

costinm added 3 commits October 24, 2018 14:37

Merge branch 'master' of github.com:istio/istio into 11-graph

444bd48

Merge branch 'master' of github.com:istio/istio into 11-graph

b79aa4d

Fix computation of outbound on connect.

0236030

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Oct 25, 2018

costinm added 7 commits October 31, 2018 07:18

Add 'isolate namespace' experiment, docs

d96bb0e

Merge branch 'master' of github.com:istio/istio into 11-graph

34d9738

Merge branch 'master' of github.com:istio/istio into 11-graph

6b3551e

Cleanup unused code

32587af

Cleanup

467495e

More comments, add deps

2c3629a

Merge branch 'master' of github.com:istio/istio into 11-graph

9a66598

costinm mentioned this pull request Nov 7, 2018

Limitations around TCP Services make Istio pretty unusable on larger multi-tenant clusters #9784

Closed

quanjielin and others added 2 commits November 9, 2018 09:47

fix typo (istio#9830)

3378c01

Merge branch 'master' of github.com:istio/istio into 11-graph

69a64fe

istio-testing added the approved label Nov 9, 2018

costinm added 4 commits November 13, 2018 09:09

Merge branch 'release-1.1' of github.com:istio/istio into 11-graph

e583847

Injection polish

de67032

Merge branch 'release-1.1' of github.com:istio/istio into 11-graph

d2f0a37

Merge branch 'release-1.1' of github.com:istio/istio into 11-graph

5bf8c5b

rshriram reviewed Dec 4, 2018

View reviewed changes

hzxuzhonghu reviewed Dec 4, 2018

View reviewed changes

costinm added 3 commits December 4, 2018 09:43

More review updates

e1bece0

Merge branch 'release-1.1' of github.com:istio/istio into 11-graph

d753538

Merge branch 'release-1.1' of github.com:istio/istio into 11-graph

6861539

hzxuzhonghu reviewed Dec 5, 2018

View reviewed changes

costinm added 3 commits December 5, 2018 10:28

Format

9924571

Fix lint - another pass by value

6cf05ff

Missed capitalization

4d8dab5

costinm requested a review from mandarjog December 6, 2018 18:32

istio-testing assigned rshriram Dec 6, 2018

istio-testing added the lgtm label Dec 6, 2018

istio-testing added the do-not-merge/hold Block automatic merging of a PR. label Dec 7, 2018

istio-testing removed the do-not-merge/hold Block automatic merging of a PR. label Dec 7, 2018

rshriram merged commit f30e345 into istio:release-1.1 Dec 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial code for NetworkScope - filter outbound listener/routes #9361

Initial code for NetworkScope - filter outbound listener/routes #9361

costinm commented Oct 16, 2018 •

edited

Loading

codecov bot commented Oct 25, 2018 •

edited

Loading

rshriram Dec 4, 2018

hzxuzhonghu Dec 4, 2018

costinm Dec 4, 2018

hzxuzhonghu Dec 4, 2018

costinm Dec 4, 2018

hzxuzhonghu Dec 4, 2018

costinm Dec 4, 2018

hzxuzhonghu Dec 4, 2018

hzxuzhonghu Dec 4, 2018

costinm Dec 4, 2018

hzxuzhonghu Dec 4, 2018

costinm Dec 4, 2018

hzxuzhonghu Dec 4, 2018

costinm Dec 4, 2018

costinm Dec 4, 2018

hzxuzhonghu Dec 5, 2018

hzxuzhonghu Dec 6, 2018

costinm Dec 7, 2018

costinm commented Dec 6, 2018

rshriram commented Dec 6, 2018

istio-testing commented Dec 6, 2018

istio-release-robot commented Dec 7, 2018

hzxuzhonghu commented Dec 7, 2018

hzxuzhonghu commented Dec 7, 2018

hzxuzhonghu commented Dec 7, 2018

istio-testing commented Dec 7, 2018

Initial code for NetworkScope - filter outbound listener/routes #9361

Initial code for NetworkScope - filter outbound listener/routes #9361

Conversation

costinm commented Oct 16, 2018 • edited Loading

codecov bot commented Oct 25, 2018 • edited Loading

Codecov Report

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

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

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

costinm commented Dec 6, 2018

rshriram commented Dec 6, 2018

istio-testing commented Dec 6, 2018

istio-release-robot commented Dec 7, 2018

hzxuzhonghu commented Dec 7, 2018

hzxuzhonghu commented Dec 7, 2018

hzxuzhonghu commented Dec 7, 2018

istio-testing commented Dec 7, 2018

costinm commented Oct 16, 2018 •

edited

Loading

codecov bot commented Oct 25, 2018 •

edited

Loading