-
Notifications
You must be signed in to change notification settings - Fork 134
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
plumb through !cgo golang tags that removes pkcs11 support #244
Conversation
Signed-off-by: Ville Aikas <[email protected]>
.ko.yaml
Outdated
# If you are deploying from M1, you can use this (uncomment this, and | ||
# comment above), though it does remove the support for the "createca" command. | ||
# But at least you can deploy it from M1 using this. | ||
#- main: . | ||
# flags: | ||
# - -tags=purego |
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 may be contentious, but my personal bias would be towards making the configs we check in by default maximally approachable by contributors.
I believe here that translates to opting INTO pkcs11 support here (like we do in cosign) vs. opting out (as this PR currently does).
Here's where cosign is doing this for pivkey
: https://github.com/sigstore/cosign/blob/a064fabac961be65bee810369e8b34c66ef265bd/cmd/cosign/cli/piv_tool_disabled.go#L2
It looks like it is using a cgo
tag, that I didn't come across in my search, which I wonder if it's set automatically 🤔
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 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.
Make up your mind, you want purego or cgo? :)
A data point (maybe crappy, but a data point regardless), is that while I was struggling with why the config was not working (spoiler, I was using older version of ko), things didn't work. Then I updated ko and things started working (I reckon it started passing the tags flag to go build), so not sure it would work out of the box. But, in any case, I'm happy to change the tags to whatever we want them to be.
Also wanted to make sure I don't break any folks that are expecting the pkcs11 support by turning off something by this change, so went for the opt-out.
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 just played a little locally and it seems that the cgo
tag is automatically defined unless CGO_ENABLED=0
is set, at which point the purego
tag is set.
What didn't work with ko
was likely the support for goreleaser
config in the .ko.yaml
, which is relatively new. Previously you had to export GOFLAGS=...
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 take that back. Only cgo
is automatically set, purego
is not. Let's use cgo
instead?
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 this keeps cgo by default and provides a toggle to implement purego as an option, I am good with that.
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 missed this when reading this the first time:
I believe here that translates to opting INTO pkcs11 support here (like we do in cosign) vs. opting out (as this PR currently does).
The PR as was (I'm changing the tag) does opt INTO pkcs11, because you had to uncomment the lines to get the non-pkcs11 support? @mattmoor did I misunderstand what you meant by 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.
My bias was towards CGO_ENABLED=0
by default, but I'd rather land this as-is, and change that later since it seems at least mildly contentious.
Signed-off-by: Ville Aikas <[email protected]>
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.
Let's land the support and then we can talk about what defaults make the most sense for cgo
, no need to hold this up.
Summary
If specifying golang env
CGO_ENABLED=0
, remove the code that uses PKCS11, because it depends on C. By default, nothing changes, this is just purely an option you must specify.Ticket Link
Fixes #240
Release Note