-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve CertAuthorityWatcher #10403
Improve CertAuthorityWatcher #10403
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -427,20 +427,12 @@ func (s *remoteSite) compareAndSwapCertAuthority(ca types.CertAuthority) error { | |||||
return trace.CompareFailed("remote certificate authority rotation has been updated") | ||||||
} | ||||||
|
||||||
func (s *remoteSite) updateCertAuthorities(retry utils.Retry, remoteClusterVersion string) { | ||||||
s.Debugf("Watching for cert authority changes.") | ||||||
func (s *remoteSite) updateCertAuthorities(retry utils.Retry, remoteWatcher *services.CertAuthorityWatcher, remoteVersion string) { | ||||||
defer remoteWatcher.Close() | ||||||
|
||||||
cas := make(map[types.CertAuthType]types.CertAuthority) | ||||||
for { | ||||||
startedWaiting := s.clock.Now() | ||||||
select { | ||||||
case t := <-retry.After(): | ||||||
s.Debugf("Initiating new cert authority watch after waiting %v.", t.Sub(startedWaiting)) | ||||||
retry.Inc() | ||||||
case <-s.ctx.Done(): | ||||||
return | ||||||
} | ||||||
|
||||||
err := s.watchCertAuthorities(remoteClusterVersion) | ||||||
err := s.watchCertAuthorities(remoteWatcher, remoteVersion, cas) | ||||||
if err != nil { | ||||||
switch { | ||||||
case trace.IsNotFound(err): | ||||||
|
@@ -456,70 +448,88 @@ func (s *remoteSite) updateCertAuthorities(retry utils.Retry, remoteClusterVersi | |||||
} | ||||||
} | ||||||
|
||||||
startedWaiting := s.clock.Now() | ||||||
select { | ||||||
case t := <-retry.After(): | ||||||
s.Debugf("Initiating new cert authority watch after waiting %v.", t.Sub(startedWaiting)) | ||||||
retry.Inc() | ||||||
case <-s.ctx.Done(): | ||||||
return | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
func (s *remoteSite) watchCertAuthorities(remoteClusterVersion string) error { | ||||||
localWatchedTypes, err := s.getLocalWatchedCerts(remoteClusterVersion) | ||||||
func (s *remoteSite) watchCertAuthorities(remoteWatcher *services.CertAuthorityWatcher, remoteVersion string, cas map[types.CertAuthType]types.CertAuthority) error { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
|
||||||
targets, err := s.getLocalWatchedCerts(remoteVersion) | ||||||
if err != nil { | ||||||
return trace.Wrap(err) | ||||||
} | ||||||
|
||||||
localWatcher, err := services.NewCertAuthorityWatcher(s.ctx, services.CertAuthorityWatcherConfig{ | ||||||
ResourceWatcherConfig: services.ResourceWatcherConfig{ | ||||||
Component: teleport.ComponentProxy, | ||||||
Log: s, | ||||||
Clock: s.clock, | ||||||
Client: s.localAccessPoint, | ||||||
}, | ||||||
WatchCertTypes: localWatchedTypes, | ||||||
}) | ||||||
localWatch, err := s.srv.CertAuthorityWatcher.Subscribe(s.ctx, targets...) | ||||||
if err != nil { | ||||||
return trace.Wrap(err) | ||||||
} | ||||||
defer localWatcher.Close() | ||||||
defer func() { | ||||||
if err := localWatch.Close(); err != nil { | ||||||
s.WithError(err).Warn("Failed to close local ca watcher subscription.") | ||||||
} | ||||||
}() | ||||||
|
||||||
remoteWatcher, err := services.NewCertAuthorityWatcher(s.ctx, services.CertAuthorityWatcherConfig{ | ||||||
ResourceWatcherConfig: services.ResourceWatcherConfig{ | ||||||
Component: teleport.ComponentProxy, | ||||||
Log: s, | ||||||
Clock: s.clock, | ||||||
Client: s.remoteAccessPoint, | ||||||
remoteWatch, err := remoteWatcher.Subscribe( | ||||||
s.ctx, | ||||||
services.CertAuthorityTarget{ | ||||||
ClusterName: s.domainName, | ||||||
Type: types.HostCA, | ||||||
}, | ||||||
WatchCertTypes: []types.CertAuthType{types.HostCA}, | ||||||
}) | ||||||
) | ||||||
if err != nil { | ||||||
return trace.Wrap(err) | ||||||
} | ||||||
defer remoteWatcher.Close() | ||||||
defer func() { | ||||||
if err := remoteWatch.Close(); err != nil { | ||||||
s.WithError(err).Warn("Failed to close remote ca watcher subscription.") | ||||||
} | ||||||
}() | ||||||
|
||||||
s.Debugf("Watching for cert authority changes.") | ||||||
for { | ||||||
select { | ||||||
case <-s.ctx.Done(): | ||||||
s.WithError(s.ctx.Err()).Debug("Context is closing.") | ||||||
return trace.Wrap(s.ctx.Err()) | ||||||
case <-localWatcher.Done(): | ||||||
case <-localWatch.Done(): | ||||||
s.Warn("Local CertAuthority watcher subscription has closed") | ||||||
return fmt.Errorf("local ca watcher for cluster %s has closed", s.srv.ClusterName) | ||||||
case <-remoteWatcher.Done(): | ||||||
case <-remoteWatch.Done(): | ||||||
s.Warn("Remote CertAuthority watcher subscription has closed") | ||||||
return fmt.Errorf("remote ca watcher for cluster %s has closed", s.domainName) | ||||||
case cas := <-localWatcher.CertAuthorityC: | ||||||
for _, localCA := range cas { | ||||||
if localCA.GetClusterName() != s.srv.ClusterName || | ||||||
!localWatcher.IsWatched(localCA.GetType()) { | ||||||
case evt := <-localWatch.Events(): | ||||||
switch evt.Type { | ||||||
case types.OpPut: | ||||||
localCA, ok := evt.Resource.(types.CertAuthority) | ||||||
if !ok { | ||||||
continue | ||||||
} | ||||||
|
||||||
ca, ok := cas[localCA.GetType()] | ||||||
if ok && services.CertAuthoritiesEquivalent(ca, localCA) { | ||||||
continue | ||||||
} | ||||||
|
||||||
// clone to prevent a race with watcher filtering | ||||||
localCA = localCA.Clone() | ||||||
if err := s.remoteClient.RotateExternalCertAuthority(s.ctx, localCA); err != nil { | ||||||
s.WithError(err).Warn("Failed to rotate external ca") | ||||||
log.WithError(err).Warn("Failed to rotate external ca") | ||||||
return trace.Wrap(err) | ||||||
} | ||||||
|
||||||
cas[localCA.GetType()] = localCA | ||||||
} | ||||||
case cas := <-remoteWatcher.CertAuthorityC: | ||||||
for _, remoteCA := range cas { | ||||||
if remoteCA.GetType() != types.HostCA || | ||||||
remoteCA.GetClusterName() != s.domainName { | ||||||
case evt := <-remoteWatch.Events(): | ||||||
switch evt.Type { | ||||||
case types.OpPut: | ||||||
remoteCA, ok := evt.Resource.(types.CertAuthority) | ||||||
if !ok { | ||||||
continue | ||||||
} | ||||||
|
||||||
|
@@ -549,8 +559,17 @@ func (s *remoteSite) watchCertAuthorities(remoteClusterVersion string) error { | |||||
} | ||||||
|
||||||
// getLocalWatchedCerts returns local certificates types that should be watched by the cert authority watcher. | ||||||
func (s *remoteSite) getLocalWatchedCerts(remoteClusterVersion string) ([]types.CertAuthType, error) { | ||||||
localWatchedTypes := []types.CertAuthType{types.HostCA, types.UserCA} | ||||||
func (s *remoteSite) getLocalWatchedCerts(remoteClusterVersion string) ([]services.CertAuthorityTarget, error) { | ||||||
localWatchedTypes := []services.CertAuthorityTarget{ | ||||||
{ | ||||||
Type: types.HostCA, | ||||||
ClusterName: s.srv.ClusterName, | ||||||
}, | ||||||
{ | ||||||
Type: types.UserCA, | ||||||
ClusterName: s.srv.ClusterName, | ||||||
}, | ||||||
} | ||||||
|
||||||
// Delete in 11.0. | ||||||
ver10orAbove, err := utils.MinVerWithoutPreRelease(remoteClusterVersion, constants.DatabaseCAMinVersion) | ||||||
|
@@ -559,7 +578,7 @@ func (s *remoteSite) getLocalWatchedCerts(remoteClusterVersion string) ([]types. | |||||
} | ||||||
|
||||||
if ver10orAbove { | ||||||
localWatchedTypes = append(localWatchedTypes, types.DatabaseCA) | ||||||
localWatchedTypes = append(localWatchedTypes, services.CertAuthorityTarget{ClusterName: s.srv.ClusterName, Type: types.DatabaseCA}) | ||||||
} else { | ||||||
s.Debugf("Connected to remote cluster of version %s. Database CA won't be propagated.", remoteClusterVersion) | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -919,10 +919,8 @@ type CertAuthorityWatcherConfig struct { | |||||
ResourceWatcherConfig | ||||||
// AuthorityGetter is responsible for fetching cert authority resources. | ||||||
AuthorityGetter | ||||||
// CertAuthorityC receives up-to-date list of all cert authority resources. | ||||||
CertAuthorityC chan []types.CertAuthority | ||||||
// WatchCertTypes stores all certificate types that should be monitored. | ||||||
WatchCertTypes []types.CertAuthType | ||||||
// Types restricts which cert authority types are retrieved via the AuthorityGetter. | ||||||
Types []types.CertAuthType | ||||||
} | ||||||
|
||||||
// CheckAndSetDefaults checks parameters and sets default values. | ||||||
|
@@ -937,15 +935,12 @@ func (cfg *CertAuthorityWatcherConfig) CheckAndSetDefaults() error { | |||||
} | ||||||
cfg.AuthorityGetter = getter | ||||||
} | ||||||
if cfg.CertAuthorityC == nil { | ||||||
cfg.CertAuthorityC = make(chan []types.CertAuthority) | ||||||
} | ||||||
return nil | ||||||
} | ||||||
|
||||||
// IsWatched return true if the given certificate auth type is being observer by the watcher. | ||||||
func (cfg *CertAuthorityWatcherConfig) IsWatched(certType types.CertAuthType) bool { | ||||||
for _, observedType := range cfg.WatchCertTypes { | ||||||
for _, observedType := range cfg.Types { | ||||||
if observedType == certType { | ||||||
return true | ||||||
} | ||||||
|
@@ -961,13 +956,20 @@ func NewCertAuthorityWatcher(ctx context.Context, cfg CertAuthorityWatcherConfig | |||||
|
||||||
collector := &caCollector{ | ||||||
CertAuthorityWatcherConfig: cfg, | ||||||
fanout: NewFanout(), | ||||||
cas: make(map[types.CertAuthType]map[string]types.CertAuthority, len(cfg.Types)), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
I also don't mind renaming the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I didn't know these were added, but looking at them, they seem really specific to the CAWatcher internals and I'm not entirely convinced that they should even be exported. // CertAuthorityMap maps clusterName -> types.CertAuthority
type CertAuthorityMap map[string]types.CertAuthority
// CertAuthorityTypeMap maps types.CertAuthType -> map(clusterName -> types.CertAuthority)
type CertAuthorityTypeMap map[types.CertAuthType]CertAuthorityMap There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not even entirely sure they are needed any more after migrating to use the fanout. Thoughts on just removing them? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't mind removing them, just |
||||||
} | ||||||
|
||||||
for _, t := range cfg.Types { | ||||||
collector.cas[t] = make(map[string]types.CertAuthority) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
|
||||||
} | ||||||
|
||||||
watcher, err := newResourceWatcher(ctx, collector, cfg.ResourceWatcherConfig) | ||||||
if err != nil { | ||||||
return nil, trace.Wrap(err) | ||||||
} | ||||||
|
||||||
collector.fanout.SetInit() | ||||||
return &CertAuthorityWatcher{watcher, collector}, nil | ||||||
} | ||||||
|
||||||
|
@@ -980,26 +982,66 @@ type CertAuthorityWatcher struct { | |||||
// caCollector accompanies resourceWatcher when monitoring cert authority resources. | ||||||
type caCollector struct { | ||||||
CertAuthorityWatcherConfig | ||||||
fanout *Fanout | ||||||
|
||||||
collectedCAs CertAuthorityTypeMap | ||||||
lock sync.RWMutex | ||||||
// lock protects concurrent access to cas | ||||||
lock sync.RWMutex | ||||||
// cas maps ca type -> cluster -> ca | ||||||
cas map[types.CertAuthType]map[string]types.CertAuthority | ||||||
} | ||||||
|
||||||
// CertAuthorityMap maps clusterName -> types.CertAuthority | ||||||
type CertAuthorityMap map[string]types.CertAuthority | ||||||
// CertAuthorityTarget lists the attributes of interactions to be disabled. | ||||||
type CertAuthorityTarget struct { | ||||||
// ClusterName specifies the name of the cluster to watch. | ||||||
ClusterName string | ||||||
// Type specifies the ca types to watch for. | ||||||
Type types.CertAuthType | ||||||
} | ||||||
|
||||||
// CertAuthorityTypeMap maps types.CertAuthType -> map(clusterName -> types.CertAuthority) | ||||||
type CertAuthorityTypeMap map[types.CertAuthType]CertAuthorityMap | ||||||
// Subscribe is used to subscribe to the lock updates. | ||||||
func (c *caCollector) Subscribe(ctx context.Context, targets ...CertAuthorityTarget) (types.Watcher, error) { | ||||||
watchKinds, err := caTargetToWatchKinds(targets) | ||||||
if err != nil { | ||||||
return nil, trace.Wrap(err) | ||||||
} | ||||||
sub, err := c.fanout.NewWatcher(ctx, types.Watch{Kinds: watchKinds}) | ||||||
if err != nil { | ||||||
return nil, trace.Wrap(err) | ||||||
} | ||||||
select { | ||||||
case event := <-sub.Events(): | ||||||
if event.Type != types.OpInit { | ||||||
return nil, trace.BadParameter("expected init event, got %v instead", event.Type) | ||||||
} | ||||||
case <-sub.Done(): | ||||||
return nil, trace.Wrap(sub.Error()) | ||||||
} | ||||||
return sub, nil | ||||||
} | ||||||
|
||||||
// ToSlice converts CertAuthorityTypeMap to a slice. | ||||||
func (cat *CertAuthorityTypeMap) ToSlice() []types.CertAuthority { | ||||||
slice := make([]types.CertAuthority, 0) | ||||||
for _, cert := range *cat { | ||||||
for _, ca := range cert { | ||||||
slice = append(slice, ca) | ||||||
func caTargetToWatchKinds(targets []CertAuthorityTarget) ([]types.WatchKind, error) { | ||||||
watchKinds := make([]types.WatchKind, 0, len(targets)) | ||||||
for _, target := range targets { | ||||||
kind := types.WatchKind{ | ||||||
Kind: types.KindCertAuthority, | ||||||
// Note that watching SubKind doesn't work for types.WatchKind - to do so it would | ||||||
// require a custom filter, which was recently added but - we can't use yet due to | ||||||
// older clients not supporting the filter. | ||||||
SubKind: string(target.Type), | ||||||
} | ||||||
|
||||||
if target.ClusterName != "" { | ||||||
kind.Name = target.ClusterName | ||||||
} | ||||||
|
||||||
watchKinds = append(watchKinds, kind) | ||||||
} | ||||||
return slice | ||||||
|
||||||
if len(watchKinds) == 0 { | ||||||
watchKinds = []types.WatchKind{{Kind: types.KindCertAuthority}} | ||||||
} | ||||||
|
||||||
return watchKinds, nil | ||||||
} | ||||||
|
||||||
// resourceKind specifies the resource kind to watch. | ||||||
|
@@ -1009,28 +1051,27 @@ func (c *caCollector) resourceKind() string { | |||||
|
||||||
// getResourcesAndUpdateCurrent refreshes the list of current resources. | ||||||
func (c *caCollector) getResourcesAndUpdateCurrent(ctx context.Context) error { | ||||||
updatedCerts := make(CertAuthorityTypeMap) | ||||||
var cas []types.CertAuthority | ||||||
|
||||||
for _, caType := range c.WatchCertTypes { | ||||||
cas, err := c.AuthorityGetter.GetCertAuthorities(ctx, caType, false) | ||||||
for _, t := range c.Types { | ||||||
authorities, err := c.AuthorityGetter.GetCertAuthorities(ctx, t, false) | ||||||
if err != nil { | ||||||
return trace.Wrap(err) | ||||||
} | ||||||
|
||||||
updatedCerts[caType] = make(CertAuthorityMap, len(cas)) | ||||||
for _, ca := range cas { | ||||||
updatedCerts[caType][ca.GetName()] = ca | ||||||
} | ||||||
cas = append(cas, authorities...) | ||||||
} | ||||||
|
||||||
c.lock.Lock() | ||||||
c.collectedCAs = updatedCerts | ||||||
c.lock.Unlock() | ||||||
defer c.lock.Unlock() | ||||||
|
||||||
select { | ||||||
case <-ctx.Done(): | ||||||
return trace.Wrap(ctx.Err()) | ||||||
case c.CertAuthorityC <- updatedCerts.ToSlice(): | ||||||
for _, ca := range cas { | ||||||
if !c.watchingType(ca.GetType()) { | ||||||
continue | ||||||
} | ||||||
|
||||||
c.cas[ca.GetType()][ca.GetName()] = ca | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you know if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. Updated to only add them if configured to watch for that ca type now |
||||||
c.fanout.Emit(types.Event{Type: types.OpPut, Resource: ca.Clone()}) | ||||||
} | ||||||
return nil | ||||||
} | ||||||
|
@@ -1046,46 +1087,44 @@ func (c *caCollector) processEventAndUpdateCurrent(ctx context.Context, event ty | |||||
switch event.Type { | ||||||
case types.OpDelete: | ||||||
caType := types.CertAuthType(event.Resource.GetSubKind()) | ||||||
|
||||||
// Check if the certificate should be processed. | ||||||
_, found := c.collectedCAs[caType] | ||||||
if found { | ||||||
delete(c.collectedCAs[caType], event.Resource.GetName()) | ||||||
if !c.watchingType(caType) { | ||||||
return | ||||||
} | ||||||
|
||||||
select { | ||||||
case <-ctx.Done(): | ||||||
case c.CertAuthorityC <- c.collectedCAs.ToSlice(): | ||||||
} | ||||||
delete(c.cas[caType], event.Resource.GetName()) | ||||||
rosstimothy marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
c.fanout.Emit(event) | ||||||
case types.OpPut: | ||||||
ca, ok := event.Resource.(types.CertAuthority) | ||||||
if !ok { | ||||||
c.Log.Warnf("Unexpected resource type %T.", event.Resource) | ||||||
return | ||||||
} | ||||||
|
||||||
caType := ca.GetType() | ||||||
_, found := c.collectedCAs[caType] | ||||||
// Check if the certificate should be processed. | ||||||
if found { | ||||||
c.collectedCAs[caType][ca.GetName()] = ca | ||||||
if !c.watchingType(ca.GetType()) { | ||||||
return | ||||||
} | ||||||
|
||||||
select { | ||||||
case <-ctx.Done(): | ||||||
case c.CertAuthorityC <- c.collectedCAs.ToSlice(): | ||||||
authority, ok := c.cas[ca.GetType()][ca.GetName()] | ||||||
if ok && CertAuthoritiesEquivalent(authority, ca) { | ||||||
return | ||||||
} | ||||||
|
||||||
c.cas[ca.GetType()][ca.GetName()] = ca | ||||||
c.fanout.Emit(event) | ||||||
rosstimothy marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
default: | ||||||
c.Log.Warnf("Unsupported event type %s.", event.Type) | ||||||
return | ||||||
} | ||||||
} | ||||||
|
||||||
// GetCurrent returns the currently stored authorities. | ||||||
func (c *caCollector) GetCurrent() []types.CertAuthority { | ||||||
c.lock.RLock() | ||||||
defer c.lock.RUnlock() | ||||||
return c.collectedCAs.ToSlice() | ||||||
func (c *caCollector) watchingType(t types.CertAuthType) bool { | ||||||
for _, caType := range c.Types { | ||||||
if caType == t { | ||||||
return true | ||||||
} | ||||||
} | ||||||
|
||||||
return false | ||||||
} | ||||||
|
||||||
func (c *caCollector) notifyStale() {} | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: