1417 policy validations

[kkavitha]

Summary

Create a validation webhook for policy CRD and their tests
Fixes: #1417

cc: @hectorj2f @mattmoor @vaikas

[codecov-commenter]

Codecov Report

Merging #1548 (524dfc4) into main (4b2c3c0) will increase coverage by 0.63%.
The diff coverage is 51.32%.

@@            Coverage Diff             @@
##             main    #1548      +/-   ##
==========================================
+ Coverage   26.49%   27.12%   +0.63%     
==========================================
  Files         126      136      +10     
  Lines        7214     7686     +472     
==========================================
+ Hits         1911     2085     +174     
- Misses       5093     5374     +281     
- Partials      210      227      +17     

Impacted Files	Coverage Δ
cmd/cosign/policy_webhook/main.go	0.00% <0.00%> (ø)
cmd/cosign/webhook/main.go	0.00% <ø> (ø)
...cosigned/v1alpha1/clusterimagepolicy_validation.go	82.85% <82.85%> (ø)
pkg/cosign/kubernetes/webhook/validation.go	74.02% <0.00%> (-7.80%)	⬇️
...s/cosigned/v1alpha1/clusterimagepolicy_defaults.go	0.00% <0.00%> (ø)
...kg/apis/cosigned/v1alpha1/zz_generated.deepcopy.go	0.00% <0.00%> (ø)
pkg/reconciler/clusterimagepolicy/controller.go	81.57% <0.00%> (ø)
pkg/apis/config/image_policies.go	71.42% <0.00%> (ø)
pkg/apis/config/store.go	100.00% <0.00%> (ø)
... and 3 more

---

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 4b2c3c0...524dfc4. Read the comment docs.

[vaikas]

I'm just curious why the test run needed to be approved since it says first time contributor, but we got the other PR merged yesterday, so I don't understand why it blocked you again? 🤔

[kkavitha]

It didn't need approval to run when I was the author. Looks like Adam has to contribute a PR under his name to get the permission. The merged PR was under my name.

[vaikas]

nit: I don't think we need this, or L34 below.

[hectorj2f]

you're right, we don't need it.

[kkavitha]

Cool, will remove it!

[vaikas]

I wonder if we might want to create something like:
./test/testadata/success
./test/testdata/failure

directories and then create files there with names like:
./test/testdata/failure/both-glob-and-regex.yaml
./test/testdata/failure/key-with-multiple-properties.yaml

And then the both-glob-and-regex.yaml would look like:

# Invalid policy: both glob and regex
apiVersion: cosigned.sigstore.dev/v1alpha1
kind: ClusterImagePolicy
metadata:
  name: image-policy
spec:
  images:
  - glob: image**
    regex: image.*
    authorities:
    - key:
        data: "---somedata---"

Then we could have a for loop for both directories and for failures they should fail, and for successes they
should be created. For success case, then we could also delete after created, so that we don't have to deal
with conflicting names.

Just thinking that having those files might make it easier then to add new tests in the future as well as have some example configurations that could also serve as documentation. They could also be used for testing the validations for the fields. Just a thought.

[hectorj2f]

If we go with the proposed filenames under testdata, I'd suggest to add a cosigned prefix to know where they are been used.

[kkavitha]

That makes sense. I will push them to new files.

[hectorj2f]

@kkavitha I checked the code, the e2e_test_cosigned.sh are failing to reject invalid pods because the cluster image policy has not been installed. We'll need to install so the tests are working again,

[vaikas]

Awesomesauce! So, if we don't hand code these files, but use a for loop here:

for i in `ls ./test/testdata/cosigned/invalid/`
do
  if kubectl create -f ./test/testdata/cosigned/invalid/$i ; then
  then
    echo $i failed when it should not have
    exit 1
  else $i rejected as expected
done

Or something like that. Then if new files are added to .../invalid/ directory, then we don't have to modify this file at all.

What do you think?

[kkavitha]

Yeah that sounds good. Will make the change!

[mattmoor]

I think this is missing its ::endgroup::

[hectorj2f]

Yes

[hectorj2f]

@kkavitha In addition to #1548 (comment), I saw the ValidationWebhookConfiguration is not set for the validating.clusterimagepolicy.sigstore.dev the rules are not defined, so the webhook won't be called for the ClusterImagePolicy.

[vaikas]

Thanks!

[vaikas]

I created this:
#1581
to track tighter validations.
But let's get this in, it's a great starting point!