Adding Evict method #693
Adding Evict method #693
Conversation
Signed-off-by: Marcos G. Yedro <marcosyedro@gmail.com>
Signed-off-by: Marcos G. Yedro <marcosyedro@gmail.com>
Signed-off-by: Marcos G. Yedro <marcosyedro@gmail.com>
Signed-off-by: Marcos G. Yedro <marcosyedro@gmail.com>
Signed-off-by: Marcos G. Yedro <marcosyedro@gmail.com>
marcosy
requested review from
amartinezfayo,
azdagron,
evan2645,
MarcosDY,
martincapello and
walmav
as code owners
Signed-off-by: Marcos G. Yedro <marcosyedro@gmail.com>
Signed-off-by: Marcos G. Yedro <marcosyedro@gmail.com>
Signed-off-by: Marcos G. Yedro <marcosyedro@gmail.com>
Signed-off-by: Marcos G. Yedro <marcosyedro@gmail.com>
cmd/spire-server/cli/agent/evict.go
Outdated
| } | ||
|
|
||
| // make sure SPIFFE ID is well formed | ||
| c.SpiffeID, err = idutil.NormalizeSpiffeID(c.SpiffeID, idutil.AllowAny()) |
There was a problem hiding this comment.
maybe idutil.AllowAnyTrustDomainAgent()?
cmd/spire-server/cli/agent/evict.go
Outdated
| } | ||
|
|
||
| func (EvictCLI) Synopsis() string { | ||
| return "De-attest an agent givent its spiffeID" |
There was a problem hiding this comment.
"Evicts an attested agent allowing it to re-attest"? I'm not sure what the right synopsis is :)
There was a problem hiding this comment.
Sounds good! will standardize on "evict" name rather than "de-attest"
cmd/spire-server/cli/agent/evict.go
Outdated
| } | ||
|
|
||
| if !evictResponse.DeleteSucceed { | ||
| fmt.Println("Failed to de-attest agent") |
There was a problem hiding this comment.
Failed to evict agent? Also maybe consider sending to STDERR and exiting with a non-zero status.
cmd/spire-server/cli/agent/evict.go
Outdated
| fmt.Println("Failed to de-attest agent") | ||
| } | ||
|
|
||
| fmt.Println("Agent de-attested successfully") |
cmd/spire-server/cli/agent/evict.go
Outdated
| c := &EvictConfig{} | ||
|
|
||
| f.StringVar(&c.RegistrationUDSPath, "registrationUDSPath", util.DefaultSocketPath, "Registration API UDS path") | ||
| f.StringVar(&c.SpiffeID, "spiffeID", "", "The SPIFFE ID of the agent to evict (core identity)") |
There was a problem hiding this comment.
what's the "core identity" part for?
There was a problem hiding this comment.
My intention was to make it clear that the spiffe ID that should be used here is the true attested node spiffeID, and not the one that the user has assigned.
For example this one spiffe://example.org/spire/agent/join_token/the_token instead of spiffe://example.org/host.
I think it is a good idea to replace it by the term "agent identity" as suggested by @evan2645 in the comment below
| ctx := context.Background() | ||
| s.createAttestedNode(ctx, spiffeIDToRemove) | ||
| evictResponse, err := s.handler.EvictAgent(ctx, evictRequest) | ||
| s.NoError(err) |
There was a problem hiding this comment.
this should either be an s.Require().NoError() or something like:
if s.NoError(err) {
s.True(...)
}
Otherwise the test case will panic when accessing members of the nil response.
| s.Len(listResponse.Nodes, 0) | ||
| } | ||
|
|
||
| func (s *HandlerSuite) createAttestedNode(ctx context.Context, spiffeID string) { |
There was a problem hiding this comment.
for a helper like this, unless you really need to parameterize the context, i'd just hardcode context.Background()
| // Represents an evict response | ||
| message EvictAgentResponse { | ||
| // If the entry is successfully deleted, deleteSucceed will be true | ||
| bool deleteSucceed = 1; |
There was a problem hiding this comment.
As suggested below, I'll modify this API to return the deleted node
|
|
||
| // Represents an evict request | ||
| message EvictAgentRequest { | ||
| // Core identity of the agent to be de-attested. |
There was a problem hiding this comment.
i'm not familiar with "core identity". is this what we are calling the true attested node SPIFFE ID (as opposed to a vanity ID)?
There was a problem hiding this comment.
I may be responsible fo this :-| I used the term when trying to describe the difference between an agent's identity and a vanity ID.... frankly I think we would do well to put some more focus on what we call these thing. I dont know if vanity ID is the right thing or not... but I think Agent Identity or something like that is close to correct for the agent's SPIFFE ID
There was a problem hiding this comment.
Ok! will use "Agent Identity" 👍
| rpc EvictAgent(EvictAgentRequest) returns (EvictAgentResponse); | ||
|
|
||
| // List all attested nodes | ||
| rpc ListAgents(spire.common.Empty) returns (ListAgentsResponse); |
There was a problem hiding this comment.
I know it there are places in the registration API that don't follow this rule, but gRPC best practices are that each request should have a distinct request and response type. I'd prefer an empty ListAgentsRequest instead of a spire.common.Empty message, since it allows us to extend it later with less fuss.
cmd/spire-server/cli/agent/evict.go
Outdated
| } | ||
|
|
||
| // make sure SPIFFE ID is well formed | ||
| c.SpiffeID, err = idutil.NormalizeSpiffeID(c.SpiffeID, idutil.AllowAny()) |
There was a problem hiding this comment.
Perhaps we can validate this with idutil.AllowAnyTrustDomainAgent()? That will take the extra step to ensure that the specified ID is actually an agent ID
cmd/spire-server/cli/agent/evict.go
Outdated
|
|
||
| //EvictCLI command for node "de-attestation" | ||
| type EvictCLI struct { | ||
| RegistrationClient registration.RegistrationClient |
There was a problem hiding this comment.
Nit: should we leave this as unexported since it's only really used for testing?
cmd/spire-server/cli/agent/evict.go
Outdated
| } | ||
|
|
||
| func (EvictCLI) Synopsis() string { | ||
| return "De-attest an agent givent its spiffeID" |
There was a problem hiding this comment.
Nit: IMO we should be consistent with the naming, maybe something like Evict an agent given its SPIFFE ID? Also, small typo s/givent/given/
There was a problem hiding this comment.
Agreed! Will standardize on "Evict"
cmd/spire-server/cli/agent/evict.go
Outdated
| if c.RegistrationClient == nil { | ||
| c.RegistrationClient, err = util.NewRegistrationClient(config.RegistrationUDSPath) | ||
| if err != nil { | ||
| fmt.Printf("Error creating registration client: %v \n", err) |
There was a problem hiding this comment.
Since this error will be seen by a user, it would be good to have something that is a little more "plain english" like Error establishing connection to the Registration API: %v ... My basic thought process here is "As an ordinary user trying out SPIRE, will I understand what a registration client is and will I know how to fix it"
cmd/spire-server/cli/agent/evict.go
Outdated
| } | ||
| evictResponse, err := c.RegistrationClient.EvictAgent(ctx, ®istration.EvictAgentRequest{SpiffeID: config.SpiffeID}) | ||
| if err != nil { | ||
| fmt.Printf("Error de-attesting agent: %v \n", err) |
There was a problem hiding this comment.
Similar nit here as before on consistent terms e.g. Error evicting agent: %v
Next few lines have a couple references too
| }, err | ||
| } | ||
|
|
||
| h.Log.Infof("Successfully de-attested agent with ID: %q", spiffeID) |
There was a problem hiding this comment.
Same nit here
Not quite sure if this should be Infof or Debugf... kind of feels like the latter, unless we also want to start logging registration entry changes at Info level
There was a problem hiding this comment.
Agreed! I believe this should be logged as "debug" 👍
| return ®istration.ListAgentsResponse{Nodes: listAttestedNodesResponse.Nodes}, nil | ||
| } | ||
|
|
||
| func (h *Handler) deleteAttestationEntry(ctx context.Context, baseSPIFFEID string) error { |
There was a problem hiding this comment.
nit: it would be nice if this matched the datastore naming convention e.g. deleteAttestedNode
There was a problem hiding this comment.
Another note here, I've been trying to avoid the term baseSPIFFEID because I don't think it carries a lot of meaning and can cause confusion... it was a term we coined very early on in the design and development process, but as time goes on, I've found I prefer agentID or something along those lines
| func (h *Handler) ListAgents(ctx context.Context, empty *common.Empty) (*registration.ListAgentsResponse, error) { | ||
| dataStore := h.Catalog.DataStores()[0] | ||
| listAttestedNodesRequest := &datastore.ListAttestedNodesRequest{} | ||
| listAttestedNodesResponse, err := dataStore.ListAttestedNodes(ctx, listAttestedNodesRequest) |
There was a problem hiding this comment.
This could be a little more succinct and easier to read if the variable names were simply req and resp... the variables are scoped to this function, the golang idiom is shorter names are better particularly if they are declared and used very close together
| // Represents an evict response | ||
| message EvictAgentResponse { | ||
| // If the entry is successfully deleted, deleteSucceed will be true | ||
| bool deleteSucceed = 1; |
There was a problem hiding this comment.
I think we should avoid this pattern, it can be error prone because consumers may forget to check the value of deleteSucceed. Instead, if the delete has failed for some reason, we should return an error directly.
Instead of returning deleteSucceed, one option is to return spire.common.AttestedNode representing the entry we just deleted
There was a problem hiding this comment.
Good point! I'll modify the API to return the deleted node
|
|
||
| // Evict removes (de-attest) an attestation entry from the attested nodes store | ||
| rpc EvictAgent(EvictAgentRequest) returns (EvictAgentResponse); | ||
|
|
There was a problem hiding this comment.
nit: can remove this space to keep the agent-related calls together
Signed-off-by: Marcos G. Yedro <marcosyedro@gmail.com>
Signed-off-by: Marcos G. Yedro <marcosyedro@gmail.com>
Pull Request check list
Affected functionality
Agent "de-attestation"
Description of change
Adding new Evict method for "de-attesting" an already attested node given its spiffeID
Which issue this PR fixes
Fixes #27