injector changes for health check, pilot agent take over app readines… by incfly · Pull Request #10175 · istio/istio

incfly · 2018-11-28T20:57:31Z

CP #9266 in release 1.1. Pilot agent status server part is already in this branch.

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.

…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.

incfly · 2018-11-28T20:59:29Z

/cc @liminw

FYI after this merge, release 1.1 mtls can work with liveness and readiness probe.

incfly · 2018-11-28T22:56:25Z

/test e2e-mixer-no_auth

costinm · 2018-11-29T00:46:39Z

istioctl/cmd/istioctl/kubeinject.go

 	injectCmd.PersistentFlags().StringVar(&imagePullPolicy, "imagePullPolicy", inject.DefaultImagePullPolicy,
 		"Sets the container image pull policy. Valid options are Always,IfNotPresent,Never."+
 			"The default policy is IfNotPresent.")
-	injectCmd.PersistentFlags().IntVar(&statusPort, "statusPort", inject.DefaultStatusPort,


I don't think we can make backward incompatible flag changes.
Keep old flag, add new if needed.

This is the same flag. I just refactor the flag name with inject.StatusPortCmdFlagName which equals to "statusPort".

costinm · 2018-11-29T00:48:47Z

pilot/pkg/kube/inject/app_probe.go

+	statusPortPattern = regexp.MustCompile(fmt.Sprintf(`^-{1,2}%s(=(?P<port>\d+))?$`, StatusPortCmdFlagName))
+)
+
+func rewriteAppHTTPProbe(spec *SidecarInjectionSpec, podSpec *corev1.PodSpec) {


For new (and risky) features - please add a flag or env variable to disable the behavior (that's a generic requirement)

costinm

In general lgtm, but please add env variable or flag, and setting in values.yaml.
I think it should be disabled by default since it changes the behavior, and existing users without mtls should not be affected ( same for mtls users using different port ).

Should this be opt-in at pod level ?

incfly · 2018-11-30T21:32:29Z

I'll do helm install option in a spearate PR on master and then cherrypick that one here.

…ease-1.1

codecov · 2018-12-06T23:03:28Z

Codecov Report

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

@@              Coverage Diff               @@
##           release-1.1   #10175     +/-   ##
==============================================
+ Coverage           64%      70%     +7%     
==============================================
  Files              559      443    -116     
  Lines            50830    41529   -9301     
==============================================
- Hits             32120    28989   -3131     
+ Misses           16904    11134   -5770     
+ Partials          1806     1406    -400

Impacted Files	Coverage Δ
istioctl/cmd/istioctl/kubeinject.go	`46% <100%> (ø)`	⬆️
pilot/pkg/kube/inject/inject.go	`80% <100%> (+1%)`	⬆️
pilot/pkg/kube/inject/app_probe.go	`80% <80%> (ø)`
mixer/adapter/inventory.gen.go	`0% <0%> (-100%)`	⬇️
mixer/adapter/rbac/rbac.go	`0% <0%> (-72%)`	⬇️
pilot/pkg/serviceregistry/memory/discovery.go	`57% <0%> (-29%)`	⬇️
pilot/pkg/proxy/envoy/discovery.go	`0% <0%> (-29%)`	⬇️
mixer/template/template.gen.go	`0% <0%> (-28%)`	⬇️
mixer/adapter/stackdriver/stackdriver.go	`50% <0%> (-26%)`	⬇️
mixer/adapter/cloudwatch/cloudwatch.go	`58% <0%> (-23%)`	⬇️
... and 170 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 360fb67...68a1bb0. Read the comment docs.

incfly · 2018-12-06T23:09:32Z

I've already have an PR on top of this one ready to do a helm flag guarded. https://github.com/incfly/istio/compare/release-1.1...incfly:health-flag?expand=1

Let's get this in first and I'll send out the other PR separately. This way we can avoid the conflicts we'll have when merging back to master.

PTAL, @costinm @nmittler @hzxuzhonghu

incfly · 2018-12-07T00:41:02Z

/test e2e-dashboard
/test e2e-mixer-no_auth

hzxuzhonghu · 2018-12-07T01:41:14Z

/lgtm

nmittler · 2018-12-07T18:18:20Z

/lgtm
/approve

istio-testing · 2018-12-07T18:18:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu, incfly, 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:

~~istioctl/OWNERS~~ [nmittler]
~~pilot/OWNERS~~ [hzxuzhonghu,nmittler]

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

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

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

Test name	Commit	Details	Rerun command
prow//e2e-simpleTests-minProfile.sh	`cc0c84a`	link	`/test e2e-simpleTestsMinProfile`

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.

incfly · 2018-12-07T18:28:07Z

/test istio-unit-tests

* [incubator/jaeger] fix hotrod demo application Signed-off-by: Sébastien Prud'homme <sebastien.prudhomme@gmail.com> * [incubator/jaeger} bump version Signed-off-by: Sébastien Prud'homme <sebastien.prudhomme@gmail.com>

googlebot added the cla: yes label Nov 28, 2018

istio-testing requested review from nmittler and rshriram November 28, 2018 20:57

incfly requested review from hzxuzhonghu and removed request for rshriram November 28, 2018 20:57

istio-testing requested a review from liminw November 28, 2018 20:59

costinm reviewed Nov 29, 2018

View reviewed changes

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

6ea3a5a

…ease-1.1

incfly mentioned this pull request Dec 6, 2018

Feature: Application liveness and readiness probes with mTLS is enabled. #9150

Closed

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

68a1bb0

…ease-1.1

incfly force-pushed the release-1.1 branch from 8264e41 to 68a1bb0 Compare December 6, 2018 23:04

istio-testing assigned hzxuzhonghu Dec 7, 2018

istio-testing added the lgtm label Dec 7, 2018

hzxuzhonghu approved these changes Dec 7, 2018

View reviewed changes

istio-testing assigned nmittler Dec 7, 2018

istio-testing added the approved label Dec 7, 2018

istio-testing merged commit a2ad0d1 into istio:release-1.1 Dec 7, 2018

incfly mentioned this pull request Dec 18, 2018

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

Closed

Conversation

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

Uh oh!

Uh oh!

incfly commented Nov 28, 2018

Uh oh!

incfly commented Nov 28, 2018

Uh oh!

costinm Nov 29, 2018

Choose a reason for hiding this comment

Uh oh!

incfly Nov 29, 2018

Choose a reason for hiding this comment

Uh oh!

costinm Nov 29, 2018

Choose a reason for hiding this comment

Uh oh!

costinm left a comment

Choose a reason for hiding this comment

Uh oh!

incfly commented Nov 30, 2018

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

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

Uh oh!

Uh oh!

incfly commented Dec 7, 2018

Uh oh!

hzxuzhonghu commented Dec 7, 2018

Uh oh!

nmittler commented Dec 7, 2018

Uh oh!

istio-testing commented Dec 7, 2018

Uh oh!

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

Uh oh!

Uh oh!

incfly commented Dec 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

incfly commented Nov 28, 2018 •

edited

Loading

codecov bot commented Dec 6, 2018 •

edited

Loading

incfly commented Dec 6, 2018 •

edited

Loading

istio-testing commented Dec 7, 2018 •

edited

Loading