Node and Workload API support for federated bundles #583
Conversation
Signed-off-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: Andrew Harding <azdagron@gmail.com>
azdagron
requested review from
amartinezfayo,
evan2645,
MarcosDY,
martincapello and
walmav
as code owners
| @@ -321,12 +321,21 @@ func (a *attestor) parseAttestationResponse(id string, r *node.AttestResponse) ( | |||
| return nil, nil, fmt.Errorf("invalid svid: %v", err) | |||
| } | |||
|
|
|||
| bundle, err := x509.ParseCertificates(r.SvidUpdate.Bundle) | |||
| if r.SvidUpdate.Bundles == nil { | |||
There was a problem hiding this comment.
I see no validations for r.SvidUpdate is it possible it can be nil?
There was a problem hiding this comment.
It is possible. I will add a check.
| @@ -138,12 +143,14 @@ func (c *client) FetchUpdates(ctx context.Context, req *node.FetchX509SVIDReques | |||
| for spiffeid, svid := range resp.SvidUpdate.Svids { | |||
| svids[spiffeid] = svid | |||
| } | |||
| lastBundle = resp.SvidUpdate.Bundle | |||
| for spiffeid, bundle := range resp.SvidUpdate.Bundles { | |||
There was a problem hiding this comment.
is it possible resp.SvidUpdate.Bundles to be nil here?
There was a problem hiding this comment.
Yes, but ranging over a nil map is not an error. It acts as if the map is empty.
pkg/agent/client/client.go
Outdated
| return &Update{ | ||
| Entries: regEntries, | ||
| SVIDs: svids, | ||
| Bundles: bundles, |
There was a problem hiding this comment.
bundle is initialized before for so it will contains data from previous responses, andit can return error here with bundles from previous calls, is it expected?
There was a problem hiding this comment.
Good catch. The old code returned aggregated update results and the error. However, the only consumer of the function does not use the returned update if there is an error. I'm not sure why the old code returned it. I think I'd rather return nil, err in this case.
pkg/agent/manager/cache/cache.go
Outdated
| @@ -35,17 +81,15 @@ type Cache interface { | |||
| DeleteEntry(regEntry *common.RegistrationEntry) bool | |||
| // Entries returns all the in force cached entries. | |||
| Entries() []*Entry | |||
| // IsEmpty returns true if this cache doesn't have any entry. | |||
| IsEmpty() bool | |||
| // Registers and returns a Subscriber, and then sends latest WorkloadUpdate on its channel | |||
| Subscribe(selectors Selectors) Subscriber | |||
| // Set the bundle | |||
pkg/agent/manager/cache/cache.go
Outdated
| SetBundle([]*x509.Certificate) | ||
| // Retrieve the bundle | ||
| SetBundles(map[string][]*x509.Certificate) | ||
| // Retrieve the bundle for the trust domain | ||
| Bundle() []*x509.Certificate | ||
| // SubscribeToBundleChanges returns a new observer.Stream of []*x509.Certificate instances. Each |
There was a problem hiding this comment.
comment is no updated, it is no loger returning observer.Stream.
pkg/agent/manager/manager.go
Outdated
| @@ -39,7 +39,7 @@ type Manager interface { | |||
|
|
|||
| // SubscribeToBundleChanges returns a new observer.Stream on which []*x509.Certificate instances are | |||
There was a problem hiding this comment.
it is no longer returning a observer.Stream
| @@ -650,8 +671,18 @@ func getExpectedFetchX509SVID(data *fetchSVIDData) *node.X509SVIDUpdate { | |||
| caCert, _, _ := util.LoadCAFixture() | |||
| svidUpdate := &node.X509SVIDUpdate{ | |||
| Svids: svids, | |||
| Bundle: caCert.Raw, | |||
| DEPRECATEDBundle: caCert.Raw, | |||
There was a problem hiding this comment.
when we'll remove those deprecated fields? must we add some anotations/comments about a prossible version where we'll remove? or at leas when it was mark as deprecated? so we can track how long we have it?
There was a problem hiding this comment.
Hmm, i think that is a larger discussion to have about API deprecation paths. For now, is git history sufficient to know when it was deprecated?
| @@ -26,12 +35,18 @@ message X509SVIDUpdate { | |||
| // keys. Map[SPIFFE_ID] => SVID. | |||
| map<string, X509SVID> svids = 1; | |||
|
|
|||
| // Latest SPIRE Server bundle | |||
| bytes bundle = 2; | |||
| // DEPRECATED. Latest SPIRE Server bundle. | |||
There was a problem hiding this comment.
can you add when it was deprecated (version)? to make it easier to track
Signed-off-by: Andrew Harding <azdagron@gmail.com>
This PR updates the Node API to return bundles with x509 and JWT SVID updates. The agent has been updated to store all bundles returned from x509 SVID updates in its cache. JWT support in the agent has not been implemented and handling the bundles will be done at that time.
The workload API has been extended to return a map of bundles with updates as well as a list of bundle ids that each workload is associated with.