Add redirect params as annotations to pod resolves issue #12906 #14171

john-a-joyce · 2019-05-16T18:17:33Z

When Helm is used to specify additional redirection parameters they are passed to the istio_init container via command line args. When the CNI is used there is normally not an istio_init container and these redirection parameters are lost. The CNI already has logic to handle all of these additional redirection parameters when applied directly to the pod spec. This PR enhances the istio kube injection logic so any parameters specified via Helm values are added to the pod spec during the injection process (only when cni is enabled) so Istio CNI will perform the proper actions.

golangcibot · 2019-05-16T18:19:17Z

pilot/pkg/kube/inject/inject.go

@@ -947,3 +957,27 @@ func potentialPodName(metadata *metav1.ObjectMeta) string {
 	}
 	return ""
 }
+
+// rewriteCniPodSPec will check if values from the sidecar injector Helm
+// values need to be inserted as Pod annotations so the CNI will apply 


File is not gofmt-ed with -s (from gofmt)

golangcibot · 2019-05-16T18:30:19Z

pilot/pkg/kube/inject/inject.go

+	if len(spec.CniExtraConfig) == 0 {
+		return nil
+	}
+	for k, _ := range annotationRegistry {


File is not gofmt-ed with -s (from gofmt)

tiswanso · 2019-05-16T19:39:06Z

Could you change the subject to something like "Add redirect params as annotations to pod" or something more descriptive?

tiswanso · 2019-05-16T19:40:06Z

install/kubernetes/helm/istio/files/injection-template.yaml

+{{- end }}
+{{- if .Values.istio_cni.enabled }}


IMO we should always add these values as annotations if they exist in the proxyConfig.

Agree, I don't think there is anything wrong with additional annotations?

Well from a quick check if we add the annotations to the pods for all cases then I have to modify 90% of the test cases. I don't have a ton of time in the next week due to kubecon so wondering if it is worth the effort.

set REFRESH_GOLDEN=1 in your env does it automatically if it is the tests I am thinking of. Just use with caution, don't blindly accept the output our the tests are really testing anything.

I don't care too much one way or another though

golangcibot · 2019-05-16T20:02:23Z

pilot/pkg/kube/inject/inject.go

+			if annotations[k] == spec.CniExtraConfig[k] {
+				continue
+			} else if annotations[k] != "" {
+				err = fmt.Errorf("Helm redirection inconsistent with pod spec annotation %s", annotations[k])


error strings should not be capitalized or end with punctuation or a newline (from golint)

howardjohn · 2019-05-16T20:25:28Z

install/kubernetes/helm/istio/files/injection-template.yaml

+{{- end }}
+{{- if .Values.istio_cni.enabled }}


Agree, I don't think there is anything wrong with additional annotations?

howardjohn · 2019-05-16T20:26:56Z

pilot/pkg/kube/inject/inject.go

+// rewriteCniPodSPec will check if values from the sidecar injector Helm
+// values need to be inserted as Pod annotations so the CNI will apply
+// the proper redirection rules.
+func rewriteCniPodSPec(annotations map[string]string, spec *SidecarInjectionSpec) error {


Need some tests. Should be as simple as adding a couple yamls using cni to the testdata directory

Starting looking to add some tests, but I don't see an example for how to make things conditional in the current test cases based helm values. I didn't search that hard yet but if there is another case or parameter to use as a recipe let me know. I might not get to this for a few days due to kubecon travel.

golangcibot · 2019-05-21T06:14:25Z

pilot/pkg/kube/inject/inject.go

+// rewriteCniPodSPec will check if values from the sidecar injector Helm
+// values need to be inserted as Pod annotations so the CNI will apply
+// the proper redirection rules.
+func rewriteCniPodSPec(annotations map[string]string, spec *SidecarInjectionSpec)  {


File is not gofmt-ed with -s (from gofmt)

tiswanso · 2019-05-22T08:02:13Z

install/kubernetes/helm/istio/files/injection-template.yaml

+{{- end }}
+podRedirectAnnot:


With merge of #13970 need to add traffic.sidecar.istio.io/excludeOutboundPorts as well.

I can add here, but the change is missing on the CNI side so it won't change any behavior until that change goes in.

Right... it will just help so when we do add the functionality to istio/cni this will already be in place.

tiswanso · 2019-05-22T08:03:08Z

looks good but need to incorporate: traffic.sidecar.istio.io/excludeOutboundPorts (#13970)

This change resolves issue 12906 by adding support for include and exclude CIDRs and ports that are specified via Helm options when the CNI is enabled.

Tests are updated to reflect the annotations being added regardless of cni being enabled.

howardjohn · 2019-05-22T17:01:57Z

pilot/pkg/kube/inject/testdata/inject/traffic-annotations-wildcards.yaml.injected

        sidecar.istio.io/status: '{"version":"","initContainers":["istio-init"],"containers":["istio-proxy"],"volumes":["istio-envoy","istio-certs"],"imagePullSecrets":null}'
-        traffic.sidecar.istio.io/excludeInboundPorts: 4,5,6
+        traffic.sidecar.istio.io/excludeInboundPorts: 4,5,6,15020


This will not impact non-CNI users, right? This annotation is read at inject time only, so this change does nothing (unless using CNI)?

AFAIK, this is the post-injected app yaml under test so it's just validating that the annotation is getting added with the correct value.

The current template logic merges the helm values and the annotations and sends that as parameters to the istio_init container. This PR will merge the helm and annotation and write back to annotation so CNI can use. So it doesn't change the non-CNI case in anyway

john-a-joyce · 2019-05-28T18:31:16Z

/test istio-pilot-e2e-envoyv2-v1alpha3-master

john-a-joyce · 2019-05-29T15:12:42Z

@tiswanso @howardjohn - i think this is ready to go after a rebase. Please have a look if you can.

howardjohn

/lgtm

howardjohn · 2019-06-09T17:39:58Z

install/kubernetes/helm/istio/files/injection-template.yaml

+   traffic.sidecar.istio.io/includeInboundPorts: "{{ annotation .ObjectMeta `traffic.sidecar.istio.io/includeInboundPorts` (includeInboundPorts .Spec.Containers) }}"
+   traffic.sidecar.istio.io/excludeInboundPorts: "{{ excludeInboundPort (annotation .ObjectMeta `status.sidecar.istio.io/port` .Values.global.proxy.statusPort) (annotation .ObjectMeta `traffic.sidecar.istio.io/excludeInboundPorts` .Values.global.proxy.excludeInboundPorts) }}"
+{{ if or (isset .ObjectMeta.Annotations `traffic.sidecar.istio.io/excludeOutboundPorts`) (ne .Values.global.proxy.excludeOutboundPorts "") }}


Any reason that this is wrapped in the if but the others are not?

@howardjohn - I was matching the logic that is used when setting the parameters passed to the init container when the init container is doing the redirection. On reflection it was done that way for the init container to set up the extra option flag and you are correct I didn't really need the if here.

istio-testing · 2019-06-09T17:40:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: howardjohn, john-a-joyce

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:

~~install/OWNERS~~ [howardjohn]
~~pilot/OWNERS~~ [howardjohn]

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

howardjohn · 2019-06-09T19:29:35Z

/test e2e-simpleTests-master

istio-testing · 2019-06-09T20:25:31Z

@john-a-joyce: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/integ-pilot-k8s-presubmit-tests.sh	`7a87d08`	link	`/test integ-pilot-k8s-presubmit-tests-master`
prow/integ-mixer-k8s-presubmit-tests.sh	`7a87d08`	link	`/test integ-mixer-k8s-presubmit-tests-master`
prow/integ-security-k8s-presubmit-tests.sh	`7a87d08`	link	`/test integ-security-k8s-presubmit-tests-master`

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 support for annotations when CNI enabled This change resolves issue 12906 by adding support for include and exclude CIDRs and ports that are specified via Helm options when the CNI is enabled. * Fix some issues found during testing * Fix lint * More lint * Improve corner case * Add annotations do the pods for all cases Tests are updated to reflect the annotations being added regardless of cni being enabled. * Remove error as return value * Lint fixes * Add support for outbound exclude ports. (cherry picked from commit 6b43aad)

* Sync changes to old installer Cherrypicks istio#13392, istio#13666, istio#13982, istio#14077, istio#14059 * Cherrypick istio#14301 * Cherrypick istio#13638 * cherrypick istio#14606 * istio#14171 * istio#14438 * istio#14674 * istio#14974 * 14904 * istio#14796 * istio#15346 * istio#15345 * istio#15383 * istio#14815 * istio#15690 * istio#15681 * istio#15014 * istio#15503 * istio#16084 * istio#16146 * istio#16147 * Fix tests

googlebot added the cla: yes label May 16, 2019

istio-testing requested review from andraxylia and sdake May 16, 2019 18:17

john-a-joyce added the area/environments/installer label May 16, 2019

golangcibot reviewed May 16, 2019

View reviewed changes

tiswanso reviewed May 16, 2019

View reviewed changes

golangcibot reviewed May 16, 2019

View reviewed changes

howardjohn reviewed May 16, 2019

View reviewed changes

john-a-joyce changed the title ~~This PR is to resolve issue #12906~~ Add redirect params as annotations to pod resolves issue #12906 May 17, 2019

howardjohn added this to the 1.2 milestone May 17, 2019

golangcibot reviewed May 21, 2019

View reviewed changes

tiswanso reviewed May 22, 2019

View reviewed changes

john-a-joyce added 8 commits May 22, 2019 12:57

Add support for annotations when CNI enabled

a909481

This change resolves issue 12906 by adding support for include and exclude CIDRs and ports that are specified via Helm options when the CNI is enabled.

Fix some issues found during testing

5759065

Fix lint

91a66b9

More lint

c66be14

Improve corner case

7aaf727

Add annotations do the pods for all cases

38a78ff

Tests are updated to reflect the annotations being added regardless of cni being enabled.

Remove error as return value

de8a959

Lint fixes

b932e18

howardjohn reviewed May 22, 2019

View reviewed changes

john-a-joyce force-pushed the iss-12906 branch from d9baf77 to 2845672 Compare May 28, 2019 16:16

Add support for outbound exclude ports.

7a87d08

john-a-joyce force-pushed the iss-12906 branch from 2845672 to 7a87d08 Compare May 28, 2019 17:17

istio-testing assigned howardjohn Jun 9, 2019

howardjohn approved these changes Jun 9, 2019

View reviewed changes

istio-testing added the lgtm label Jun 9, 2019

istio-testing added the approved label Jun 9, 2019

istio-testing merged commit 6b43aad into istio:master Jun 9, 2019

john-a-joyce mentioned this pull request Jun 12, 2019

excludeOutboundPorts to facilitate headless data stores istio/cni#117

Merged

john-a-joyce deleted the iss-12906 branch June 13, 2019 19:13

tiswanso added the action/merge-to-release-branch label Jun 14, 2019

mergify bot mentioned this pull request Jun 14, 2019

Add redirect params as annotations to pod resolves issue #12906 (bp #14171) #14858

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add redirect params as annotations to pod resolves issue #12906 #14171

Add redirect params as annotations to pod resolves issue #12906 #14171

john-a-joyce commented May 16, 2019

golangcibot May 16, 2019

golangcibot May 16, 2019

tiswanso commented May 16, 2019

tiswanso May 16, 2019

howardjohn May 16, 2019

john-a-joyce May 17, 2019

howardjohn May 17, 2019

golangcibot May 16, 2019

howardjohn May 16, 2019

howardjohn May 16, 2019

john-a-joyce May 17, 2019

golangcibot May 21, 2019

tiswanso May 22, 2019

john-a-joyce May 22, 2019

tiswanso May 23, 2019

tiswanso commented May 22, 2019

howardjohn May 22, 2019

tiswanso May 23, 2019

john-a-joyce May 23, 2019

john-a-joyce commented May 28, 2019

john-a-joyce commented May 29, 2019

howardjohn left a comment

howardjohn Jun 9, 2019

john-a-joyce Jun 10, 2019

istio-testing commented Jun 9, 2019

howardjohn commented Jun 9, 2019

istio-testing commented Jun 9, 2019

		{{- end }}
		{{- if .Values.istio_cni.enabled }}

		{{- end }}
		{{- if .Values.istio_cni.enabled }}

		{{- end }}
		podRedirectAnnot:

Add redirect params as annotations to pod resolves issue #12906 #14171

Add redirect params as annotations to pod resolves issue #12906 #14171

Conversation

john-a-joyce commented May 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiswanso commented May 16, 2019

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

tiswanso commented May 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-a-joyce commented May 28, 2019

john-a-joyce commented May 29, 2019

howardjohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

istio-testing commented Jun 9, 2019

howardjohn commented Jun 9, 2019

istio-testing commented Jun 9, 2019