Improve CertAuthorityWatcher #10403

rosstimothy · 2022-02-16T17:55:05Z

CertAuthorityWatcher and its usage are refactored to allow for
all the following:

eliminate retransmission of the same CAs
reduce memory usage by having one local watcher per proxy
adds the ability to filter only the CAs that are desired
reduce the time required to send the first CAs

watchCertAuthorities now compares all CAs it receives from the
watcher with the previous CA of the same type and only sends to
the remote site if they are not identical. This is to reduce
unnecessary network traffic which can be problematic for a
root cluster with a larger number of leafs.

The CertAuthorityWatcher is refactored to leverage a fanout
to emit events to any number of watchers, each subscription
can be for a subset of the configured CA types. The proxy
now has only one CertAuthorityWatcher that is passed around
similarly to the LockWatcher. This reduces the memory usage
for proxies, which prior to this has one local CAWatcher per
remote site.

updateCertAuthorities no longer waits on the utils.Retry it
is provided with before starting to watch CAs. By doing this
the proxy no longer has to wait ~8 minutes before it even
starts to watch CAs.

lib/reversetunnel/srv.go

espadolini · 2022-05-17T12:20:03Z

lib/services/watcher.go

-		return trace.Wrap(ctx.Err())
-	case c.CertAuthorityC <- updatedCerts.ToSlice():
+	for _, ca := range cas {
+		c.cas[ca.GetType()][ca.GetName()] = ca


Do you know if GetCertAuthorities is guaranteed to doublecheck the type of the CAs? If not we should probably doublecheck it here.

Good catch. Updated to only add them if configured to watch for that ca type now

CertAuthorityWatcher and its usage are refactored to allow for all the following: - eliminate retransmission of the same CAs - reduce memory usage by having one local watcher per proxy - adds the ability to filter only the CAs that are desired - reduce the time required to send the first CAs watchCertAuthorities now compares all CAs it receives from the watcher with the previous CA of the same type and only sends to the remote site if they are not identical. This is to reduce unnecessary network traffic which can be problematic for a root cluster with a larger number of leafs. The CertAuthorityWatcher is refactored to leverage a fanout to emit events to any number of watchers, each subscription can be for a subset of the configured CA types. The proxy now has only one CertAuthorityWatcher that is passed around similarly to the LockWatcher. This reduces the memory usage for proxies, which prior to this has one local CAWatcher per remote site. updateCertAuthorities no longer waits on the utils.Retry it is provided with before starting to watch CAs. By doing this the proxy no longer has to wait ~8 minutes before it even starts to watch CAs.

jakule · 2022-05-17T14:34:43Z

lib/services/watcher.go

@@ -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)),


nit:

Suggested change

cas: make(map[types.CertAuthType]map[string]types.CertAuthority, len(cfg.Types)),

cas: make(types.CertAuthorityTypeMap, len(cfg.Types)),

I also don't mind renaming the CertAuthorityTypeMap if you have any better idea.

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

I'm not even entirely sure they are needed any more after migrating to use the fanout. Thoughts on just removing them?

I don't mind removing them, just map[types.CertAuthType]map[string]types.CertAuthority looks a bit "scary" in my opinion.

jakule · 2022-05-17T14:35:04Z

lib/services/watcher.go

