Include hint field in agent's workload api #3993

guilhermocc · 2023-03-16T20:41:09Z

Pull Request check list

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

Affected functionality

spire-agent fetch jwt and fetch x509 commands
cache and lru_cache methods that get records for a given selector set
spire agent's workload api FetchJWTSVID and FetchX509SVID methods

Description of change
Include hint field in agent's workload api responses, update cache methods for ensuring hint uniqueness amongst the fetched svids.

Which issue this PR fixes
ongoing work for #2588

Dependencies
This PR depends on spiffe/go-spiffe#220, please review it before this PR

guilhermocc · 2023-03-16T20:44:35Z

go.mod

@@ -2,6 +2,8 @@ module github.com/spiffe/spire

 go 1.19

+replace github.com/spiffe/go-spiffe/v2 => github.com/guilhermocc/go-spiffe/v2 v2.0.0-20230308163427-b2b17d68664c


The commit that inserted this temporary dependency should be dropped after https://github.com/spiffe/go-spiffe/pull/220/files gets merged

pkg/agent/manager/cache/cache.go

pkg/agent/manager/cache/cache_test.go

pkg/agent/endpoints/workload/handler.go

pkg/agent/endpoints/workload/handler_test.go

cmd/spire-agent/cli/api/api_test.go

maxlambrecht · 2023-03-21T20:45:59Z

cmd/spire-agent/cli/api/printer.go

