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

1417 policy webhooks rev2 #3

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,6 @@

bin*
dist/

sample.yaml

9 changes: 9 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@ cross:
go build -trimpath -ldflags "$(LDFLAGS)" -o cosign-$(GOOS)-$(GOARCH) ./cmd/cosign; \
shasum -a 256 cosign-$(GOOS)-$(GOARCH) > cosign-$(GOOS)-$(GOARCH).sha256 ))) \

.PHONY: manifests
manifests:
controller-gen object crd:trivialVersions=true,preserveUnknownFields=false paths="./pkg/cosign/kubernetes/apis/..." output:crd:artifacts:config=config

#####################
# lint / test section
#####################
Expand Down Expand Up @@ -163,6 +167,11 @@ ko-local:
--tags $(GIT_VERSION) --tags $(GIT_HASH) --local \
github.com/sigstore/cosign/cmd/cosign

LDFLAGS="$(LDFLAGS)" GIT_HASH=$(GIT_HASH) GIT_VERSION=$(GIT_VERSION) \
KOCACHE=$(KOCACHE_PATH) ko build --base-import-paths --bare \
--tags $(GIT_VERSION) --tags $(GIT_HASH) --local \
github.com/sigstore/cosign/cmd/cosign/webhook

##################
# help
##################
Expand Down
33 changes: 33 additions & 0 deletions cmd/cosign/webhook/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import (
"knative.dev/pkg/webhook/resourcesemantics/validation"
"sigs.k8s.io/release-utils/version"

"github.com/sigstore/cosign/pkg/cosign/kubernetes/apis/v1alpha1"
cwebhook "github.com/sigstore/cosign/pkg/cosign/kubernetes/webhook"
)

Expand Down Expand Up @@ -66,6 +67,8 @@ func main() {
certificates.NewController,
NewValidatingAdmissionController,
NewMutatingAdmissionController,
NewPolicyValidatingAdmissionController,
NewPolicyMutatingAdmissionController,
)
}

Expand Down Expand Up @@ -138,3 +141,33 @@ func NewMutatingAdmissionController(ctx context.Context, cmw configmap.Watcher)
false,
)
}

func NewPolicyValidatingAdmissionController(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
return validation.NewAdmissionController(
ctx,
"validating.clusterimagepolicy.sigstore.dev",
"/validate-sigstore-dev-v1alpha1-clusterimagepolicy",
map[schema.GroupVersionKind]resourcesemantics.GenericCRD{
v1alpha1.SchemeGroupVersion.WithKind("ClusterImagePolicy"): &v1alpha1.ClusterImagePolicy{},
},
func(ctx context.Context) context.Context {
return ctx
},
true,
)
}

func NewPolicyMutatingAdmissionController(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
return defaulting.NewAdmissionController(
ctx,
"defaulting.clusterimagepolicy.sigstore.dev",

Choose a reason for hiding this comment

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

Suggestion: I'm wondering if we want to add cosigned.sigstore.dev here. I don't have strong feelings.

Choose a reason for hiding this comment

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

"/default-sigstore-dev-v1alpha1-clusterimagepolicy",
map[schema.GroupVersionKind]resourcesemantics.GenericCRD{
v1alpha1.SchemeGroupVersion.WithKind("ClusterImagePolicy"): &v1alpha1.ClusterImagePolicy{},
},
func(ctx context.Context) context.Context {
return ctx
},
true,
)
}
9 changes: 4 additions & 5 deletions config/200-clusterrole.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

kind: ClusterRole
apiVersion: rbac.authorization.k8s.io/v1
metadata:
Expand All @@ -20,23 +19,23 @@ rules:
- apiGroups: [""]
resources: ["events"]
verbs: ["create"]

# Allow the reconciliation of exactly our validating and mutating webhooks.
- apiGroups: ["admissionregistration.k8s.io"]
resources: ["validatingwebhookconfigurations", "mutatingwebhookconfigurations"]
verbs: ["list", "watch"]
- apiGroups: ["admissionregistration.k8s.io"]
resources: ["validatingwebhookconfigurations", "mutatingwebhookconfigurations"]
verbs: ["get", "update"]
resourceNames: ["cosigned.sigstore.dev"]

