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: cache auth tokens in GenServer per connection #302

Merged
merged 4 commits into from
Feb 5, 2024
Merged

feat: cache auth tokens in GenServer per connection #302

merged 4 commits into from
Feb 5, 2024

Conversation

elliottneilclark
Copy link
Contributor

@elliottneilclark elliottneilclark commented Jan 22, 2024

When connecting to the kube api there are often authentication data that's needed for each request. The lifecycle of that authentication token data is important to stability and performance. This pull request will add on running processes that cache and refresh tokens and certificates.

Tokens can be generated via a process. Shelling out will return a ExecCredentialStatus. The ExecWorker added in this PR will cache that value (meaning that subsequent requests will not need to fork out many times). Additionally this PR will implement a refresh on a timer. That has the added benefit of making distributed systems much more reliable as we are able to retry a few times befor expiary, and we can add jitter.

Certificates in cloud envioriments can be ephemeral. They are not created and then kept forever. Often they have lifespans in the minutes. This PR will add on a CertificateWorker that caches the value from disk, and refresh it before the timeout.


Requirements for all pull requests

  • Entry in CHANGELOG.md was created

Additional requirements for new features

  • New unit tests were created
  • New integration tests were created
  • PR description contains a link to the according kubernetes documentation

Copy link
Contributor

@JTarasovic JTarasovic left a comment

Choose a reason for hiding this comment

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

Quick initial review.

lib/k8s/conn/auth/exec_worker.ex Outdated Show resolved Hide resolved
lib/k8s/conn/auth/exec_worker.ex Outdated Show resolved Hide resolved
Copy link
Collaborator

@mruoss mruoss left a comment

Choose a reason for hiding this comment

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

already looks very nice!

lib/k8s/conn/auth/exec_worker.ex Outdated Show resolved Hide resolved
lib/k8s/conn/auth/exec_worker.ex Show resolved Hide resolved
lib/k8s/conn.ex Show resolved Hide resolved
@elliottneilclark elliottneilclark marked this pull request as ready for review January 23, 2024 20:18
Summary:

- Start `K8s.Conn.Auth.ProviderSupervisor`
- Start `K8s.Conn.Auth.ExecWorker` for kube context with auth exec
  configured. This respects expiration and will not give out known
  expired tokens.
- Start `K8s.Conn.Auth.ServiceAccountWorker` for connections started
  with `K8s.Conn.from_service_account/2`. This periodically refreshes
  the token.
- Add refresh timers with jitter on for all of those
- Start `K8s.Conn.Auth.CertificateWorker` for kube context where given a
  .pem file. Some cloud providers are giving very short duration certs.

Test Plan:

- Added test for exec parsing
- Added test for exec worker
- Added test for service account
- Added integration-ish test for service account worker refresh
- Changed test for certificate to ensure genserver
- Added test for certificate worker

`mix test`
Summary:
This adds on a registry and a standard name for many of the auth
providers. It allows for shared workers when the paths are the same and
for genserver to crash, get re-started and not have errors due to stale
pids.

Test Plan:
Tests were updated
@mruoss
Copy link
Collaborator

mruoss commented Feb 1, 2024

Hey @elliottneilclark, is this PR ready for review?

lib/k8s/conn/auth/certificate.ex Outdated Show resolved Hide resolved
lib/k8s/conn/auth/exec.ex Show resolved Hide resolved
lib/k8s/conn/auth/service_account.ex Outdated Show resolved Hide resolved
@elliottneilclark
Copy link
Contributor Author

@mruoss All the review comments are addressed. This should be good to go.

@mruoss
Copy link
Collaborator

mruoss commented Feb 5, 2024

very nice, thanks @elliottneilclark

@mruoss mruoss merged commit 92a6704 into coryodaniel:develop Feb 5, 2024
13 checks passed
@elliottneilclark
Copy link
Contributor Author

@mruoss Any chance of a released version? I would love to not point at git :-)

@mruoss
Copy link
Collaborator

mruoss commented Apr 2, 2024

ouff absolutely! Sorry about that!

@elliottneilclark
Copy link
Contributor Author

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