+	}
+
+	for _, t := range cfg.Types {
+		collector.cas[t] = make(map[string]types.CertAuthority)


nit:

Suggested change

collector.cas[t] = make(map[string]types.CertAuthority)

collector.cas[t] = make(CertAuthorityMap)

jakule · 2022-05-17T14:35:48Z

lib/services/watcher.go

@@ -980,9 +982,63 @@ type CertAuthorityWatcher struct {
 // caCollector accompanies resourceWatcher when monitoring cert authority resources.
 type caCollector struct {
 	CertAuthorityWatcherConfig
+	lock   sync.RWMutex
+	cas    map[types.CertAuthType]map[string]types.CertAuthority


nit:

Suggested change

cas map[types.CertAuthType]map[string]types.CertAuthority

cas CertAuthorityTypeMap

lib/services/watcher.go

integration/db_integration_test.go

jakule · 2022-05-17T14:58:46Z

lib/reversetunnel/remotesite.go


+	cas := make(map[types.CertAuthType]types.CertAuthority)


nit:

Suggested change

cas := make(map[types.CertAuthType]types.CertAuthority)

cas := make(CertAuthorityMap)

jakule · 2022-05-17T14:59:22Z

lib/reversetunnel/remotesite.go

 	}
 }

-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 {


nit:

Suggested change

func (s *remoteSite) watchCertAuthorities(remoteWatcher *services.CertAuthorityWatcher, remoteVersion string, cas map[types.CertAuthType]types.CertAuthority) error {

func (s *remoteSite) watchCertAuthorities(remoteWatcher *services.CertAuthorityWatcher, remoteVersion string, cas CertAuthorityMap) error {

jakule · 2022-05-17T15:07:14Z

Unit test failure looks like something related:

    sshserver_test.go:1888: 
        	Error Trace:	sshserver_test.go:1888
        	Error:      	Received unexpected error:
        	            	missing parameter CertAuthorityWatcher
        	Test:       	TestIgnorePuTTYSimpleChannel

github-actions · 2022-05-17T19:07:51Z

@rosstimothy See the table below for backport results.

Branch	Result
branch/v7	Failed
branch/v8	Failed
branch/v9	Failed

rosstimothy · 2022-05-17T20:55:31Z

💚 All backports created successfully

Status	Branch	Result
✅	branch/v7
✅	branch/v8
✅	branch/v9

Questions ?

Please refer to the Backport tool documentation

* Improve CertAuthorityWatcher CertAuthorityWatcher and its usage are refactored to allow for all the following: - eliminate retransmission of the same CAs - reduce memory usage by having one local watcher per proxy - adds the ability to filter only the CAs that are desired - reduce the time required to send the first CAs watchCertAuthorities now compares all CAs it receives from the watcher with the previous CA of the same type and only sends to the remote site if they are not identical. This is to reduce unnecessary network traffic which can be problematic for a root cluster with a larger number of leafs. The CertAuthorityWatcher is refactored to leverage a fanout to emit events to any number of watchers, each subscription can be for a subset of the configured CA types. The proxy now has only one CertAuthorityWatcher that is passed around similarly to the LockWatcher. This reduces the memory usage for proxies, which prior to this has one local CAWatcher per remote site. updateCertAuthorities no longer waits on the utils.Retry it is provided with before starting to watch CAs. By doing this the proxy no longer has to wait ~8 minutes before it even starts to watch CAs. (cherry picked from commit 1ac0957)

espadolini self-requested a review February 16, 2022 17:57

rosstimothy force-pushed the tross/fix_ca_watcher branch from d2eed85 to 724f644 Compare February 23, 2022 15:10

rosstimothy force-pushed the tross/fix_ca_watcher branch 3 times, most recently from ab57537 to 5b12c90 Compare May 11, 2022 17:48

rosstimothy requested a review from jakule May 11, 2022 19:23

rosstimothy marked this pull request as ready for review May 11, 2022 19:23

github-actions bot requested review from capnspacehook and probakowski May 11, 2022 19:23

rosstimothy removed request for probakowski and capnspacehook May 11, 2022 19:23

rosstimothy added backport/branch/v9 labels May 11, 2022

espadolini approved these changes May 17, 2022

View reviewed changes

rosstimothy added 2 commits May 17, 2022 09:23

address feedback

58b5d5c

rosstimothy force-pushed the tross/fix_ca_watcher branch from a420eba to 58b5d5c Compare May 17, 2022 13:29

jakule reviewed May 17, 2022

View reviewed changes

fix tests and clean up ca map types

b8ba1f1

jakule approved these changes May 17, 2022

View reviewed changes

Merge branch 'master' into tross/fix_ca_watcher

dc9b3b4

rosstimothy enabled auto-merge (squash) May 17, 2022 17:02

rosstimothy merged commit 1ac0957 into master May 17, 2022

rosstimothy mentioned this pull request May 17, 2022

[branch/v7] Improve CertAuthorityWatcher (#10403) #12722

Merged

rosstimothy mentioned this pull request May 17, 2022

[branch/v8] Improve CertAuthorityWatcher (#10403) #12723

Merged

rosstimothy mentioned this pull request May 17, 2022

[branch/v9] Improve CertAuthorityWatcher (#10403) #12724

Merged

webvictim mentioned this pull request Jun 8, 2022

Opened in error #13285

Closed

alexatcanva mentioned this pull request Oct 13, 2022

BUGFIX | Fix Teleport ALPN Proxy not being HTTP CONNECT Proxy Aware alexatcanva/teleport#30

Open

rosstimothy deleted the tross/fix_ca_watcher branch January 5, 2023 20:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve CertAuthorityWatcher #10403

Improve CertAuthorityWatcher #10403

rosstimothy commented Feb 16, 2022 •

edited

Loading

espadolini May 17, 2022

rosstimothy May 17, 2022

jakule May 17, 2022

rosstimothy May 17, 2022

rosstimothy May 17, 2022

jakule May 17, 2022

jakule May 17, 2022

jakule May 17, 2022

jakule May 17, 2022

jakule May 17, 2022

jakule commented May 17, 2022

github-actions bot commented May 17, 2022

rosstimothy commented May 17, 2022

	cas: make(map[types.CertAuthType]map[string]types.CertAuthority, len(cfg.Types)),
	cas: make(types.CertAuthorityTypeMap, len(cfg.Types)),

	collector.cas[t] = make(map[string]types.CertAuthority)
	collector.cas[t] = make(CertAuthorityMap)

	cas map[types.CertAuthType]map[string]types.CertAuthority
	cas CertAuthorityTypeMap

	cas := make(map[types.CertAuthType]types.CertAuthority)
	cas := make(CertAuthorityMap)

	func (s remoteSite) watchCertAuthorities(remoteWatcher services.CertAuthorityWatcher, remoteVersion string, cas map[types.CertAuthType]types.CertAuthority) error {
	func (s remoteSite) watchCertAuthorities(remoteWatcher services.CertAuthorityWatcher, remoteVersion string, cas CertAuthorityMap) error {

Improve CertAuthorityWatcher #10403

Improve CertAuthorityWatcher #10403

Conversation

rosstimothy commented Feb 16, 2022 • edited Loading

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

jakule commented May 17, 2022

github-actions bot commented May 17, 2022

rosstimothy commented May 17, 2022

💚 All backports created successfully

Questions ?

rosstimothy commented Feb 16, 2022 •

edited

Loading