enabled CGO for Macos otelcol contrib #626
Conversation
|
Nice work :) |
There was a problem hiding this comment.
Can you please go over our security audit? it lists a few recommendations in case we ever change to CGO, which this PR is doing.
https://7asecurity.com/reports/pentest-report-opentelemetry.pdf
|
Related: #618 |
|
The checksec.sh is not working for macos ref PIE: True I am not a security expert, so need advice other tools that can check macos binary security. |
| @@ -0,0 +1,11 @@ | |||
| # OpenTelemetry Collector Contrib CGO Distro | |||
There was a problem hiding this comment.
As a user, I would find it confusing. Why should I care? What does it bring that others don't?
I'm not sure I like the idea of a distribution with just a different platform or compilation mechanism. I would prefer a solution where we built with the right options based on what we need to ship.
Does hostmetrics require cgo when compiled for Darwin? Then we should use cgo to compile the distributions that include that component for Darwin.
I find a separate cgo distribution confusing, and opens the door for a cgo distribution for other platforms, which is not something we want.
There was a problem hiding this comment.
My concern is that when CGO is enabled, the binary is coupled to certain OS versions and will support less OS versions.
There was a problem hiding this comment.
@jpkrohling
Hi, I have updated the PR to just enable CGO for Macos otelcol contrib, could you review it again?
There was a problem hiding this comment.
I would like to have it discussed as part of the SIG and have buy-in from other maintainers.
There was a problem hiding this comment.
Hi, I have tested with golang 10.15 and it works, which is also the oldest macos version that golang >= 1.21 supported ref
I think we could just turn on the CGO option without a separated distribution.
Co-authored-by: Juraci Paixão Kröhling <[email protected]>
pureklkl
changed the title
|
configure.go need to be modified to fix the build, and it will be affected by whether we want a new distribution. I will fix the build once the community have a decision. |
| func Archive(dist string) config.Archive { | ||
| func Archive(dist string, cgo string) config.Archive { | ||
| builds := []string{dist} | ||
| if len(cgo) > 0 { |
There was a problem hiding this comment.
what's the benefit of having cgo set as a string instead of a bool? Do we plan to have more conditional logic based on the content of cgo if we have more distributions built with cgo in the future?
There was a problem hiding this comment.
I put it to string in case we need other OS to support cgo in the future. I also wrote it in the cgoFlag description.
|
have you made a modified fork of this pr with references to repos changed to your fork, and run a release in your fork? |
Add a CGO enabled otelcol-contrib for macos
open-telemetry/opentelemetry-collector-contrib#33393
There are some test failures need be fixed by this PR
open-telemetry/opentelemetry-collector-contrib#33921