-
Notifications
You must be signed in to change notification settings - Fork 54
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
Feature: Create an interface for downstream CIP integrations. #480
Conversation
Staging this mostly for CI for now, I want to knock off the following before this lands:
(I'll keep updating this comment as more things come to mind) |
Codecov Report
@@ Coverage Diff @@
## main #480 +/- ##
==========================================
+ Coverage 54.73% 56.13% +1.40%
==========================================
Files 38 42 +4
Lines 4129 4407 +278
==========================================
+ Hits 2260 2474 +214
- Misses 1688 1737 +49
- Partials 181 196 +15
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
e867625
to
602772e
Compare
🎁 This change factors a new small library `./pkg/policy` which is intended to streamline incorporating CIP validation into downstream tooling. For a (much) more verbose explanation see [here](ko-build/ko#356 (comment)), but the general idea behind this is to allow CIP's to gate consumption of images in other contexts, for example the base images in build tools such as `ko` or `kaniko`. The idea is to enable the tool providers to bake-in default policies for default base images, and optionally expose configuration to let users write policies to authorize base images prior to consumption. For example, I might write the following `.ko.yaml`: ```yaml verification: noMatchPolicy: deny policies: - data: | # inline policy - url: https://github.com/foo/bar/blobs/main/POLICY.yaml ``` With this library, it is likely <100 LoC to add base image policy verification to `ko`, and significantly simplifies our own `policy-tester` which has spaghetti code replicating some of this functionality. /kind feature Signed-off-by: Matt Moore <[email protected]>
602772e
to
214c2dd
Compare
For context, this is motivated to support efforts like ko-build/ko#918 🤞 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet!
switch i.verification.NoMatchPolicy { | ||
case "allow": | ||
return nil | ||
case "warn": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a library, I'm wondering if it would be better to wrap this as a warning "error type' (like it's done by the functions that we call below) and return it instead of having a separate function that one has to implement to see the warnings. Reasoning being that it would be easier for the implementor to control how to handle warnings and act on them. Looking at the warningwriter to see errors does not seem that friendly. Here and below. I guess I'm thinking that it would be useful to act in some cases on the fact that it's a warning, and we have the information already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally because most Go code treats normal return values and errors as mutually exclusive, so carrying warnings as wrapped errors when also continuing with normal execution is sort of awkward. Originally I just logged it, but that presents its own "fun" (I actually need to tweak some of our logging for this to be usable, since as-is some paths spew zap logs 😞 ).
With the apis.Validatable
pattern, the use of wrapped errors was pragmatic because:
- It only returns an error, so it doesn't have the "other result" problem,
- It only deals with
*apis.FieldError
, so we could add methods to mix/separate warnings/errors, - This kept us from breaking a several years old API! 😅
I opted for this because you can trivially accumulate warnings with a little function like:
func(s string, i ...interface{}) {
warns = append(warns, fmt.Sprintf(s, i...))
}
... and then we don't have to worry about plumbing anything via return values that virtually every Go programmer will hold wrong. 😅 You can also log it with log.Printf
or treat them as errors with log.Fatalf
(I think I use t.Errorf
in one of the tests 🤓 ).
Happy to discuss more. Sorry for the wall of text, just trying to dump some of my thinking here as I go through the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, in other places here, you have a return signature splitting warn/errs, so maybe we could change the return signature to return:
warnings, errors
But, this is not something I'm feeling super strong about, just was thinking from the perspective of the user that wants to easily distinguish if there are errors (already handled), or just warnings. Easy part was not intuitive given the signature of the warning handler function since it only took an interface :)
return | ||
} | ||
|
||
case corev1.SchemeGroupVersion.WithKind("Secret"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems odd to me. We don't do anything with the Secret so validating (or even knowing about) it seems odd. Why are we doing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make reasoning about this Validate function clearer. That way the comment could read: "containing zero or more CIPs and validating them".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the idea is that you can just cat a directory containing whatever into a single yaml, I guess I could see it, but that's not case, since if there's an unknown type we err, so yeah, just curious why the secret is special here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so this is here because I was lazy and just open sourced some of our own functions for parsing and validating some of this stuff, since I was just going to rewrite most of it anyways.
The Secret handling is currently unused here, but I thought it might be useful in a follow-up to support simulating secret references support in CIPs, WDYT? I can remove it, or open a tracking issue and //TODO(#1234)
it, just lmk which you prefer.
I have a mild preference for the latter, since then we can just delete the code I open sourced once we pull this in, but it isn't that much code to maintain either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am mainly thinking how this will start to look like assuming we start adding more things: configmaps, trustroots into the mix. That's going to make it more complicated to do more meaningful validation without starting to bleed more complexity here that will need to also be then replicated into the Compile. Secret reference support is actually a great example of that. Would the Validate unmarshal the key and make sure it's of the right type to make sure it's ok? Only to just to throw it away and redo that work in the Compile. As it stands now it doesn't do much as a validator, and actually could fail if the policy-controller is installed into a different namespace.
Not trying to be difficult here, just trying to visualize and see how this will work with more complex cases and what the division of labor is between Validate and Compile, and if Validate actually provides enough value besides making sure that the YAML can be semantically turned into objects we care about, does that make sense?
I think for CIPs it makes sense to do the Validate but for others, I am not quite convinced. At the very least, I think I'd remove the hardcoded cosign-system requirement.
Could you open a tracking issue for how much validation we really want to do besides for objects that we do not "own" (like secrets, configmaps) here vs. in Compile so we can maybe discuss it there.
return nil, fmt.Errorf("unsupported policy shape: %v", p) | ||
} | ||
|
||
l, warns, err := ParseClusterImagePolicies(ctx, content) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is kind of what I was talking about in the 'Validate' function :) But I'm wondering why we parse them above and throw away the results (in the Validate function). Given the comment below on L107 about compilation, wondering if it would make sense to to rename Validate to Compile and have it return the parsed/fetched policies instead of redoing that work here again, and any changes have to be put in both places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or put another way, wonder if there's any reason to separate Validate / Compile into separate exposed functions, or just drop Validate and have Compile alone. Just wondering since Validate is public method, it's a bit confusing since Compile already calls it and does double the work right after calling Validate. The only thing it seems to do in addition is to convert to evaluatable policies. Also since Validate does not check for duplicate policies, seems better validation to just call Compile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There wasn't as much duplication when I originally went with this shape because I only had Data
! :)
Check out the way I refactored this in the next push (tl;dr private fetch
method to share the logic to produce (string, error)
).
I'd sort of like to keep the Validate
function to drop our original version of the same method downstream (as I mentioned in the Secret
comment, but as I said there, I don't care that much, just a nice to have).
07d59dc
to
d082473
Compare
Signed-off-by: Matt Moore <[email protected]>
d082473
to
766d824
Compare
Failure looks like a flake, but I can't rerun it 😞 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reran the tests and indeed it was a flake 😠 things pass now.
I'm going to merge it for now, and we can deal with the cleanups later.
return | ||
} | ||
|
||
case corev1.SchemeGroupVersion.WithKind("Secret"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am mainly thinking how this will start to look like assuming we start adding more things: configmaps, trustroots into the mix. That's going to make it more complicated to do more meaningful validation without starting to bleed more complexity here that will need to also be then replicated into the Compile. Secret reference support is actually a great example of that. Would the Validate unmarshal the key and make sure it's of the right type to make sure it's ok? Only to just to throw it away and redo that work in the Compile. As it stands now it doesn't do much as a validator, and actually could fail if the policy-controller is installed into a different namespace.
Not trying to be difficult here, just trying to visualize and see how this will work with more complex cases and what the division of labor is between Validate and Compile, and if Validate actually provides enough value besides making sure that the YAML can be semantically turned into objects we care about, does that make sense?
I think for CIPs it makes sense to do the Validate but for others, I am not quite convinced. At the very least, I think I'd remove the hardcoded cosign-system requirement.
Could you open a tracking issue for how much validation we really want to do besides for objects that we do not "own" (like secrets, configmaps) here vs. in Compile so we can maybe discuss it there.
images: | ||
- glob: '*' | ||
`, | ||
wantErr: errors.New("missing field(s): spec.authorities"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer if we could reuse the YAMLs from the existing test/testdata for these tests rather than adding these here. But we can do this in a followup if you agree. They are used for validating the admission webhook, so would be nice to keep them in sync I think.
https://github.com/sigstore/policy-controller/tree/main/test/testdata/policy-controller
warn,invalid,valid, empty-authorities
Each of the warn, error has the expected failure baked into it, like so:
https://github.com/sigstore/policy-controller/blob/main/test/testdata/policy-controller/warn/v1beta1-missing-identity.yaml#L15
So thinking we would get more thorough testing and as we add more testdata files, we'll get those test cases for free.
🎁 This change factors a new small library
./pkg/policy
which is intended to streamline incorporating CIP validation into downstream tooling.For a (much) more verbose explanation see here, but the general idea behind this is to allow CIP's to gate consumption of images in other contexts, for example the base images in build tools such as
ko
orkaniko
. The idea is to enable the tool providers to bake-in default policies for default base images, and optionally expose configuration to let users write policies to authorize base images prior to consumption.For example, I might write the following
.ko.yaml
:With this library, it is likely <100 LoC to add base image policy verification to
ko
, and significantly simplifies our ownpolicy-tester
which has spaghetti code replicating some of this functionality./kind feature
Signed-off-by: Matt Moore [email protected]
Release Note
This adds a new
./pkg/policy
that can be used to integrate policy verification into additional tooling.Documentation
N/A