Allow traffic going outside the cluster by mdemirhan · Pull Request #1123 · knative/serving

mdemirhan · 2018-06-09T02:31:42Z

Changed the default installation to allow traffic going outside without getting terminated by Istio's sidecar.
Refactored config map informer in route controller to use a shared factory and reuse the same factory within the revision controller as well.
NOTE: k8sflag with Dynamic setting was tested; however it was very inconsistent in getting updates to the config map, missing more than half of the updates. Because of that, ConfigMap informers are used to get changes in the config map.

…hout getting terminated by Istio's sidecar. * Refactored config map informer in route controller to use a shared factory and reuse the same factory within the revision controller as well. NOTE: k8sflag with Dynamic setting was tested; however it was very inconsistent in getting updates to the config map, missing more than half of the updates. Because of that, ConfigMap informers are used to get changes in the config map.

mdemirhan · 2018-06-09T02:33:28Z

@vaikas-google and @grantr Funnily enough, we now need config map updates in another controller as well and I refactored my checkin today to do the thing I said we didn't need :) Please take a look.

My first shot at this change was to use k8sflag package but it was very frequently missing updates and never getting them. That is why k8sflag with Dynamic option is not used here.

knative-metrics-robot · 2018-06-09T02:33:51Z

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/controller/route/route.go	78.9%	78.9%	-0.0
pkg/controller/route/route_test.go	78.9%	78.9%	-0.0
pkg/controller/revision/ela_pod.go	92.0%	94.6%	2.6
pkg/controller/revision/revision.go	74.8%	76.0%	1.1
pkg/controller/revision/revision_test.go	74.8%	76.0%	1.1

*TestCoverage feature is being tested, do not rely on any info here yet

vaikas · 2018-06-09T20:24:34Z

 	elaInformerFactory := informers.NewSharedInformerFactory(elaClient, time.Second*30)
 	buildInformerFactory := buildinformers.NewSharedInformerFactory(buildClient, time.Second*30)
+	servingSystemInformerFactory := kubeinformers.NewFilteredSharedInformerFactory(kubeClient,
+		time.Minute*5, pkg.GetServingSystemNamespace(), nil)


Just curious where the 5 minutes comes from. Others are in 30 resync so just curious.

With other informers, we end up reconciling everything every 30 seconds. I didn't think that we needed that aggressive reconcilliation for domain configuration or the network configuration - those should change only once in the lifetime of the cluster if ever. Also, the reconcilliation is needed only if we somehow missed the original notification that gets sent when the configuration changes (I don't know how we would miss the notification - may be when there are successive deletes and creates the ordering might cause an issue?).

Let me know if you want me to change this to 30 seconds.

vaikas · 2018-06-09T20:25:32Z

+  namespace: knative-serving-system
+data:
+  # Specifies the IP ranges that Istio sidecar will intercept.
+  # Replace this with the IP ranges of your cluster.


Perhaps add an example of how to determine what this range is?

hehe, I see it now below. perhaps:
Replace this with the IP ranges of your cluster (see below for some examples).

or just leave it as is. Just when I was reading this the first time, I was like roh-roh?

I will add an extra (see below...) :)

vaikas · 2018-06-09T20:27:44Z

 	podTemplateAnnotations[sidecarIstioInjectAnnotation] = "true"

+	// Inject the IP ranges for istio sidecar configuration.
+	// We will inject this value only if all of the following is true:


following are true

vaikas · 2018-06-09T20:28:39Z

+	// Inject the IP ranges for istio sidecar configuration.
+	// We will inject this value only if all of the following is true:
+	// - the config map contains a non-empty value
+	// - the user doesn't specify this annotation in their configuration


'in their configuration', meaning the pod template? Perhaps clarify.

Yes, that is correct. Clarified in the comments.

vaikas · 2018-06-09T20:29:12Z

+	// - the user doesn't specify this annotation in their configuration
+	// - configured values are valid CIDR notation IP addresses
+	// If these conditions are not met, this value will be left untouched.
+	// * is a special value that is accepted as a valid.


What does * mean? It's valid, but is it obvious from Istio configuration? Does this mean all traffic is then intercepted?

Yes, that is correct. I clarified in the comments.

knative-metrics-robot · 2018-06-09T20:37:42Z

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/controller/route/route.go	78.9%	78.9%	-0.0
pkg/controller/route/route_test.go	78.9%	78.9%	-0.0
pkg/controller/revision/ela_pod.go	92.0%	94.6%	2.6
pkg/controller/revision/revision.go	74.8%	75.7%	0.9
pkg/controller/revision/revision_test.go	74.8%	75.7%	0.9

