Agent cache manager #233

walmav · 2017-10-10T21:44:12Z

agent cache manager:
renews expired SVID in cache
updates cache with new registration entry SVIDs
changed FetchSVID endpoint to use grpc streams

#TODO Increase test coverage for cache manager(48.9%)

fixes #233

evan2645 · 2017-10-10T22:30:34Z

pkg/agent/agent.go

-			break Drain
-		}
+	for err = range a.config.ErrorCh {
+		a.config.Log.Errorf(err.Error())


When ranging over a channel, the channel must be explicitly closed in order to break the range - otherwise, it will block forever

evan2645 · 2017-10-10T22:33:54Z

pkg/agent/agent.go

 		if err != nil {
 			return err
 		}
+		uri := a.config.TrustDomain
+		uri.Path = "spiffe/cp"
+		cacheManager := manager.NewManager(


Maybe this should be in cache package? It would then be cache.NewManager

makes sense will change the package

evan2645 · 2017-10-10T22:34:27Z

pkg/agent/agent.go

+		cacheManager := manager.NewManager(
+			a.Cache, a.serverCerts,
+			uri.String(), a.config.ServerAddress.String(),
+			a.config.ErrorCh, a.BaseSVID, a.baseSVIDKey, regEntries, a.config.Log)


We should use a config struct for all these

evan2645 · 2017-10-10T22:38:41Z

pkg/agent/agent.go

+			a.Cache, a.serverCerts,
+			uri.String(), a.config.ServerAddress.String(),
+			a.config.ErrorCh, a.BaseSVID, a.baseSVIDKey, regEntries, a.config.Log)
+		go cacheManager.UpdateCache(a.stop)


@boz had once recommended not passing channels around (particularly between packages), and after having played around with them a bit I tend to agree. Here's the pattern I used the last time I ran into this:

spire/pkg/server/endpoint/endpoint.go

Lines 178 to 210 in 18eb814

func (e *endpoint) listenAndServe() error {

grpcListener, err := net.Listen(e.grpcAddr.Network(), e.grpcAddr.String())

if err != nil {

return err

}

errChan := make(chan error)

go func() {

errChan <- e.grpcServer.Serve(grpcListener)

}()

go func() {

errChan <- e.httpServer.ListenAndServe()

}()

// Differentiate between shutdown and an actual error condition

err = <-errChan

if e.isCleanShutdown(err) {

// Ensure that the second server shutdown cleanly

err = <-errChan

if e.isCleanShutdown(err) {

return nil

}

return err

}

e.grpcServer.Stop()

_ = e.httpServer.Close()

_ = <-errChan

return err

}

And then on the consuming side:

spire/pkg/server/server.go

Line 153 in 18eb814

go func() { server.Config.ErrorCh <- server.endpoints.ListenAndServe() }()

I have found other common libraries to have similar block-forever behavior (like net/http server and grpc server)

Passing in a channel to signal when to stop isn't all that bad here -- it's similar to passing around a context.Context and having things listen to Done() -- but it gets pretty messy if it's not just closed to signal the shutdown.

If you want to continue using a channel, UpdateCache() should take a <-chan struct{} to ensure that the signaling only goes one way.

With that said, I think the general approach to these things in this project needs work. Yes, as @evan2645 said, things like Config.ErrorCh are red flags.

Try decoupling the objects - give them all a common interface of something along the lines of:

interface AsyncObject { Shutdown() // signals shutdown, internally the `context.CancelFunc` is called. Done() <- chan struct{} // channel is readable when all gofuncs have returned Err() error // blocks until done and then returns error if one occurred. }

Use context.Context for shutting down - when an object is created it can use context.WithCancel() to derive a new one for itself and also have a context.CancelFunc to use for it to shutdown itself.

For composite objects:

wait for any of the child objects to be Done() or for your context to be Done().

Shutdown() all children.

Wait for all children to be Done().

Derive an error from the Err() of the children.

return from main loop and close Done() channel.

With something like this then an object only needs to manage the lifecycle of its direct descendants and each object is only coupled to its parent via the context.Context that it was given.

Thank you @boz for explaining it so well. I will make the necessary changes to use context.Context instead of a stop channel and implement the decoupling pattern you explain here.

Welp, looks like I stand corrected :) blocking forever can be dangerous - I just hit this one in the very example I linked previously golang/go#20239

evan2645 · 2017-10-10T22:41:13Z

pkg/agent/cache/cache.go

@@ -31,6 +31,7 @@ type Cache interface {
 	Entry([]*common.Selector) (entry []CacheEntry)
 	SetEntry(cacheEntry CacheEntry)
 	DeleteEntry([]*common.Selector) (deleted bool)
+	GetEntries() map[string][]CacheEntry


nit: Entries?

evan2645 · 2017-10-10T22:55:27Z

proto/api/node/node.proto

@@ -87,7 +87,7 @@ service Node {
    /** Get Workload, Node Agent certs and CA trust bundles. Also used for rotation 
    Base Node SVID or the Registered Node SVID used for this call)
    List can be empty to allow Node Agent cache refresh). */
-    rpc FetchSVID(FetchSVIDRequest) returns (FetchSVIDResponse);
+    rpc FetchSVID(stream FetchSVIDRequest) returns (stream FetchSVIDResponse);


evan2645 · 2017-10-10T22:56:57Z

test/fixture/certs/baseSVID.pem

+jzQOkQ9Fgsxyb3MSBkdYD15zZxv8ZQ6LQRWKcVRqre3ym8TIiPHpTKwh5bbcraz6
+xTNvTPLU+n+ixjXf+Uu7SYaN9WpH8UPFH0pmi3z+z3mysgh+glW43AUUUTUqaS1m
+U3Y3FCpyg28KaxZQbDp/ucvm2KzFA9i03HtwmA==
+-----END CERTIFICATE-----


an svid.pem has been checked in that you can use or replace

yea the rebase pulled in two other pem. will consolidate.

evan2645 · 2017-10-10T22:57:47Z

test/fixture/certs/blogCSR.pem

+VFODJfDiU1hDrpDTxW2uWKS2m0Dd6yyH5ueudOavLxqbr/LosmMAq335zSxK72tO
+6TiszGCKZLnjFOzedZ1feg4mx+is8fHRz/IbWPzVbCKCE7NtujviV/ipi37J0JhJ
+0kLfE98TrrIKjTibJpINiuv7FOo5zSbtemOU5mFukmlWjApSsX0=
+-----END CERTIFICATE REQUEST-----


is this different than the existing blog_csr.pem fixture?

evan2645 · 2017-10-10T22:58:07Z

test/fixture/certs/blogSVID.pem

+Q47tsVSSs9OzKp2++krbsAhDo4EgUdK+b3R9vE+Ocq8h6XHZ6O7+7QGwlx+jtDL2
+AVd09l/tNnGY0ICp0JgZsY0yhrbWtigzSRpZzU8CWSftFk3A/9VKTLvwtNGTM0Xv
+paea0g==
+-----END CERTIFICATE-----


same here for blog_cert.pem?

evan2645 · 2017-10-10T22:59:22Z

test/fixture/registration/fixture.go

+	"github.com/spiffe/spire/proto/common"
+)
+
+func GetRegistrationEntries() []*common.RegistrationEntry {


Maybe this should go in test/util?

It could also read the content from https://github.com/spiffe/spire/blob/master/test/fixture/registration/good.json

boz

Looks like it needs some work on lifecycle management and error handling.

Let me know if you want some examples for lifecycle management options.

boz · 2017-10-11T11:17:41Z

pkg/agent/agent.go

@@ -90,6 +91,7 @@ func New(c *Config) *Agent {
 // and then blocks on the main event loop.
 func (a *Agent) Run() error {
 	a.Cache = cache.NewCache()
+	a.stop = make(chan struct{})


Any reason why these are initialized here? What happens if Run() is called twice? It's generally preferable to have as little mutation after construction as possible IMO.

makes sense. I will move these initializations to the newAgent.

boz · 2017-10-11T11:56:40Z

pkg/agent/agent.go

+			a.Cache, a.serverCerts,
+			uri.String(), a.config.ServerAddress.String(),
+			a.config.ErrorCh, a.BaseSVID, a.baseSVIDKey, regEntries, a.config.Log)
+		go cacheManager.UpdateCache(a.stop)


Passing in a channel to signal when to stop isn't all that bad here -- it's similar to passing around a context.Context and having things listen to Done() -- but it gets pretty messy if it's not just closed to signal the shutdown.

If you want to continue using a channel, UpdateCache() should take a <-chan struct{} to ensure that the signaling only goes one way.

With that said, I think the general approach to these things in this project needs work. Yes, as @evan2645 said, things like Config.ErrorCh are red flags.

Try decoupling the objects - give them all a common interface of something along the lines of:

interface AsyncObject { Shutdown() // signals shutdown, internally the `context.CancelFunc` is called. Done() <- chan struct{} // channel is readable when all gofuncs have returned Err() error // blocks until done and then returns error if one occurred. }

Use context.Context for shutting down - when an object is created it can use context.WithCancel() to derive a new one for itself and also have a context.CancelFunc to use for it to shutdown itself.

For composite objects:

wait for any of the child objects to be Done() or for your context to be Done().

Shutdown() all children.

Wait for all children to be Done().

Derive an error from the Err() of the children.

return from main loop and close Done() channel.

With something like this then an object only needs to manage the lifecycle of its direct descendants and each object is only coupled to its parent via the context.Context that it was given.

boz · 2017-10-11T12:01:01Z