Populate and prune entry event table

[faisal-memon]

Pull Request check list

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

Affected functionality
SPIRE datastore

Description of change
Second part of datastore enhancement:

• Creates an event every time an entry is created/updated/deleted.
• Events are pruned every 12 hours. Pruning interval is configurable.
• Creates 2 new datastore APIs to list and prune events
• Listing events can be filtered to be greater than an event id

The cache is not updated with this pr. Node events is not populated yet either. That will be in the next PR.

Which issue this PR fixes
Related to #2182

[edwbuck]

@amartinezfayo Can we start a review on this with priority?

[azdagron]

I think we're still waiting on an experimental flag that encapsulates the whole feature. We can't assume the tables used by the queries will exist until one minor release after the minor release in which the table migrations landed.

[azdagron]

We talked about this again today and realized that we probably shouldn't have an experimental flag for this until the whole feature is in place. Until then it probably belongs under a developer feature flag. We shouldn't wire up any of the new components (including populating the events table), unless that flag is on. Once the whole feature is in place, we can remove the developer feature flag in favor of an experimental flag that folks can use to exercise the new cache. And eventually once we have confidence, we can make this the default behavior.

[MarcosDY]

What this FirstEventID is?
is it id of the first elment in the response? how is it passed (or affected) on pagination?

[faisal-memon]

yes first id of element in responses.

[faisal-memon]

Im not sure about pagination. let me test

[faisal-memon]

Do we need pagination for this api?

[edwbuck]

I'd put it in. Bulk deletion / addition APIs could (even though the data per event is small) overflow the single page response.

[faisal-memon]

I tried to create unit tests for the pagination but it seems to overlap some with the specific greater than event id filter for this api. This is similar to the Token half of the pagination, but with an unlimited page size. Thats what we need for this functionality, all events that happened since the last time we polled the api.

Given the current implementation fetches all entries with no pagination, im not sure pagination is needed here. @edwbuck what do you think of removing the pagination? This would simplify the implementation.

[edwbuck]

I don't have an issue with non-pagination. I just wanted to mention the bulk updates.

In my mind, if pagination becomes and issue, we can address it then. Honestly, the events have so few data items in them (just references to other items) that I imagine one would need a lot of them to overflow the response.

[MarcosDY]

I dont have strong opinion with pagination.
We can get into s bulk insert with several entries, or a server that is not consuming updates for some time, in that case we'll be returning several IDs here and fetching all of them together, maybe we can ""paginate" the response in the caller? just to prevent protobuf limits? (in case someone has a lot of entries to update)

But I dont have issues about not supporting pagination. @azdagron do you have something against removing pagination here?

[MarcosDY]

looks like tehre is no unit tests for pruning, I think we must validate that path too

[faisal-memon]

added test for pruning

[MarcosDY]

may we compare the expected IDs? tehe same comment is for all equal compartions
Using a list of expected

[faisal-memon]

Added comparisons

[MarcosDY]

if we are adding default at this level, I think we must validate that value using a test

[MarcosDY]

so, every 12hs it will try to remove all entries that are older than 12hs,
if we have e1, e2, e3 and e1 was created 13hs ago, and e2 11hs ago,
if we run prune, e1 will be removed now, but e2 will be removed in another 12hs (it will have 23hs)

I'm not sure if we must have 2 intervals,
first to know how ofter we try to remove (it can be every 1m or 10m or 1h) and
another one to know the older entries we support.

[edwbuck]

It has been a while, but I believe the cache_reload_interval is now used to see how often to check the cache, and the eventsPruneInterval is the time from "now" into the past that sets the pruning limit.

[faisal-memon]

We can do that way and prune every time we reload the cache. Though there would be more delete operations.

Can also have the pruner run at a faster rate than it prunes. So runs every 6 hours and prunes everything over 12 hours old.

[faisal-memon]

Changed the name of the configurable to PruneEventsOlderThan and have the pruner task trigger at half of that.

[MarcosDY]

any of this functions are actualy using prune,
I think we'll need to add unit tests to validate prune too

[faisal-memon]

test added

[faisal-memon]

@azdagron @MarcosDY All comments have been addressed. Ready for review.

[MarcosDY]

can you add unit tests for this? (if we are going to merge this config)

[faisal-memon]

added

[MarcosDY]

I dont have strong opinion with pagination.
We can get into s bulk insert with several entries, or a server that is not consuming updates for some time, in that case we'll be returning several IDs here and fetching all of them together, maybe we can ""paginate" the response in the caller? just to prevent protobuf limits? (in case someone has a lot of entries to update)

But I dont have issues about not supporting pagination. @azdagron do you have something against removing pagination here?

[MarcosDY]

what do you think about adding clock to ds layer (and a mock here), and use that instead of time.Now.
That will allow you to add test cases where you can advance time and have more robust test cases where you have more events and prune partially

[faisal-memon]

I think thats a great idea. Is it ok to that in a follow on pr? I think its a little out of scope for this one.

[MarcosDY]

you can relay on ctx done here

[faisal-memon]

Are you requesting to put case <-ctx.Done():?

[MarcosDY]

since context will finish (with timeout) you can rely ctx.Done()

[faisal-memon]

In this case we are calling cancel() just a few lines above on line 239. If we use ctx.Done() that will always be true so we never check for the watchErr on line 241.

[MarcosDY]

you can depends on ctx .Done

[faisal-memon]

Changed.

[faisal-memon]

DCO fixed and branch up to date with latest.

[MarcosDY]

Since this is experimental it must be called only when feature flag is enabled

Suggested change

	func listRegistrationEntriesEvents(tx *gorm.DB, req *datastore.ListRegistrationEntriesEventsRequest) (*datastore.ListRegistrationEntriesEventsResponse, error) {
	var events []RegisteredEntryEvent
	func listRegistrationEntriesEvents(tx *gorm.DB, req *datastore.ListRegistrationEntriesEventsRequest) (*datastore.ListRegistrationEntriesEventsResponse, error) {
	if !fflag.IsSet(fflag.FlagEventsBasedCache) {
	return &datastore.ListRegistrationEntriesEventsRespons{}, nil
	}
	var events []RegisteredEntryEvent

[MarcosDY]

Since this is experimental it must be called only when feature flag is enabled

Suggested change

	func pruneRegistrationEntriesEvents(tx *gorm.DB, olderThan time.Duration) error {
	if err := tx.Where("created_at < ?", time.Now().Add(-olderThan)).Delete(&RegisteredEntryEvent{}).Error; err != nil {
	func pruneRegistrationEntriesEvents(tx *gorm.DB, olderThan time.Duration) error {
	if !fflag.IsSet(fflag.FlagEventsBasedCache) {
	return nil
	}
	if err := tx.Where("created_at < ?", time.Now().Add(-olderThan)).Delete(&RegisteredEntryEvent{}).Error; err != nil {