@@ -31,6 +31,9 @@ func printX509SVID(env *commoncli.Env, svid *X509SVID) {
 	// Print SPIFFE ID first so if we run into a problem, we
 	// get to know which record it was
 	env.Printf("SPIFFE ID:\t\t%s\n", svid.SPIFFEID)
+	if svid.Hint != "" {
+		env.Printf("HINT:\t\t\t%s\n", svid.Hint)


Shouldn't it be printed a "Hint", as it's not an acronym?

I thought we were using only uppercase here for all fields printed, but just saw that's not the case, you are right, going to update it!

maxlambrecht · 2023-03-22T13:39:43Z

pkg/agent/manager/cache/lru_cache.go

+
+	for hint, recordsWithSameHint := range hintsMap {
+		// If there is more than one entry with the same hint, remove all but the oldest one to ensure hint uniqueness
+		if len(recordsWithSameHint) > 1 {


Can this be rewritten as in the other cache method?

if len(recordsWithSameHint) <= 1 { continue }

MarcosDY · 2023-03-27T14:18:02Z

cmd/spire-agent/cli/api/fetch_jwt.go

@@ -93,6 +93,9 @@ func printPrettyResult(env *commoncli.Env, results ...interface{}) error {

 	for _, svid := range svidResp.Svids {
 		env.Printf("token(%s):\n\t%s\n", svid.SpiffeId, svid.Svid)
+		if svid.Hint != "" {
+			env.Printf("hint(%s):\n\t%s\n", svid.SpiffeId, svid.Hint)


mmm looks like spiffeID here is redundant, since you already printed spiffeID with token

We also include the spiffe ID in the printed bundle (on line 101), I tried to maintain the same logic here for hint, what do you think?

I cant think in another good ouput... so keeping the duplicated SPIFFEID may be good

MarcosDY · 2023-03-27T14:20:06Z

cmd/spire-agent/cli/api/printer.go

@@ -31,6 +31,9 @@ func printX509SVID(env *commoncli.Env, svid *X509SVID) {
 	// Print SPIFFE ID first so if we run into a problem, we
 	// get to know which record it was
 	env.Printf("SPIFFE ID:\t\t%s\n", svid.SPIFFEID)
+	if svid.Hint != "" {
+		env.Printf("Hint:\t\t\t%s\n", svid.Hint)


looks like it has 3 tabs, but spiffeID has only 2, do you want to add some hierarchy level?

Just for maintaining the output aligned
Two tabs

Three tabs

MarcosDY · 2023-03-27T14:24:30Z

pkg/agent/endpoints/workload/handler.go

 	for _, entry := range entries {
 		if req.SpiffeId != "" && entry.SpiffeId != req.SpiffeId {
 			continue
 		}
+		loopLog := log.WithField(telemetry.SPIFFEID, entry.SpiffeId)


NIT: since loopLog is used only on h.fetchJWTSVID may we remove the break line? and couple both lines?

MarcosDY · 2023-03-27T14:30:48Z

pkg/agent/endpoints/workload/handler.go

@@ -395,6 +400,7 @@ func composeX509SVIDResponse(update *cache.WorkloadUpdate) (*workload.X509SVIDRe
 			X509Svid:    x509util.DERFromCertificates(identity.SVID),
 			X509SvidKey: keyData,
 			Bundle:      bundle,
+			Hint:        identity.Entry.Hint,


is there a warranty that entry is not nil?

I believe so, as you can see on line 391, we are already taking the SPIFFE ID from the entry.

MarcosDY · 2023-03-27T14:37:38Z

pkg/agent/endpoints/workload/handler_test.go

-					var tokenIDs []spiffeid.ID
-					for _, svid := range resp.Svids {
-						parsedSVID, err := jwtsvid.ParseInsecure(svid.Svid, tt.audience)
+					assert.Len(t, resp.Svids, len(tt.expectedResp))


why not to create a list of responses and do a equal between expected and response list? with that we can no longer validate length and get a single error message

MarcosDY · 2023-03-27T14:45:20Z

pkg/agent/manager/cache/cache.go

since we are adding hint, how it will affect to SVID Store? may we add hint to persisted SVIDs too?

MarcosDY · 2023-03-27T14:58:11Z

pkg/agent/manager/cache/cache.go

 	return records, recordsDone
 }

+func (c *Cache) removeEntriesWithNonUniqueHint(records recordSet) {


this worry me, so we are stop propagating Identities, just because a field is set, with the same value than another entry on response,
we MUST add documentation so users can understand than that is possible it to happens, and can result in some confusion

and is it something we must do at cache level? it sounds to me like this must be done on workload api instead

As discussed in the last contributor sync, we decided to move this filter to the workload API layer

rturner3

Based on comments from @MarcosDY and discussion in today's contributor sync, this will have potentially unwanted consequences on the SDS APIs and SVID store functionality. Removing my +1 until the filtering added in the Agent cache layer is moved to the Workload API handlers.

maxlambrecht

Removing my approval based on discussion in today's contributor sync.

azdagron · 2023-03-31T11:06:02Z

go.mod

@@ -2,6 +2,8 @@ module github.com/spiffe/spire

 go 1.19

+replace github.com/spiffe/go-spiffe/v2 => github.com/guilhermocc/go-spiffe/v2 v2.0.0-20230327182353-926e51ab6802


The required change has been merged and released in version v2.1.4. Please update accordingly!

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

Signed-off-by: Guilherme Carvalho <guilhermocc@proton.me>

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

pkg/agent/endpoints/workload/handler.go

pkg/agent/endpoints/workload/handler_test.go

Signed-off-by: Guilherme Carvalho <guilhermocc@proton.me>

rturner3

Thank you for your patience on this one @guilhermocc! Really appreciate the good work.

maxlambrecht

🚀

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com> Signed-off-by: Basavaraju-G <basavaraju013@gmail.com>

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com> Signed-off-by: Dmitry Gorochovsky <d.goro@yahoo.com>

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

guilhermocc commented Mar 16, 2023

View reviewed changes

rturner3 added this to the 1.6.2 milestone Mar 17, 2023

rturner3 reviewed Mar 18, 2023

View reviewed changes

guilhermocc mentioned this pull request Mar 20, 2023

Implement SVID labels in SPIRE #2588

Closed

guilhermocc requested a review from rturner3 March 20, 2023 18:03

rturner3 self-assigned this Mar 20, 2023

guilhermocc mentioned this pull request Mar 20, 2023

Add SVID hints on workload api client spiffe/java-spiffe#114

Merged

maxlambrecht reviewed Mar 22, 2023

View reviewed changes

guilhermocc force-pushed the include-hint-field-workload-api branch 2 times, most recently from 4e98217 to 25cab4a Compare March 22, 2023 21:03

rturner3 approved these changes Mar 24, 2023

View reviewed changes

maxlambrecht approved these changes Mar 25, 2023

View reviewed changes

MarcosDY reviewed Mar 27, 2023

View reviewed changes

rturner3 requested changes Mar 28, 2023

View reviewed changes

maxlambrecht suggested changes Mar 28, 2023

View reviewed changes

guilhermocc force-pushed the include-hint-field-workload-api branch from 25cab4a to 970a948 Compare March 29, 2023 19:33

guilhermocc requested review from rturner3 and MarcosDY and removed request for rturner3 March 29, 2023 19:58

azdagron reviewed Mar 31, 2023

View reviewed changes

guilhermocc added 5 commits March 31, 2023 10:11

Update go-spiffe version

fec0b09

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

Add hint to spire-agent fetch svid commands

eee2754

Signed-off-by: Guilherme Carvalho <guilhermocc@proton.me>

Add hint to agent client

a6e5c49

Signed-off-by: Guilherme Carvalho <guilhermocc@proton.me>

Crate new telemetry label for service

32b80db

Signed-off-by: Guilherme Carvalho <guilhermocc@proton.me>

Update handler test to include hint uniqueness check

a390e1c

Signed-off-by: Guilherme Carvalho <guilhermocc@proton.me>

guilhermocc and others added 5 commits March 31, 2023 10:11

Add hint uniqueness filter in workload API handler

86d5514

Signed-off-by: Guilherme Carvalho <guilhermocc@proton.me>

Update methods comments

498790f

Signed-off-by: Guilherme Carvalho <guilhermocc@proton.me>

Refactor function position

846a2fe

Signed-off-by: Guilherme Carvalho <guilhermocc@proton.me>

Refactor tests

5b817f3

Signed-off-by: Guilherme Carvalho <guilhermocc@proton.me>

Remove SetHint method

38b2cc2

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>

guilhermocc force-pushed the include-hint-field-workload-api branch from dc7741f to 38b2cc2 Compare March 31, 2023 13:30

rturner3 reviewed Mar 31, 2023

View reviewed changes

pkg/agent/endpoints/workload/handler.go Outdated Show resolved Hide resolved

pkg/agent/endpoints/workload/handler_test.go Show resolved Hide resolved

Address review comments

a18dfea

Signed-off-by: Guilherme Carvalho <guilhermocc@proton.me>

guilhermocc requested a review from rturner3 April 3, 2023 12:56

rturner3 approved these changes Apr 4, 2023

View reviewed changes

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

maxlambrecht approved these changes Apr 10, 2023

View reviewed changes

Merge branch 'main' into include-hint-field-workload-api

98fa0e6

azdagron merged commit 0573cf3 into spiffe:main Apr 12, 2023

Basavaraju-G pushed a commit to Basavaraju-G/spire that referenced this pull request May 3, 2023

Include hint field in agent's workload api (spiffe#3993)

9c00abd

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com> Signed-off-by: Basavaraju-G <basavaraju013@gmail.com>

d-goro pushed a commit to d-goro/spire that referenced this pull request May 18, 2023

Include hint field in agent's workload api (spiffe#3993)

6e244f0

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com> Signed-off-by: Dmitry Gorochovsky <d.goro@yahoo.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include hint field in agent's workload api #3993

Include hint field in agent's workload api #3993

guilhermocc commented Mar 16, 2023 •

edited

Loading

guilhermocc Mar 16, 2023

maxlambrecht Mar 21, 2023

guilhermocc Mar 22, 2023

maxlambrecht Mar 22, 2023

MarcosDY Mar 27, 2023

guilhermocc Mar 29, 2023

MarcosDY Mar 31, 2023

MarcosDY Mar 27, 2023

guilhermocc Mar 29, 2023

MarcosDY Mar 27, 2023

MarcosDY Mar 27, 2023

guilhermocc Mar 29, 2023

MarcosDY Mar 31, 2023

MarcosDY Mar 27, 2023

MarcosDY Mar 27, 2023

MarcosDY Mar 27, 2023

MarcosDY Mar 27, 2023

guilhermocc Mar 29, 2023

rturner3 left a comment

maxlambrecht left a comment

azdagron Mar 31, 2023

rturner3 left a comment

maxlambrecht left a comment

		@@ -2,6 +2,8 @@ module github.com/spiffe/spire

		go 1.19

		replace github.com/spiffe/go-spiffe/v2 => github.com/guilhermocc/go-spiffe/v2 v2.0.0-20230308163427-b2b17d68664c

Include hint field in agent's workload api #3993

Include hint field in agent's workload api #3993

Conversation

guilhermocc commented Mar 16, 2023 • edited Loading

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

rturner3 left a comment

Choose a reason for hiding this comment

maxlambrecht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rturner3 left a comment

Choose a reason for hiding this comment

maxlambrecht left a comment

Choose a reason for hiding this comment

guilhermocc commented Mar 16, 2023 •

edited

Loading