Reapply sidecar injector CA bundle to prevent overwrite #6435

ostromart · 2018-06-20T18:13:07Z

Fixes #6069
Tell me if this looks reasonable and I will test it.

ayj · 2018-06-20T18:53:28Z

pilot/pkg/kube/inject/webhook.go

@@ -210,7 +211,11 @@ func (wh *Webhook) Run(stop <-chan struct{}) {
 				log.Errorf("update error: %v", err)
 				break
 			}
-
+			caCertPEM, err := ioutil.ReadFile(wh.certFile)


Does the server's cert file (wh.certFile) include the CA bundle?

Ah, that was a mistake - fixed.

ayj · 2018-06-20T18:54:43Z

pilot/pkg/kube/inject/webhook.go

@@ -241,6 +247,13 @@ func (wh *Webhook) Run(stop <-chan struct{}) {
 	}
 }

+// GetCABundlePEM returns the CA bundle PEM, read from the cert file. Safe for concurrent use.


Is the intent here to support CA cert rotation? If not, the caCertPEM re-load code can be removed.

Yes, might as well include this since it's a minor addition.

ayj · 2018-06-20T18:56:47Z

pilot/cmd/sidecar-injector/main.go

@@ -86,6 +86,9 @@ var (
 				return multierror.Prefix(err, "failed to patch webhook config")
 			}

