Fix events based cache issues introduced in 1.11.0 #5842
Conversation
sorindumitru
requested review from
evan2645,
amartinezfayo,
MarcosDY and
rturner3
as code owners
sorindumitru
force-pushed
the
events-based-fixes
branch
from
09fb16e to
05a4781
Compare
| @@ -215,7 +215,7 @@ func (a *attestedNodes) updateCachedNodes(ctx context.Context) error { | |||
| for spiffeId := range a.fetchNodes { | |||
| node, err := a.ds.FetchAttestedNode(ctx, spiffeId) | |||
| if err != nil { | |||
| return err | |||
| continue | |||
There was a problem hiding this comment.
Having an error here is really an expected situation?
The FetchAttestedNode function will not return an error if the record was not found (https://github.com/spiffe/spire/blob/main/pkg/server/datastore/sqlstore/sqlstore.go#L1577), so it would only return an error if there was an SQL error. Are there other error situations that we expect as normal behavior?
There was a problem hiding this comment.
Yes, this would be SQL errors, either the something wrong with the request (which wouldn't be a problem) or likely transient errors which we should retry at some point.
There was a problem hiding this comment.
It will retry next iteration so this should be ok.
|
i can help review |
| @@ -40,9 +40,6 @@ type registrationEntries struct { | |||
| } | |||
|
|
|||
| func (a *registrationEntries) captureChangedEntries(ctx context.Context) error { | |||
| // first, reset the entries we might fetch. | |||
| a.fetchEntries = make(map[string]struct{}) | |||
There was a problem hiding this comment.
Where is this map being allocated now?
There was a problem hiding this comment.
It's allocated in buildRegistrationEntriesCache, which is how the structure is created.
Signed-off-by: Sorin Dumitru <[email protected]>
If we encounter issues fetching any of the entries we need to fetch we should retry later. Signed-off-by: Sorin Dumitru <[email protected]>
sorindumitru
force-pushed
the
events-based-fixes
branch
from
05a4781 to
9226490
Compare
Pull Request check list
Affected functionality
spire-server events based cache
Description of change
Which issue this PR fixes
fixes #5771