add webhook validation #1158

ayj · 2017-09-01T01:29:24Z

What this PR does / why we need it:

Add the istio.io/pilot/platform/kube/admit package which implements the ExternalAdmissionWebhook for server-side validation of Pilot configuration. This includes a script to enable the workaround described here.

This feature requires 1.7.x with external-admission-webhooks. General instructions to enable are here. Specific testing instructions for GKE and Minikube are documented here.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Fixes #1066.

Special notes for your reviewer:

UPDATED: this PR includes integration with the istio-pilot discovery service. The admission registration is disabled by default since it requires an additional workaround for GKE (see platform/kube/admit/webhook-workaround.sh).

Users can experiment with this feature on GKE using the e2e pre-submit test with the following sequence of commands.

run bin/e2e.sh -use-admission-webhook
cancel the test after the deployment and test pods are created
run platform/kube/admit/webhook-workaround.sh and specify the server-name, secret-name, namespace, and port number of the pilot discovery service, e.g.

platform/kube/admit/webhook-workaround.sh \ 
    --service-name istio-pilot \
    --secret-name pilot-webhook \
    --namespace istio-test-w1g0f \
    --port 443

This script converts the istio-pilot discovery service to type=LoadBalancer, waits for ingress IP, creates the workaround external service/endpoint, generates required CA and server certs for webhook, and creates the corresponding k8s secret which is consumed by the admission webhook server.

wait until the LoadBalancer IP forwarding rule is propagated in GCP. You can try curl'ing <ExternalIP>:8000/v1/registration until a valid response is returned.
Create a (in)valid pilot configuration via kubectl (or istioctl). Invalid configuration should be rejected by apiserver with appropriate error message.

Release note:

Add the `istio.io/pilot/platform/kube/admit` package which implements the ExternalAdmissionWebhook for server-side validation of Pilot configuration. This includes a script to enable the workaround described [here](https://github.com/kubernetes/kubernetes/issues/49987#issuecomment-319739227).

This feature requires 1.7.x with [external-admission-webhooks](https://kubernetes.io/docs/admin/extensible-admission-controllers/#external-admission-webhooks). General instructions to enable are [here](https://kubernetes.io/docs/admin/extensible-admission-controllers/#enable-external-admission-webhooks). Specific testing instructions for GKE and Minikube are documented [here](https://github.com/istio/pilot/blob/master/doc/testing.md#create-development-cluster).

ayj · 2017-09-01T04:00:26Z

/test pilot-e2e-smoketest

ayj · 2017-09-01T16:39:21Z

/retest

ayj · 2017-09-01T16:41:05Z

/retest

kyessenov

Looks good to me (modulo minor comments and requests for clarification). Can you document cluster requirements for this PR? Is this already available on GKE?

kyessenov · 2017-09-01T19:27:12Z

platform/kube/admit/admit.go

+	DefaultAdmissionServiceName = "istio-pilot-config"
+)
+
+// Admit implements the external admission webhook for validation


validation of

kyessenov · 2017-09-01T19:28:12Z

platform/kube/admit/admit.go

+
+	// DefaultAdmissionServiceName is the default service of the
+	// validation webhook.
+	DefaultAdmissionServiceName = "istio-pilot-config"


should this be pilot itself? seems unnecessary to allocate a service just for validation.

Yes, though we'll need a new port since this is HTTPS. We'll also need to temporary use a different service name as a workaround for the issue described here, but that should eventually go away (k8s 1.8+).

I see, then it makes sense but deserves a note in the comment.

kyessenov · 2017-09-01T19:32:44Z

platform/kube/admit/admit.go

+	validateNamespaces []string
+}
+
+// GetAPIServerExtensionCACert gets the CA cert that will signed the


will be signed by

kyessenov · 2017-09-01T19:33:23Z

platform/kube/admit/admit.go

+// controller.
+func GetAPIServerExtensionCACert(cl kubernetes.Interface) ([]byte, error) {
+	const name = "extension-apiserver-authentication"
+	c, err := cl.CoreV1().ConfigMaps(metav1.NamespaceSystem).Get(name, metav1.GetOptions{})


Why not a secret to store CA cert?

Is kube-system the right place to store the config? Is the name of the config standard?

That is where Kubernetes stores the aggregate apiserver's client CA certificate. We don't control this.

$ kubectl -n kube-system get cm extension-apiserver-authentication NAME DATA AGE extension-apiserver-authentication 6 17d

Please mention this. It might appear as if we're choosing these names.

kyessenov · 2017-09-01T19:35:26Z

platform/kube/admit/admit.go

+}
+
+// New creates a new instance of the admission webhook controller.
+func New(descriptor model.ConfigDescriptor, hookConfigName, hookName, serviceName, serviceNamespace string, validateNamespaces []string, caBundle []byte) *Admit { // nolint: lll


nit: constructors are usually called Create or Make around here. Too bad there's no standard name for the pattern. Also, Admit does not sound good as a thing, maybe call it AdmissionController.

kyessenov · 2017-09-01T19:37:06Z

platform/kube/admit/admit.go

+	}
+}
+
+// Unregister registers the external admission webhook


