fix: Allow to change WebhookConfiguration name and change preInstall crd image

[jtyr]

MutatingWebhookConfigurations and ValidatingWebhookConfigurations are executed in alphabetical order. That's why the name of those resources matter to maintain the right order of the Webhooks execution. For example there is a MutatingWebhookConfiguration on all AKS clusters that's called aks-webhook-admission-controller. If I want to create AssignMetadata that inserts annotation that the AKS Mutating Webhook is looking for, that won't work without this PR as the current MutatingWebhookConfiguration is gatekeeper-mutating-webhook-configuration which comes alphabetically after aks-webhook-admission-controller. That's why we need this PR to allow to give the MutatingWebhookConfiguration and the ValidatingWebhookConfiguration a different name.

[sozercan]

@jtyr thanks for the PR! Looks like we need DCO to be signed.

[jtyr]

@sozercan All checks but the "Upgrade test" are fine. The "Upgrade test" is failing due to missing AssignImage CRD. Any idea why that is missing in the e2e installetion? This CRD is present in the manifest_staging/charts/gatekeeper/crds/assignimage-customresourcedefinition.yaml file.

[acpana]

should we make a call out about these in the docs? maybe in a separate PR?

[jtyr]

Please could you point me to where exactly do you want me to document this?

[maxsmythe]

@acpana

IMO it being available in --help and in the helm chart README is documentation enough.

We document how to run --help here:

https://open-policy-agent.github.io/gatekeeper/website/docs/customize-startup#other-configuration-options

[maxsmythe]

LGTM

Thank you for the PR!

[codecov-commenter]

Codecov Report

Base: 53.41% // Head: 53.47% // Increases project coverage by +0.05% 🎉

Coverage data is based on head (1cfa0f2) compared to base (5ab923e).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2563      +/-   ##
==========================================
+ Coverage   53.41%   53.47%   +0.05%     
==========================================
  Files         120      120              
  Lines       10634    10634              
==========================================
+ Hits         5680     5686       +6     
+ Misses       4516     4511       -5     
+ Partials      438      437       -1     

Flag	Coverage Δ
unittests	53.47% <0.00%> (+0.05%)	⬆️

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

Impacted Files	Coverage Δ
pkg/webhook/common.go	65.21% <ø> (ø)
pkg/webhook/mutation.go	25.51% <0.00%> (ø)
pkg/webhook/policy.go	38.73% <0.00%> (ø)
...onstrainttemplate/constrainttemplate_controller.go	57.17% <0.00%> (+1.43%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[jtyr]

Any idea why the Gator test is failing?

[sozercan]

gator test fail is due to #2569

[sozercan]

looks like after this change image.crdRepository becomes unused. do we want to remove that? what would be the best way to migrate to preInstall.image.crdRepository?

[ritazh]

looks like after this change image.crdRepository becomes unused. do we want to remove that? what would be the best way to migrate to preInstall.image.crdRepository?

Is removing image.crdRepository going to break the Upgrade test? how about back compatibility impact? If it breaks upgrade, then maybe we can add a warning to the release note and open an issue to remove it in a future release? WDYT?

[jtyr]

We can use YAML anchor as shown in the last commit to keep it backward compatible and remove it in the future release. Btw. the image.crdRepository isn't documented in the Helm chart README.md file so theoretically nobody should use it.

[ritazh]

the image.crdRepository isn't documented in the Helm chart README.md file so theoretically nobody should use it.

I assume users probably updated this field (along with other image fields) in the values yaml as best practice is to build and replace with your own image in your own registry.

[jtyr]

the image.crdRepository isn't documented in the Helm chart README.md file so theoretically nobody should use it.

I assume users probably updated this field (along with other image fields) in the values yaml as best practice is to build and replace with your own image in your own registry.

Well, that's exactly the reason why I have added the preInstall.crdRepository.image.repository because it wasn't possible to use your own image only for the gatekeeper-update-crds-hook.

[jtyr]

@sozercan Please can you review and approve?

[sozercan]

@jtyr sorry about the delay. I tested this with:

helm template . --set image.repository=foo --set image.crdRepository=bar

looks like this does not set the value, it's kept as openpolicyagent/gatekeeper-crds

is this intended?

[jtyr]

@jtyr sorry about the delay. I tested this with:

helm template . --set image.repository=foo --set image.crdRepository=bar

looks like this does not set the value, it's kept as openpolicyagent/gatekeeper-crds

is this intended?

No, that's not how I thought it will work. So it looks like a breaking change. How do you want to proceed?

[sozercan]

@jtyr since this is a breaking change, should we keep the core change (allowing webhook names to be configurable) but keep crd image config as is?

@ritazh @maxsmythe thoughts?

[ritazh]

should we keep the core change (allowing webhook names to be configurable) but keep crd image config as is?

+1

[maxsmythe]

+1 as well

[jtyr]

In the last commit, I have changed the behavior that the image.release is used by default (the original behavior for CRD image) and only if the preInstall.crdRepository.image.repository is set, the new behavior is in effect (specific image is used for the CRD upgrade hook). What do you think about such solution?

[ritazh]

LGTM