Fix replicalimits ConstraintTemplate to handle scaling to zero properly. Fixes #419

[skaven81]

What this PR does / why we need it: The replicalimits ConstraintTemplate has two bugs that are fixed in this PR. First, the input_replica_limit() function contained a reference to global input.review.object.spec instead of using the local spec argument. Second, when kubectl scale <resource> --replicas=0 is issued, a Scale resource is created with an empty spec: { }, not spec: { replicas: 0 }. Using object.get() with a default value of 0 ensures that the ConstraintTemplate works properly in this condition.,

Which issue(s) does this PR fix:
Fixes #419

[skaven81]

I'm going to need some help here. I thought that I followed the instructions properly for modifying the source code. But I found that make generate-all doesn't seem to do anything at all with the src_test.rego, so I manually inserted my updated tests into the library tree. I suspect that is what is causing the CI tests to fail. But as I can't seem to get the tests to build at all using make generate-all, how do I properly validate this in my own working copy?

[apeabody]

I'm going to need some help here. I thought that I followed the instructions properly for modifying the source code. But I found that make generate-all doesn't seem to do anything at all with the src_test.rego, so I manually inserted my updated tests into the library tree. I suspect that is what is causing the CI tests to fail. But as I can't seem to get the tests to build at all using make generate-all, how do I properly validate this in my own working copy?

Hi @skaven81 the separate ./test.sh script should run all tests, however from the CI it initially appears the website documentation updates didn't occur for some reason, which should be included in make generate-all.

[apeabody]

I'm going to need some help here. I thought that I followed the instructions properly for modifying the source code. But I found that make generate-all doesn't seem to do anything at all with the src_test.rego, so I manually inserted my updated tests into the library tree. I suspect that is what is causing the CI tests to fail. But as I can't seem to get the tests to build at all using make generate-all, how do I properly validate this in my own working copy?

Hi @skaven81 the separate ./test.sh script should run all tests, however from the CI it initially appears the website documentation updates didn't occur for some reason, which should be included in make generate-all.

diff --git a/website/docs/validation/replicalimits.md b/website/docs/validation/replicalimits.md
index 4f458dd..343e990 100644
--- a/website/docs/validation/replicalimits.md
+++ b/website/docs/validation/replicalimits.md
@@ -92,6 +92,8 @@ spec:
     kinds:
       - apiGroups: ["apps"]
         kinds: ["Deployment"]
+      - apiGroups: ["autoscaling"]
+        kinds: ["Scale"]
   parameters:
     ranges:
     - min_replicas: 3
@@ -139,6 +141,26 @@ Usage
 kubectl apply -f https://raw.githubusercontent.com/open-policy-agent/gatekeeper-library/master/library/general/replicalimits/samples/replicalimits/example_allowed.yaml

+
+

+

example-scale-allowed

+
+```yaml
+apiVersion: autoscaling/v1
+kind: Scale
+metadata:

• name: allowed-deployment
  +spec:
• replicas: 3

+ + +Usage + +shell
+kubectl apply -f https://raw.githubusercontent.com/open-policy-agent/gatekeeper-library/master/library/general/replicalimits/samples/replicalimits/example_scale_allowed.yaml
+```
+

example-disallowed

@@ -172,6 +194,168 @@ Usage kubectl apply -f https://raw.githubusercontent.com/open-policy-agent/gatekeeper-library/master/library/general/replicalimits/samples/replicalimits/example_disallowed.yaml ```

+

+

+

example-scale-disallowed

