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

Signing of Porter bundles #3082

Merged
merged 15 commits into from
May 13, 2024
Merged

Signing of Porter bundles #3082

merged 15 commits into from
May 13, 2024

Conversation

kichristensen
Copy link
Contributor

@kichristensen kichristensen commented Apr 14, 2024

What does this change

Sign porter bundles.
Signing is implemented as an internal plugin, allowing us to support different signing providers in the future. The current ones implemented is Cosign and Notation.

What issue does it fix

Implementation of #2902

Notes for the reviewer

Regarding documentation: I will create another PR with the documentation. That PR should be merged just around the release of the version containing the signing functionality, to avoid the documentation being published before the release.

Checklist

  • Did you write tests?
  • Did you write documentation?
  • Did you change porter.yaml or a storage document record? Update the corresponding schema file.
  • If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Signed-off-by: Kim Christensen <kimworking@gmail.com>
Signed-off-by: Kim Christensen <kimworking@gmail.com>
Signed-off-by: Kim Christensen <kimworking@gmail.com>
Signed-off-by: Kim Christensen <kimworking@gmail.com>
@schristoff
Copy link
Member

😶‍🌫️💖 love this already

kichristensen and others added 4 commits April 22, 2024 23:59
Signed-off-by: Kim Christensen <kimworking@gmail.com>
Signed-off-by: Kim Christensen <kimworking@gmail.com>
Signed-off-by: Kim Christensen <kimworking@gmail.com>
@kichristensen kichristensen marked this pull request as ready for review April 24, 2024 09:37
Copy link
Member

@schristoff schristoff left a comment

Choose a reason for hiding this comment

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

I could nit on more changes, but I first want to clarify my understanding, and I have two main questions:

  • Why grpc service?
  • Why close in the interface, and is there a different way we can rework this without putting it on the user to have to call close?

Also an ask: documentation on this (but I'm unsure where it would go, under plugins?)
I'm gonna keep looking at this and kicking at it though.

pkg/signing/helpers.go Outdated Show resolved Hide resolved
pkg/signing/plugins/cosign/cosign.go Show resolved Hide resolved
@kichristensen
Copy link
Contributor Author

kichristensen commented Apr 30, 2024

I could nit on more changes, but I first want to clarify my understanding, and I have two main questions:

  • Why grpc service?
  • Why close in the interface, and is there a different way we can rework this without putting it on the user to have to call close?

Also an ask: documentation on this (but I'm unsure where it would go, under plugins?) I'm gonna keep looking at this and kicking at it though.

The reason for the

I could nit on more changes, but I first want to clarify my understanding, and I have two main questions:

  • Why grpc service?
  • Why close in the interface, and is there a different way we can rework this without putting it on the user to have to call close?

Also an ask: documentation on this (but I'm unsure where it would go, under plugins?) I'm gonna keep looking at this and kicking at it though.

@schristoff
Thanks for the feedback, feel free to nit on everything, the more feedback the better 😄

Why gRPC service? This was done in order to implement the signing as a plugin in the same way as secrets, storage, etc.

Why close in the interface? Your question made me think exactly the same. It is not needed, I will remove it.

Regarding documentation, it should be created. As mentioned in the "Notes for the reviewer" section, we have to discuss how to handle this in such a way that the website is not updated with the documentation until the next release to not confuse users. I have also added that as a topic for the Community Meeting. I would like to avoid giving people the impression that it is available in the current version. Maybe it means that documentation should be added in a follow up PR, to be merged just before or right after the release? Or the way documentation is released should be changed to handle these kind of situations?

kichristensen and others added 6 commits May 1, 2024 00:13
Signed-off-by: Kim Christensen <kimworking@gmail.com>
Signed-off-by: Kim Christensen <kimworking@gmail.com>
Signed-off-by: Kim Christensen <kimworking@gmail.com>
Signed-off-by: Kim Christensen <kimworking@gmail.com>
Signed-off-by: Kim Christensen <kimworking@gmail.com>
@kichristensen
Copy link
Contributor Author

@schristoff Once this PR is approved and ready to be merged I will create a draft PR with documentation. So documentation is ready for for merge when we do the next release.

Copy link
Member

@schristoff schristoff left a comment

Choose a reason for hiding this comment

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

👍

@kichristensen kichristensen merged commit 826511a into getporter:main May 13, 2024
15 checks passed
@kichristensen kichristensen deleted the signing branch May 13, 2024 20:15
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.

2 participants