unregisters

kyessenov · 2017-09-01T19:37:54Z

platform/kube/admit/admit.go

+				Rules: []admissionregistrationv1alpha1.RuleWithOperations{{
+					Operations: []admissionregistrationv1alpha1.OperationType{
+						admissionregistrationv1alpha1.Create,
+						admissionregistrationv1alpha1.Update,


There is no PATCH operation for admission (see https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/admissionregistration/types.go#L186

kyessenov · 2017-09-01T19:38:32Z

platform/kube/admit/admit.go

+func (a *Admit) Register(client admissionClient.ExternalAdmissionHookConfigurationInterface) error {
+	var resources []string
+	for _, schema := range a.descriptor {
+		resources = append(resources, schema.Plural)


is this expecting the kind name? then it should "RouteRules", not "route-rules"

This is the resource name, e.g. pods and not Pod (see https://kubernetes.io/docs/admin/extensible-admission-controllers/#configure-initializers-on-the-fly)

then it's probably to remove "-" if the library code doesn't deal with it well.

kyessenov · 2017-09-01T19:39:17Z

platform/kube/admit/admit.go

+			},
+		},
+	}
+	client.Delete(webhook.Name, nil) // nolint: errcheck


might be worth logging even if ignoring the error, for debugging purposes

kyessenov · 2017-09-01T19:42:51Z

platform/kube/admit/testcerts.go

+// This file was generated using openssl by the gencerts.sh script
+// and holds raw certificates for the webhook tests.
+
+package admit


admit_test is probably better to make it clear these are test artifacts

…ebhook-step-1

…ebhook-step-3

codecov · 2017-09-05T18:09:06Z

Codecov Report

Merging #1158 into master will decrease coverage by 0.54%.
The diff coverage is 54.03%.

@@            Coverage Diff             @@
##           master    #1158      +/-   ##
==========================================
- Coverage   70.47%   69.93%   -0.55%     
==========================================
  Files          55       54       -1     
  Lines        6399     6732     +333     
==========================================
+ Hits         4510     4708     +198     
- Misses       1668     1798     +130     
- Partials      221      226       +5

Impacted Files	Coverage Δ
adapter/config/crd/conversion.go	`89.09% <100%> (ø)`	⬆️
platform/kube/admit/admit.go	`50.44% <50.44%> (ø)`
adapter/config/crd/controller.go	`68.21% <80%> (ø)`	⬆️
adapter/config/crd/client.go	`69.05% <92.3%> (ø)`	⬆️
platform/kube/controller.go	`57.19% <0%> (-1.37%)`	⬇️
proxy/envoy/config.go	`83.24% <0%> (-0.28%)`	⬇️
test/mock/config.go	`0% <0%> (ø)`	⬆️
adapter/serviceregistry/aggregate/controller.go
platform/eureka/serviceaccounts.go
... and 3 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 4f34dbb...2035394. Read the comment docs.

rshriram · 2017-09-06T15:27:40Z

/retest

rshriram · 2017-09-06T15:28:34Z

cc @GregHanson .. you might want to update your k8s image to 1.7.4

…ebhook-step-2

ldemailly · 2017-09-06T17:19:04Z

bin/check.sh

@@ -1,7 +1,7 @@
 #!/bin/bash
 set -ex

-buildifier -showlog -mode=check $(find . -type f \( -name 'BUILD' -or -name 'WORKSPACE' -or -wholename '.*bazel' -or -wholename '.*bzl' \) -print )
+buildifier -showlog -mode=check $(find . -type f \( -name 'BUILD' -or -name 'WORKSPACE' -or -wholename '.*bazel$' -or -wholename '.*bzl$' \) -print )


…ebhook-step-1

ayj · 2017-09-07T05:39:51Z

I went ahead and integrated the webhook with istio-pilot discovery service. It's off by default and requires an additional helper script to work on GKE (see platform/kube/admit/webhook-workaround.sh). I set things up so the majority of the code can be used as-is with some minimal changes to cert derivation/distribution w/o webhook-workaround.sh once k8s 1.8 on GKE is available.

ayj · 2017-09-07T19:52:41Z

/retest

kyessenov

Looks good. Thanks, Jason!

kyessenov · 2017-09-07T20:36:32Z

platform/kube/admit/admit.go

+	// // validate webhook's service certificate.
+	// CABundle []byte
+
+	// DomainSuffix is the DNS domain suffix for Istio CRD resources, e.g. local.cluster.


nit: cluster.local

kyessenov · 2017-09-07T20:40:39Z

test/integration/testdata/rbac-beta.yaml.tmpl

@@ -29,6 +29,11 @@ rules:
 - apiGroups: [""]
  resources: ["namespaces", "nodes", "secrets"]
  verbs: ["get", "list", "watch"]
+{{if .UseAdmissionWebhook}}


nit: may drop this, since it's a superset of RBAC permissions.

…ebhook-step-1

ayj · 2017-09-07T21:13:53Z

/retest

…ebhook-step-1

rshriram · 2017-09-07T22:08:21Z

@ayj please merge with master.. and upload test logs from your local test cluster run.. Need e2e tests from istio/istio to pass.

…ebhook-step-1

kyessenov · 2017-09-08T02:45:57Z

rebase please.

…ebhook-step-1

ayj requested a review from kyessenov September 1, 2017 01:29

googlebot added the cla: yes label Sep 1, 2017

add webhook validation

c755afb

ayj force-pushed the crd-validation-webhook-step-1 branch from bb3b1e7 to c755afb Compare September 1, 2017 01:31

kyessenov reviewed Sep 1, 2017

View reviewed changes

ayj added 6 commits September 1, 2017 14:52

address kyessenov review comments

a8684a2

Merge branch 'master' of github.com:istio/pilot into crd-validation-w…

7c9d66f

…ebhook-step-1

fix linter errors for generated certs

c455fc9

Merge branch 'master' of github.com:istio/pilot into crd-validation-w…

a667bba

…ebhook-step-3

add domainSuffix to admission controller

75e41c5

make fmt and bin/gazelle

0d7416d

ayj added 2 commits September 6, 2017 08:45

Merge branch 'master' of github.com:istio/pilot into crd-validation-w…

7fc01cd

…ebhook-step-2

fix bin/check.sh

352cf6f

ldemailly reviewed Sep 6, 2017

View reviewed changes

ayj added 3 commits September 6, 2017 22:04

Merge branch 'master' of github.com:istio/pilot into crd-validation-w…

c004ad2

…ebhook-step-1

integrate webhook in pilot-discovery and add workaround script

9e9333c

quiet aligncheck linter error

6cd8d28

revert ResyncPeriod for discovery

1e4c547

kyessenov approved these changes Sep 7, 2017

View reviewed changes

ayj added 2 commits September 7, 2017 13:49

nits

9c20d75

Merge branch 'master' of github.com:istio/pilot into crd-validation-w…

6725b92

…ebhook-step-1

Merge branch 'master' of github.com:istio/pilot into crd-validation-w…

ce28204

…ebhook-step-1

ayj added 2 commits September 7, 2017 15:14

Merge branch 'master' of github.com:istio/pilot into crd-validation-w…

8fe2be3

…ebhook-step-1

fix bad merge

793a42a

Merge branch 'master' of github.com:istio/pilot into crd-validation-w…

2035394

…ebhook-step-1

rshriram merged commit e621733 into istio:master Sep 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add webhook validation #1158

add webhook validation #1158

ayj commented Sep 1, 2017 •

edited

Loading

ayj commented Sep 1, 2017

ayj commented Sep 1, 2017 •

edited

Loading

ayj commented Sep 1, 2017

kyessenov left a comment

kyessenov Sep 1, 2017

kyessenov Sep 1, 2017

ayj Sep 1, 2017

kyessenov Sep 1, 2017

kyessenov Sep 1, 2017

kyessenov Sep 1, 2017

kyessenov Sep 1, 2017

ayj Sep 1, 2017

kyessenov Sep 1, 2017

kyessenov Sep 1, 2017

kyessenov Sep 1, 2017

kyessenov Sep 1, 2017

ayj Sep 1, 2017

kyessenov Sep 1, 2017

ayj Sep 1, 2017

kyessenov Sep 1, 2017

kyessenov Sep 1, 2017

kyessenov Sep 1, 2017

codecov bot commented Sep 5, 2017 •

edited

Loading

rshriram commented Sep 6, 2017

rshriram commented Sep 6, 2017

ldemailly Sep 6, 2017

ayj commented Sep 7, 2017

ayj commented Sep 7, 2017

kyessenov left a comment

kyessenov Sep 7, 2017

kyessenov Sep 7, 2017

ayj commented Sep 7, 2017

rshriram commented Sep 7, 2017

kyessenov commented Sep 8, 2017

add webhook validation #1158

add webhook validation #1158

Conversation

ayj commented Sep 1, 2017 • edited Loading

ayj commented Sep 1, 2017

ayj commented Sep 1, 2017 • edited Loading

ayj commented Sep 1, 2017

kyessenov left a 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

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

codecov bot commented Sep 5, 2017 • edited Loading

Codecov Report

rshriram commented Sep 6, 2017

rshriram commented Sep 6, 2017

Choose a reason for hiding this comment

ayj commented Sep 7, 2017

ayj commented Sep 7, 2017

kyessenov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayj commented Sep 7, 2017

rshriram commented Sep 7, 2017

kyessenov commented Sep 8, 2017

ayj commented Sep 1, 2017 •

edited

Loading

ayj commented Sep 1, 2017 •

edited

Loading

codecov bot commented Sep 5, 2017 •

edited

Loading