Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(authentication): per method cleanup process #1161

Merged
merged 35 commits into from
Nov 30, 2022
Merged

Conversation

GeorgeMac
Copy link
Member

@GeorgeMac GeorgeMac commented Nov 25, 2022

This adds a new per authentication method cleanup process.

The process creates 1 background goroutine per enabled authentication method.
This goroutine will attempt to acquire a lock via the storage layer and then deleted expired authentications from the backing store.

The process is repeated on a configurable periodic basis.
The delete process includes a configurable grace period for what is deemed "expired".
Allowing for expired authentications to be left untouched until some elapsed duration.

The new configuration levers can be seen in the following patch:

authentication:
  methods:
    token:
      enabled: true // cleanup is only initiated if the method is enabled
+     cleanup:
+       interval: 1h
+       grace_period: 1h

Each key beneath cleanup configures the associated authentication method.
The clean-up process is only initiated if the method is also enabled.

By default, for an enabled auth method, cleanup is also configured.
It will be configured with 1h interval and 30m grace period.

These defaults are debatable.

@GeorgeMac GeorgeMac marked this pull request as ready for review November 25, 2022 17:05
@GeorgeMac GeorgeMac requested a review from a team as a code owner November 25, 2022 17:05
Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this all looks great!

one question I have is around the configuration, does it make sense to nest cleanup under the already existing method? ie:

authentication:
  methods:
    token:
      enabled: true // cleanup is only started if the method is also enabled
      cleanup:
          interval: 1h
          grace_period: 1h

@GeorgeMac
Copy link
Member Author

@markphelps yeah, this had also crossed my mind shortly after opening the PR.

Ill take a stab at that real quick.

@GeorgeMac
Copy link
Member Author

@markphelps yep, much better 👍

@GeorgeMac GeorgeMac requested a review from markphelps November 28, 2022 10:52
@codecov-commenter
Copy link

codecov-commenter commented Nov 28, 2022

Codecov Report

Merging #1161 (c86fa95) into main (2638b10) will increase coverage by 1.06%.
The diff coverage is 90.41%.

@@            Coverage Diff             @@
##             main    #1161      +/-   ##
==========================================
+ Coverage   78.83%   79.89%   +1.06%     
==========================================
  Files          35       38       +3     
  Lines        2528     2761     +233     
==========================================
+ Hits         1993     2206     +213     
- Misses        435      451      +16     
- Partials      100      104       +4     
Impacted Files Coverage Δ
internal/config/errors.go 100.00% <ø> (ø)
internal/storage/sql/migrator.go 23.45% <ø> (ø)
internal/cleanup/cleanup.go 77.41% <77.41%> (ø)
internal/config/authentication.go 88.88% <86.84%> (-11.12%) ⬇️
internal/storage/oplock/sql/sql.go 96.33% <96.33%> (ø)
internal/config/config.go 82.30% <100.00%> (+2.65%) ⬆️
internal/storage/auth/memory/store.go 77.86% <100.00%> (+0.18%) ⬆️
internal/storage/oplock/memory/memory.go 100.00% <100.00%> (ø)
internal/storage/sql/db.go 91.11% <100.00%> (+0.20%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@markphelps
Copy link
Collaborator

Setting interval to a negative value puts the cleanup process in an infinite loop 😲

authentication:
  required: true
  methods:
    token:
      enabled: true
      cleanup:
        interval: -2h
        grace_period: 48h
2022-11-29T09:42:38-05:00       INFO    cleanup process deleting authentications        {"server": "grpc", "expired_before": "2022-11-27T14:42:38Z"}
2022-11-29T09:42:38-05:00       INFO    cleanup process deleting authentications        {"server": "grpc", "expired_before": "2022-11-27T14:42:38Z"}
2022-11-29T09:42:38-05:00       INFO    cleanup process deleting authentications        {"server": "grpc", "expired_before": "2022-11-27T14:42:38Z"}
2022-11-29T09:42:38-05:00       INFO    cleanup process deleting authentications        {"server": "grpc", "expired_before": "2022-11-27T14:42:38Z"}
2022-11-29T09:42:38-05:00       INFO    cleanup process deleting authentications        {"server": "grpc", "expired_before": "2022-11-27T14:42:38Z"}

We should prob have some validation there that its a positive number greater than 0

Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above about ♾️

@GeorgeMac GeorgeMac requested a review from markphelps November 29, 2022 17:49
Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noice!

@markphelps markphelps enabled auto-merge (squash) November 30, 2022 16:11
@markphelps markphelps merged commit 8ba7ada into main Nov 30, 2022
@markphelps markphelps deleted the gm/auth-cleanup branch November 30, 2022 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants