-
Notifications
You must be signed in to change notification settings - Fork 29
feat: add secret expiration reconciler #588
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: add secret expiration reconciler #588
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v1alpha1 #588 +/- ##
============================================
- Coverage 79.35% 77.26% -2.09%
============================================
Files 11 11
Lines 1172 1267 +95
============================================
+ Hits 930 979 +49
- Misses 217 258 +41
- Partials 25 30 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4c1bcd3
to
a955848
Compare
@sthaha I have tested with two controllers and with a ticker in a single controller (power monitor internal). They both work as expected under varying conditions (upon creation, upon cr changes, upon removal, etc). I have also confirmed that they are seamless too (will not cause any metrics to disappear). I believe using a second controller is better because if we need to add more secrets or secret tokens with expirations, then this controller can be adapted to handle that. |
@KaiyiLiu1234 , a small question, may I know where secret used for? and it sounds like a cert, so after this PR, how we integrate with cert mgr? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can avoid a lot of repetition of logic by using right interfaces.
6ce25cd
to
d4b8f60
Compare
@SamYuan1990 The secret is called prometheus-user-workload-token and it is used for authorization so kube rbac proxy can identify/authenticate this as prometheus-user-workload (token review). This is critical part of the security for kube rbac proxy as without the token, kube rbac proxy cannot identify the object attempting to contact it. This is not a certificate. This is a secret that contains a jwt token of the service account. |
d4b8f60
to
2ecfc7f
Compare
@sthaha flags have been added and code improvements have been made. Manual check using the ttl and refresh flags is required (by default, ttl is 7 days and refresh is 1 day). |
6e87714
to
3aeb732
Compare
|
||
//go:embed assets/dashboards/power-monitor-namespace-info.json | ||
namespaceInfoDashboardJson string | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comment specifying its a default value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add UT for the new changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same UT for this as well?
3aeb732
to
66d163e
Compare
@KaiyiLiu1234 Are there new code changes? I see the tests are failing after recent push: https://github.com/sustainable-computing-io/kepler-operator/actions/runs/17871112754/job/51119968780?pr=588 |
66d163e
to
85dade7
Compare
85dade7
to
5c15778
Compare
- op: add | ||
path: /spec/template/spec/containers/0/args/0 | ||
value: --deployment-namespace=kepler | ||
- op: add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need for k8s as UWM is specific to OCP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I can remove that. Also, can we just merge this if after your tests it works as expected? I can add the reconciler tests afterwards in a separate PR.
@KaiyiLiu1234 Unrelated to your changes but something we should fix which is that sometime deleter takes time:
|
5c15778
to
66db119
Compare
Before this patch, the Prometheus User Workload Token Secret did not have a reliable reconciliation pattern to handle expirations. This patch added a controller which will remove any secrets that have expired, so they can be reconciled. The UWM Token Secret has its expiration date reduced from 1 year to 1 week, and the controller will check for expired tokens once per day. Signed-off-by: Kaiyi Liu <[email protected]>
66db119
to
bb42bad
Compare
06f822b
into
sustainable-computing-io:v1alpha1
Before this patch, the Prometheus User Workload Token Secret did not have a reliable reconciliation pattern to handle expirations. This patch added a controller which will remove any secrets that have expired, so they can be reconciled. The UWM Token Secret has its expiration date reduced from 1 year to 1 week, and the controller will check for expired tokens once per day.