Skip to content

Commit

Permalink
fix galley validation key/cert rotation (#17995)
Browse files Browse the repository at this point in the history 
* make validation watch and reload key/certs (again)

Galley watched and reloaded key/certs prior to istio
1.3. #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 #17718

* add integration test and fix some bugs

* fix dashboard test, linter errors, and TestReloadConfig

* print cert info on reload

* improve logging

* deflake pod fetch
  • Loading branch information
@ayj @istio-testing
ayj authored and istio-testing committed Oct 18, 2019
1 parent 156d828 commit 39635d7
Show file tree
Hide file tree
Showing 7 changed files with 269 additions and 117 deletions.
68 changes: 6 additions & 62 deletions galley/pkg/crd/validation/config.go
View file
Expand Up @@ -15,7 +15,6 @@
package validation

import (
"crypto/tls"
"crypto/x509"
"encoding/pem"
"errors"
Expand All @@ -25,7 +24,6 @@ import (
"os"
"path/filepath"
"reflect"
"sync"
"time"

"github.com/ghodss/yaml"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
}

Expand All @@ -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
Expand All @@ -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 {
Expand All @@ -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

Expand Down Expand Up @@ -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:
Expand Down
43 changes: 26 additions & 17 deletions galley/pkg/crd/validation/config_test.go
View file
Expand Up @@ -16,7 +16,6 @@ package validation

import (
"bytes"
"crypto/tls"
"fmt"
"io/ioutil"
"os"
Expand All @@ -41,6 +40,9 @@ import (
var (
failurePolicyFailVal = admissionregistrationv1beta1.Fail
failurePolicyFail = &failurePolicyFailVal

failurePolicyIgnoreVal = admissionregistrationv1beta1.Ignore
failurePolicyIgnore = &failurePolicyIgnoreVal
)

func createTestWebhookConfigController(
Expand Down Expand Up @@ -375,16 +377,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()
Expand Down Expand Up @@ -418,7 +410,7 @@ func TestDeleteValidatingWebhookConfig(t *testing.T) {
})
}

func TestReloadCert(t *testing.T) {
func TestReloadConfig(t *testing.T) {
whc, cleanup := createTestWebhookConfigController(t,
fake.NewSimpleClientset(),
createFakeWebhookSource(),
Expand All @@ -427,19 +419,36 @@ func TestReloadCert(t *testing.T) {
stop := make(chan struct{})
defer func() { close(stop) }()
go whc.reconcile(stop)
checkCert(t, whc, testcerts.ServerCert, testcerts.ServerKey)

g := gomega.NewGomegaWithT(t)

g.Eventually(func() bool {
if whc.webhookConfiguration == nil {
return false
}
return *whc.webhookConfiguration.Webhooks[0].FailurePolicy == failurePolicyFailVal
}, "10s", "100ms").Should(gomega.BeTrue())

updatedConfig := dummyConfig.DeepCopy()
updatedConfig.Webhooks[0].FailurePolicy = failurePolicyIgnore
updatedConfigBytes, err := yaml.Marshal(&updatedConfig)
if err != nil {
t.Fatalf("failed to create updated webhook config: %v", err)
}

// Update cert/key files.
if err := ioutil.WriteFile(whc.webhookParameters.CertFile, testcerts.RotatedCert, 0644); err != nil { // nolint: vetshadow
if err := ioutil.WriteFile(whc.webhookParameters.CACertFile, 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
if err := ioutil.WriteFile(whc.webhookParameters.WebhookConfigFile, updatedConfigBytes, 0644); err != nil { // nolint: vetshadow
cleanup()
t.Fatalf("WriteFile(%v) failed: %v", whc.webhookParameters.KeyFile, err)
}
g := gomega.NewGomegaWithT(t)

// wait for config to update
g.Eventually(func() bool {
return checkCert(t, whc, testcerts.RotatedCert, testcerts.RotatedKey)
return *whc.webhookConfiguration.Webhooks[0].FailurePolicy == failurePolicyIgnoreVal
}, "10s", "100ms").Should(gomega.BeTrue())
}

Expand Down
10 changes: 3 additions & 7 deletions galley/pkg/crd/validation/validation.go
View file
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -114,13 +113,13 @@ func RunValidation(ready, stopCh chan struct{}, vc *WebhookParameters, kubeConfi
for {
if err := webhookHTTPSHandlerReady(client, vc); err != nil {
validationReadinessProbe.SetAvailable(errors.New("not ready"))
scope.Infof("https handler for validation webhook is not ready: %v", err)
scope.Infof("https handler for validation webhook is not ready: %v\n", err)
ready = false
} else {
validationReadinessProbe.SetAvailable(nil)

if !ready {
scope.Info("https handler for validation webhook is ready")
scope.Info("https handler for validation webhook is ready\n")
ready = true
}
}
Expand All @@ -136,9 +135,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
Expand Down

0 comments on commit 39635d7

Please sign in to comment.