Add helm flags to control the app probe rewrite beahvior. by incfly · Pull Request #10344 · istio/istio

incfly · 2018-12-07T19:47:23Z

istioctl kube-inject and auto injection has slightly different code path. The existing override changes only happen on istioctl level.

.Values.global.sidecarInjectorWebhook.rewriteAppHTTPProbe controls the sidecar injection template's option rewriteAppHTTPProbe. During injection, we'll then consult this flag to determine whether spec will be rewritten.

…s check. (istio#9266) * WIP injector change to modify istio-proxy. * move out to app_probe.go * Iterating sidecartmpl to find the statusPort. * use the same name for ready path. * Get rewrite work, almost. * Some clean up on test and check one container criteria. * fix the injected test file. * Add inject test for readiness probe itself. * Add missing added test file. * fix helm test. * fix lint. * update header based finding the port. * return to previous injected file status. * fixing TestIntoResource test. * sed fixing all remaining injecting files. * handling named port. * fixing merginge failure. * remove the debug print. * lint fixing. * Apply the suggestions for finding statusPort arg. * Address comments, regex support more port value format. * add app_probe_test.go * add more test. * merge fix the test.

…ease-1.1

probe.

…lth-flag

nmittler · 2018-12-12T17:56:46Z

istioctl/cmd/istioctl/kubeinject.go

 				if sidecarTemplate, err = inject.GenerateTemplateFromParams(&inject.Params{
 					InitImage:                    inject.InitImageName(hub, tag, debugMode),
 					ProxyImage:                   inject.ProxyImageName(hub, tag, debugMode),
+					RewriteAppHTTPProbe:          false,


should this be a flag?

nmittler · 2018-12-12T17:57:50Z

pilot/pkg/kube/inject/webhook.go

 		}
 	}

+	// TODO: plumbing wh.sidecarConfig.RewriteHTTPPRobe to the injectionData.


nit: add your github handle in the TODO

nmittler

/lgtm
/approve

costinm · 2018-12-12T20:25:16Z

install/kubernetes/helm/istio-remote/templates/sidecar-injector-configmap.yaml

 data:
  config: |-
    policy: {{ .Values.global.proxy.autoInject }}
+    rewriteAppHTTPProbe: {{ .Values.sidecarInjectorWebhook.rewriteAppHTTPProbe }}


Confused - why ?

Typically we use pod annotations to allow user to opt in/out of a specific feature.

costinm · 2018-12-12T20:25:32Z

install/kubernetes/helm/istio/values.yaml

 #
 sidecarInjectorWebhook:
  enabled: true
+  rewriteAppHTTPProbe: false


Lots of comments please.

Once again - after reverting please add comments explaining what it does. Nobody can understand - rewrite how, when to set, why, etc. Don't assume all users are familiar with the your implementation details and goals.

costinm · 2018-12-12T20:26:08Z

istioctl/cmd/istioctl/kubeinject.go

 	injectCmd.PersistentFlags().BoolVar(&enableCoreDump, "coreDump",
 		true, "Enable/Disable core dumps in injected Envoy sidecar (--coreDump=true affects "+
 			"all pods in a node and should only be used the cluster admin)")
+	injectCmd.PersistentFlags().BoolVar(&rewriteAppHTTPProbe, "rewriteAppProbe", false, "Whether injector "+


If it's a flag - why do you need it on the config map ?

mandarjog

/lgtm

istio-testing · 2018-12-12T23:05:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: incfly, mandarjog, nmittler

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:

~~install/OWNERS~~ [mandarjog]
~~istioctl/OWNERS~~ [nmittler]
~~pilot/OWNERS~~ [nmittler]

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

tiswanso · 2018-12-18T02:09:59Z

This PR is causing e2e-simple to fail when run with auto-sidecar injection. Seems like the new helm chart param sidecarInjectorWebhook.rewriteAppHTTPProbe should default to true (maybe only when auth is enabled?).

The istio-cni release-1.1 nightly is failing due to it.

costinm

Please revert - it seems to break stuff, the 'rewriteAppHTTPProbe is in different places ( under confg in istio-remote, under template in istio/templates.

costinm · 2018-12-18T16:50:34Z

pilot/pkg/kube/inject/app_probe_test.go

 		{
 			name: "one-container",
 			sidecar: &SidecarInjectionSpec{
+				RewriteAppHTTPProbe: true,


This doesn't make any sense - why would this be part of the spec, next to containers ?

costinm

As I mentioned in other PRs, having a global setting without a way to gradually rollout and test is extremely dangerous and should be avoided. We should certainly not set global options unless a feature
is extensively tested ( including by production users with real workloads, like @Stono )

Please add an annotation, so users can turn on for testing and off if they run into troubles for specific workloads.

nmittler · 2018-12-18T17:55:42Z

@incfly can you address the concerns above from @costinm?

incfly · 2018-12-18T18:08:58Z

I'll first take a look at the breakage first. rewriteAppHTTPProbe is default to false to retain the current behavior. Not sure where it introduce the delta.
It is possible to gradually rolled out. You don't turn on rewriteAppProbe and istio kube-inject has additional flag to control on per deployment base.
I do remember to add an e2e test for this. Look at some issues, go test -v ./tests/integration2/examples/pilot does not work #10524. Right now the helm flag is off everywhere.

tiswanso · 2018-12-18T18:09:22Z

To add info on what I've seen with this change... I think this has introduced a delta (at least for e2e-simple) for the e2e test framework run with sidecar injection enabled v. the default with istioctl kube-inject. I don't see how the e2e-simple would pass without the rewriteAppHTTPProbe=true so, I'm suspecting that the e2e test is running with it set for the istioctl kube-inject case. IMO, that delta in behavior is really bad and should be fixed by either changing the default for the flag or by having the makefile set it all the time for the e2e tests.

incfly · 2018-12-18T18:13:40Z

I filed bug #10557. Let's move AI(s) over there.

tiswanso · 2018-12-18T18:15:32Z

@incfly -- I think pilot/pkg/kube/inject/app_probe.go L51-53 is what changed the default behavior--at least compared to master. I'm not familiar with what was happening for the liveness probes prior to the existence of app_probe.go.

costinm · 2018-12-18T21:55:04Z

kube-inject is not an option - many users use automatic sidecar injector, and it is what we recommend.

Please use a pod annotation - so users can opt in and out per pod, like we do with many other features ( including mixer, policy, etc). There is not use case for having this as (only) a global setting.

…tio#10344)" This reverts commit f265b86.

Jianfei Hu added 6 commits November 28, 2018 12:55

Merge branch 'release-1.1' of https://github.com/istio/istio into rel…

6ea3a5a

…ease-1.1

Merge branch 'release-1.1' of https://github.com/istio/istio into rel…

68a1bb0

…ease-1.1

add flags for controling istioctl kube-inject on whether rewrite app

8264e41

probe.

fix typo.

6a0a4fe

Merge branch 'release-1.1' of https://github.com/istio/istio into hea…

1adcd35

…lth-flag

incfly requested review from costinm and nmittler December 7, 2018 19:47

istio-testing requested review from GregHanson and hzxuzhonghu December 7, 2018 19:47

incfly removed the request for review from GregHanson December 7, 2018 19:47

googlebot added the cla: yes label Dec 7, 2018

Jianfei Hu added 3 commits December 7, 2018 14:16

fix the typo.

55ef661

fix the app_probe_test.

7120b70

Merge branch 'release-1.1' of https://github.com/istio/istio into hea…

71a6a88

…lth-flag

nmittler reviewed Dec 12, 2018

View reviewed changes

passing flags from kubeinject.

a2a0df5

nmittler approved these changes Dec 12, 2018

View reviewed changes

istio-testing assigned nmittler Dec 12, 2018

istio-testing added the lgtm label Dec 12, 2018

costinm reviewed Dec 12, 2018

View reviewed changes

istio-testing assigned mandarjog Dec 12, 2018

mandarjog approved these changes Dec 12, 2018

View reviewed changes

istio-testing added the approved label Dec 12, 2018

hzxuzhonghu approved these changes Dec 13, 2018

View reviewed changes

istio-testing merged commit f265b86 into istio:release-1.1 Dec 13, 2018

incfly deleted the health-flag branch December 13, 2018 19:50

costinm reviewed Dec 18, 2018

View reviewed changes

incfly mentioned this pull request Dec 18, 2018

Potential cni e2e-simple breakage due to app http probe rewrite #10557

Closed

incfly pushed a commit to incfly/istio that referenced this pull request Dec 19, 2018

Revert "Add helm flags to control the app probe rewrite beahvior. (is…

f6f6635

…tio#10344)" This reverts commit f265b86.

incfly mentioned this pull request Dec 19, 2018

Revert #10344 #10582

Closed

ayj mentioned this pull request Mar 8, 2019

Deprecate non-working kube-inject options #12293

Closed

Conversation

incfly commented Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

nmittler left a comment

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!

mandarjog left a comment

Choose a reason for hiding this comment

Uh oh!

istio-testing commented Dec 12, 2018

Uh oh!

tiswanso commented Dec 18, 2018

Uh oh!

costinm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costinm left a comment

Choose a reason for hiding this comment

Uh oh!

nmittler commented Dec 18, 2018

Uh oh!

incfly commented Dec 18, 2018

Uh oh!

tiswanso commented Dec 18, 2018

Uh oh!

incfly commented Dec 18, 2018

Uh oh!

tiswanso commented Dec 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

costinm commented Dec 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

incfly commented Dec 7, 2018 •

edited

Loading

tiswanso commented Dec 18, 2018 •

edited

Loading