Add agent purge command #3982

guilhermocc · 2023-03-15T16:17:52Z

Pull Request check list

Commit conforms to CONTRIBUTING.md?
Proper tests/regressions included?
Documentation updated?

Affected functionality
Add new agent purge command for cleaning expired agents attested using a non-TOFU security model.

Description of change
Added the new command, including unitary tests

Which issue this PR fixes
Fixes #1836

evan2645

Sorry for the delay in this review @guilhermocc

evan2645 · 2023-03-21T18:41:59Z

cmd/spire-server/cli/agent/clean.go

+	"google.golang.org/protobuf/types/known/wrapperspb"
+)
+
+type cleanCommand struct {


I feel like the clean subcommand is kind of ambiguous ... we use the verb "prune" in bundle management, which means flushing out expired keys from the bundle. Perhaps that would be better? spire-server agent prune?

Sure! I was in doubt between clean and prune, but prune seems to be better!

evan2645 · 2023-03-21T20:56:35Z

cmd/spire-server/cli/agent/agent_posix_test.go

+  -dryRun
+    	Indicates that the command will not perform any action, but will print the agents that would be purged.
+  -expiredBefore string
+    	Specifies the date before which all expired agents should be deleted. The value should be a date time string in the format "YYYY-MM-DD HH:MM:SS". Any agents that expired before this date will be deleted.


What other time format options do we have? I wonder what other projects do that require time input on a CLI param .. the first thing I noticed here is that no timezone is specified. The second thing is that there's a space in it 😂

You have a point here, this field is too complex. Well, we have plenty of different date or datetime formats that we can use, for simplifying things we could use just the date format YYYY-MM-DD. Another option that we can use is duration value, in which the user could provide the duration value in "ns", "us" (or "µs"), "ms", "s", "m" and "h". I would go with the date format on this, what do you think?

time.RFC3339 seems like a good choice for this.

A fixed time seems safer than a user trying to cobble together a duration that happens to line up with the time they want :)

time.RFC3339 would give the user full precision on the purge, it sounds good

evan2645 · 2023-03-28T14:13:57Z

cmd/spire-server/cli/agent/clean.go