*TestCoverage feature is being tested, do not rely on any info here yet

knative-metrics-robot · 2018-06-09T20:47:38Z

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/controller/route/route.go	78.9%	78.9%	-0.0
pkg/controller/route/route_test.go	78.9%	78.9%	-0.0
pkg/controller/revision/revision.go	74.8%	75.7%	0.9
pkg/controller/revision/revision_test.go	74.8%	75.7%	0.9
pkg/controller/revision/ela_pod.go	92.0%	94.6%	2.6

*TestCoverage feature is being tested, do not rely on any info here yet

vaikas · 2018-06-09T21:15:41Z

/lgtm

josephburnett · 2018-06-11T15:41:01Z

My first shot at this change was to use k8sflag package but it was very frequently missing updates and never getting them. That is why k8sflag with Dynamic option is not used here.

Can you tell me more about this? It does take a minute for the kubelet to pickup config map changes and update the mounted filesystem.

# Conflicts: # pkg/controller/service/service.go

knative-metrics-robot · 2018-06-12T19:06:20Z

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/controller/route/route.go	78.2%	78.0%	-0.3
pkg/controller/route/route_test.go	78.2%	78.0%	-0.3
pkg/controller/revision/pod.go	92.0%	94.6%	2.6
pkg/controller/revision/revision.go	74.3%	75.2%	1.0
pkg/controller/revision/revision_test.go	74.3%	75.2%	1.0

*TestCoverage feature is being tested, do not rely on any info here yet

vaikas · 2018-06-12T19:26:38Z

/lgtm
/approve

google-prow-robot · 2018-06-12T19:26:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mdemirhan, vaikas-google

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:

~~OWNERS~~ [vaikas-google]

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

knative-metrics-robot · 2018-06-12T19:28:41Z

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/controller/route/route.go	78.0%	78.2%	0.2
pkg/controller/route/route_test.go	78.0%	78.2%	0.2
pkg/controller/revision/pod.go	92.0%	94.6%	2.6
pkg/controller/revision/revision.go	74.3%	75.2%	1.0
pkg/controller/revision/revision_test.go	74.3%	75.2%	1.0

*TestCoverage feature is being tested, do not rely on any info here yet

google-prow-robot · 2018-06-12T19:46:53Z

@mdemirhan: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-knative-serving-go-coverage	`f955a8a`	link	`/test pull-knative-serving-go-coverage`

Full PR test history. Your PR dashboard.

Details

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.

* Add update-ci to generate ci configs * Update README * Exclude mutli and init container image * Update to better message

mdemirhan requested review from grantr, tcnghia and vaikas June 9, 2018 02:31

google-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 9, 2018

vaikas reviewed Jun 9, 2018

View reviewed changes

* Require network config map to exist before starting the controller

cb13a4e

* Address PR feedback.

03c4037

google-prow-robot assigned vaikas Jun 9, 2018

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 9, 2018

mdemirhan mentioned this pull request Jun 11, 2018

The stock-rest-app sample can't access stock quotes web service #872

Closed

Merge branch 'master-upstream' into istioegress2

f955a8a

# Conflicts: # pkg/controller/service/service.go

google-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 12, 2018

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 12, 2018

google-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 12, 2018

mdemirhan merged commit ad03dbb into knative:master Jun 12, 2018

mdemirhan deleted the istioegress2 branch June 13, 2018 19:21

tcnghia mentioned this pull request Oct 19, 2018

Add mdemirhan to /OWNERS #2264

Merged

nak3 added a commit to nak3/serving that referenced this pull request Jun 2, 2022

Add update-ci to generate ci configs automatically (knative#1123)

de2e573

* Add update-ci to generate ci configs * Update README * Exclude mutli and init container image * Update to better message

Conversation

mdemirhan commented Jun 9, 2018

Uh oh!

mdemirhan commented Jun 9, 2018

Uh oh!

knative-metrics-robot commented Jun 9, 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!

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!

knative-metrics-robot commented Jun 9, 2018

Uh oh!

knative-metrics-robot commented Jun 9, 2018

Uh oh!

vaikas commented Jun 9, 2018

Uh oh!

josephburnett commented Jun 11, 2018

Uh oh!

knative-metrics-robot commented Jun 12, 2018

Uh oh!

vaikas commented Jun 12, 2018

Uh oh!

google-prow-robot commented Jun 12, 2018

Uh oh!

knative-metrics-robot commented Jun 12, 2018

Uh oh!

google-prow-robot commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

google-prow-robot commented Jun 12, 2018 •

edited

Loading