resourceNames:
- "cosigned.sigstore.dev"
- "defaulting.clusterimagepolicy.sigstore.dev"
- "validating.clusterimagepolicy.sigstore.dev"
- apiGroups: [""]
resources: ["namespaces"]
verbs: ["get"]
# The webhook configured the namespace as the OwnerRef on various cluster-scoped resources,
# which requires we can Get the system namespace.
resourceNames: ["cosign-system"]

# This is needed by k8schain to support fetching pull secrets attached to pod specs
# or their service accounts. If pull secrets aren't used, the "secrets" below can
# be safely dropped, but the logic will fetch the service account to check for pull
Expand Down
55 changes: 55 additions & 0 deletions config/501-policy-webhook-configuration.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
creationTimestamp: null
name: defaulting.clusterimagepolicy.sigstore.dev
webhooks:
- admissionReviewVersions:
- v1
clientConfig:
service:
name: webhook
namespace: cosign-system
path: /default-sigstore-dev-v1alpha1-clusterimagepolicy
failurePolicy: Fail
matchPolicy: Equivalent
name: defaulting.clusterimagepolicy.sigstore.dev
rules:
- apiGroups:
- sigstore.dev
apiVersions:
- v1alpha1
operations:
- CREATE
- UPDATE
resources:
- clusterimagepolicies
Comment on lines +17 to +26

Choose a reason for hiding this comment

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

This will be automatically populated by the webhook when it adds the caBundle

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh cool, we didnt realize that knative was modifying the webhook configurations. Made the changes now!

sideEffects: None
---
apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingWebhookConfiguration
metadata:
creationTimestamp: null
name: validating.clusterimagepolicy.sigstore.dev
webhooks:
- admissionReviewVersions:
- v1
clientConfig:
service:
name: webhook
namespace: cosign-system
path: /validate-sigstore-dev-v1alpha1-clusterimagepolicy
failurePolicy: Fail
matchPolicy: Equivalent
name: validating.clusterimagepolicy.sigstore.dev
rules:
- apiGroups:
- sigstore.dev
apiVersions:
- v1alpha1
operations:
- CREATE
- UPDATE
resources:
- clusterimagepolicies
Comment on lines +45 to +54

Choose a reason for hiding this comment

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

same

sideEffects: None
101 changes: 101 additions & 0 deletions config/sigstore.dev_clusterimagepolicies.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@

---
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: v0.4.1
creationTimestamp: null
name: clusterimagepolicies.sigstore.dev
spec:
group: sigstore.dev
names:
kind: ClusterImagePolicy
listKind: ClusterImagePolicyList
plural: clusterimagepolicies
singular: clusterimagepolicy
scope: Cluster
versions:
- name: v1alpha1
schema:
openAPIV3Schema:
properties:
apiVersion:
description: 'APIVersion defines the versioned schema of this representation
of an object. Servers should convert recognized schemas to the latest
internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
type: string
kind:
description: 'Kind is a string value representing the REST resource this
object represents. Servers may infer this from the endpoint the client
submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
type: string
metadata:
type: object
spec:
properties:
images:
items:
properties:
authorities:
items:
properties:
ctlog:
properties:
url:
type: string
type: object
key:
properties:
data:
type: string
kms:
type: string
secretRef:
properties:
name:
type: string
type: object
type: object
keyless:
properties:
ca-key:
properties:
data:
type: string
name:
type: string
type: object
identities:
items:
properties:
issuer:
type: string
subject:
type: string
type: object
type: array
type: object
source:
items:
properties:
oci:
type: string
type: object
type: array
type: object
type: array
pattern:
type: string
type: object
type: array
type: object
type: object
served: true
storage: true
status:
acceptedNames:
kind: ""
plural: ""
conditions: []
storedVersions: []
Comment on lines +96 to +101

