Parametrize delete operations and timeout for webhook in helm chart #1051

jonnylangefeld · 2021-01-05T23:41:01Z

The readme mentions how to enable delete operations by saying you should modify the yaml for the ValidatingWebhookConfiguration. However, there is no way to do this in the helm chart withough using your own fork.
This fix allows to overwrite the value if you use the helm chart as a dependency.

Additionally the webhook timeout was parametrized because in our organization we actually have a few clusters with slower response times.

Both changes are not breaking any existing integrations, they just allow to overwrite default values

ritazh · 2021-01-06T01:29:40Z

Thanks for the PR @jonnylangefeld

Please update the yamls in the manifest_staging/ folder, where we host the staging charts and deployment yamls. All the yaml changes will then be promoted into the released charts folder with the next release. Please also add the new configurable values to the configuration table.

codecov-io · 2021-01-06T20:23:15Z

Codecov Report

Merging #1051 (ae52ff6) into master (c36b54c) will decrease coverage by 0.16%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1051      +/-   ##
==========================================
- Coverage   47.66%   47.50%   -0.17%     
==========================================
  Files          62       62              
  Lines        4225     4225              
==========================================
- Hits         2014     2007       -7     
- Misses       1956     1960       +4     
- Partials      255      258       +3

Flag	Coverage Δ
unittests	`47.50% <ø> (-0.17%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...onstrainttemplate/constrainttemplate_controller.go	`52.59% <0.00%> (-2.93%)`	⬇️
pkg/readiness/ready_tracker.go	`69.81% <0.00%> (+0.72%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c36b54c...ae52ff6. Read the comment docs.

jonnylangefeld · 2021-01-06T20:23:35Z

@ritazh thanks for the review, I added the things you asked for.
Doe you have an ETA when we can expect this fix in a release?

sozercan · 2021-01-06T21:08:11Z

charts/gatekeeper/README.md

-| disableValidatingWebhook         | Disable ValidatingWebhook                                                                                                                                                                              | `false`                                                                   |
+| disableValidatingWebhook         | Disable the validating webhook                                                                                                                                                                       | `false`                                                                   |
+| validatingWebhookTimeoutSeconds  | The timeout for the validating webhook in seconds                                                                                                                                                                                        | `3`                                                                       |
+| enableDeleteOperations         | Enable validating webhook for delete operations                                                                                                                                                                                     | `false`                                                                   |


this change should only be in manifest_staging

sozercan · 2021-01-06T21:09:37Z

README.md

@@ -475,6 +475,8 @@ Note: For admission webhooks registered for DELETE operations, use Kubernetes v1
    - DELETE
 ```

+If you use the helm chart, set `enableDeleteOperations=true`.


We should update readme after release is cut

will that happen automatically based on the readme in staging_docs?

sozercan · 2021-01-06T21:10:22Z

...er/templates/gatekeeper-validating-webhook-configuration-validatingwebhookconfiguration.yaml

@@ -31,10 +31,13 @@ webhooks:
    operations:
    - CREATE
    - UPDATE
+    {{- if .Values.enableDeleteOperations }}


same here, you can revert any changes in charts

Even though I don't fully understand (isn't there a discrepancy now between the chart in charts and in manifest_staging?), I have reverted all changes in charts.

when we cut a new release, everything existing in charts is wiped and manifest_staging/charts gets promoted to charts.

jonnylangefeld · 2021-01-06T22:52:25Z

@sozercan @ritazh this PR doesn't touch the file reported in the code coverage report constrainttemplate_controller.go. How come that's blocking due to dropped test coverage?

The readme mentions how to [enable delete operations](https://github.com/open-policy-agent/gatekeeper\#enable-delete-operations) by saying you should modify the yaml for the ValidatingWebhookConfiguration. However, there is no way to do this in the helm chart withough using your own fork. This fix allows to overwrite the value if you use the helm chart as a dependency. Additionally the webhook timeout was parametrized because in our organization we actually have a few clusters with slower response times. Both changes are not breaking any existing integrations, they just allow to overwrite default values Signed-off-by: Jonny Langefeld <jonny.langefeld@gmail.com>

Signed-off-by: Jonny Langefeld <jonny.langefeld@gmail.com>

maxsmythe

LGTM

jonnylangefeld force-pushed the jlf/parametrize-webhook branch from e598302 to 8eea2b7 Compare January 5, 2021 23:41

jonnylangefeld force-pushed the jlf/parametrize-webhook branch 3 times, most recently from 93a81f7 to 34dec8e Compare January 6, 2021 20:22

sozercan reviewed Jan 6, 2021

View reviewed changes

jonnylangefeld force-pushed the jlf/parametrize-webhook branch from d4fa053 to a2488d4 Compare January 6, 2021 21:29

jonnylangefeld added 3 commits January 7, 2021 09:48

Implement pull request feedback

dcaec89

Signed-off-by: Jonny Langefeld <jonny.langefeld@gmail.com>

Revert changes in charts

a8517c9

Signed-off-by: Jonny Langefeld <jonny.langefeld@gmail.com>

jonnylangefeld force-pushed the jlf/parametrize-webhook branch from caabd36 to a8517c9 Compare January 7, 2021 17:48

ritazh approved these changes Jan 7, 2021

View reviewed changes

ritazh requested a review from a team January 7, 2021 22:46

maxsmythe approved these changes Jan 8, 2021

View reviewed changes

jonnylangefeld and others added 2 commits January 7, 2021 19:37

Merge branch 'master' into jlf/parametrize-webhook

ae52ff6

Merge branch 'master' into jlf/parametrize-webhook

ae750f6

sozercan merged commit e68a39e into open-policy-agent:master Jan 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parametrize delete operations and timeout for webhook in helm chart #1051

Parametrize delete operations and timeout for webhook in helm chart #1051

jonnylangefeld commented Jan 5, 2021

ritazh commented Jan 6, 2021

codecov-io commented Jan 6, 2021 •

edited

Loading

jonnylangefeld commented Jan 6, 2021

sozercan Jan 6, 2021 •

edited

Loading

sozercan Jan 6, 2021

jonnylangefeld Jan 6, 2021

sozercan Jan 6, 2021

jonnylangefeld Jan 6, 2021

sozercan Jan 6, 2021

jonnylangefeld commented Jan 6, 2021

maxsmythe left a comment

Parametrize delete operations and timeout for webhook in helm chart #1051

Parametrize delete operations and timeout for webhook in helm chart #1051

Conversation

jonnylangefeld commented Jan 5, 2021

ritazh commented Jan 6, 2021

codecov-io commented Jan 6, 2021 • edited Loading

Codecov Report

jonnylangefeld commented Jan 6, 2021

sozercan Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

sozercan Jan 6, 2021

Choose a reason for hiding this comment

jonnylangefeld Jan 6, 2021

Choose a reason for hiding this comment

sozercan Jan 6, 2021

Choose a reason for hiding this comment

jonnylangefeld Jan 6, 2021

Choose a reason for hiding this comment

sozercan Jan 6, 2021

Choose a reason for hiding this comment

jonnylangefeld commented Jan 6, 2021

maxsmythe left a comment

Choose a reason for hiding this comment

codecov-io commented Jan 6, 2021 •

edited

Loading

sozercan Jan 6, 2021 •

edited

Loading