fix: Disable ESO refresh/push custom actions when they would do nothing

[crabique]

When spec.refreshInterval is 0s for either ExternalSecret or PushSecret, adding the annotation to it will not trigger a forced refresh/push, so the custom action are misleading as it would seem like something happens, but nothing actually does.

Related ESO issue: external-secrets/external-secrets#4447

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• The title of the PR conforms to the Toolchain Guide
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).
• My new feature complies with the feature status guidelines.
• I have added a brief description of why this PR is necessary and/or what this PR solves.
• Optional. My organization is added to USERS.md.
• Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

[bunnyshell]

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

• 🚀 /bns:deploy to deploy the environment

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (master@2afcb6f). Learn more about missing BASE report.
⚠️ Report is 735 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #22305   +/-   ##
=========================================
  Coverage          ?   55.88%           
=========================================
  Files             ?      343           
  Lines             ?    57327           
  Branches          ?        0           
=========================================
  Hits              ?    32039           
  Misses            ?    22646           
  Partials          ?     2642           

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

[crenshaw-dev]

Basic idea lgtm, just have one request to cover more possible values.

[crenshaw-dev]

Looks like refreshInterval is based on go's time.ParseDuration, so we'd also need to handle 0, 0ns, 0us, 0µs, 0ms, 0m, and 0h to cover all cases.

[crabique]

Yeah I was checking that too, it seems that in modern versions of Go it always serializes to 0s, at least according to kubernetes/kubernetes#40783

For any 0-ish duration it's printed as 0s: https://go.dev/play/p/mKxx1R5ruHN

[crenshaw-dev]

It looks like the Lua script receives whatever is in the YAML, not the sting-serialized version.

Screen.Recording.2025-03-14.at.10.20.57.AM.mov

[crabique]

@crenshaw-dev do you think something like string.find(obj.spec.refreshInterval, "^0[^0-9]*$") would be acceptable? Could also be a more explicit "^0[nuµsmh]*$"..

[crenshaw-dev]

I like the explicit regex, assuming we have access to a regex lib in Lua

[crabique]

The * is to handle the composite suffixes (ns, us, µs, ms), and to match a suffix-less 0, as I understand it can be put in YAML as well.

Regarding the pattern matching library, this should be the standard lib: didn't require any imports to work with vanilla interpreter.

[crenshaw-dev]

Ah fair... maybe this:

^0(0*(ns|us|µs|ms|s|m|h))?$

That would handle 0, 0s, 000h but would reject the invalid 00. Probably not a perfect match with what time.ParseDuration supports, but close enough.

[crabique]

Unfortunately, it didn't work because this isn't really a regex engine and it doesn't support groupings of characters.

The only thing that worked reliably so far is the ugly "^0*[nuµsmh]*$", but even splitting it to "^0[nuµm]?[smh]?$" stops matching 0µs for some ASCII reasons I assume.

Maybe a pivot to something more complex, but readable instead?

disable_refresh = false
local time_units = {"ns", "us", "µs", "s", "m", "h"}
local digits = obj.spec.refreshInterval
for _, time_unit in ipairs(time_units) do
    digits, _ = digits:gsub(time_unit, "")
    if tonumber(digits) == 0 then
        disable_refresh = true
        break
    end
end

I'm sure it has great performance, but it can also handle stuff like 0h0m0s0ms0µs0ns which is technically a correct time.ParseDuration input 😁

[crenshaw-dev]

I'm cool with complex! Especially nice that it handles more possibilities.

[crabique]

Thanks for the help! PR updated, PTAL

[KyriosGN0]

Hey, while the issue statement is valid, it is possible to set RefreshPolicy: OnChange on ExternalSecret (and i think PushSecret as well) and then this option becomes valid, would you be open a to PR to change that behavior? or do i need to open an issue to discuss this? @crenshaw-dev