Get resolver based on attestation method #652
Conversation
Signed-off-by: Marcos G. Yedro <marcosyedro@gmail.com>
marcosy
requested review from
amartinezfayo,
azdagron,
evan2645,
MarcosDY,
martincapello and
walmav
as code owners
pkg/server/endpoints/node/handler.go
Outdated
| selectors = append(selectors, resolved.Entries...) | ||
| if nodeResolver == nil { | ||
| // If not matching node resolver found, skip adding additional selectors | ||
| h.c.Log.Debug("could not find node resolver type %s", attestationType) |
There was a problem hiding this comment.
%q is a little nicer for logging arbitrary strings
| @@ -38,12 +38,12 @@ func (c *Catalog) DataStores() []*catalog.ManagedDataStore { | |||
| return c.dataStores | |||
| } | |||
|
|
|||
| func (c *Catalog) SetNodeAttestors(nodeAttestors ...nodeattestor.NodeAttestor) { | |||
| func (c *Catalog) SetNodeAttestors(baseName string, nodeAttestors ...nodeattestor.NodeAttestor) { | |||
There was a problem hiding this comment.
this feels a little clunky to me... i'd think i'd rather leave the Set methods as they are and instead introduce a new method (which the Set method could use):
func (c *Catalog) SetNodeAttestors(nodeAttestors ...nodeattestor.NodeAttestor) {
c.nodeAttestors = nil
for i, nodeAttestor := range nodeAttestors {
c.AddNodeAttestorNamed(pluginName("nodeattestor", i), nodeAttestor)
}
}
func (c *Catalog) AddNodeAttestorNamed(name string, nodeAttestor nodeattestor.NodeAttestor) {
c.nodeAttestors = append(c.nodeAttestors, catalog.NewManagedNodeAttestor(
nodeAttestor, common.PluginConfig{
PluginName: name,
}))
}
There was a problem hiding this comment.
Right, that looks better 👍
| @@ -60,7 +60,7 @@ type HandlerTestSuite struct { | |||
| now time.Time | |||
| } | |||
|
|
|||
| func SetupHandlerTest(t *testing.T) *HandlerTestSuite { | |||
| func SetupHandlerTest(t *testing.T, attestorBaseName, resolverBaseName string) *HandlerTestSuite { | |||
There was a problem hiding this comment.
rather than requiring each test to pass these values, i think it might be cleaner and clearer if SetupHandlerTest adds the node attestor using some default name, like "fake" or something, and then all of the other tests don't bother adding a resolver. Then there can be a test that adds a resolver under the right name, and one that adds a resolver under the wrong name (and depending on the rest of the tests, a test that has no resolver).
All of the above assumes you add a new field to the suite to hold onto the catalog...
There was a problem hiding this comment.
Yes, I totally agree!
Signed-off-by: Marcos G. Yedro <marcosyedro@gmail.com>
Pull Request check list
Affected functionality
Node resolver selection (spire-server)
Description of change
Currently, spire-server selects the first configured node resolver when trying to discover additional node selectors. This PR introduces a change to make this selection based on attestation method.
Which issue this PR fixes
Fixes #555