+	agents := resp.GetAgents()
+	expiredAgents := &ExpiredAgents{Agents: []*ExpiredAgent{}}
+
+	for _, agent := range agents {


I think this is fine for now, though my preference would be for the prune logic to happen in SPIRE core, and expose an RPC. In the future, I think we'll want to regularly call this prune logic against agents that we know are ~safe to flush out

I also thought about that, but wasn't sure if this would fit into this issue scope since it is a change in the agent's service api, right?

evan2645 · 2023-03-28T14:22:29Z

cmd/spire-server/cli/agent/clean.go

+			result := &ExpiredAgent{AgentID: id}
+
+			if !c.dryRun {
+				if _, err := agentClient.DeleteAgent(ctx, &agentv1.DeleteAgentRequest{Id: agent.Id}); err == nil {


We check this error, but we don't log it or otherwise notify the user .. they'll see that some agents weren't purged and won't know why. Maybe ExpiredAgent needs an error field?

Definitely!

evan2645 · 2023-03-28T14:23:40Z

cmd/spire-server/cli/agent/clean.go

+type ExpiredAgents struct {
+	Agents []*ExpiredAgent `json:"expired_agents"`
+}
+
+type ExpiredAgent struct {
+	AgentID spiffeid.ID `json:"agent_id"`
+	Deleted bool        `json:"deleted"`
+}


Should these be exported or unexported?

unexported 🙈

guilhermocc · 2023-04-12T13:02:23Z

As discussed in the last contributor sync, an -expiredFor flag would improve the command usability, so I'm replacing the -expiredBefore with -expiredFor 👍

maxlambrecht · 2023-04-17T19:19:08Z

cmd/spire-server/cli/agent/agent_posix_test.go

+  -dryRun
+    	Indicates that the command will not perform any action, but will print the agents that would be purged.
+  -expiredFor duration
+    	Specifies the time since the agent's SVID has expired, used for filtering agents to purge. (default 24h0m0s)


Suggested change

Specifies the time since the agent's SVID has expired, used for filtering agents to purge. (default 24h0m0s)

Amount of time that has passed since the agent's SVID has expired. It is used to determine which agents to purge (default: 24h0m0s)

maxlambrecht · 2023-04-17T19:20:08Z

cmd/spire-server/cli/agent/agent_windows_test.go

+  -dryRun
+    	Indicates that the command will not perform any action, but will print the agents that would be purged.
+  -expiredFor duration
+    	Specifies the time since the agent's SVID has expired, used for filtering agents to purge. (default 24h0m0s)


Suggested change

Specifies the time since the agent's SVID has expired, used for filtering agents to purge. (default 24h0m0s)

Amount of time that has passed since the agent's SVID has expired. It is used to determine which agents to purge (default: 24h0m0s)

maxlambrecht · 2023-04-17T19:21:30Z

cmd/spire-server/cli/agent/purge.go

+}
+
+func (*purgeCommand) Synopsis() string {
+	return "Delete expired agents that attested using a non-TOFU security model based on a given time"


Suggested change

return "Delete expired agents that attested using a non-TOFU security model based on a given time"

return "Purge expired agents that were attested using a non-TOFU security model based on a given time"

maxlambrecht · 2023-04-17T19:29:17Z

cmd/spire-server/cli/agent/purge.go

+}
+
+func (c *purgeCommand) AppendFlags(fs *flag.FlagSet) {
+	fs.DurationVar(&c.expiredFor, "expiredFor", 24*time.Hour, "Specifies the time since the agent's SVID has expired, used for filtering agents to purge.")


Suggested change

fs.DurationVar(&c.expiredFor, "expiredFor", 24*time.Hour, "Specifies the time since the agent's SVID has expired, used for filtering agents to purge.")

fs.DurationVar(&c.expiredFor, "expiredFor", 24*time.Hour, "Amount of time that has passed since the agent's SVID has expired. It is used to determine which agents to purge.")

Signed-off-by: Guilherme Carvalho <[email protected]>

evan2645

Thanks so much for the contrib and patience @guilhermocc and also for the review @maxlambrecht

Left a couple comments/nits here and there, but this is looking good to me

evan2645 · 2023-04-20T19:38:58Z

cmd/spire-server/cli/agent/agent_posix_test.go

+  -dryRun
+    	Indicates that the command will not perform any action, but will print the agents that would be purged.
+  -expiredFor duration
+    	Amount of time that has passed since the agent's SVID has expired. It is used to determine which agents to purge. (default 24h0m0s)


nit: I feel it is pretty common for agents to go away and come back, judging by the volume of questions etc in the slack and GH issues. Based on that, and also a bit of "IMO" , I wonder if this default should be closer to one month than one day. For what purposes would someone prune, and for what outcome? Feels like being conservative here is a good idea unless the use case(s) call for something more aggressive

Sure, it makes sense to be more conservative here 👍

evan2645 · 2023-04-20T19:44:45Z

cmd/spire-server/cli/agent/purge.go

+		}
+
+		msg := fmt.Sprintf("Found %d expired ", len(expAgents.Agents))
+		msg = util.Pluralizer(msg, "agent", "agents", len(expAgents.Agents))


❤️

small details like this make a big difference in user perception and experience. thank you 🙌

evan2645 · 2023-04-20T19:50:20Z

cmd/spire-server/cli/agent/purge.go

+	c.env.Println("Agents not purged:")
+	for _, result := range agentsNotPurged {
+		c.env.Printf("SPIFFE ID         : %s\n", result.AgentID.String())
+		if result.Error != "" {


In what case can we have an agent that wasn't purged and also no error string?

This case that you described is unreachable; an agent that wasn't purged (not using dryRun) will always come with an error

Signed-off-by: Guilherme Carvalho <[email protected]>

* Add agent clean command --------- Signed-off-by: Guilherme Carvalho <[email protected]> Co-authored-by: Evan Gilman <[email protected]> Signed-off-by: Dmitry Gorochovsky <[email protected]>

guilhermocc requested review from evan2645, amartinezfayo, azdagron, MarcosDY and rturner3 as code owners March 15, 2023 16:17

amartinezfayo assigned evan2645 Mar 16, 2023

evan2645 reviewed Mar 28, 2023

View reviewed changes

guilhermocc changed the title ~~Add agent clean command~~ Add agent purge command Mar 28, 2023

guilhermocc requested a review from evan2645 March 28, 2023 18:15

MarcosDY added this to the 1.6.2 milestone Mar 30, 2023

amartinezfayo modified the milestones: 1.6.2, 1.6.3, 1.6.4 Apr 5, 2023

guilhermocc force-pushed the new-clean-agents-command branch from 1e30efd to d9e2ae7 Compare April 12, 2023 12:58

maxlambrecht reviewed Apr 17, 2023

View reviewed changes

guilhermocc added 6 commits April 19, 2023 15:25

Add agent clean command

7bfd4d6

Signed-off-by: Guilherme Carvalho <[email protected]>

Refactor agent clean command

06cfb91

Signed-off-by: Guilherme Carvalho <[email protected]>

Address review comments

765d89d

Signed-off-by: Guilherme Carvalho <[email protected]>

Use rfc3339 format

30f9ef8

Signed-off-by: Guilherme Carvalho <[email protected]>

Use expiredFor flag

2a63303

Signed-off-by: Guilherme Carvalho <[email protected]>

Address review comments

f743c97

Signed-off-by: Guilherme Carvalho <[email protected]>

guilhermocc force-pushed the new-clean-agents-command branch from 06c00a4 to f743c97 Compare April 19, 2023 18:25

evan2645 previously approved these changes Apr 20, 2023

View reviewed changes

Address review comments

8901932

Signed-off-by: Guilherme Carvalho <[email protected]>

guilhermocc dismissed evan2645’s stale review via 8901932 April 25, 2023 13:14

evan2645 approved these changes Apr 25, 2023

View reviewed changes

Merge branch 'main' into new-clean-agents-command

0f2db12

evan2645 merged commit ef96482 into spiffe:main Apr 25, 2023

evan2645 mentioned this pull request May 9, 2023

Support a prune RPC in the SPIRE Server's Agent API spiffe/spire-api-sdk#41

Open

evan2645 mentioned this pull request Mar 5, 2024

Run SPIRE Agent on top of k8s as a workload sidecar #4804

Closed

ValFadeev mentioned this pull request Jun 24, 2025

Implemented periodic purging of expired agents #6152

Merged

3 tasks

	Specifies the time since the agent's SVID has expired, used for filtering agents to purge. (default 24h0m0s)
	Amount of time that has passed since the agent's SVID has expired. It is used to determine which agents to purge (default: 24h0m0s)

	return "Delete expired agents that attested using a non-TOFU security model based on a given time"
	return "Purge expired agents that were attested using a non-TOFU security model based on a given time"

	fs.DurationVar(&c.expiredFor, "expiredFor", 24*time.Hour, "Specifies the time since the agent's SVID has expired, used for filtering agents to purge.")
	fs.DurationVar(&c.expiredFor, "expiredFor", 24*time.Hour, "Amount of time that has passed since the agent's SVID has expired. It is used to determine which agents to purge.")

Add agent purge command #3982

Add agent purge command #3982

Uh oh!

Conversation

guilhermocc commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evan2645 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guilhermocc commented Apr 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guilhermocc Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guilhermocc Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guilhermocc Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guilhermocc Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evan2645 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

guilhermocc commented Mar 15, 2023 •

edited

Loading

guilhermocc Apr 19, 2023 •

edited

Loading

guilhermocc Apr 19, 2023 •

edited

Loading

guilhermocc Apr 19, 2023 •

edited

Loading

guilhermocc Apr 19, 2023 •

edited

Loading