1417 policy webhooks rev2 #3
Conversation
| rules: | ||
| - apiGroups: | ||
| - sigstore.dev | ||
| apiVersions: | ||
| - v1alpha1 | ||
| operations: | ||
| - CREATE | ||
| - UPDATE | ||
| resources: | ||
| - clusterimagepolicies |
There was a problem hiding this comment.
This will be automatically populated by the webhook when it adds the caBundle
There was a problem hiding this comment.
Oh cool, we didnt realize that knative was modifying the webhook configurations. Made the changes now!
| rules: | ||
| - apiGroups: | ||
| - sigstore.dev | ||
| apiVersions: | ||
| - v1alpha1 | ||
| operations: | ||
| - CREATE | ||
| - UPDATE | ||
| resources: | ||
| - clusterimagepolicies |
| status: | ||
| acceptedNames: | ||
| kind: "" | ||
| plural: "" | ||
| conditions: [] | ||
| storedVersions: [] |
There was a problem hiding this comment.
Can we avoid checking in status with crd-gen?
There was a problem hiding this comment.
sure! we will remove this.
| @@ -42,7 +42,7 @@ spec: | |||
| - name: webhook | |||
| # This is the Go import path for the binary that is containerized | |||
| # and substituted here. | |||
| image: ko://github.com/sigstore/cosign/cmd/cosign/webhook | |||
| image: ko.local/webhook:70c8e156ba665ade1745f695dad6052f7f265e91c28aaaab6710ece2d5274224 | |||
There was a problem hiding this comment.
yeah we are creating a new branch with merge commits and these changes removed
| // +kubebuilder:object:root=true | ||
| // +kubebuilder:resource:path=clusterimagepolicies,scope=Cluster | ||
| // +kubebuilder:storageversion |
There was a problem hiding this comment.
I think we're pretty quickly get tired of juggling both of these, and I know when Knative Eventing briefly used controller-runtime they ended up having to maintain their own fork at times due to dependency hell.
There was a problem hiding this comment.
Sorry, I dont understand. Are you saying we should not use kubebuilder?
There was a problem hiding this comment.
I'm saying that if we use it, it shouldn't be "load bearing" and we should be prepared to rip it out if we need to (e.g. due to dep hell).
You can get basically every piece of codegen we need from K8s/Knative directly.
There was a problem hiding this comment.
@mattmoor Do you think we could use as example this https://github.com/knative-sandbox/sample-controller/blob/main/hack/update-codegen.sh#L34 and https://github.com/knative-sandbox/sample-controller/blob/main/pkg/apis/samples/v1alpha1/simple_deployment_types.go ? I think it does the same as the old kube-builder/controller-gen.
There was a problem hiding this comment.
We don't generate the whole CRD, which is mostly boilerplate. We have tooling to generate the openapiv3 schemas here: https://github.com/knative-sandbox/sample-controller/blob/6a2c2d197b4f37e6020ea19639a1077b7f6e50ab/cmd/schema/main.go#L30-L31
I can take care of adding this stuff (and the webhooks) if you want to focus on filling out the schema/defaulting/validation?
There was a problem hiding this comment.
There was a problem hiding this comment.
oh it is okay, we can add it. We were looking to see examples of how it is generated, thats all.
There was a problem hiding this comment.
Most of the boilerplate is, like additionalPrinterColumns, but this is the output from the tool:
https://github.com/knative-sandbox/sample-controller/blob/6a2c2d197b4f37e6020ea19639a1077b7f6e50ab/config/300-simpledeployment.yaml#L32-L86
There was a problem hiding this comment.
We can probably use yq to get this added to ./hack/update-codegen.sh
| func NewPolicyMutatingAdmissionController(ctx context.Context, cmw configmap.Watcher) *controller.Impl { | ||
| return defaulting.NewAdmissionController( | ||
| ctx, | ||
| "defaulting.clusterimagepolicy.sigstore.dev", |
There was a problem hiding this comment.
Suggestion: I'm wondering if we want to add cosigned.sigstore.dev here. I don't have strong feelings.
There was a problem hiding this comment.
| } | ||
|
|
||
| type SecretRef struct { | ||
| Name string `json:"name,omitempty"` |
There was a problem hiding this comment.
I suggest you use instead *v1.SecretReference from ``k8s.io/api/core/v1` instead of creating a new type.
| ) | ||
|
|
||
| type ClusterImagePolicySpec struct { | ||
| Images []ImagePattern `json:"images,omitempty"` |
There was a problem hiding this comment.
I don't think the Images field should be omitted if empty.
There was a problem hiding this comment.
We've dropped omitempty for now.
| } | ||
|
|
||
| type ImagePattern struct { | ||
| Pattern string `json:"pattern,omitempty"` |
| OCI string `json:"oci"` | ||
| } | ||
|
|
||
| type TLog struct { |
There was a problem hiding this comment.
suggestion: it'd be great to have some golang documentation to explain the purpose of each field. That would generate some valuable documentation.
There was a problem hiding this comment.
cool, sounds good. we will add it.
1413194 to
0850842
Compare
This PR will be submitted upstream to address cosign issue sigstore#1417, creating the basic types and CRD for ClusterImagePolicy as proposed in https://docs.google.com/document/d/1gBLEOOHWOmvHVsoJbgGU74GdwA6CGxMRp3MAeEB50l4/edit#