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

fips: otelcol images for linux and windows #5384

Merged
merged 2 commits into from
Oct 2, 2024
Merged

Conversation

jeffreyc-splunk
Copy link
Contributor

@jeffreyc-splunk jeffreyc-splunk commented Sep 18, 2024

  • Requires fips: otelcol binaries for linux and windows #5378
  • Images only include the fips-enabled collector and config files (no smart agent bundle, jmx gatherer, etc)
  • The config files are just copied from https://github.com/signalfx/splunk-otel-collector/tree/main/cmd/otelcol/config/collector, but without the smartagent components
  • TODO: Update gitlab pipeline to build, sign, and release the images/manifests. Need to decide where to push:
    • New repo, e.g. quay.io/signalfx/splunk-otel-collector-fips:<version>. The latest tag would be available.
    • Existing repo with new tags, e.g. quay.io/signalfx/splunk-otel-collector:<version>-fips. The latest tag would be unavailable since they are reserved for the non-fips manifests.

@jeffreyc-splunk jeffreyc-splunk force-pushed the otelcol-fips-docker branch 2 times, most recently from e1e9dc6 to 3313cb2 Compare September 19, 2024 19:03
Base automatically changed from otelcol-fips-binaries to main October 2, 2024 22:23
@atoulme atoulme marked this pull request as ready for review October 2, 2024 22:23
@atoulme atoulme requested review from a team as code owners October 2, 2024 22:23
Copy link
Contributor

@hughesjj hughesjj left a comment

Choose a reason for hiding this comment

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

Nothing blocking merge but there's some code smells imo

Comment on lines +29 to +30
OTELCOL_DIR="${OTELCOL_DIR}/fips"
DIST_DIR="${OTELCOL_DIR}/dist"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Maybe we should bring down the instantiation for the non-fips setting of these variables to live in the "true" conditional block?

if [ ! -f "${REPO_DIR}/dist/agent-bundle_linux_${arch}.tar.gz" ]; then
echo "${REPO_DIR}/dist/agent-bundle_linux_${arch}.tar.gz not found!" >&2
exit 1
if [ ! -f "${REPO_DIR}/dist/agent-bundle_linux_${arch}.tar.gz" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

not for this PR, but does this mean if we set SKIP_BUNDLE="true", the build will fail due to this line? (given FIPS!="true")

It's hard to know if the nested IF statements are intentionally nooping the uncovered else case or if it's just because we didn't want to if [STATEMENT_A] && [STATEMENT_B] the conditional in the parent if block... I'm starting to think it's the latter?

@atoulme
Copy link
Contributor

atoulme commented Oct 2, 2024

Thanks for review James. There is more to do but if this passes muster, I'm happy to merge it and we can work on this more. There are still TODOs listed in the PR description that will require us to review further.

@atoulme atoulme merged commit 2561dd8 into main Oct 2, 2024
244 of 245 checks passed
@atoulme atoulme deleted the otelcol-fips-docker branch October 2, 2024 23:52
@github-actions github-actions bot locked and limited conversation to collaborators Oct 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants