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.

Sign up for GitHub

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

Jump to bottom

feat: exposes --external-data-provider-response-cache-ttl via helm chart #2978

Merged
merged 3 commits into from
Sep 13, 2023
Merged

feat: exposes --external-data-provider-response-cache-ttl via helm chart #2978

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
234c593
feat: exposes --external-data-provider-response-cache-ttl via helm chart
nilekhc Aug 29, 2023
5d3d8bb
Merge branch 'master' into edrc-flag
nilekhc Sep 6, 2023
7a927a2
Merge branch 'master' into edrc-flag
nilekhc Sep 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cmd/build/helmify/kustomize-for-helm.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,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 }}
- HELMBUST_ENABLE_TLS_APISERVER_AUTHENTICATION
- HELMSUBST_METRICS_BACKEND_ARG
- HELMSUBST_TLS_HEALTHCHECK_ENABLED_ARG
Expand Down Expand Up @@ -180,6 +181,7 @@ spec:
- HELMSUBST_METRICS_BACKEND_ARG
- HELMSUBST_DEPLOYMENT_AUDIT_LOGFILE
- --disable-cert-rotation={{ or .Values.audit.disableCertRotation .Values.externalCertInjection.enabled }}
- --external-data-provider-response-cache-ttl={{ .Values.externaldataProviderResponseCacheTTL }}
imagePullPolicy: "{{ .Values.image.pullPolicy }}"
HELMSUBST_AUDIT_CONTROLLER_MANAGER_DEPLOYMENT_IMAGE_RELEASE: ""
ports:
Expand Down
1 change: 1 addition & 0 deletions cmd/build/helmify/static/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ information._
| rbac.create | Enable the creation of RBAC resources | `true` |
| externalCertInjection.enabled | Enable the injection of an external certificate. This disables automatic certificate generation and rotation | `false` |
| externalCertInjection.secretName | Name of secret for injected certificate | `gatekeeper-webhook-server-cert` |
| externaldataProviderResponseCacheTTL | TTL for the external data provider response cache. Specify the duration in 'h', 'm', or 's' for hours, minutes, or seconds respectively. | `3m` |

## Contributing Changes

Expand Down
1 change: 1 addition & 0 deletions cmd/build/helmify/static/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ emitAuditEvents: false
admissionEventsInvolvedNamespace: false
auditEventsInvolvedNamespace: false
resourceQuota: true
externaldataProviderResponseCacheTTL: 3m
image:
repository: openpolicyagent/gatekeeper
crdRepository: openpolicyagent/gatekeeper-crds
Expand Down
1 change: 1 addition & 0 deletions manifest_staging/charts/gatekeeper/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ information._
| rbac.create | Enable the creation of RBAC resources | `true` |
| externalCertInjection.enabled | Enable the injection of an external certificate. This disables automatic certificate generation and rotation | `false` |
| externalCertInjection.secretName | Name of secret for injected certificate | `gatekeeper-webhook-server-cert` |
| externaldataProviderResponseCacheTTL | TTL for the external data provider response cache. Specify the duration in 'h', 'm', or 's' for hours, minutes, or seconds respectively. | `3m` |

## Contributing Changes

Expand Down
1 change: 1 addition & 0 deletions manifest_staging/charts/gatekeeper/templates/gatekeeper-audit-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ spec:
- --log-file={{ .Values.audit.logFile }}
{{- end }}
- --disable-cert-rotation={{ or .Values.audit.disableCertRotation .Values.externalCertInjection.enabled }}
- --external-data-provider-response-cache-ttl={{ .Values.externaldataProviderResponseCacheTTL }}
command:
- /manager
env:
Expand Down
1 change: 1 addition & 0 deletions manifest_staging/charts/gatekeeper/templates/gatekeeper-controller-manager-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

thanks @maxsmythe for the explanation

Copy link
Member

@ritazh ritazh Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have set it to the same value for now

{{ if ne .Values.controllerManager.clientCertName "" }}- --client-cert-name={{ .Values.controllerManager.clientCertName }}{{- end }}

{{- range .Values.metricsBackends}}
Expand Down
1 change: 1 addition & 0 deletions manifest_staging/charts/gatekeeper/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ emitAuditEvents: false
admissionEventsInvolvedNamespace: false
auditEventsInvolvedNamespace: false
resourceQuota: true
externaldataProviderResponseCacheTTL: 3m
image:
repository: openpolicyagent/gatekeeper
crdRepository: openpolicyagent/gatekeeper-crds
Expand Down
Loading