-
Notifications
You must be signed in to change notification settings - Fork 404
feat: add status subresource with last sync and generation tracking #133
feat: add status subresource with last sync and generation tracking #133
Conversation
5e1d49f
to
30e05c6
Compare
@silasbw @jeffpearce Please have a look :D |
Hi @Flydiverny. It seems like it's much simpler to just always upsert the secret when we start up. Do you have concerns around doing it that way? |
@jeffpearce Cost would be a concern, and also it defeats the purpose of having a polling interval setting if its not respected. As mentioned in #117 having a longer interval (which we do as well) the interval will currently (version 1.3.1) never happen or possibly happen in a totally different interval if either: the watch stream disconnects or the pod is restarted. Say the watch stream doesn't seem to remain connected forever so if I then use a 24h interval and the watch stream only lasts 1h, I would never trigger the interval at 24h because every hour the 24h countdown is restarted as the poller restarts. I don't see any way around this without keeping the state somewhere (which in this PR is in the created secrets). Further building onto this I think we could allow for specifying a polling interval per external secret as well. Could be useful for testing or in cases where some secrets rotate more often. Possibly even disabling polling for some and only update when external secret is updated. |
Hi @Flydiverny, which cost are you thinking of? The cost of polling AWS SM? (Sorry for the delay, been on vacation) |
Any chance of us getting a new release with better support for the longer poll intervals either with/via this PR or with the simpler fix already on master? |
Yes this was referencing the costs of polling AWS SM, after running external-secrets for awhile now in our environments we can see we get unnecessarily expensive bills for the excessive amount of polling that is done. |
Refactored to use status subresource instead. Got some tests to fix if there's any interest in this. Refactor will also got a bit of the way on #185 See also #190 which points out the issue I'm trying to solve here :) |
40836bc
to
37be08b
Compare
eda1cb3
to
8747ee7
Compare
8747ee7
to
6a8792a
Compare
Tests fixed and stuff squashed and rebased to madness. |
d9bff6f
to
49f26f8
Compare
For anyone that wants to try this i pushed an image here https://hub.docker.com/r/flydiverny/kubernetes-external-secrets/tags docker pull flydiverny/kubernetes-external-secrets:1.6.0-with.pr.133 To run it in existing chart you need to manually add rbac to access status. - apiGroups: ["kubernetes-client.io"]
resources: ["externalsecrets/status"]
verbs: ["get", "update"] |
@jeffpearce @silasbw @keweilu Please see updated description and review updated PR! :) |
Thanks for your work here @Flydiverny 🎉 |
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.
Looks good -- thanks for taking another pass on this!
A few nits and one question about typing / naming.
d317363
to
85b8705
Compare
- Remove excessive polls by tracking last sync on external secrets resource - Track generation to poll when resource is modified - Show status when using `kubectl get externalsecrets`
…ed and added events
85b8705
to
2ed11b5
Compare
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.
One more point / question.
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.
Nice work 🏅 🌶️
9608ddb
to
9e86e05
Compare
Currently kubernetes-external-secrets does excessive polling of all external secrets and doesn't respect the polling intervall. Everytime the watch stream disconnects (happens somewhat regularly) all the secrets are regenerated. In our relatively small dev cluster this generates 22 million events to each of KMS, SM and GuardDuty which generates unnecessary billing, and a general waste of compute cycles for the sake of nothing.
This PR uses the status subresource to track generation of external secret, and the last sync time.
Will use these values to determine how to queue up the next poll, either instant or time remaining for next poll intervall to pass. This helps for setting long poll intervals as currently setting something like 24h and having the pod moved or watch stream disconnected would restart the interval from that time, instead of forcing unwanted polls.
Updated CRD to show fields when using kubectl:
closes #131
fixes #117
fixes #153
fixes #190
start of #185