Add common config validation service #4811

ayj · 2018-04-07T00:43:09Z

Create a common webhook service for server-side configuration validation. This uses the existing pilot and mixer validation code for now. The k8s webhook specific code, cert provisioning, and registration has been copied from the sidecar-injector.

This service is optional (requires >= k8s 1.9) so I created a new component. Instead of creating a throwaway service (i.e. config-validator), and since Galley will be responsible for config validation anyway, I went ahead and created the galley component, charts, etc. The validation service is run from the validator subcommand.

fixes #1894

TODO in follow-up PRs

add integration test
update e2e test(s) to use server-side validation. Depends on Standardize on Helm templates for yaml generation #4340
update istio.io documentation

xiaolanz

/lgtm

xiaolanz · 2018-04-07T18:06:36Z

install/kubernetes/helm/istio/charts/galley/templates/clusterrolebinding.yaml

+  - kind: ServiceAccount
+    name: istio-galley-service-account
+    namespace: {{ .Release.Namespace }}
+{{- end }}


add last blank line

xiaolanz · 2018-04-07T18:08:12Z

install/kubernetes/helm/istio/charts/galley/templates/validatingwebhook.yaml

+        - enduserauthenticationpolicyspecs
+        - enduserauthenticationpolicyspecbindings
+        - serviceroles
+        - servicerolebindings


missing policy.authentication.istio.io

validation. This uses the existing pilot and mixer validation code for now. The k8s webhook specific code, cert provisioning, and registration has been copied from the sidecar-injector. This service is optional (requires >= k8s 1.9) so I created a new component. Instead of creating a throwaway service (i.e. config-validator), and since Galley will be responsible for config validation anyway, I went ahead and created the galley component, charts, etc. The validation service is run from the `validator` subcommand. fixes istio#1894 TODO - [ ] add integration test - [ ] update e2e test(s) to use server-side validation - [ ] update istio.io documentation

codecov · 2018-04-10T18:18:37Z

Codecov Report

Merging #4811 into master will increase coverage by 1%.
The diff coverage is 82%.

@@           Coverage Diff           @@
##           master   #4811    +/-   ##
=======================================
+ Coverage      72%     73%    +1%     
=======================================
  Files         302     300     -2     
  Lines       25376   25035   -341     
=======================================
- Hits        18189   18076   -113     
+ Misses       6446    6227   -219     
+ Partials      741     732     -9

Impacted Files	Coverage Δ
mixer/cmd/mixs/cmd/root.go	`0% <ø> (ø)`	⬆️
pilot/cmd/pilot-agent/main.go	`35% <ø> (ø)`	⬆️
mixer/pkg/config/crd/store.go	`99% <100%> (ø)`	⬆️
pkg/util/webhookpatch.go	`77% <79%> (+2%)`	⬆️
galley/pkg/crd/validation/webhook.go	`82% <82%> (ø)`
mixer/adapter/solarwinds/metrics_handler.go	`75% <0%> (-8%)`	⬇️
mixer/adapter/stackdriver/log/log.go	`87% <0%> (-5%)`	⬇️
mixer/adapter/servicecontrol/reportprocessor.go	`79% <0%> (-1%)`	⬇️
mixer/adapter/statsd/statsd.go	`96% <0%> (-1%)`	⬇️
mixer/adapter/rbac/rbacStore.go	`84% <0%> (ø)`	⬇️
... and 14 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 95ffd4a...c44b0aa. Read the comment docs.

ayj · 2018-04-10T19:36:19Z

security/cmd/istio_ca/main.go