+
+```yaml
+apiVersion: autoscaling/v1
+kind: Scale
+metadata:

• name: allowed-deployment
  +spec:
• replicas: 100

+ + +Usage + +shell
+kubectl apply -f https://raw.githubusercontent.com/open-policy-agent/gatekeeper-library/master/library/general/replicalimits/samples/replicalimits/example_scale_disallowed.yaml
+ + +</details> + + +</blockquote></details><details> +<summary>replica-limit-zero</summary><blockquote> + +<details> +<summary>constraint</summary> + +yaml
+apiVersion: constraints.gatekeeper.sh/v1beta1
+kind: K8sReplicaLimits
+metadata:

• name: replica-limits
  +spec:
• match:
• kinds:

•  - apiGroups: ["apps"]
  

•    kinds: ["Deployment"]
  

•  - apiGroups: ["autoscaling"]
  

•    kinds: ["Scale"]
  

• parameters:
• ranges:
• • min_replicas: 0

•  max_replicas: 50
  

+ + +Usage + +shell
+kubectl apply -f https://raw.githubusercontent.com/open-policy-agent/gatekeeper-library/master/library/general/replicalimits/samples/replicalimits_zero/constraint.yaml
+ + +</details> + +<details> +<summary>example-allowed</summary> + +yaml
+apiVersion: apps/v1
+kind: Deployment
+metadata:

• name: allowed-deployment
  +spec:
• selector:
• matchLabels:

•  app: nginx
  

• replicas: 0
• template:
• metadata:

•  labels:
  

•    app: nginx
  

• spec:

•  containers:
  

•  - name: nginx
  

•    image: nginx:1.14.2
  

•    ports:
  

•    - containerPort: 80
  

+ + +Usage + +shell
+kubectl apply -f https://raw.githubusercontent.com/open-policy-agent/gatekeeper-library/master/library/general/replicalimits/samples/replicalimits_zero/example_allowed.yaml
+ + +</details> +<details> +<summary>example-scale-allowed</summary> + +yaml
+apiVersion: autoscaling/v1
+kind: Scale
+metadata:

• name: allowed-deployment
  +# kubectl scale deploy --replicas=0 creates a Scale
  +# resource with an empty spec, not replicas:0
  +spec: {}

+ + +Usage + +shell
+kubectl apply -f https://raw.githubusercontent.com/open-policy-agent/gatekeeper-library/master/library/general/replicalimits/samples/replicalimits_zero/example_scale_allowed.yaml
+ + +</details> +<details> +<summary>example-disallowed</summary> + +yaml
+apiVersion: apps/v1
+kind: Deployment
+metadata:

• name: disallowed-deployment
  +spec:
• selector:
• matchLabels:

•  app: nginx
  

• replicas: 100
• template:
• metadata:

•  labels:
  

•    app: nginx
  

• spec:

•  containers:
  

•  - name: nginx
  

•    image: nginx:1.14.2
  

•    ports:
  

•    - containerPort: 80
  

+ + +Usage + +shell
+kubectl apply -f https://raw.githubusercontent.com/open-policy-agent/gatekeeper-library/master/library/general/replicalimits/samples/replicalimits_zero/example_disallowed.yaml
+ + +</details> +<details> +<summary>example-scale-disallowed</summary> + +yaml
+apiVersion: autoscaling/v1
+kind: Scale
+metadata:

• name: allowed-deployment
  +spec:
• replicas: 100

+ + +Usage + +shell
+kubectl apply -f https://raw.githubusercontent.com/open-policy-agent/gatekeeper-library/master/library/general/replicalimits/samples/replicalimits_zero/example_scale_disallowed.yaml
+```
+

Please run 'make generate generate-website-docs generate-artifacthub-artifacts' to generate the templates and docs

[skaven81]

When I run make generate-artifacthub-artifacts (which is part of make generate-all) it creates a new directory with hundreds of files in it, that weren't in the original repository. It didn't seem right to add that to the commit (I'm pretty sure that's part of what I did wrong in my prior PR).

The website docs did get regenerated, and they're in my commit: 33b39e1#diff-578c418f3682058d7a372ca0fd2d3dafb3cd57c31df54872bbc81890ad63b261

There's something missing from the contribution process that regular contributors must be doing without thinking about it, but isn't explicit in the documentation.

Here's what I did:

1. Edit src/general/replicalimits/ files, updating version from 1.0.1 to 1.1.0, applying the constraint template code update, and tweaking the test generation Rego to account for the expected test behavior.
2. Run make generate-all - this generated updates to library/general/replicalimits/template.yaml and website/docs/validation/replicalimits.md, but did nothing to the tests under library/general/replicalimits/.
3. Add my own updated tests under library/general/replicalimits/ since the build process didn't update them.
4. Commit and push, open PR.

