feat: exposes --external-data-provider-response-cache-ttl via helm chart #2978
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #2978 +/- ##
==========================================
+ Coverage 53.07% 53.14% +0.06%
==========================================
Files 135 135
Lines 11813 11813
==========================================
+ Hits 6270 6278 +8
+ Misses 5055 5049 -6
+ Partials 488 486 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
nilekhc
force-pushed
the
edrc-flag
branch
2 times, most recently
from
093b365
to
13fef70
Compare
| @@ -70,6 +70,7 @@ spec: | |||
| - --tls-min-version={{ .Values.controllerManager.tlsMinVersion }} | |||
| - --validating-webhook-configuration-name={{ .Values.validatingWebhookName }} | |||
| - --mutating-webhook-configuration-name={{ .Values.mutatingWebhookName }} | |||
| - --external-data-provider-response-cache-ttl={{ .Values.externaldataProviderResponseCacheTTL }} | |||
There was a problem hiding this comment.
Does the audit deployment also have ED cache response ttl option? From my perspective, it makes sense for audit to have a longer ttl so that results from a single audit interval are consistent. However, admission scenarios should have a shorter ttl? If the ED responds with an error for one of the keys due to some transient error, we should not have such a long ttl before the next request to the ED.
There was a problem hiding this comment.
it makes sense for audit to have a longer ttl so that results from a single audit interval are consistent.
To nit:
I don't think a TTL helps with that one way or another, since they are only loosely correlated. If the TTL is too long, it will refresh during the next audit, making that audit's response inconsistent. If an audit cycle runs long, the TTL will still refresh.
Audit, in general, is subject to time aliasing. Since we are reading from the API server (or a watch of resources from the API server), the state of resources can change underneath us as audit progresses.
If point-in-time consistency is important, shift-left via something like a precommit hook is your best bet. At that point, I'd also choose a different caching mechanism, like caching per-run.
If we had the ability to cache "per-session-id", then we could scope the cache duration to a given audit run. This is something we considered, but decided to start with this because of the advantages TTL has for the webhook (which can run multiple validations in parallel).
That being said, I have no objection to being able to express a different TTL value for audit vs. webhook, just important to be clear about the benefit.
There was a problem hiding this comment.
If the ED responds with an error for one of the keys due to some transient error, we should not have such a long ttl before the next request to the ED.
IIRC errors are not cached, so any erroneous request would be re-ran as a cache miss.
There was a problem hiding this comment.
thanks @maxsmythe for the explanation
There was a problem hiding this comment.
do we not need to set external-data-provider-response-cache-ttl in the audit deployment? since it's a separate controller/deployment from the controller-manager deployment yaml?
There was a problem hiding this comment.
We should def. define a TTL both places. I thought the issue was whether they needed to be set to the same value, or if we had knobs for setting different values.
There was a problem hiding this comment.
I have set it to the same value for now
Signed-off-by: Nilekh Chaudhari <1626598+nilekhc@users.noreply.github.com>
|
@maxsmythe Could you PTAL when you get a chance? Thanks. |
…art (open-policy-agent#2978) Signed-off-by: Nilekh Chaudhari <1626598+nilekhc@users.noreply.github.com>
…art (open-policy-agent#2978) Signed-off-by: Nilekh Chaudhari <1626598+nilekhc@users.noreply.github.com>
What this PR does / why we need it:
This PR exposes
--external-data-provider-response-cache-ttlvia helm chart.Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Fixes #2977
Special notes for your reviewer: