Enable Istio Sidecar injection for activator by akyyy · Pull Request #833 · knative/serving

akyyy · 2018-05-08T17:21:22Z

Fixes #838

Proposed Changes

Enable istio sidecar injection for activator.
Since istio does not support direct pod ip access unable to connect to in-mesh TCP ports using pod ip directly istio/istio#3398, we need to use FQDN when proxy requests.

google-prow-robot · 2018-05-08T17:21:28Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

Assign the PR to them by writing /assign @vaikas-google 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:

OWNERS

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

ZhiminXiang · 2018-05-08T17:25:00Z

 kind: Namespace
 metadata:
+  labels:
+    istio-injection: enabled


I am wondering if this will enable istio sidecar injection for other pods in namespace ela-system. I guess we only want to inject sidecar for activator and autoscaler (maybe).

The particular deployment needs a special annotation to get istio sidecar enabled.
annotations:
sidecar.istio.io/inject: "true"

so we should be fine.

mdemirhan · 2018-05-08T17:49:33Z

 	}
-	if len(endpoint.Subsets[0].Ports) != 1 {
-		return nil, fmt.Errorf("need just one port. Found %v ports", len(endpoint.Subsets[0].Ports))
+	svc, err = services.Get(controller.GetElaK8SServiceNameForRevision(revision), metav1.GetOptions{})


Any reason this is called a second time here? (same call is made on line 97 as well)

oops. That's an error.

mdemirhan · 2018-05-08T17:51:21Z

-	if len(endpoint.Subsets[0].Ports) != 1 {
-		return nil, fmt.Errorf("need just one port. Found %v ports", len(endpoint.Subsets[0].Ports))
+	svc, err = services.Get(controller.GetElaK8SServiceNameForRevision(revision), metav1.GetOptions{})
+	if len(svc.Spec.Ports) != 1 {


Do we need to restrict this to only one port? What if more ports are added in the future (for example, when we enable mutual TLS, we might need to add a second port for health checks, separate from the serving port) - it will require changes in this code as well. Can this instead look for a specific named port?

I opened an issue for this #837.

mdemirhan · 2018-05-08T17:52:44Z


 func (a *Activator) proxyRequest(revRequest RevisionRequest, serviceURL *url.URL) {
-	glog.Infof("Sending a proxy request to %q", serviceURL)
+	glog.Infof("Sending the request to %q", serviceURL)


I recommend eventually removing this. This will easily get very verbose. This is the perfect place to integrate to the distributed tracing :)

Deleted this log line. There is an issue to track tracing/metrics for activator #743.

mdemirhan · 2018-05-08T17:54:10Z

 		t.Errorf("Error in getRevisionTargetURL %v", err)
 	}
-	expectedURL := "http://abc:1234"
+	expectedURL := "http://test-rev-service.default.svc.cluster.local:1234"


we probably should have a check here that tests that the request host is also empty.

akyyy · 2018-05-08T17:54:26Z

+	// https://github.com/elafros/elafros/blob/2b3ee4f3c46118b3759aa95d9e6c41747c32d6c5/pkg/controller/route/ela_ingress.go#L38
+	// Since host values like "route-example.default.demo-domain.com" do not exist, we need to
+	// clear it to make istio sidecar happy if it's injected.
+	revRequest.r.Host = ""


@mattmoor Is this the best thing to do to overcome 404's?

cc @tcnghia

akyyy

Nghia syned up with me. He thinks the right fix could be additional route rules or check with istio team. I'll try those first.

akyyy · 2018-05-08T21:05:30Z


 func (a *Activator) proxyRequest(revRequest RevisionRequest, serviceURL *url.URL) {
-	glog.Infof("Sending a proxy request to %q", serviceURL)
+	glog.Infof("Sending the request to %q", serviceURL)


Deleted this log line. There is an issue to track tracing/metrics for activator #743.

akyyy · 2018-05-08T21:10:50Z

-	if len(endpoint.Subsets[0].Ports) != 1 {
-		return nil, fmt.Errorf("need just one port. Found %v ports", len(endpoint.Subsets[0].Ports))
+	svc, err = services.Get(controller.GetElaK8SServiceNameForRevision(revision), metav1.GetOptions{})
+	if len(svc.Spec.Ports) != 1 {


I opened an issue for this #837.

akyyy · 2018-05-08T21:14:46Z

 	}
-	if len(endpoint.Subsets[0].Ports) != 1 {
-		return nil, fmt.Errorf("need just one port. Found %v ports", len(endpoint.Subsets[0].Ports))
+	svc, err = services.Get(controller.GetElaK8SServiceNameForRevision(revision), metav1.GetOptions{})


oops. That's an error.

google-prow-robot · 2018-05-10T20:16:22Z

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

Test name	Commit	Details	Rerun command
pull-elafros-elafros-test	`8861351`	link	`/test pull-elafros-elafros-test`

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.

akyyy · 2018-05-16T21:18:09Z

Nicole took over the issue and she will work on this. Thanks!

nikkithurmond · 2018-05-24T23:27:19Z

I'm continuing this PR in #966

mattmoor · 2018-06-02T04:48:40Z

Is this subsumed by #966?

akyyy · 2018-06-06T20:40:31Z

This has been replaced by #966

* Remove config/domainmapping * Add 0.23 label

… (knative#836) * Add root ca for Controller HA test with https (knative#11471) This patch adds `test.AddRootCAtoTransport` for prober in TestControllerHA. * stick serverless-operator branch * Revert "stick serverless-operator branch" This reverts commit d2be74a. * Add v0.23 label to manifests (knative#833) * Remove config/domainmapping * Add 0.23 label * Update label * Add back DomainMapping

akyyy added 2 commits May 8, 2018 10:17

enable sidecar injection for activator

8524b09

Merge remote-tracking branch 'upstream/master' into sidecar

da609ad

akyyy requested a review from josephburnett as a code owner May 8, 2018 17:21

akyyy requested a review from a team May 8, 2018 17:21

google-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 8, 2018

akyyy requested review from ZhiminXiang, mattmoor, mdemirhan and tcnghia May 8, 2018 17:21

ZhiminXiang reviewed May 8, 2018

View reviewed changes

akyyy requested a review from ian-mi May 8, 2018 17:41

mdemirhan suggested changes May 8, 2018

View reviewed changes

akyyy commented May 8, 2018

View reviewed changes

akyyy added the WIP label May 8, 2018

why 404?

8861351

akyyy mentioned this pull request May 9, 2018

Activator in Istio mesh #838

Closed

akyyy added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 10, 2018

This was referenced May 24, 2018

Host must be cleared to avoid 404's #964

Closed

Enable Istio Sidecar injection for activator #966

Merged

mattmoor added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed WIP labels Jun 2, 2018

akyyy closed this Jun 6, 2018

markusthoemmes pushed a commit to markusthoemmes/knative-serving that referenced this pull request Jul 1, 2021

Add v0.23 label to manifests (knative#833)

90b639c

* Remove config/domainmapping * Add 0.23 label

nak3 added a commit to nak3/serving that referenced this pull request Jul 1, 2021

Add v0.23 label to manifests (knative#833)

a48cbb5

* Remove config/domainmapping * Add 0.23 label

Conversation

akyyy commented May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

google-prow-robot commented May 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akyyy May 8, 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!

akyyy May 8, 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!

akyyy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akyyy May 8, 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!

google-prow-robot commented May 10, 2018

Uh oh!

akyyy commented May 16, 2018

Uh oh!

nikkithurmond commented May 24, 2018

Uh oh!

mattmoor commented Jun 2, 2018

Uh oh!

akyyy commented Jun 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

akyyy commented May 8, 2018 •

edited

Loading

akyyy May 8, 2018 •

edited

Loading

akyyy May 8, 2018 •

edited

Loading

akyyy May 8, 2018 •

edited

Loading