Step 3 was almost certainly wrong. But the documentation (https://github.com/open-policy-agent/gatekeeper-library#development) makes no mention of how to build the tests, and the Makefile doesn't seem to have anything in there that touches src_test.rego files. Also the documentation isn't explicitly for "you need to do these things, in this order, to contribute to this project" ... it's just a generalized list of tools available while developing in the repo. The documentation above (https://github.com/open-policy-agent/gatekeeper-library#new-policy) is for adding a new policy ... and that does mention src_test.rego but also makes no mention of what commands need to be run to prepare the repo for a PR.

[apeabody]

When I run make generate-artifacthub-artifacts (which is part of make generate-all) it creates a new directory with hundreds of files in it, that weren't in the original repository. It didn't seem right to add that to the commit (I'm pretty sure that's part of what I did wrong in my prior PR).

The website docs did get regenerated, and they're in my commit: 33b39e1#diff-578c418f3682058d7a372ca0fd2d3dafb3cd57c31df54872bbc81890ad63b261

There's something missing from the contribution process that regular contributors must be doing without thinking about it, but isn't explicit in the documentation.

Here's what I did:

1. Edit src/general/replicalimits/ files, updating version from 1.0.1 to 1.1.0, applying the constraint template code update, and tweaking the test generation Rego to account for the expected test behavior.
2. Run make generate-all - this generated updates to library/general/replicalimits/template.yaml and website/docs/validation/replicalimits.md, but did nothing to the tests under library/general/replicalimits/.
3. Add my own updated tests under library/general/replicalimits/ since the build process didn't update them.
4. Commit and push, open PR.

Step 3 was almost certainly wrong. But the documentation (https://github.com/open-policy-agent/gatekeeper-library#development) makes no mention of how to build the tests, and the Makefile doesn't seem to have anything in there that touches src_test.rego files. Also the documentation isn't explicitly for "you need to do these things, in this order, to contribute to this project" ... it's just a generalized list of tools available while developing in the repo. The documentation above (https://github.com/open-policy-agent/gatekeeper-library#new-policy) is for adding a new policy ... and that does mention src_test.rego but also makes no mention of what commands need to be run to prepare the repo for a PR.

Hi @skaven81 - Sorry any confusion, the make generate-all step should be run after all changes (swap steps 2 and 3), as the tests are also used to build the template documentation. Unlike the template generation, the Rego tests under src/general/replicalimits/ do not build the KRM tests under library/general/replicalimits/, unfortunatly they need to be added independently.

I did a quick test run of make generate-all on your branch and saw 14 new files added to artifacthub/library/general/replicalimits/1.1.0 and several updates to website/docs/validation/replicalimits.md - both expected. If you are still seeing something different let me know, all the other CI tests (e.g. those which exercise the tests, etc) currently look good.

[skaven81]

OK I re-ran make generate-all and added the additional changes to the website and artifacthub bits. Let's see if the CI tests pass this time 🤞

[apeabody]

Thanks for the contribution @skaven81!

[apeabody]

nit: It might be nice to support a test where spec.replicas is specifically set to 0?

[skaven81]

The rule just below with replicas == 0 is what tests that, by blanking out spec (that's what the whole PR is about). I agree that having an additional test that explicitly checks spec.replicas=0 would be good, but it would require a rather large refactoring of the source-to-tests logic to allow for review(replicas) to return two different objects when replicas=0. In fact, that very idea (a Rego function that returns multiple values for the same input) is strictly forbidden by Rego. So the whole thing would need to be reworked from scratch.

[skaven81]

That said, I see no evidence that src_test.rego is even being read by anything -- it certainly doesn't seem to be generating the tests under library/general/replicalimits, so I suppose I could add such a test manually and it would likely still pass all the CI tests.

[apeabody]

Hi @skaven81 - The Rego tests under src/general/replicalimits/ do not build the KRM tests under library/general/replicalimits/, however they are run by theCI / Unit test on {} using the opa test command for each folder under src/ - this is basically the same as running ./test.sh locally.

Needs a fresh make generate-all - Thanks!

[skaven81]

I don't know why we keep dancing back and forth on this. Every time I fix my branch so that the tests all complete successfully, somebody comes along and does a merge from master which reverts some of my changes, and the tests start failing again.

Somebody please just spend 10 minutes to manually review the actual PR and merge it in, instead of depending on the automation alone.

[ritazh]

@skaven81 Thank you for the PR and apologies for the back and forth. The merge from main didnt show any conflicts. If you see any, can you please help update the conflicts? We will review this today.

Error from CI:

looks like template.yaml is updated but the version is not. Please update the 'metadata.gatekeeper.sh/version' annotation in the template.yaml source

Looks like a previously merged PR updated the template version https://github.com/open-policy-agent/gatekeeper-library/pull/429/files#diff-c3f23f7cfeabf9bf68a4be9f0d84eb695dcaf8b31f3cdcbdaaa6aa269fcbb823L7. Can you please bump it again? Thank you!

[ritazh]

@apeabody other than the template version, LGTY?

[apeabody]

@apeabody other than the template version, LGTY?

Thanks @ritazh, agreed!

Subsequent to bumping the template version will need a make generate-all. I also believe the stale changes under artifacthub/library/general/replicalimits/1.0.2/ will need to be manually reverted.

[stale]

This issue/PR has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.