Add agent bootstrap logic #94

evan2645 · 2017-08-30T00:56:24Z

This is an initial pass at the spire agent bootstrap logic. This change includes all the core business logic for spire agent to get up and attest against spire server. It also does a few other things:

Reorganizes agent codebase to be more idiomatic
Commits fixes to keymanager
Fixes node attestor following upstream interface changes
Checks in dev config and trust bundle

Tests are coming soon. Getting this out early for review

Outstanding:

Plugin manager should call Configure

Commit light refactor of agent and cli code

boz · 2017-08-30T01:07:07Z

pkg/agent/agent.go

+	PluginTypeMap = map[string]plugin.Plugin{
+		"KeyManager":       &keymanager.KeyManagerPlugin{},
+		"NodeAttestor":     &nodeattestor.NodeAttestorPlugin{},
+		"WorkloadAttestor": &workloadattestor.WorkloadAttestorPlugin{},


maybe have const PluginType = "KeyManager" in keymanagerso u can usekeymanager.PluginType here

That's a good idea! That pattern will probably help to solve some of the other challenges we have been facing with plugin management

Most of the code related to this is pre-existing and this PR just moved it from one place to another. We also need to send a change to the plugin catalog to handle a call to Configure so if it's OK, I'm thinking to reserve this for that PR since we'd probably want to update it across the board (server + agent).

boz · 2017-08-30T01:08:16Z

pkg/agent/agent.go

+	MaxPlugins = map[string]int{
+		"KeyManager":       1,
+		"NodeAttestor":     1,
+		"WorkloadAttestor": 1,


boz · 2017-08-30T01:11:31Z

pkg/agent/agent.go

+
+	go func() {
+		listener, err := net.Listen(a.Config.BindAddress.Network(), a.Config.BindAddress.String())
+		if err != nil {


Is there a way to gracefully shut down the connection? I vaguely remember being able to make a listener that uses context.Context.

Good thought, I'll investigate this. Same code exists in spire server we'll need to update it there too

@boz I looked into this. No support for context that I could find, however I was able to get graceful shutdown by maintaining a pointer to the grpc server on the agent struct: 7d54205

boz · 2017-08-30T01:12:22Z

pkg/agent/agent.go

+	a.Config.Log.Log("msg", "Preparing to attest against %s", a.Config.ServerAddress.String())
+
+	// Look up the node attestor plugin
+	pluginClients := a.Catalog.GetPluginsByType("NodeAttestor")


another use for nodeattestor.PluginType

kunzimariano · 2017-08-30T15:26:25Z

pkg/agent/agent.go

+	// Main event loop
+	a.Config.Log.Log("msg", "SPIRE Agent is now running")
+	for {
+		select {


kunzimariano · 2017-08-30T15:27:06Z

pkg/agent/agent.go

+}
+
+func (a *Agent) initPlugins() error {
+	a.Config.Log.Log("msg", "Starting plugins")


Maybe logger is a better name.
a.Config.Logger.Log

Yea :( I'm not a fan of the interface this logger exposes. When I wrote this originally I was hoping to directly access leveled logger here, but figured out later that it was more complicated than I thought. I'll change it on your suggestion

mlakewood · 2017-08-30T15:59:49Z

cmd/spire-agent/cli/command/run.go

+	defaultDataDir     = "."
+	defaultLogFile     = "spire-server.log"
+	defaultLogLevel    = "INFO"
+	defaultPluginDir   = "../../plugin/agent/.conf"


should this be a relative path? I guess I can see pro's and cons for each.

Ultimately I think we'd want to make this something a little more usable (like etc or opt) but for now this works well during development. Definitely should change to absolute path at some point IMO

y2bishop2y

🚢

kunzimariano

LGTM

evan2645 · 2017-08-30T19:12:49Z

@boz hmm I'm not sure I follow. The event loop in Run receives on the error channel, and grpc.Serve blocks indefinitely. Can you explain a bit differently?

evan2645 · 2017-08-30T19:53:40Z

Thanks for the explanation @boz

I updated the PR to pick up your second suggestion. I'll circle back around later on to make the channels private, and add proper Shutdown func etc

* fix: added error return to AttestContainerSignatures method * fix: changed variable from camel to snake case * fix: add returned type to error message in getBundleSignatureContent method * fix: changed log from camel to snake case * fix: fixed nil payload test case * fix: fix import order * fix: sync with remote * fix: change import order

* fix: added error return to AttestContainerSignatures method * fix: changed variable from camel to snake case * fix: add returned type to error message in getBundleSignatureContent method * fix: changed log from camel to snake case * fix: fixed nil payload test case * fix: fix import order * fix: sync with remote * fix: change import order Signed-off-by: Willian Alves <wiilliian.alves@gmail.com>

* fix: added error return to AttestContainerSignatures method * fix: changed variable from camel to snake case * fix: add returned type to error message in getBundleSignatureContent method * fix: changed log from camel to snake case * fix: fixed nil payload test case * fix: fix import order * fix: sync with remote * fix: change import order Signed-off-by: Willian Alves <wiilliian.alves@gmail.com> Signed-off-by: Matheus Santos <matheusdefariascs@gmail.com>

evan2645 added 4 commits August 29, 2017 10:57

Check in spire agent attestation logic

66d9c43

Commit light refactor of agent and cli code

Merge branch 'master' into add-agent-bootstrap-logic

f7bdadb

Spire agent bugfixes and updates

647e674

Agent and keymanager bugfixes

9437a2c

evan2645 requested review from amartinezfayo, kunzimariano, paul-argeniss, walmav and y2bishop2y as code owners August 30, 2017 00:56

boz reviewed Aug 30, 2017

View reviewed changes

pkg/agent/agent.go

MaxPlugins = map[string]int{

"KeyManager": 1,

"NodeAttestor": 1,

"WorkloadAttestor": 1,

Copy link

Contributor

boz Aug 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here too

boz reviewed Aug 30, 2017

View reviewed changes

kunzimariano previously approved these changes Aug 30, 2017

View reviewed changes

mlakewood reviewed Aug 30, 2017

View reviewed changes

Update agent struct log -> logger

af26db6

evan2645 dismissed kunzimariano’s stale review via af26db6 August 30, 2017 18:21

evan2645 added 3 commits August 30, 2017 11:33

Enable graceful shutdown of agent grpc server

7d54205

Fix default log filename for agent

4dd3796

Check for errors in agent when starting grpc listener

5a0553f

y2bishop2y previously approved these changes Aug 30, 2017

View reviewed changes

kunzimariano previously approved these changes Aug 30, 2017

View reviewed changes

Read from ErrorCh to unblock grpc goroutine during shutdown

7933cfc

evan2645 dismissed stale reviews from kunzimariano and y2bishop2y via 7933cfc August 30, 2017 19:52

evan2645 merged commit b56a62e into spiffe:master Aug 30, 2017

evan2645 deleted the add-agent-bootstrap-logic branch August 30, 2017 20:03

evan2645 mentioned this pull request Sep 5, 2017

Implement Consumer of Node API (Control Plane) #21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add agent bootstrap logic #94

Add agent bootstrap logic #94

evan2645 commented Aug 30, 2017

boz Aug 30, 2017

evan2645 Aug 30, 2017

boz Aug 30, 2017

boz Aug 30, 2017

evan2645 Aug 30, 2017

evan2645 Aug 30, 2017

boz Aug 30, 2017

kunzimariano Aug 30, 2017

kunzimariano Aug 30, 2017

evan2645 Aug 30, 2017

evan2645 Aug 30, 2017

mlakewood Aug 30, 2017

evan2645 Aug 30, 2017

y2bishop2y left a comment

kunzimariano left a comment

evan2645 commented Aug 30, 2017

evan2645 commented Aug 30, 2017

Add agent bootstrap logic #94

Add agent bootstrap logic #94

Conversation

evan2645 commented Aug 30, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y2bishop2y left a comment

Choose a reason for hiding this comment

kunzimariano left a comment

Choose a reason for hiding this comment

evan2645 commented Aug 30, 2017

evan2645 commented Aug 30, 2017