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

build: Enhance binary #416

Merged
merged 3 commits into from
Oct 17, 2022
Merged

Conversation

Garfield96
Copy link
Contributor

What this PR does / why we need it:

This PR changes multiple go compiler options:

  • remove -i: This flag is deprecated and no longer required.
  • add -buildmode=pie: This option increases the security by compiling the binary as a position-independent executable to enable address space layout randomization (ASLR). The downsides of enabling this option are an increasing binary size as well as a small performance degradation (usually 5-10 %). However, ASLR is commonly used (e.g. many distributions use it for package builds by default), and this application doesn't require very high performance.
  • add -trimpath: This option reduces the binary size by using relative paths instead of absolute paths. Since the paths no longer depend on the build environment, the build is more reproducible, and stack traces are easier to read.

Documentation for these options can be found here: https://pkg.go.dev/cmd/go#hdr-Compile_packages_and_dependencies

Which issue(s) this PR fixes:

Fixes #

Does this PR introduced a user-facing change?

None

Additional documentation, usage docs, etc.:


Signed-off-by: Christian Menges <christian.menges@outlook.com>
Signed-off-by: Christian Menges <christian.menges@outlook.com>
@wenchajun
Copy link
Member

Here the test run failed, I see that the reason for the error report is timeout, should you adjust it?
https://github.com/fluent/fluent-operator/blob/master/tests/scripts/fluentd_helm_e2e.sh#L23

@benjaminhuo
Copy link
Member

  • remove -i: This flag is deprecated and no longer required.
  • add -buildmode=pie: This option increases the security by compiling the binary as a position-independent executable to enable address space layout randomization (ASLR). The downsides of enabling this option are an increasing binary size as well as a small performance degradation (usually 5-10 %). However, ASLR is commonly used (e.g. many distributions use it for package builds by default), and this application doesn't require very high performance.
  • add -trimpath: This option reduces the binary size by using relative paths instead of absolute paths. Since the paths no longer depend on the build environment, the build is more reproducible, and stack traces are easier to read.

@Garfield96 Thank you! I like this PR

@Garfield96
Copy link
Contributor Author

I was able to reproduce the error locally. Unfortunately, -buildmode=pie causes the go compiler to create a dynamic executable. PIE technically does not imply a dynamic executable, but it's a deficiency of the go compiler. Since there is no runtime linker in the base image, the container does not get ready in the tests. I didn't recognize this before, because I tested this flag only outside of the container and since there is a runtime linker on my system, the issue didn't occur. I see the following options to resolve the problem:

  • By using an external linker, a static PIE could be built. However, this makes the build process more difficult.
  • PIE is dropped from this PR
  • A different base image is used which provides a runtime linker.

Postponing PIE until the go compiler supports static builds, is probably the best choice. The current state of the implementation is tracked here: golang/go#26492

@benjaminhuo
Copy link
Member

benjaminhuo commented Oct 7, 2022

  • A different base image is used which provides a runtime linker.

@Garfield96 Which base build image do you think might have the runtime linker?
golang:1.19-buster in https://hub.docker.com/_/golang?

Can we use golang:1.19-buster to replace golang:1.19.1-alpine3.16 https://github.com/fluent/fluent-operator/blob/master/cmd/fluent-watcher/fluentbit/Dockerfile#L1 ?

Signed-off-by: Christian Menges <christian.menges@outlook.com>
@Garfield96
Copy link
Contributor Author

@benjaminhuo Sorry for the late reply. We would need to replace https://github.com/fluent/fluent-operator/blob/master/cmd/fluent-manager/Dockerfile#L23 by e.g. Alpine. However, the watcher images are fine, since they use base images which include a runtime linker. I removed -buildmode=pie from the fluent-manager image and now the tests are green.

@benjaminhuo
Copy link
Member

@benjaminhuo Sorry for the late reply. We would need to replace https://github.com/fluent/fluent-operator/blob/master/cmd/fluent-manager/Dockerfile#L23 by e.g. Alpine. However, the watcher images are fine, since they use base images which include a runtime linker. I removed -buildmode=pie from the fluent-manager image and now the tests are green.

Got it. Thank you!

@benjaminhuo benjaminhuo merged commit 379302d into fluent:master Oct 17, 2022
@benjaminhuo
Copy link
Member

@Garfield96 Revert this PR for now because some users reported #429 and @wenchajun found it's related to this PR.

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

3 participants