Choose a reason for hiding this comment

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

Can we avoid checking in status with crd-gen?

Copy link
Collaborator

Choose a reason for hiding this comment

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

sure! we will remove this.

4 changes: 2 additions & 2 deletions config/webhook.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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

Choose a reason for hiding this comment

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

Revert before going upstream.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah we are creating a new branch with merge commits and these changes removed

args: ["-secret-name=verification-key"]
resources:
requests:
Expand Down Expand Up @@ -103,4 +103,4 @@ metadata:
namespace: cosign-system
# stringData:
# cosign.pub: |
# <PEM encoded public key>
# <PEM encoded public key>
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package v1alpha1

import (
"context"
)

func (ip *ClusterImagePolicy) SetDefaults(ctx context.Context) {
}
70 changes: 70 additions & 0 deletions pkg/cosign/kubernetes/apis/v1alpha1/clusterimagepolicy_types.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// +kubebuilder:validation:Optional
package v1alpha1

import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"knative.dev/pkg/apis"
)

// +kubebuilder:object:root=true
// +kubebuilder:resource:path=clusterimagepolicies,scope=Cluster
// +kubebuilder:storageversion
Comment on lines +9 to +11

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I dont understand. Are you saying we should not use kubebuilder?

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

That’s exactly what I’d use

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh it is okay, we can add it. We were looking to see examples of how it is generated, thats all.

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

We can probably use yq to get this added to ./hack/update-codegen.sh

type ClusterImagePolicy struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`
Spec ClusterImagePolicySpec `json:"spec,omitempty"`
}

var (
_ apis.Validatable = (*ClusterImagePolicy)(nil)
_ apis.Defaultable = (*ClusterImagePolicy)(nil)
)

type ClusterImagePolicySpec struct {
Images []ImagePattern `json:"images,omitempty"`

Choose a reason for hiding this comment

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

I don't think the Images field should be omitted if empty.

Copy link
Owner Author

Choose a reason for hiding this comment

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

We've dropped omitempty for now.

}

type ImagePattern struct {
Pattern string `json:"pattern,omitempty"`

Choose a reason for hiding this comment

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

same as above, json:"pattern:

Authorities []Authority `json:"authorities,anyOf,omitempty"`
}

type Authority struct {
Key KeyRef `json:"key,omitempty"`
Keyless KeylessRef `json:"keyless,omitempty"`
Sources []Source `json:"source,anyOf,omitempty"`
CTLog TLog `json:"ctlog,omitempty"`
}

type KeyRef struct {
SecretRef SecretRef `json:"secretRef,omitempty"`
Data string `json:"data,omitempty"`
KMS string `json:"kms,omitempty"`
}

type SecretRef struct {
Name string `json:"name,omitempty"`

Choose a reason for hiding this comment

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

I suggest you use instead *v1.SecretReference from ``k8s.io/api/core/v1` instead of creating a new type.

}

type Source struct {
OCI string `json:"oci"`
}

type TLog struct {

Choose a reason for hiding this comment

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

suggestion: it'd be great to have some golang documentation to explain the purpose of each field. That would generate some valuable documentation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

cool, sounds good. we will add it.

URL string `json:"url,omitempty"`
}

type KeylessRef struct {
Identities []Identity `json:"identities,anyOf,omitempty"`
CAKey CAKey `json:"ca-key,omitempty"`
}

type Identity struct {
Issuer string `json:"issuer,omitempty"`
Subject string `json:"subject,omitempty"`
}

type CAKey struct {
Name string `json:"name,omitempty"`
Data string `json:"data,omitempty"`
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package v1alpha1

import (
"context"

"knative.dev/pkg/apis"
)

func (ip *ClusterImagePolicy) Validate(ctx context.Context) *apis.FieldError {
return nil
}
4 changes: 4 additions & 0 deletions pkg/cosign/kubernetes/apis/v1alpha1/doc.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// +groupName=sigstore.dev
// +kubebuilder:object:generate=true

package v1alpha1
Loading