@@ -144,13 +144,11 @@ var (
 	// TODO: move it to a configmap later when we have more services to support.
 	webhookServiceAccounts = []string{
 		"istio-sidecar-injector-service-account",
-		"istio-mixer-validator-service-account",
-		"istio-pilot-service-account",
+		"istio-galley-service-account",


fyi @yusuoh

ayj · 2018-04-10T19:37:51Z

cc @mandarjog for helm/install review and top-level OWNERS approval

ozevren · 2018-04-10T20:32:47Z

galley/cmd/gals/cmd/root.go

+)
+
+// resolveConfig checks whether to use the in-cluster or out-of-cluster config
+func resolveConfig(kubeconfig string) (string, error) {


Do we need all of this? (It looks like this is from Pilot?).
In the code I have, I just ask for config and use BuildConfigFromFlags (I'd prefer to be very explicit with the user, but that's just me.)

If we do indeed keep this, can we move this to to a common package in istio/pkg/? It looks like this should be standardized among all command-line tools expecting a config.

Simple is good. I removed resolvedConfig in favor of calling BuildConfigFromFlags directly with kubeconfig.

ozevren · 2018-04-10T20:32:56Z

galley/cmd/gals/cmd/root.go

+	}
+	if kubeconfig == "" {
+		defaultCfg := path.Join(homedir.HomeDir(), ".kube/config")
+		fmt.Printf("(3) %v\n", defaultCfg)


Runaway Printf?

ozevren · 2018-04-10T20:36:01Z

galley/cmd/gals/cmd/validator.go

+)
+
+// waitSignal awaits for SIGINT or SIGTERM and closes the channel
+func waitSignal(stop chan struct{}) {


Seems like we can use this for other commands as well. Hoist to a different file?

I created isto.io/istio/pkt/cmd/ for common command functions.

ozevren · 2018-04-10T21:04:40Z

galley/cmd/gals/cmd/validator.go

+				webhookConfigName, err)
+			select {
+			case <-stop:
+				// canceled by caller


Nit : cancelled

https://www.grammarly.com/blog/canceled-vs-cancelled. Given my emacs dictionary is american I chose "cancel". :)

ozevren · 2018-04-10T21:08:19Z

galley/cmd/gals/cmd/validator.go

+		},
+	}
+
+	validatorCmd.PersistentFlags().StringVar(&webhookConfigName,


Why do we need the webhook-name flags? Seems like these should be constant.

For testing purposes in a shared cluster we might need to scope Istio to a single namespace. Multiple validation webhooks would exist and their names ought to be unique.

ozevren · 2018-04-10T21:12:21Z

galley/cmd/gals/cmd/validator.go

+			stop := make(chan struct{})
+
+			go func() {
+				if err := patchCert(stop); err != nil {


What happens when patchCert fails? It looks like the webhook will still try to serve?

Validation will be broken. The webhook will continue to serve but api-server won't call it. The sidecar injector has a similar problem. I'm hoping the environments WG can address this problem as part of k8s install/upgrade process. cc @yusuoh

sidecar injector service will fail if it cannot patch the cert after several retries.

https://github.com/istio/istio/blob/master/pilot/cmd/sidecar-injector/main.go#L86

Do we have a tracking bug for this? It would be nice to tag this with a TODO with reference to the bug (otherwise it will most likely get lost and lurk as an issue).

ozevren · 2018-04-10T21:16:34Z

galley/pkg/crd/validation/webhook.go

+
+// CreateMixerValidator creates a mixer backend validator.
+func CreateMixerValidator(kubeconfig string) (store.BackendValidator, error) {
+	info := generatedTmplRepo.SupportedTmplInfo


These seem problematic. Specifically, this depends on Mixer's own built-in templates, which effectively exposes Mixer internals to Galley. (which is also apparent from the imports in this file).

I wasn't expecting to pull this much Mixer code into Galley. Would it be better if we split this CR and do some refactoring before implementing the webhook in Galley?

Per our offline discussion I'd prefer to reverse the order since config validation is P0 for Istio 1.0. I created #4887 to track the follow-up refactoring.

@geeknoid
FYI. Hopefully, after Mandar lands CRD normalization, we can start utilizing a declarative config check for adapter configs and reduce the scope of dependencies to Mixer to the pkg/ level.

ozevren · 2018-04-10T21:22:42Z

galley/pkg/crd/validation/webhook.go

+			}
+		case err := <-wh.watcher.Error:
+			log.Errorf("Watcher error: %v", err)
+		case <-healthC:


Why do we need the health check file here? Is this needed for web hooks in general?

All Istio components should have health checks. https://goo.gl/QRRdwm recommends that components use file based health checking.

Yes, but can we move this to use the standard health-check library?

costinm

A bit too large to understand, but it's good to move it out of pilot.

I don't think this was enabled in pilot - so would feel more comfortable with keeping it off by default in 0.8.

costinm · 2018-04-11T16:55:25Z

install/kubernetes/helm/istio/values-istio-all.yaml

@@ -8,6 +8,9 @@ global:
 sidecar-injector:
  enabled: true

+galley:
+  enabled: true


Should it be enabled by default in 0.8 ? What are the dependencies / setup / etc ?

k8s >= 1.9

Config validation is targeted BETA for 1.0. I could disable for 0.8 but would like to (re)enable for 0.9 for additional testing prior to 1.0.

istio-merge-robot · 2018-04-11T23:40:38Z

@ayj PR needs rebase

…lidation-two

costinm

/lgtm
/approve

istio-testing · 2018-04-13T18:20:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: costinm, xiaolanz

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:

~~OWNERS~~ [costinm]

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

istio-merge-robot · 2018-04-13T18:20:51Z

/test all [submit-queue is verifying that this PR is safe to merge]

istio-merge-robot · 2018-04-13T19:02:14Z

Automatic merge from submit-queue.

ayj requested review from ozevren and xiaolanz April 7, 2018 00:43

googlebot added the cla: yes label Apr 7, 2018

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Apr 7, 2018

ayj added area/config kind/enhancement kind/needed for 1.0 labels Apr 7, 2018

xiaolanz reviewed Apr 7, 2018

View reviewed changes

istio-testing assigned xiaolanz Apr 7, 2018

istio-testing added the lgtm label Apr 7, 2018

ayj mentioned this pull request Apr 9, 2018

admit_test.go cannot find ca.crt in extension-apiserver-authentication #4737

Closed

ayj force-pushed the enable-config-validation branch from dcec14d to 08afacb Compare April 10, 2018 18:18

istio-testing removed the lgtm label Apr 10, 2018

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Apr 10, 2018

ayj added 2 commits April 10, 2018 11:31

format and linter fixes

493c6e7

fix moved import

44f9c15

ayj changed the title ~~WIP: add common config validation service~~ Add common config validation service Apr 10, 2018

ayj added 2 commits April 10, 2018 11:49

more linter fixes

1a7c54b

copy gals to from ISTIO_OUT

c76d6c2

ayj commented Apr 10, 2018

View reviewed changes

ozevren reviewed Apr 10, 2018

View reviewed changes

costinm reviewed Apr 11, 2018

View reviewed changes

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Apr 11, 2018

ayj added 2 commits April 11, 2018 21:46

address review comments

20fb6db

Merge branch 'master' of github.com:istio/istio into enable-config-va…

027146d

…lidation-two

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Apr 12, 2018

ayj added 4 commits April 11, 2018 22:34

linter fix

abc38c2

increase test coverage

4f652f9

Merge branch 'master' of github.com:istio/istio into enable-config-va…

cf1b77e

…lidation-two

fix formatting

c44b0aa

costinm approved these changes Apr 13, 2018

View reviewed changes

istio-testing assigned costinm Apr 13, 2018

istio-testing added the lgtm label Apr 13, 2018

istio-testing added the approved label Apr 13, 2018

istio-merge-robot merged commit 8d8a990 into istio:master Apr 13, 2018

ayj deleted the enable-config-validation branch April 30, 2018 19:19

ayj unassigned xiaolanz Sep 6, 2018

Add common config validation service #4811

Add common config validation service #4811

Uh oh!

Conversation

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

Uh oh!

Uh oh!

xiaolanz 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!

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

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayj commented Apr 10, 2018

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!

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!

yusuoh Apr 11, 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!

ozevren Apr 13, 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!

costinm 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!

istio-merge-robot commented Apr 11, 2018

Uh oh!

costinm left a comment

Choose a reason for hiding this comment

Uh oh!

istio-testing commented Apr 13, 2018

Uh oh!

ayj commented Apr 7, 2018 •

edited

Loading

codecov bot commented Apr 10, 2018 •

edited

Loading

yusuoh Apr 11, 2018 •

edited

Loading

ozevren Apr 13, 2018 •

edited

Loading