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: add Dockerfile for controller, build with bottlerocket-sdk #85

Merged
merged 2 commits into from Aug 24, 2021

Conversation

etungsten
Copy link
Contributor

@etungsten etungsten commented Aug 19, 2021

Note: Depends on #84

Issue number:
Closes #11

Description of changes:

commit cab144a3efc5675b75ea33c482a9f6798ff857fa
Author: Erikson Tung <etung@amazon.com>
Date:   Mon Aug 23 15:22:47 2021 -0700

    dependency: use default kube-rs features, disable rustls

Author: Erikson Tung <etung@amazon.com>
Date:   Thu Aug 19 13:54:29 2021 -0700

    build: add Dockerfile for controller, build with bottlerocket-sdk
    
    Build the binaries with the bottlerocket-sdk.
    
    Dockerfile: install openssl with musl for controller, test-agent

Testing done:
Able to build the example-test-agent and the controller and run the binaries with their respective container images.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@etungsten etungsten changed the title Build with sdk build: add Dockerfile for controller, build with bottlerocket-sdk Aug 19, 2021
@webern webern added the in-review A PR is open that will close this issue label Aug 19, 2021
@webern webern added this to the sprint-number-1 milestone Aug 19, 2021
Copy link
Member

@webern webern left a comment

Choose a reason for hiding this comment

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

We might needs something like this to make TLS implementations (whether OpenSSL or Rustls) happy https://github.com/bottlerocket-os/bottlerocket-ecs-updater/blob/develop/Dockerfile#L34..L35

Obviously merge #84 first, but this looks fine. Thanks!

controller/Dockerfile Outdated Show resolved Hide resolved
@etungsten
Copy link
Contributor Author

Push above adds the CA certificates store to the container images.

@jpculp
Copy link
Member

jpculp commented Aug 23, 2021

This seems fine to me after #84 is merged.

@etungsten
Copy link
Contributor Author

Push above rebases onto develop.

@etungsten etungsten marked this pull request as ready for review August 23, 2021 17:42
controller/Dockerfile Outdated Show resolved Hide resolved
test-agent/examples/example_test_agent/Dockerfile Outdated Show resolved Hide resolved
@etungsten
Copy link
Contributor Author

Push above changes a few things:

Also removed redundant WORKDIR as pointed out by @arnaldo2792

Copy link
Member

@webern webern left a comment

Choose a reason for hiding this comment

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

I think we should build an image tagged e.g. bottlerocket-sdk-with-openssl and then use it in both FROM lines. This deduplicates the code. I don't think we need to push the tagged image anywhere, but the makefile can ensure that other docker builds depend on it so we know it will be built and available locally.

@webern
Copy link
Member

webern commented Aug 23, 2021

depend on it so we know it will be built and available locally.

We might also see if Ben thinks the use case seems likely enough to warrant including musl openssl-devl in the SDK itself, which would be ideal.

Makefile Outdated Show resolved Hide resolved
@etungsten
Copy link
Contributor Author

Push above separates out the openssl musl build into a separate Dockerfile that creates an augmented bottlerocket-sdk with openssl built with the musl toolchain that we use for building the other container images.

@etungsten
Copy link
Contributor Author

Push above changes some Makefile target names as recommended by @webern

Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@etungsten
Copy link
Contributor Author

etungsten commented Aug 24, 2021

Push above relocates the OPENSSL_* env vars to the sdk-openssl image. Also some minor changes to names.
Push below changes test-sys to testsys and removes --network=host for docker builds.

Images still build fine.

Copy link
Member

@webern webern left a comment

Choose a reason for hiding this comment

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

🍰

Copy link
Contributor

@arnaldo2792 arnaldo2792 left a comment

Choose a reason for hiding this comment

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

A few nits, but approved 👍

Dockerfile.sdk_openssl Outdated Show resolved Hide resolved
Dockerfile.sdk_openssl Outdated Show resolved Hide resolved
Comment on lines +26 to +29
ENV PKG_CONFIG_ALLOW_CROSS=1
ENV OPENSSL_STATIC=true
ENV OPENSSL_DIR=/musl
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Shouldn't ENVs be closer to ARGs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's absolutely necessary since it's not used for building openssl. These are environment variables we should set after we're able to successfully build openssl with musl. It's intended for building the openssl-sys crate later when we build the controller image. I'll add a comment here to indicate what these environment variables are for.

Build the binaries with the bottlerocket-sdk.

Dockerfile: install openssl with musl for controller, test-agent
@etungsten
Copy link
Contributor Author

etungsten commented Aug 24, 2021

Push above and below addresses @arnaldo2792 's comments.

Tested the builds and they still work.

Copy link
Contributor

@arnaldo2792 arnaldo2792 left a comment

Choose a reason for hiding this comment

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

👀

@etungsten etungsten merged commit a84e9b8 into develop Aug 24, 2021
@etungsten etungsten deleted the build-with-sdk branch August 24, 2021 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-review A PR is open that will close this issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build: create controller image
4 participants