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

Add policy-controller annotations #732

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

elfotografo007
Copy link
Contributor

@elfotografo007 elfotografo007 commented Apr 13, 2023

Summary

Add annotations to a resource with the validation results.
Closes #611

Example Output

Given the following policy, and enabling the policy-controller in the namespace my-secure-namespace:

apiVersion: policy.sigstore.dev/v1beta1
kind: ClusterImagePolicy
metadata:
  name: image-policy
spec:
  images:
  - glob: "gcr.io/projectsigstore/cosign*"
  authorities:
  - name: official-cosign-key
    key:
      data: |
        -----BEGIN PUBLIC KEY-----
        MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEhyQCx0E9wQWSFI9ULGwy3BuRklnt
        IqozONbbdbqz11hlRJy9c7SG+hdcFl9jE9uE/dwtuwU2MqU9T/cN0YkWww==
        -----END PUBLIC KEY-----

Creating a pod with kubectl run cosign --image=gcr.io/projectsigstore/cosign:v1.2.1 --dry-run=server -n my-secure-namespace -o yaml will generate the following annotation:

apiVersion: v1
kind: Pod
metadata:
  annotations:
    policy.sigstore.dev/policy-controller-results: '{"containerResults":[{"index":0,"name":"cosign","image":"gcr.io/projectsigstore/cosign@sha256:68801416e6ae0a48820baa3f071146d18846d8cd26ca8ec3a1e87fca8a735498","field":"containers","result":"allow","resultMsg":"Validated 1 policies for image gcr.io/projectsigstore/cosign@sha256:68801416e6ae0a48820baa3f071146d18846d8cd26ca8ec3a1e87fca8a735498","policyResults":{"image-policy":{"authorityMatches":{"official-cosign-key":{"signatures":[{"id":"fe11e4e3fc2d0b19518341289fabcb3d36102308bf8b5d773a2eefd1251df086"},{"id":"9da9bce3022befe4f55e9dea0680a2ae48141570c98df6c541c557fa533fe2f1"}]}}}}}]}'
  creationTimestamp: "2023-04-13T16:56:25Z"
  labels:
    run: cosign
  name: cosign
  namespace: my-secure-namespace
...

The annotations are in JSON format.

Release Note

Added annotations to validated resources.

Documentation

Docs PR

Signed-off-by: Andrés Torres <andrest@vmware.com>
@codecov
Copy link

codecov bot commented Apr 13, 2023

Codecov Report

Merging #732 (93ed0c1) into main (4b62311) will increase coverage by 0.57%.
The diff coverage is 65.26%.

@@            Coverage Diff             @@
##             main     #732      +/-   ##
==========================================
+ Coverage   55.37%   55.94%   +0.57%     
==========================================
  Files          45       45              
  Lines        4791     5069     +278     
==========================================
+ Hits         2653     2836     +183     
- Misses       1934     2018      +84     
- Partials      204      215      +11     
Impacted Files Coverage Δ
cmd/webhook/main.go 0.00% <0.00%> (ø)
pkg/webhook/validator.go 63.48% <66.41%> (+0.68%) ⬆️
pkg/config/store.go 100.00% <100.00%> (ø)

Signed-off-by: Andrés Torres <andrest@vmware.com>
Signed-off-by: Andrés Torres <andrest@vmware.com>
@elfotografo007 elfotografo007 marked this pull request as ready for review April 13, 2023 20:23
Signed-off-by: Andrés Torres <andrest@vmware.com>
Copy link
Collaborator

@hectorj2f hectorj2f left a comment

Choose a reason for hiding this comment

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

We need to add a new github action worklflow to test this functionality on a running cluster similar to what we did with kind-cluster-image-policy-tsa.yaml or kind-cluster-image-policy-no-tuf.

pkg/config/store.go Outdated Show resolved Hide resolved
pkg/config/store.go Outdated Show resolved Hide resolved
cmd/webhook/main.go Show resolved Hide resolved
ImagePullSecrets: imagePullSecrets,
}

v.annotatePodSpec(ctx, ns, p.Kind, p.APIVersion, &p.ObjectMeta, &p.Spec, opt)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could get from the context whether annotate-results- configuration is enabled or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I am not sure what you are referring to. I make the check from the context in one place in line 1206.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh! This comment was related to this other one #732 (comment). I believe we could simply get from the context whether our system had enabled the annotations on resources or not.That'd reduce the amount of code for this feature while keeping the behaviour configurable.

@hectorj2f
Copy link
Collaborator

@elfotografo007 Let me know if you have any questions or need help.

elfotografo007 and others added 7 commits May 10, 2023 09:40
Signed-off-by: Andrés Torres <andrest@vmware.com>
Signed-off-by: Andrés Torres <andrest@vmware.com>
Signed-off-by: Andrés Torres <andrest@vmware.com>
Signed-off-by: Andrés Torres <andrest@vmware.com>
Signed-off-by: Andrés Torres <andrest@vmware.com>
Signed-off-by: Andrés Torres <andrest@vmware.com>
@elfotografo007
Copy link
Contributor Author

@hectorj2f Can you re-run the tests? I believe they failed due to a GH outage.

@hectorj2f
Copy link
Collaborator

@elfotografo007 Done

Signed-off-by: Andrés Torres <andrest@vmware.com>
@elfotografo007
Copy link
Contributor Author

Thanks, @hectorj2f Everything is passing now 😃

@vaikas
Copy link
Collaborator

vaikas commented May 24, 2023

Just to make sure I understand, does this mean that we run through the policy evaluations twice if the annotations are enabled? Once during defaulting (and that's when the annotation gets applied), and again once all the defaultings are done and the actual admission check happens we run through the same evaluations again?
If that's the case it creates 2x the load on registries, but more importantly there is a chance that what gets written to the annotation is not what gets evaluated.
Just wanted to check my understanding.

@elfotografo007
Copy link
Contributor Author

You are totally right, everything you are mentioning is what is happening. Is there a way to evaluate and create annotations at the same time?

@vaikas
Copy link
Collaborator

vaikas commented May 30, 2023

You are totally right, everything you are mentioning is what is happening. Is there a way to evaluate and create annotations at the same time?

I don't believe so in a sense that you want to do the admission / create annotations at the same time. I thought there was a way to return more information along the admission webhook response. It really depends on what behaviour we want to provide.

One option that I was thinking was using the 'AuditAnnotations' from here:
https://pkg.go.dev/k8s.io/kubernetes/pkg/apis/admission#AdmissionResponse

@evenh
Copy link

evenh commented Jul 3, 2023

Looking forward to this feature 🤞🏻

@hectorj2f
Copy link
Collaborator

@elfotografo007 Do you have any update about this one ? Are you still working on these changes ? :)

@ElementTech
Copy link

@elfotografo007 @hectorj2f Did someone look at this part of the admission controller documentation? Where you could do a JSON Patch to an admitted object. I really need this feature so I tried looking in the code where a response is returned but didn't manage to understand.

Basically Base64 encoding this [{"op": "add", "path": "/spec/replicas", "value": 3}] for example:

{
  "apiVersion": "admission.k8s.io/v1",
  "kind": "AdmissionReview",
  "response": {
    "uid": "<value from request.uid>",
    "allowed": true,
    "patchType": "JSONPatch",
    "patch": "W3sib3AiOiAiYWRkIiwgInBhdGgiOiAiL3NwZWMvcmVwbGljYXMiLCAidmFsdWUiOiAzfV0="
  }
}

@hectorj2f
Copy link
Collaborator

@ElementTech no, we didn't. How do you want to use it?

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.

Add annotations to objects admitted by policy-controller
5 participants