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

[cosigned] The webhook name is now configurable via --webhook-name flag #1726

Merged

Conversation

vpnachev
Copy link
Contributor

@vpnachev vpnachev commented Apr 8, 2022

Summary

The webhook name is now configurable via --webhook-name flag for cosigned. This way, cosigned can be deployed multiple times in the same cluster with different webhook configurations, e.g. namespace and/or object selectors. Also, each webhook can be served by different instance of the cosigned using different public keys.

Ticket Link

Fixes

Release Note

The name of the cosigned webhook can be now configured via the flag `--webhook-name`, the default value is `cosigned.sigstore.dev`.

@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2022

Codecov Report

Merging #1726 (62e6028) into main (f983706) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main    #1726   +/-   ##
=======================================
  Coverage   29.48%   29.48%           
=======================================
  Files         141      141           
  Lines        8505     8505           
=======================================
  Hits         2508     2508           
  Misses       5726     5726           
  Partials      271      271           
Impacted Files Coverage Δ
cmd/cosign/webhook/main.go 0.00% <0.00%> (ø)

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 f983706...62e6028. Read the comment docs.

Signed-off-by: Vladimir Nachev <vladimir.nachev@sap.com>
@vpnachev vpnachev force-pushed the feature/make-webhook-name-configurable branch from 2c059d7 to 56af7f3 Compare April 12, 2022 06:54
@@ -92,7 +92,7 @@ func NewValidatingAdmissionController(ctx context.Context, cmw configmap.Watcher

return validation.NewAdmissionController(ctx,
// Name of the resource webhook.
webhookName,
*webhookName,
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: If you change this, you will to change on the scripts too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the helm templates and config files? If yes, I rely on this comment

// webhookName holds the name of the validating webhook to set up with the
// types we are watching. If this changes, you must also change:
// ./config/500-webhook-configuration.yaml
to get the attention to whomever tries to change it. Do you suggest to enhance the help string of the flag?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we'll need to update the comment in that piece of code accordingly.
Also we'll need to add that field in the values.yaml of the helm-chart, so everything gets changed based on the value of the field. I could take care of that.

Copy link
Contributor Author

@vpnachev vpnachev Apr 12, 2022

Choose a reason for hiding this comment

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

I have updated the comment and the flag doc with 62e6028.
Looking in the webhook initialization in [1] and [2] there is a lot of configurations that can be injected, but also skipped in different conditions, or even fail when URL is used instead of in-cluster service. I doubt I can describe all of them with just few words, and I don't think here is the right place for detailed documentation.
If you have better suggestion/wording, please let me know and I will contribute it.

[1] https://github.com/knative/pkg/blob/e325df66cb51d341b5d1142fa42f75539dfe290a/webhook/resourcesemantics/validation/reconcile_config.go#L99
[2] https://github.com/knative/pkg/blob/e325df66cb51d341b5d1142fa42f75539dfe290a/webhook/resourcesemantics/defaulting/defaulting.go#L170

Copy link
Contributor

Choose a reason for hiding this comment

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

We'll get some documentation for cosigned new changes soon.

Signed-off-by: Vladimir Nachev <vladimir.nachev@sap.com>
@dlorenc
Copy link
Member

dlorenc commented Apr 13, 2022

@hectorj2f is this ready to merge?

@hectorj2f
Copy link
Contributor

@dlorenc Yes.
@vpnachev Could you add the flag and respective changes to the webhook configuration if the flag is set in sigstore/helm-charts/cosigned ?

@dlorenc dlorenc merged commit 36afb67 into sigstore:main Apr 13, 2022
@github-actions github-actions bot added this to the v1.8.0 milestone Apr 13, 2022
@vpnachev vpnachev deleted the feature/make-webhook-name-configurable branch April 13, 2022 13:49
@vpnachev
Copy link
Contributor Author

Yes, I can open a PR in the next days about it.

mlieberman85 pushed a commit to mlieberman85/cosign that referenced this pull request May 6, 2022
…ag (sigstore#1726)

* [cosigned] The webhook name is now configurable via --webhook-name flag

Signed-off-by: Vladimir Nachev <vladimir.nachev@sap.com>

* [cosigned] Update doc string for webhookName

Signed-off-by: Vladimir Nachev <vladimir.nachev@sap.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.

None yet

4 participants