Enable the config validation webhook in istiod (3 of 3) by ayj · Pull Request #19985 · istio/istio

ayj · 2020-01-08T01:14:31Z

Part 3 of 3 in a set of changes to unify webhook validation.

This PR enables Pilot to provide config validation in the absence of Galley. It includes the following changes:

Pilot now starts the webhook validation server and optionally the corresponding controller for config reconciliation.
The injection and validation webhook handlers are now served by the same https server.
The validatingwebhookconfiguration was made more concise and renamed to not be component specific.

ayj · 2020-01-08T01:16:13Z

This PR is not ready for review yet. I misclicked when switching to a draft PR.

howardjohn

lgtm overall, just nitpicks and the PR says WIP so not pressing approve yet

pilot/pkg/bootstrap/options.go

pilot/pkg/bootstrap/server.go

manifests/istio-control/istio-discovery/templates/deployment.yaml

pilot/pkg/bootstrap/certcontroller.go

pilot/pkg/bootstrap/sidecarinjector.go

googlebot · 2020-01-16T20:09:41Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

tests/integration/galley/webhook/webhook_test.go

…hook-validation-pilot-use-new-controller-server

howardjohn · 2020-01-18T03:22:47Z

pkg/test/framework/components/istio/config.go

 }

+func (c *Config) IsIstiodEnabled() bool {
+	return c.Values["global.istiod.enabled"] == "true"


Suggested change

return c.Values["global.istiod.enabled"] == "true"

if c.Operator {

return c.Values["global.istiod.enabled"] != "true"

}

return false

Values only contains overrides, so we have to rely on the fact that istiod is default on operator and is never set for helm.

howardjohn · 2020-01-18T03:23:49Z

tests/integration/galley/webhook/webhook_test.go

 			ctx.NewSubTest("key/cert reload").
 				Run(func(t framework.TestContext) {
-					addr, done := startGalleyPortForwarderOrFail(t, env, istioNs)
+					if i.Settings().IsIstiodEnabled() {


Suggested change

if i.Settings().IsIstiodEnabled() {

cfg := i.Settings()

if cfg.IsIstiodEnabled() {

…hook-validation-pilot-use-new-controller-server

howardjohn · 2020-01-18T04:31:33Z

looks like this is passing now except gencheck, but if not I copied this down and made a couple tiny tweaks and it seemed to work if it helps #20292

ayj · 2020-01-18T04:54:54Z

Thanks @howardjohn. One-line change to fix a bracket indentation. Now to re-run all of the tests again.

incfly · 2020-01-18T15:23:51Z

tests/integration/security/webhook/webhook_test.go

 		NewTest(t).
 		Run(func(ctx framework.TestContext) {
+			cfg := inst.Settings()
+			if cfg.IsIstiodEnabled() {


@lei-tang fyi.

When istiod is enabled, users are unable to manage webhooks through operator?

This is only temporary. We need to update operator/istioctl to skip the k8s secret check with istiod.

incfly

approval modulo on

for code freeze
the issue should be resolved and having webhook test enabled with istiod. (Given we want all these thing in 1.5)

ayj requested review from costinm, howardjohn, linsun and rshriram as code owners January 8, 2020 01:14

ayj requested a review from a team January 8, 2020 01:14

ayj requested review from a team as code owners January 8, 2020 01:14

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jan 8, 2020

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jan 8, 2020

istio-testing added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 8, 2020

ayj changed the title ~~WIP - Make pilot use the new webhook controller and server packages (3 of 3)~~ (NOT READY FOR REVIEW) - Make pilot use the new webhook controller and server packages (3 of 3) Jan 8, 2020

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jan 8, 2020

ayj force-pushed the 0002-webhook-validation-pilot-use-new-controller-server branch from 261acc8 to 1c2437c Compare January 13, 2020 22:40

ayj requested a review from a team as a code owner January 13, 2020 22:40

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 13, 2020

howardjohn reviewed Jan 13, 2020

View reviewed changes

pilot/pkg/bootstrap/options.go Outdated Show resolved Hide resolved

pilot/pkg/bootstrap/options.go Outdated Show resolved Hide resolved

pilot/pkg/bootstrap/options.go Outdated Show resolved Hide resolved

pilot/pkg/bootstrap/server.go Outdated Show resolved Hide resolved

ayj force-pushed the 0002-webhook-validation-pilot-use-new-controller-server branch from 1c2437c to ae85c15 Compare January 14, 2020 01:36

ayj requested a review from a team as a code owner January 14, 2020 01:36

istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 14, 2020

howardjohn reviewed Jan 15, 2020

View reviewed changes

manifests/istio-control/istio-discovery/templates/deployment.yaml Outdated Show resolved Hide resolved

howardjohn reviewed Jan 15, 2020

View reviewed changes

pilot/pkg/bootstrap/certcontroller.go Outdated Show resolved Hide resolved

pilot/pkg/bootstrap/certcontroller.go Outdated Show resolved Hide resolved

pilot/pkg/bootstrap/sidecarinjector.go Outdated Show resolved Hide resolved

ayj requested a review from a team January 16, 2020 20:09

ayj requested review from a team as code owners January 16, 2020 20:09

istio-testing added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 16, 2020

howardjohn reviewed Jan 17, 2020

View reviewed changes

tests/integration/galley/webhook/webhook_test.go Outdated Show resolved Hide resolved

use pilot webhook port

d9abb41

howardjohn approved these changes Jan 17, 2020

View reviewed changes

ayj added 4 commits January 17, 2020 15:13

fix cert load

a05199f

undo test changes

4f3fff0

remove unused var

a7140e9

skip key/cert reload test. istiod doesn't support rotation.

553cb81

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 18, 2020

Merge branch 'master' of https://github.com/istio/istio into 0002-web…

81dad6a

…hook-validation-pilot-use-new-controller-server

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 18, 2020

skip istioctl webhook test until its updated to work with istiod

d79453c

ayj mentioned this pull request Jan 18, 2020

remove the ability for pilot to register CRDs itself #20085

Merged

update webhook tests to support istiod and non-istiod cases

2374aac

howardjohn reviewed Jan 18, 2020

View reviewed changes

fix istiod check

b50e97f

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 18, 2020

ayj added 2 commits January 17, 2020 20:08

Merge branch 'master' of https://github.com/istio/istio into 0002-web…

ef3e663

…hook-validation-pilot-use-new-controller-server

check override values

fef8135

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 18, 2020

gencheck

d9faf59

howardjohn assigned myidpt, incfly and JimmyCYJ Jan 18, 2020

incfly reviewed Jan 18, 2020

View reviewed changes

incfly approved these changes Jan 18, 2020

View reviewed changes

istio-testing merged commit 20cac5b into istio:master Jan 18, 2020

ayj deleted the 0002-webhook-validation-pilot-use-new-controller-server branch January 18, 2020 21:04

-	return c.Values["global.istiod.enabled"] == "true"
+	if c.Operator {
+		return c.Values["global.istiod.enabled"] != "true"
+	}
+	return false

	if i.Settings().IsIstiodEnabled() {
	cfg := i.Settings()
	if cfg.IsIstiodEnabled() {

Conversation

ayj commented Jan 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ayj commented Jan 8, 2020

Uh oh!

howardjohn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

googlebot commented Jan 16, 2020

Uh oh!

Uh oh!

howardjohn Jan 18, 2020

Choose a reason for hiding this comment

Uh oh!

howardjohn Jan 18, 2020

Choose a reason for hiding this comment

Uh oh!

howardjohn commented Jan 18, 2020

Uh oh!

ayj commented Jan 18, 2020

Uh oh!

incfly Jan 18, 2020

Choose a reason for hiding this comment

Uh oh!

lei-tang Jan 18, 2020

Choose a reason for hiding this comment

Uh oh!

ayj Jan 18, 2020 • 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!

incfly left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ayj commented Jan 8, 2020 •

edited

Loading

ayj Jan 18, 2020 •

edited

Loading