Add more filtering options to entry count/show and agent count/list #4714

FedeNQ · 2023-12-06T17:10:21Z

Pull Request check list

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

Affected functionality
Provides the possibility of add multiple flags to entry show, entry count, agent list and agent count commands.
Requires the approval of this PR in SPIRE-API-SDK

Description of change

Add the following flags:

Agent count/list:
- selectors, matchSelectorsOn, banned, expiresBefore, attestationType, canReattest.
Entry count:
- parentID, spiffeID, hint, federatesWith, downstream, matchFederatesWithOn, matchSelectorsOn
Entry show:
- downstream

Which issue this PR fixes
Fixes #2137

amartinezfayo

Thank you @FedeNQ for this contribution, and sorry for the delay in the review.
I left some comments.

amartinezfayo · 2024-01-11T21:44:42Z

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

+	if len(s) > 10 {
+		return fmt.Errorf("date is too long")
+	}
+	_, err := time.Parse("2006-01-02", s)


Any reason to exclude the time here?
From a user experience perspective, I would probably consume this CLI with data that I got from other CLI commands. When the user list/shows agents, the output format of the Expiration time also includes the time (which I think it's useful). So when I'm filtering by expiration time, I would expect to use the same layout, like "2006-01-02 15:04:05 -0700 -07".

I don't have a particular reason, so it could be changed.
Use time.Parse(time.RFC3339, s) it could be a good change?

amartinezfayo · 2024-01-11T21:59:21Z

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

+	if len(s) < 10 {
+		return fmt.Errorf("date is too short")
+	}
+	if len(s) > 10 {
+		return fmt.Errorf("date is too long")
+	}


I'm not sure that we really need this validation, I would probably just leave the validation that is done by the time.Parse function. Were your intention to have more descriptive errors?

Yes, it was expected to guide the user about the error.

amartinezfayo · 2024-01-11T22:07:19Z

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

@@ -14,6 +14,14 @@ var (
    	Path to the SPIRE Server API socket (default "/tmp/spire-server/private/api.sock")
 `
 	listUsage = `Usage of agent list:
+  -attestationType string
+    	The SPIFFE ID of the nodes to list


The description doesn't match here. Since the name may not obviously indicate that this refers to the node attestor used, I would also probably include examples. Something like:
Filter by attestation type, like join_token or x509pop.

amartinezfayo · 2024-01-12T11:43:10Z

pkg/common/cli/flags.go

+
+func (b *BoolFlag) Set(val string) error {
+	if val == "false" {
+		*b = 1


We could probably define constants for the true, false and default values. Something like cli.BoolFlagTrue, cli.BoolFlagFalse and cli.BoolFlagDefault. Then, consumers of the package can refer to those instead of having to look at this code to figure out that the 1, 2, and 0 values are used for that.

amartinezfayo · 2024-01-12T13:15:24Z

pkg/server/datastore/sqlstore/sqlstore.go

+			ByHint:          req.ByHint,
+			ByDownstream:    req.ByDownstream,
+		})
+		return int32(len(resp.Entries)), err


This will retrieve all the records for the registration entries from the backend database, while we need to retrieve a single record with the count. Maybe we could reuse the SQL query used in the listRegistrationEntries and perform a SQL COUNT operation over it? That way we would get as a result a single record with the count.

I think it could be a valid approach, will try to change it

amartinezfayo · 2024-01-12T13:45:09Z

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

 	fs.Var(&c.selectors, "selector", "A colon-delimited type:value selector. Can be used more than once")
+	fs.StringVar(&c.attestationType, "attestationType", "", "The SPIFFE ID of the nodes to list")


The description doesn't match here.

amartinezfayo · 2024-01-12T13:45:28Z

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

@@ -49,6 +123,12 @@ func (c *countCommand) Run(ctx context.Context, _ *commoncli.Env, serverClient u
 }

 func (c *countCommand) AppendFlags(fs *flag.FlagSet) {
+	fs.Var(&c.selectors, "selector", "A colon-delimited type:value selector. Can be used more than once")
+	fs.StringVar(&c.attestationType, "attestationType", "", "The SPIFFE ID of the nodes to count")


The description doesn't match here.

amartinezfayo · 2024-01-12T13:46:10Z

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

@@ -40,8 +48,20 @@ var (
    	The SPIFFE ID of the agent to evict (agent identity)
 `
 	countUsage = `Usage of agent count:
+  -attestationType string


The description doesn't match here.

amartinezfayo · 2024-01-12T13:46:57Z

doc/spire_server.md

+| `-canReattest`      | Filter based on string received, 'true': agents that can reattest, 'false': agents that can't reattest, other value will return all |                                    |
+| `-banned`    |   Filter based on string received, 'true': banned agents, 'false': not banned agents, other value will return all |                |
+| `-expiresBefore`      | A date that indicates the time it should expired before. (format: YYYY-MM-DD)|                                    |
+| `-attestationType`      |  Filters agents to those matching the attestation type. |         |


amartinezfayo · 2024-01-12T14:32:59Z

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

+	fs.StringVar(&c.attestationType, "attestationType", "", "The SPIFFE ID of the nodes to count")
+	fs.Var(&c.canReattest, "canReattest", "Filter based on string received, 'true': agents that can reattest, 'false': agents that can't reattest, other value will return all.")
+	fs.Var(&c.banned, "banned", "Filter based on string received, 'true': banned agents, 'false': not banned agents, other value will return all.")
+	fs.StringVar(&c.expiresBefore, "expiresBefore", "", "A date that indicates the time it should expired before, (format: YYYY-MM-DD)")


In the same way that we make it explicit in the description of other flags that the flag will apply a filter, I think that we should make it explicit here. Something like:
Filter by expiration time (format: "2006-01-02 15:04:05 -0700 -07").
I've commented about the format separately.

The same applies to other flags added that don't mention that a filter will be applied.

Signed-off-by: FedeNQ <[email protected]>

Signed-off-by: Federico Nahuel Quijada <[email protected]>

Signed-off-by: FedeNQ <[email protected]>

amartinezfayo · 2024-03-25T22:46:08Z

pkg/server/api/agent/v1/service.go

+			countReq.ByAttestationType = filter.ByAttestationType
+		}
+
+		// err is verified previously


I'm not sure if this comment should really be here?

amartinezfayo · 2024-03-25T22:46:46Z

pkg/server/api/agent/v1/service.go

+		// err is verified previously
+		// countReq.ByExpiresBefore, _ = time.Parse("2006-01-02", filter.ByExpiresBefore)


Looks like we should remove this?

amartinezfayo · 2024-03-26T14:00:14Z

pkg/server/datastore/sqlstore/sqlstore.go

-func (ds *Plugin) CountAttestedNodes(ctx context.Context) (count int32, err error) {
+func (ds *Plugin) CountAttestedNodes(ctx context.Context, req *datastore.CountAttestedNodesRequest) (count int32, err error) {
+	if countAttestedNodesHasFilters(req) {
+		resp, err := listAttestedNodes(ctx, ds.db, ds.log, &datastore.ListAttestedNodesRequest{


I think that we need to make paginated requests here so we don't get all the records at once.

amartinezfayo · 2024-03-26T14:04:18Z

pkg/server/datastore/sqlstore/sqlstore.go

+			FetchSelectors:    req.FetchSelectors,
+			ByCanReattest:     req.ByCanReattest,
+		})
+		return int32(len(resp.Nodes)), err


Discussed this with @FedeNQ offline. Since the changes to avoid retrieving all the records are too complex, I suggest to keep it as is so we minimize the changes required in the datastore, which has a complex code already.

Signed-off-by: FedeNQ <[email protected]>

Signed-off-by: Federico Nahuel Quijada <[email protected]>

amartinezfayo

Thank you @FedeNQ for this contribution, and for the patience in the review!

…piffe#4714) * add filtering options to count command Signed-off-by: FedeNQ <[email protected]> * add more fields Signed-off-by: FedeNQ <[email protected]> * Add filtering to entry & agent count/show/list commands Signed-off-by: FedeNQ <[email protected]> * fix lint Signed-off-by: FedeNQ <[email protected]> * add more unit test Signed-off-by: FedeNQ <[email protected]> * Change count & list for entries Signed-off-by: FedeNQ <[email protected]> * rollback Signed-off-by: FedeNQ <[email protected]> * fix Signed-off-by: FedeNQ <[email protected]> * fix lint Signed-off-by: FedeNQ <[email protected]> * update go.mod & go.sum Signed-off-by: FedeNQ <[email protected]> * fix windows message Signed-off-by: FedeNQ <[email protected]> * update agent & entry message Signed-off-by: FedeNQ <[email protected]> * update agent message Signed-off-by: FedeNQ <[email protected]> * count entries & agent now uses pagination Signed-off-by: FedeNQ <[email protected]> * remove comment Signed-off-by: FedeNQ <[email protected]> * fix lint Signed-off-by: FedeNQ <[email protected]> * rollback Signed-off-by: FedeNQ <[email protected]> --------- Signed-off-by: FedeNQ <[email protected]> Signed-off-by: Federico Nahuel Quijada <[email protected]>

FedeNQ requested review from evan2645, amartinezfayo, azdagron, MarcosDY and rturner3 as code owners December 6, 2023 17:10

evan2645 assigned amartinezfayo Dec 7, 2023

amartinezfayo reviewed Jan 12, 2024

View reviewed changes

FedeNQ added 5 commits February 7, 2024 12:55

add filtering options to count command

27b6001

Signed-off-by: FedeNQ <[email protected]>

add more fields

2291905

Signed-off-by: FedeNQ <[email protected]>

Add filtering to entry & agent count/show/list commands

c834729

Signed-off-by: FedeNQ <[email protected]>

fix lint

745716d

Signed-off-by: FedeNQ <[email protected]>

add more unit test

b58a0d0

Signed-off-by: FedeNQ <[email protected]>

FedeNQ force-pushed the add-filtering branch from 4ac5235 to b58a0d0 Compare February 7, 2024 15:55

FedeNQ and others added 9 commits February 27, 2024 16:07

Change count & list for entries

3ed4f7e

Signed-off-by: FedeNQ <[email protected]>

rollback

6df58c7

Signed-off-by: FedeNQ <[email protected]>

Merge branch 'main' into add-filtering

39406e8

Signed-off-by: Federico Nahuel Quijada <[email protected]>

fix

a102dc2

Signed-off-by: FedeNQ <[email protected]>

fix lint

c8ad89f

Signed-off-by: FedeNQ <[email protected]>

update go.mod & go.sum

e8547ff

Signed-off-by: FedeNQ <[email protected]>

fix windows message

f9ebb67

Signed-off-by: FedeNQ <[email protected]>

update agent & entry message

7751c40

Signed-off-by: FedeNQ <[email protected]>

update agent message

268ccd0

Signed-off-by: FedeNQ <[email protected]>

amartinezfayo reviewed Mar 26, 2024

View reviewed changes

FedeNQ added 4 commits March 27, 2024 12:07

count entries & agent now uses pagination

efd0548

Signed-off-by: FedeNQ <[email protected]>

remove comment

1475ee4

Signed-off-by: FedeNQ <[email protected]>

fix lint

a3bb1fe

Signed-off-by: FedeNQ <[email protected]>

rollback

296a77f

Signed-off-by: FedeNQ <[email protected]>

FedeNQ force-pushed the add-filtering branch 2 times, most recently from adec60c to 296a77f Compare March 27, 2024 21:32

Merge branch 'main' into add-filtering

b98d2d7

Signed-off-by: Federico Nahuel Quijada <[email protected]>

amartinezfayo approved these changes Mar 27, 2024

View reviewed changes

amartinezfayo merged commit 0727fa6 into spiffe:main Mar 27, 2024
32 checks passed

amartinezfayo added this to the 1.9.3 milestone Mar 27, 2024

amartinezfayo modified the milestones: 1.9.3, 1.9.4, 1.9.5 Apr 4, 2024

amartinezfayo modified the milestones: 1.9.5, 1.9.6 May 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more filtering options to entry count/show and agent count/list #4714

Add more filtering options to entry count/show and agent count/list #4714

FedeNQ commented Dec 6, 2023

amartinezfayo left a comment

amartinezfayo Jan 11, 2024

FedeNQ Jan 12, 2024

amartinezfayo Jan 11, 2024

FedeNQ Jan 12, 2024

amartinezfayo Jan 11, 2024

amartinezfayo Jan 12, 2024

amartinezfayo Jan 12, 2024

FedeNQ Jan 12, 2024

amartinezfayo Jan 12, 2024

amartinezfayo Jan 12, 2024

amartinezfayo Jan 12, 2024

amartinezfayo Jan 12, 2024

amartinezfayo Jan 12, 2024

amartinezfayo Mar 25, 2024

amartinezfayo Mar 25, 2024

amartinezfayo Mar 26, 2024

amartinezfayo Mar 26, 2024

amartinezfayo left a comment

		fs.Var(&c.selectors, "selector", "A colon-delimited type:value selector. Can be used more than once")
		fs.StringVar(&c.attestationType, "attestationType", "", "The SPIFFE ID of the nodes to list")

	\| `-attestationType` \| Filters agents to those matching the attestation type. \| \|
	\| `-attestationType` \| Filters agents to those matching the attestation type, like join_token or x509pop. \| \|

		// err is verified previously
		// countReq.ByExpiresBefore, _ = time.Parse("2006-01-02", filter.ByExpiresBefore)

Add more filtering options to entry count/show and agent count/list #4714

Add more filtering options to entry count/show and agent count/list #4714

Conversation

FedeNQ commented Dec 6, 2023

amartinezfayo left a 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

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

amartinezfayo left a comment

Choose a reason for hiding this comment