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

deny by default. #279

Merged
merged 3 commits into from
Sep 29, 2022
Merged

deny by default. #279

merged 3 commits into from
Sep 29, 2022

Conversation

vaikas
Copy link
Collaborator

@vaikas vaikas commented Sep 29, 2022

Signed-off-by: Ville Aikas vaikas@chainguard.dev

Summary

fixed #235

Release Note

  • Any image not matching a policy is 'deny' by default.

Documentation

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2022

Codecov Report

Merging #279 (706179d) into main (7183541) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #279   +/-   ##
=======================================
  Coverage   62.41%   62.41%           
=======================================
  Files          28       28           
  Lines        2650     2650           
=======================================
  Hits         1654     1654           
  Misses        910      910           
  Partials       86       86           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
Signed-off-by: Ville Aikas <vaikas@chainguard.dev>
Copy link
Contributor

@k4leung4 k4leung4 left a comment

Choose a reason for hiding this comment

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

thanks for the fix.

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.

lgtm to deny-all

@vaikas vaikas merged commit 0c0fe98 into sigstore:main Sep 29, 2022
@vaikas vaikas deleted the deny-default branch September 29, 2022 23:37
@@ -18,4 +18,4 @@ metadata:
name: config-policy-controller
namespace: cosign-system
data:
no-match-policy: allow
no-match-policy: deny
Copy link
Member

Choose a reason for hiding this comment

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

If this is the default, then I'd recommend that we omit it entirely.

By specifying this, we make it so that folks upgrading using our configs will overwrite any changes they make to these.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But this configmap hasn't been released yet. I don't see any harm on doing this change to the config file.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, my point is a bit of nuance around kubectl apply 3-way merges.

If a user is installing via kubectl apply and we specify a value, which they change, then when they upgrade to vNext the apply will overwrite the value.

If we don't specify a value, and they change it (via kubectl edit), then when they upgrade to vNext the apply will NOT overwrite the value.

This is a silly game of nuance we played with configmap knobs in Knative since we were seeing users edits being reverted on upgrades 😞

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, i understood the intention now.

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.

Should the controller deny an image if no policy matches ?
5 participants