+			if err := patchCertLoop(wh.GetCABundlePEM); err != nil {


patchCert() can be removed with this change.

Currently if we don't succeed within 1 min we get an error and exit. If this is removed, we would continue and just log the error. Is that acceptable?

ayj · 2018-06-20T18:59:03Z

pilot/cmd/sidecar-injector/main.go

@@ -131,6 +134,24 @@ func patchCert() error {
 	return err
 }

+// patchCertLoop continually reapplies the caBundle PEM. This is required because it can be overwritten with empty
+// values if the original yaml is reapplied (https://github.com/istio/istio/issues/6069).
+func patchCertLoop(getCABundlePEM func() []byte) error {


This is an improvement over the existing (broken) behavior. It will lead to some noise in api-server logs. That could be mitigated by updating PatchMutatingWebhookConfig to skip invoking PATCH if the strategic merge patch is empty.

Ideally this would be driven by watching the webhook configuration (via watch or informer) and only PATCH'ing if change is observed. Is there a follow-up tracking issue for that work item?

There is now: #6451

codecov · 2018-06-20T23:54:24Z

Codecov Report

Merging #6435 into master will increase coverage by 1%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #6435    +/-   ##
=======================================
+ Coverage      68%     68%    +1%     
=======================================
  Files         357     351     -6     
  Lines       31086   30833   -253     
=======================================
- Hits        21010   20847   -163     
+ Misses       9240    9144    -96     
- Partials      836     842     +6

Impacted Files	Coverage Δ
pkg/util/webhookpatch.go	`78% <100%> (+1%)`	⬆️
galley/pkg/server/server.go	`81% <0%> (-9%)`	⬇️
pilot/pkg/config/memory/monitor.go	`82% <0%> (-9%)`	⬇️
mixer/adapter/circonus/circonus.go	`68% <0%> (-7%)`	⬇️
mixer/adapter/signalfx/registry.go	`86% <0%> (-5%)`	⬇️
pilot/pkg/serviceregistry/kube/controller.go	`64% <0%> (-4%)`	⬇️
galley/pkg/mcp/snapshot/snapshot.go	`96% <0%> (-4%)`	⬇️
pilot/pkg/serviceregistry/kube/queue.go	`86% <0%> (-3%)`	⬇️
pilot/pkg/proxy/envoy/v2/debug.go	`30% <0%> (-2%)`	⬇️
galley/pkg/mcp/server/server.go	`85% <0%> (-2%)`	⬇️
... and 74 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 ab75e24...24ca447. Read the comment docs.

ostromart · 2018-06-20T23:56:45Z

pilot/cmd/sidecar-injector/main.go

@@ -86,6 +86,9 @@ var (
 				return multierror.Prefix(err, "failed to patch webhook config")
 			}

+			if err := patchCertLoop(wh.GetCABundlePEM); err != nil {


Currently if we don't succeed within 1 min we get an error and exit. If this is removed, we would continue and just log the error. Is that acceptable?

ostromart · 2018-06-21T00:01:15Z

pilot/cmd/sidecar-injector/main.go

@@ -131,6 +134,24 @@ func patchCert() error {
 	return err
 }

+// patchCertLoop continually reapplies the caBundle PEM. This is required because it can be overwritten with empty
+// values if the original yaml is reapplied (https://github.com/istio/istio/issues/6069).
+func patchCertLoop(getCABundlePEM func() []byte) error {


There is now: #6451

ostromart · 2018-06-21T00:01:39Z

pilot/pkg/kube/inject/webhook.go

@@ -210,7 +211,11 @@ func (wh *Webhook) Run(stop <-chan struct{}) {
 				log.Errorf("update error: %v", err)
 				break
 			}
-
+			caCertPEM, err := ioutil.ReadFile(wh.certFile)


Ah, that was a mistake - fixed.

ostromart · 2018-06-21T00:02:40Z

pilot/pkg/kube/inject/webhook.go

@@ -241,6 +247,13 @@ func (wh *Webhook) Run(stop <-chan struct{}) {
 	}
 }

+// GetCABundlePEM returns the CA bundle PEM, read from the cert file. Safe for concurrent use.


Yes, might as well include this since it's a minor addition.

ostromart · 2018-06-21T00:03:26Z

pilot/pkg/kube/inject/webhook.go

+	defer wh.mu.Unlock()
+	return wh.caCertPEM
+}
+


I didn't like entangling the patch functionality into this type, so I moved it entirely into main.

ayj · 2018-06-21T17:19:47Z

pkg/util/webhookpatch.go

+
+	if string(patch) != "{}" {
+		_, err = client.Patch(webhookConfigName, types.StrategicMergePatchType, patch)
+	}


Lines 103-105 are correct for validating webhook. You need to make the same change for the mutating webhook on line 61.

ayj · 2018-06-21T17:21:42Z

pilot/cmd/sidecar-injector/main.go

-	const retryTimes = 6 // Try for one minute.
+// patchCertLoop continually reapplies the caBundle PEM. This is required because it can be overwritten with empty
+// values if the original yaml is reapplied (https://github.com/istio/istio/issues/6069).
+func patchCertLoop() error {


Can you also reference the tracking issue for watch triggered patch (#6451)? e.g.

// TODO(https://github.com/istio/istio/issues/6451) - only patch when caBundle changes

ayj · 2018-06-21T17:29:45Z

pilot/cmd/sidecar-injector/main.go

+			}
+
+		case <-watcher.Event:
+			caCertPem, err = ioutil.ReadFile(flags.caCertFile)


This doesn't filter on file event type (e.g. delete). Any chance that this read will fail in such cases and overwrite caCertPem with nil? Maybe something like the following?

if b, err := ioutil.ReadFile(flags.caCertFile); err != nil { /* error */ } else { caCertPem = b }

ayj

/lgtm

Also, pausing auto-merge until fix is manually verified
/hold

ostromart · 2018-06-22T22:15:46Z

Verified that reapplying istio.yaml blows away caBundle but it is immediately restored.

ostromart · 2018-06-22T22:15:56Z

/hold cancel

ayj · 2018-06-22T23:12:18Z

/lgtm

hklai · 2018-06-23T00:38:26Z

/lgtm

istio-testing · 2018-06-23T00:38:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ayj, hklai, ostromart

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:

~~pilot/OWNERS~~ [ayj,hklai]
~~pkg/OWNERS~~ [hklai]

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

hklai · 2018-06-23T02:02:55Z

/test istio-unit-tests

istio-testing · 2018-06-23T02:08:50Z

@ostromart: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/e2e-dashboard.sh	`24ca447`	link	`/test e2e-dashboard`
prow/istio-unit-tests.sh	`24ca447`	link	`/test istio-unit-tests`

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.

See related issue istio#6435.

* Continuously reapply galley CA bundle to prevent overwrite See related issue #6435. * fix linter error * refactor CreateClientset() into common pkg/kube * linter fixes

* Reapply CA bundle to prevent overrwrite * Add watcher, move cert reapply out of webhook * Fix review comments * Patch loop should be in goroutine * Lint

* Continuously reapply galley CA bundle to prevent overwrite See related issue istio#6435. * fix linter error * refactor CreateClientset() into common pkg/kube * linter fixes

googlebot added the cla: yes label Jun 20, 2018

ostromart requested a review from ayj June 20, 2018 18:13

ayj reviewed Jun 20, 2018

View reviewed changes

ostromart mentioned this pull request Jun 21, 2018

Istio webhook config should use informer/watch to monitor caBundle #6451

Closed

ostromart force-pushed the cabundle_fix branch from db7a3bf to 5c3c6ed Compare June 21, 2018 00:32

ostromart commented Jun 21, 2018

View reviewed changes

ayj reviewed Jun 21, 2018

View reviewed changes

ostromart mentioned this pull request Jun 22, 2018

Reapplying istio-demo.yml causes cert errors in injector #6069

Closed

ayj changed the title ~~Reapply CA bundle to prevent overwrite~~ Reapply sidecar injector CA bundle to prevent overwrite Jun 22, 2018

ayj mentioned this pull request Jun 22, 2018

Make re-applying galley validation yaml idempotent #6532

Closed

ayj approved these changes Jun 22, 2018

View reviewed changes

istio-testing assigned ayj Jun 22, 2018

istio-testing added do-not-merge/hold Block automatic merging of a PR. lgtm labels Jun 22, 2018

ostromart force-pushed the cabundle_fix branch from 648c952 to 6e317fc Compare June 22, 2018 22:12

istio-testing removed the lgtm label Jun 22, 2018

istio-testing removed the do-not-merge/hold Block automatic merging of a PR. label Jun 22, 2018

ostromart force-pushed the cabundle_fix branch 2 times, most recently from e7b8a0d to 6556a0a Compare June 22, 2018 22:51

istio-testing added the lgtm label Jun 22, 2018

ostromart added 4 commits June 22, 2018 17:08

Reapply CA bundle to prevent overrwrite

78d04e5

Add watcher, move cert reapply out of webhook

88ec755

Fix review comments

3f9d0bd

Patch loop should be in goroutine

40ebbcc

Lint

24ca447

ostromart force-pushed the cabundle_fix branch from 6556a0a to 24ca447 Compare June 23, 2018 00:09

istio-testing removed the lgtm label Jun 23, 2018

istio-testing assigned hklai Jun 23, 2018

istio-testing added the lgtm label Jun 23, 2018

istio-testing added the approved label Jun 23, 2018

istio-testing merged commit 67ac3e2 into istio:master Jun 23, 2018

ayj added a commit to ayj/istio that referenced this pull request Jun 25, 2018

Continuously reapply galley CA bundle to prevent overwrite

9f921b3

See related issue istio#6435.

ayj mentioned this pull request Jun 25, 2018

Continuously reapply galley CA bundle to prevent overwrite #6599

Merged

This was referenced Jun 25, 2018

Admission control webhooks (e.g. sidecar injector) don't work on EKS istio/old_issues_repo#271

Closed

istio 0.8.0 didn't automatic injection sidecar #6504

Closed

hklai mentioned this pull request Jun 26, 2018

Istio 0.8.1 Tracker #5982

Closed

hklai added the kind/backport label Jun 27, 2018

ostromart deleted the cabundle_fix branch November 17, 2018 00:14

Reapply sidecar injector CA bundle to prevent overwrite #6435

Reapply sidecar injector CA bundle to prevent overwrite #6435

Conversation

ostromart commented Jun 20, 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!

codecov bot commented Jun 20, 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!

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!

ayj left a comment

Choose a reason for hiding this comment

Uh oh!

ostromart commented Jun 22, 2018

Uh oh!

ostromart commented Jun 22, 2018

Uh oh!

ayj commented Jun 22, 2018

Uh oh!

hklai commented Jun 23, 2018

Uh oh!

istio-testing commented Jun 23, 2018

Uh oh!

hklai commented Jun 23, 2018

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jun 20, 2018 •

edited

Loading

istio-testing commented Jun 23, 2018 •

edited

Loading