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.

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

feat: Support setting custom rules in validating/mutatingwebhookconfigurations #1806

Merged
merged 2 commits into from Jan 25, 2022

Conversation

mac-chaffee
Copy link
Contributor

@mac-chaffee mac-chaffee commented Jan 24, 2022

What this PR does / why we need it:

This PR allows users of the helm chart to override the default rules that control which API resources trigger the webhook.
I think this is a valid use-case for gatekeeper to support because:

  1. Every single constraint (EDIT: except required labels) in gatekeeper-library can be enforced by just checking Pods, Services, Ingresses, Deployments, ClusterRoles, and Namespaces. I predict there are lots of APIs that people have no need to enforce constraints on.
  2. Requiring the webhook for all APIs when 90% don't have constraints impacts performance of the cluster unnecessarily (slows down helm installs, and slows down CLI tools that perform SelfSubjectAccessReviews)
  3. And requiring the kube-apiserver to invoke the webhook for every request can lead to issues like this where gatekeeper is causing kube-scheduler/kube-apiserver to be unable to obtain their Leases for leaderelection.

Which issue(s) this PR fixes
Fixes #299

Special notes for your reviewer:

To verify backwards compatibility, I confirmed this change does not alter the helm chart at all by default:

# checkout this branch
$ helm template manifest_staging/charts/gatekeeper > /tmp/after.yaml
$ git co HEAD~
$ helm template manifest_staging/charts/gatekeeper> /tmp/before.yaml
$ diff --ignore-space-change /tmp/before.yaml /tmp/after.yaml
$

Also: I bumped the chart version since other repos tend to do that, but not sure what the procedure is here.

charts/gatekeeper/Chart.yaml Outdated Show resolved Hide resolved
…gurations

Signed-off-by: Mac Chaffee <machaffe@renci.org>
Copy link
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM

@maxsmythe
Copy link
Contributor

To be clear, I have no objections to making this field customizable, however I do take exception to the reasons as-worded. Listed below:

(1) is not accurate... required labels can be enforced against any resource

(2) is a matter of preference. The downside is that it couples enforcement of policies to how Gatekeeper is deployed. This means either:

A) There is a risk of under-enforcement due to a new policy being added but the VWH config not being updated

B) Increased scope of enforcement causing cluster instability due to a sudden increase in requests being sent from the API server to G8r. Either the API server or G8r may not be sized to handle the increased load. This stability vs. cost argument will have different weights depending on the user.

  1. IMO this is a design flaw of K8s... user-space policies should not be able to break the cluster. See this bug comment for context. K8s and GK have some ways of exempting resources. In theory K8s label/namespace selectors should be sufficient, but labels are hard to lock down. Exempting by kind can be reasonable, but may not be sufficient (Config Maps can be used for leader election, but also may make sense as a constraint target, for example).

For me, I think there is no one-size-fits-all solution here, so customizability (with conservative defaults) makes sense.

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Mac Chaffee <machaffe@renci.org>
Copy link
Member

@ritazh ritazh left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @mac-chaffee!
LGTM

@sozercan sozercan merged commit 16254ee into open-policy-agent:master Jan 25, 2022
@frumpleswift
Copy link

When will these changes be reflected in the published helm chart? v3.7.1 does not contain the validatingWebhookCustomRules logic.

@mac-chaffee
Copy link
Contributor Author

Looks like 3.7.1 was a bugfix release, from the existing release-3.7 branch. This will likely land in 3.8.0 when they cut a new release-3.8 branch off of the master branch.

@frumpleswift
Copy link

Any idea when 3.8 will be released?

@maxsmythe
Copy link
Contributor

Target is the 14th.

@ritazh @sozercan, is that still enough time to review open-policy-agent/frameworks#202 (review) ?

@ritazh
Copy link
Member

ritazh commented Mar 9, 2022

Let's try. But given there are still few other blocking changes remaining, 14th might not be realistic. Given that this PR only touches helm chart, should we alternatively consider cutting v3.7.2?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deploy gatekeeper validatingwebhookconfiguration to only listen for certain resources?
5 participants