New Mutable Authorized Entry Cache #4451
Conversation
azdagron
force-pushed
the
authorizedentrycache-new
branch
from
5c7820c to
f9dca51
Compare
|
Thinking a little more about agent expiry, there won't be an event to know when an agent expires. Needs a little more work there. |
|
Done in concert with #4411 |
azdagron
force-pushed
the
authorizedentrycache-new
branch
from
edd7a57 to
850c927
Compare
Signed-off-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: Andrew Harding <azdagron@gmail.com>
azdagron
force-pushed
the
authorizedentrycache-new
branch
from
850c927 to
029b6a8
Compare
azdagron
requested review from
evan2645,
amartinezfayo and
rturner3
as code owners
azdagron
changed the title
|
@faisal-memon, the cache has now been updated with the capability of having expired agent information pruned from the cache by an outside caller. The "hydrator" glue should call this function on some cadence (maybe hourly? or at the same rate it prunes events?). |
| } | ||
| } | ||
|
|
||
| func (c *Cache) stats() cacheStats { |
There was a problem hiding this comment.
If this function is only used by tests, maybe can be moved to cache_test.go?
There was a problem hiding this comment.
I don't feel super strongly, but my preference would be to keep all of the methods of Cache in the same file. Let me know if you feel more strongly than me and I'll move it :)
| c.agentsByID.ReplaceOrInsert(agent) | ||
| c.agentsByExpiresAt.ReplaceOrInsert(agent) |
There was a problem hiding this comment.
NIT: looks like it is always done by couple (replace or Delete) in all cache objects that are related.
I wonder if we must have a function that call both.. func (c xxxx) updateAgent(agent) { c.agentsByID.ReplaceOrInsert(agent) \n c.agentsByExpiresAt.ReplaceOrInsert}
but at the same time it feels as an overkill.. because those are called in the function that basically takes care of updating/deleting records...
what do you think?
There was a problem hiding this comment.
Meh, also feels like overkill to me. I'd rather keep it for now.
| return true | ||
| }) | ||
|
|
||
| for _, record := range entryRecordsToDelete { |
There was a problem hiding this comment.
if we found the entryID as an entry that means it will no be an alias right?
so we can avoid next iteration?
There was a problem hiding this comment.
Yep. Will short-circuit.
| func spiffeIDFromProto(id *types.SPIFFEID) string { | ||
| return fmt.Sprintf("spiffe://%s%s", id.TrustDomain, id.Path) | ||
| } |
There was a problem hiding this comment.
no a comment for this PR, but it makes me think that we must have parsers like this on in a helper function.
same for isNodeAlias
| AliasesBySelector: 2, // one for each selector | ||
| }, cache.stats()) | ||
|
|
||
| cache.RemoveEntry(entry1.Id) |
There was a problem hiding this comment.
may we add a test case that when removing an invalid entryID nothing change?
| expiresAt := now.Add(time.Hour * time.Duration(1+len(a.agents))) | ||
| a.agents[node] = agentInfo{ |
There was a problem hiding this comment.
is not it preventing to test cases where we have 2 agents with the same time? may we test that scenario?
There was a problem hiding this comment.
Restructured the test to test this.
| case a.EntryID > b.EntryID: | ||
| return false | ||
| default: | ||
| return a.ParentID < b.ParentID |
There was a problem hiding this comment.
maybe I'm missing something, but this means that we have 2 entries with the same EntryID? so technically we may never get into this right?
There was a problem hiding this comment.
No, you are right. I'll clean this up.
Signed-off-by: Andrew Harding <azdagron@gmail.com>
azdagron
force-pushed
the
authorizedentrycache-new
branch
from
be21b22 to
463f446
Compare
|
Ugg, i really need to add my commit hook back.... thanks :) |
Signed-off-by: Andrew Harding <azdagron@gmail.com> Signed-off-by: Faisal Memon <fymemon@yahoo.com>
Introduces a new mutable authorized entry cache. This cache can by dynamically updated with changes to entries and agent selectors.
There is a commit on here go move to go1.21 which can be removed once #4450 is merged.
This cache is not wired up to anything. It is intended to be the caching layer that is built via the entry+agent events polled via the database that @faisal-memon is working on.