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

Fix/restart on tenanttokenchange without fieldchange (release-1.4) #4376

Merged

Conversation

gkrenn
Copy link
Contributor

@gkrenn gkrenn commented Jan 28, 2025

Description

The tenant and ActiveGate tokens can be rotated using the tenant's rotation API.

This process updates the oneagent-tenant-secret and ActiveGate secret. To activate these changes, the OneAgents and ActiveGates need to be restarted.

To accomplish this, this PR adds a hash as an annotation to the DaemonSet and StatefulSet. If the hash changes, the pods will be restarted.

To not introduce a new field on a bugfix version, it uses the old bugfix implementation. For the new implementation including a new field look at PR #4361

Jira

How can this be tested?

  • Deploy dynakube with oneagents
  • Navigate to the api docs in your tenant
  • Start the token rotation
  • the oneagents will be updated after some time.

@gkrenn gkrenn added the bug Something isn't working label Jan 28, 2025
@gkrenn gkrenn requested a review from a team as a code owner January 28, 2025 14:53
@gkrenn gkrenn requested review from andriisoldatenko, 0sewa0, chrismuellner, waodim, StefanHauth and aorcholski and removed request for a team January 28, 2025 14:53
@codecov-commenter
Copy link

codecov-commenter commented Jan 28, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 80.89888% with 17 lines in your changes missing coverage. Please review.

Project coverage is 64.29%. Comparing base (c62f39b) to head (495cd73).
Report is 1 commits behind head on release-1.4.

Files with missing lines Patch % Lines
...ntrollers/dynakube/oneagent/oneagent_reconciler.go 59.09% 7 Missing and 2 partials ⚠️
...kube/activegate/internal/statefulset/reconciler.go 52.94% 6 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@               Coverage Diff               @@
##           release-1.4    #4376      +/-   ##
===============================================
+ Coverage        64.28%   64.29%   +0.01%     
===============================================
  Files              401      402       +1     
  Lines            27072    27135      +63     
===============================================
+ Hits             17403    17447      +44     
- Misses            8302     8317      +15     
- Partials          1367     1371       +4     
Flag Coverage Δ
unittests 64.29% <80.89%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@StefanHauth StefanHauth changed the title Fix/restart on tenanttokenchange without fieldchange Fix/restart on tenanttokenchange without fieldchange (release-1.4) Jan 28, 2025
Copy link
Collaborator

@StefanHauth StefanHauth left a comment

Choose a reason for hiding this comment

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

Works on my machine for OneAgent. For whatever reason the AG token doesn't get rotated when I start a token rotation.
I'd like to have a second opinion on this PR, please.

Copy link
Contributor

@aorcholski aorcholski left a comment

Choose a reason for hiding this comment

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

AG reports an error

... message has an error: REQUEST_REJECTED

The same error if I deploy dynakube again from scratch.

if err != nil {
log.Error(err, "secret for tenant token was not available at DaemonSet build time", "dynakube", r.dk.Name)
conditions.SetKubeApiError(r.dk.Conditions(), conditionType, err)
}
Copy link
Contributor

@aorcholski aorcholski Jan 31, 2025

Choose a reason for hiding this comment

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

IMHO we should return err and shouldn't change the template if K8S API error encountered. We don't know if the secret/tenant has changed or not.

if err != nil {
log.Error(err, "secret for tenant token was not available at DaemonSet build time", "dynakube", r.dk.Name)
conditions.SetKubeApiError(r.dk.Conditions(), oaConditionType, err)
}
Copy link
Contributor

@aorcholski aorcholski Jan 31, 2025

Choose a reason for hiding this comment

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

IMHO we should return err and shouldn't change the template if K8S API error encountered. We don't know if the secret/tenant has changed or not.


if err != nil {
log.Error(err, "secret for activegate token was not available at StatefulSet build time", "dynakube", r.dk.Name)
}
Copy link
Contributor

@aorcholski aorcholski Jan 31, 2025

Choose a reason for hiding this comment

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

IMHO we should return err and should'n change the template if K8S API error encountered. We don't know if the secret/tenant has changed or not.

@gkrenn
Copy link
Contributor Author

gkrenn commented Jan 31, 2025

Thanks for your detailed checks, I adapted the PR according to the review.

@gkrenn gkrenn requested a review from aorcholski January 31, 2025 15:06
Copy link
Contributor

@aorcholski aorcholski left a comment

Choose a reason for hiding this comment

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

LGTM

@gkrenn gkrenn enabled auto-merge (squash) February 3, 2025 09:35
@gkrenn gkrenn merged commit f1dfd55 into release-1.4 Feb 3, 2025
15 checks passed
@gkrenn gkrenn deleted the fix/restart-on-tenanttokenchange-without-fieldchange branch February 3, 2025 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants