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 fields in policy CRD #1540

Merged
merged 3 commits into from
Mar 3, 2022
Merged

Conversation

kkavitha
Copy link
Contributor

@kkavitha kkavitha commented Mar 2, 2022

Signed-off-by: Kavitha Krishnan <krishnanka@vmware.com>
Copy link
Contributor

@vaikas vaikas left a comment

Choose a reason for hiding this comment

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

Awesomesauce! Few comments about the datastructures, thanks for splitting out only the API changes.


// This references a public verification key stored in
// a secret in the cosign-system namespace.
type KeyRef struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding a comment stating that exactly one must be specified.

Choose a reason for hiding this comment

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

+1

pkg/apis/cosigned/v1alpha1/clusterimagepolicy_types.go Outdated Show resolved Hide resolved
pkg/apis/cosigned/v1alpha1/clusterimagepolicy_types.go Outdated Show resolved Hide resolved
// against which to verify. KeylessRef will contain either the URL to the verifying
// certificate, or it will contain the certificate data inline or in a secret.
type KeylessRef struct {
URL string `json:"url"`
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto here for the URL
URL *apis.URL json:"url,omitempty"

pkg/apis/cosigned/v1alpha1/clusterimagepolicy_types.go Outdated Show resolved Hide resolved
pkg/apis/cosigned/v1alpha1/clusterimagepolicy_types.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #1540 (8e41f74) into main (712c493) will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1540      +/-   ##
==========================================
- Coverage   26.49%   26.44%   -0.05%     
==========================================
  Files         126      126              
  Lines        7214     7214              
==========================================
- Hits         1911     1908       -3     
- Misses       5093     5095       +2     
- Partials      210      211       +1     
Impacted Files Coverage Δ
pkg/cosign/tuf/client.go 62.34% <0.00%> (-0.95%) ⬇️

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 712c493...8e41f74. Read the comment docs.

Signed-off-by: Kavitha Krishnan <krishnanka@vmware.com>
// A KeyRef must specify only one of SecretRef, Data or KMS
type KeyRef struct {
// +optional
SecretRef *v1.SecretReference `json:"secretref,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This does allow for the namespace to be specified. I personally like this flexibility, and we can validate if a particular instance does not allow for per-namespace secrets. Just jotting it down, because in the api working group we had talked about the problem of having to manage secrets in many namespaces and decided that the way around that would be restrict that they live in a single namespace to reduce management headaches. This allows us to do both 👍

Copy link
Contributor

@vaikas vaikas left a comment

Choose a reason for hiding this comment

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

Woot! Couple of nits. Thanks for the quick turnaround!

pkg/apis/cosigned/v1alpha1/clusterimagepolicy_types.go Outdated Show resolved Hide resolved
pkg/apis/cosigned/v1alpha1/clusterimagepolicy_types.go Outdated Show resolved Hide resolved
pkg/apis/cosigned/v1alpha1/clusterimagepolicy_types.go Outdated Show resolved Hide resolved
@vaikas
Copy link
Contributor

vaikas commented Mar 2, 2022

@kkavitha looks like there's a few style issues also, just a heads up.

Signed-off-by: Kavitha Krishnan <krishnanka@vmware.com>
Copy link
Contributor

@vaikas vaikas left a comment

Choose a reason for hiding this comment

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

Thank you so much for getting this done. Let's get it in so we can start sharding the remaining work. We can always change things if during implementation we find some oopses here.

@vaikas vaikas merged commit 9b5f432 into sigstore:main Mar 3, 2022
@github-actions github-actions bot added this to the v1.6.0 milestone Mar 3, 2022
@hectorj2f
Copy link
Contributor

❤️ Awesome work!

mlieberman85 pushed a commit to mlieberman85/cosign that referenced this pull request May 6, 2022
* Add fields in policy CRD

Signed-off-by: Kavitha Krishnan <krishnanka@vmware.com>

* [1417] Update policy type

Signed-off-by: Kavitha Krishnan <krishnanka@vmware.com>

* Fix lint errors;incorporate PR feedback

Signed-off-by: Kavitha Krishnan <krishnanka@vmware.com>
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.

feature: create image policy types and CRDs
5 participants