From 077d9b8bc687c75ab2952217b8883cbdde2672a8 Mon Sep 17 00:00:00 2001 From: Jason Young Date: Thu, 17 Oct 2019 11:50:35 -0700 Subject: [PATCH] make validation watch and reload key/certs (again) Galley watched and reloaded key/certs prior to istio 1.3. https://github.com/istio/istio/pull/12571 refactored galley's validation into two parts: (1) config controller and (2) the webhook server. watch/reload logic was retained in (1) and not added at all to (2). This PR adds the missing watch/reload code to (2). fix https://github.com/istio/istio/issues/17718 --- galley/pkg/crd/validation/config.go | 68 ++-------------------- galley/pkg/crd/validation/config_test.go | 37 ------------ galley/pkg/crd/validation/validation.go | 5 +- galley/pkg/crd/validation/webhook.go | 58 +++++++++++++++++- galley/pkg/crd/validation/webhook_test.go | 44 +++++++++++++- galley/pkg/server/components/validation.go | 4 ++ 6 files changed, 111 insertions(+), 105 deletions(-) diff --git a/galley/pkg/crd/validation/config.go b/galley/pkg/crd/validation/config.go index ae1e9379a45d..c2bed7395910 100644 --- a/galley/pkg/crd/validation/config.go +++ b/galley/pkg/crd/validation/config.go @@ -15,7 +15,6 @@ package validation import ( - "crypto/tls" "crypto/x509" "encoding/pem" "errors" @@ -25,7 +24,6 @@ import ( "os" "path/filepath" "reflect" - "sync" "time" "github.com/ghodss/yaml" @@ -60,9 +58,6 @@ var ( // WebhookConfigController implements the validating admission webhook for validating Istio configuration. type WebhookConfigController struct { - mu sync.RWMutex - cert *tls.Certificate - keyCertWatcher *fsnotify.Watcher configWatcher *fsnotify.Watcher webhookParameters *WebhookParameters ownerRefs []metav1.OwnerReference @@ -288,65 +283,25 @@ func rebuildWebhookConfigHelper( return &webhookConfig, nil } -// Reload the server's cert/key for TLS from file. -func (whc *WebhookConfigController) reloadKeyCert() { - pair, err := tls.LoadX509KeyPair(whc.webhookParameters.CertFile, whc.webhookParameters.KeyFile) - if err != nil { - reportValidationCertKeyUpdateError(err) - scope.Errorf("Cert/Key reload error: %v", err) - return - } - whc.mu.Lock() - whc.cert = &pair - whc.mu.Unlock() - - reportValidationCertKeyUpdate() - scope.Info("Cert and Key reloaded") -} - // NewWebhookConfigController manages validating webhook configuration. func NewWebhookConfigController(p WebhookParameters) (*WebhookConfigController, error) { - pair, err := tls.LoadX509KeyPair(p.CertFile, p.KeyFile) - if err != nil { - return nil, err - } - // This is not strictly necessary, but is a workaround for having the dashboard pass. The migration - // to OpenCensus metrics means that zero value metrics are not exported, and the dashboard tests - // expect data for metrics. - reportValidationCertKeyUpdate() - certKeyWatcher, err := fsnotify.NewWatcher() - if err != nil { - return nil, err - } - // watch the parent directory of the target files so we can catch - // symlink updates of k8s secrets - for _, file := range []string{p.CertFile, p.KeyFile, p.CACertFile, p.WebhookConfigFile} { - watchDir, _ := filepath.Split(file) - if err := certKeyWatcher.Watch(watchDir); err != nil { - return nil, fmt.Errorf("could not watch %v: %v", file, err) - } - } - // configuration must be updated whenever the caBundle changes. - // NOTE: Use a separate watcher to differentiate config/ca from cert/key updates. This is - // useful to avoid unnecessary updates and, more importantly, makes its easier to more - // accurately capture logs/metrics when files change. - configWatcher, err := fsnotify.NewWatcher() + // Configuration must be updated whenever the caBundle changes. watch the parent directory of + // the target files so we can catch symlink updates of k8s secrets. + fileWatcher, err := fsnotify.NewWatcher() if err != nil { return nil, err } for _, file := range []string{p.CACertFile, p.WebhookConfigFile} { watchDir, _ := filepath.Split(file) - if err := configWatcher.Watch(watchDir); err != nil { + if err := fileWatcher.Watch(watchDir); err != nil { return nil, fmt.Errorf("could not watch %v: %v", file, err) } } whc := &WebhookConfigController{ - keyCertWatcher: certKeyWatcher, - configWatcher: configWatcher, + configWatcher: fileWatcher, webhookParameters: &p, - cert: &pair, createInformerWebhookSource: defaultCreateInformerWebhookSource, } @@ -370,8 +325,7 @@ func NewWebhookConfigController(p WebhookParameters) (*WebhookConfigController, //reconcile monitors the keycert and webhook configuration changes, rebuild and reconcile the configuration func (whc *WebhookConfigController) reconcile(stopCh <-chan struct{}) { - defer whc.keyCertWatcher.Close() // nolint: errcheck - defer whc.configWatcher.Close() // nolint: errcheck + defer whc.configWatcher.Close() // nolint: errcheck // Try to create the initial webhook configuration (if it doesn't // already exist). Setup a persistent monitor to reconcile the @@ -384,7 +338,6 @@ func (whc *WebhookConfigController) reconcile(stopCh <-chan struct{}) { webhookChangedCh := whc.monitorWebhookChanges(stopCh) // use a timer to debounce file updates - var keyCertTimerC <-chan time.Time var configTimerC <-chan time.Time if retryAfterSetup { @@ -394,9 +347,6 @@ func (whc *WebhookConfigController) reconcile(stopCh <-chan struct{}) { var retrying bool for { select { - case <-keyCertTimerC: - keyCertTimerC = nil - whc.reloadKeyCert() case <-configTimerC: configTimerC = nil @@ -430,16 +380,10 @@ func (whc *WebhookConfigController) reconcile(stopCh <-chan struct{}) { if retry { time.AfterFunc(retryUpdateAfterFailureTimeout, func() { webhookChangedCh <- struct{}{} }) } - case event, more := <-whc.keyCertWatcher.Event: - if more && (event.IsModify() || event.IsCreate()) && keyCertTimerC == nil { - keyCertTimerC = time.After(watchDebounceDelay) - } case event, more := <-whc.configWatcher.Event: if more && (event.IsModify() || event.IsCreate()) && configTimerC == nil { configTimerC = time.After(watchDebounceDelay) } - case err := <-whc.keyCertWatcher.Error: - scope.Errorf("keyCertWatcher error: %v", err) case err := <-whc.configWatcher.Error: scope.Errorf("configWatcher error: %v", err) case <-stopCh: diff --git a/galley/pkg/crd/validation/config_test.go b/galley/pkg/crd/validation/config_test.go index 5ef660528ce4..dbd15caea038 100644 --- a/galley/pkg/crd/validation/config_test.go +++ b/galley/pkg/crd/validation/config_test.go @@ -16,7 +16,6 @@ package validation import ( "bytes" - "crypto/tls" "fmt" "io/ioutil" "os" @@ -26,7 +25,6 @@ import ( "testing" "github.com/ghodss/yaml" - "github.com/onsi/gomega" admissionregistrationv1beta1 "k8s.io/api/admissionregistration/v1beta1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -375,16 +373,6 @@ func initValidatingWebhookConfiguration() *admissionregistrationv1beta1.Validati } } -func checkCert(t *testing.T, whc *WebhookConfigController, cert, key []byte) bool { - t.Helper() - actual := whc.cert - expected, err := tls.X509KeyPair(cert, key) - if err != nil { - t.Fatalf("fail to load test certs.") - } - return bytes.Equal(actual.Certificate[0], expected.Certificate[0]) -} - func TestDeleteValidatingWebhookConfig(t *testing.T) { initConfig := initValidatingWebhookConfiguration() @@ -418,31 +406,6 @@ func TestDeleteValidatingWebhookConfig(t *testing.T) { }) } -func TestReloadCert(t *testing.T) { - whc, cleanup := createTestWebhookConfigController(t, - fake.NewSimpleClientset(), - createFakeWebhookSource(), - dummyConfig) - defer cleanup() - stop := make(chan struct{}) - defer func() { close(stop) }() - go whc.reconcile(stop) - checkCert(t, whc, testcerts.ServerCert, testcerts.ServerKey) - // Update cert/key files. - if err := ioutil.WriteFile(whc.webhookParameters.CertFile, testcerts.RotatedCert, 0644); err != nil { // nolint: vetshadow - cleanup() - t.Fatalf("WriteFile(%v) failed: %v", whc.webhookParameters.CertFile, err) - } - if err := ioutil.WriteFile(whc.webhookParameters.KeyFile, testcerts.RotatedKey, 0644); err != nil { // nolint: vetshadow - cleanup() - t.Fatalf("WriteFile(%v) failed: %v", whc.webhookParameters.KeyFile, err) - } - g := gomega.NewGomegaWithT(t) - g.Eventually(func() bool { - return checkCert(t, whc, testcerts.RotatedCert, testcerts.RotatedKey) - }, "10s", "100ms").Should(gomega.BeTrue()) -} - func TestLoadCaCertPem(t *testing.T) { cases := []struct { name string diff --git a/galley/pkg/crd/validation/validation.go b/galley/pkg/crd/validation/validation.go index e309ea4922e8..18dd4383a444 100644 --- a/galley/pkg/crd/validation/validation.go +++ b/galley/pkg/crd/validation/validation.go @@ -29,7 +29,6 @@ import ( "istio.io/pkg/probe" mixervalidate "istio.io/istio/mixer/pkg/validate" - "istio.io/istio/pkg/cmd" "istio.io/istio/pkg/config/schemas" "istio.io/istio/pkg/kube" ) @@ -73,7 +72,7 @@ func webhookHTTPSHandlerReady(client httpClient, vc *WebhookParameters) error { } //RunValidation start running Galley validation mode -func RunValidation(ready, stopCh chan struct{}, vc *WebhookParameters, kubeConfig string, +func RunValidation(ready chan<- struct{}, stopCh chan struct{}, vc *WebhookParameters, kubeConfig string, livenessProbeController, readinessProbeController probe.Controller) { log.Infof("Galley validation started with\n%s", vc) mixerValidator := mixervalidate.NewDefaultValidator(false) @@ -137,8 +136,6 @@ func RunValidation(ready, stopCh chan struct{}, vc *WebhookParameters, kubeConfi go wh.Run(ready, stopCh) defer wh.Stop() - - cmd.WaitSignal(stopCh) } // isDNS1123Label tests for a string that conforms to the definition of a label in diff --git a/galley/pkg/crd/validation/webhook.go b/galley/pkg/crd/validation/webhook.go index 1422c3547662..8ebcf241a3d3 100644 --- a/galley/pkg/crd/validation/webhook.go +++ b/galley/pkg/crd/validation/webhook.go @@ -21,10 +21,12 @@ import ( "fmt" "io/ioutil" "net/http" + "path/filepath" "sync" "time" "github.com/ghodss/yaml" + "github.com/howeyc/fsnotify" admissionv1beta1 "k8s.io/api/admission/v1beta1" "k8s.io/api/admissionregistration/v1beta1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -174,6 +176,8 @@ func DefaultArgs() *WebhookParameters { // Webhook implements the validating admission webhook for validating Istio configuration. type Webhook struct { + keyCertWatcher *fsnotify.Watcher + mu sync.RWMutex cert *tls.Certificate @@ -190,11 +194,29 @@ type Webhook struct { deploymentName string serviceName string webhookName string + keyFile string + certFile string // test hook for informers createInformerEndpointSource createInformerEndpointSource } +// Reload the server's cert/key for TLS from file. +func (wh *Webhook) reloadKeyCert() { + pair, err := tls.LoadX509KeyPair(wh.certFile, wh.keyFile) + if err != nil { + reportValidationCertKeyUpdateError(err) + scope.Errorf("Cert/Key reload error: %v", err) + return + } + wh.mu.Lock() + wh.cert = &pair + wh.mu.Unlock() + + reportValidationCertKeyUpdate() + scope.Info("Cert and Key reloaded") +} + // NewWebhook creates a new instance of the admission webhook controller. func NewWebhook(p WebhookParameters) (*Webhook, error) { pair, err := tls.LoadX509KeyPair(p.CertFile, p.KeyFile) @@ -202,10 +224,26 @@ func NewWebhook(p WebhookParameters) (*Webhook, error) { return nil, err } + // Configuration must be updated whenever the caBundle changes. Watch the parent directory of + // the target files so we can catch symlink updates of k8s secrets. + keyCertWatcher, err := fsnotify.NewWatcher() + if err != nil { + return nil, err + } + for _, file := range []string{p.CertFile, p.KeyFile} { + watchDir, _ := filepath.Split(file) + if err := keyCertWatcher.Watch(watchDir); err != nil { + return nil, fmt.Errorf("could not watch %v: %v", file, err) + } + } + wh := &Webhook{ server: &http.Server{ Addr: fmt.Sprintf(":%v", p.Port), }, + keyFile: p.KeyFile, + certFile: p.CertFile, + keyCertWatcher: keyCertWatcher, cert: &pair, descriptor: p.PilotDescriptor, validator: p.MixerValidator, @@ -234,7 +272,7 @@ func (wh *Webhook) Stop() { } // Run implements the webhook server -func (wh *Webhook) Run(ready chan struct{}, stopCh <-chan struct{}) { +func (wh *Webhook) Run(ready chan<- struct{}, stopCh <-chan struct{}) { go func() { if err := wh.server.ListenAndServeTLS("", ""); err != nil && err != http.ErrServerClosed { scope.Fatalf("admission webhook ListenAndServeTLS failed: %v", err) @@ -255,6 +293,24 @@ func (wh *Webhook) Run(ready chan struct{}, stopCh <-chan struct{}) { ready <- struct{}{} + // use a timer to debounce key/cert updates + var keyCertTimerC <-chan time.Time + + for { + select { + case <-keyCertTimerC: + keyCertTimerC = nil + wh.reloadKeyCert() + case event, more := <-wh.keyCertWatcher.Event: + if more && (event.IsModify() || event.IsCreate()) && keyCertTimerC == nil { + keyCertTimerC = time.After(watchDebounceDelay) + } + case err := <-wh.keyCertWatcher.Error: + scope.Errorf("configWatcher error: %v", err) + case <-stopCh: + return + } + } } func (wh *Webhook) getCert(*tls.ClientHelloInfo) (*tls.Certificate, error) { diff --git a/galley/pkg/crd/validation/webhook_test.go b/galley/pkg/crd/validation/webhook_test.go index 0c810c28bc47..c90ed33e5d91 100644 --- a/galley/pkg/crd/validation/webhook_test.go +++ b/galley/pkg/crd/validation/webhook_test.go @@ -16,6 +16,7 @@ package validation import ( "bytes" + "crypto/tls" "encoding/json" "errors" "fmt" @@ -29,6 +30,7 @@ import ( "time" "github.com/ghodss/yaml" + "github.com/onsi/gomega" admissionv1beta1 "k8s.io/api/admission/v1beta1" admissionregistrationv1beta1 "k8s.io/api/admissionregistration/v1beta1" v1 "k8s.io/api/core/v1" @@ -554,9 +556,9 @@ func TestServe(t *testing.T) { ready := make(chan struct{}) defer func() { close(stop) - close(ready) }() go wh.Run(ready, stop) + <-ready validReview := makeTestReview(t, true) invalidReview := makeTestReview(t, false) @@ -642,3 +644,43 @@ func TestServe(t *testing.T) { }) } } + +func checkCert(t *testing.T, whc *Webhook, cert, key []byte) bool { + t.Helper() + actual := whc.cert + expected, err := tls.X509KeyPair(cert, key) + if err != nil { + t.Fatalf("fail to load test certs.") + } + return bytes.Equal(actual.Certificate[0], expected.Certificate[0]) +} + +func TestReloadCert(t *testing.T) { + wh, cleanup := createTestWebhook(t, + fake.NewSimpleClientset(), + createFakeEndpointsSource(), + dummyConfig) + defer cleanup() + stop := make(chan struct{}) + ready := make(chan struct{}) + defer func() { + close(stop) + }() + go wh.Run(ready, stop) + <-ready + + checkCert(t, wh, testcerts.ServerCert, testcerts.ServerKey) + // Update cert/key files. + if err := ioutil.WriteFile(wh.certFile, testcerts.RotatedCert, 0644); err != nil { // nolint: vetshadow + cleanup() + t.Fatalf("WriteFile(%v) failed: %v", wh.certFile, err) + } + if err := ioutil.WriteFile(wh.keyFile, testcerts.RotatedKey, 0644); err != nil { // nolint: vetshadow + cleanup() + t.Fatalf("WriteFile(%v) failed: %v", wh.keyFile, err) + } + g := gomega.NewGomegaWithT(t) + g.Eventually(func() bool { + return checkCert(t, wh, testcerts.RotatedCert, testcerts.RotatedKey) + }, "10s", "100ms").Should(gomega.BeTrue()) +} diff --git a/galley/pkg/server/components/validation.go b/galley/pkg/server/components/validation.go index 3219c6870c79..200ba0c526e6 100644 --- a/galley/pkg/server/components/validation.go +++ b/galley/pkg/server/components/validation.go @@ -19,6 +19,7 @@ import ( "istio.io/istio/galley/pkg/crd/validation" "istio.io/istio/galley/pkg/server/process" + "istio.io/istio/pkg/cmd" ) // NewValidation returns a new validation component. @@ -35,6 +36,9 @@ func NewValidation(kubeConfig string, params *validation.WebhookParameters, live if params.EnableReconcileWebhookConfiguration { go validation.ReconcileWebhookConfiguration(webhookServerReady, stopCh, params, kubeConfig) } + if params.EnableValidation || params.EnableReconcileWebhookConfiguration { + go cmd.WaitSignal(stopCh) + } return nil }, // stop