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

Base ECK docker image on distroless instead of UBI by default #5580

Merged
merged 21 commits into from
Apr 29, 2022

Conversation

naemono
Copy link
Contributor

@naemono naemono commented Apr 13, 2022

Closes #4561

This will update the default Dockerfile for ECK operator to use a scratch image instead of UBI. An additional UBI dockerfile has been added, which is leveraged in CI tooling to build/push an additional image to both:

  1. docker.elastic.co
  2. docker.io

So, a total of 4 images (not including different platforms) will be built when building a new release (non-SNAPSHOT)

  1. docker.elastic.co/elastic/eck-operator:x.y.z
  2. docker.io/elastic/eck-operator:x.y.z
  3. docker.elastic.co/elastic/eck-operator:x.y.z-ubi
  4. docker.io/elastic/eck-operator:x.y.z-ubi

This will be paired with an additional PR for the internal redhat tooling leveraged during release.

Reason for Draft:

Additional testing is being done now to ensure:

  • These new scratch images work in non-Openshift environment.
  • The UBI images work in Openshift environments.

@naemono naemono added the :ci Things related to Continuous Integration, automation and releases label Apr 13, 2022
@botelastic botelastic bot added the triage label Apr 13, 2022
Update makefile to add new section for building ubi image.

Make spacing consistent in makefile
include '-ubi' suffix in image* when building CSV files for redhat.

Ensure UBI build references the ubi dockerfile
@naemono naemono force-pushed the 4561-replace-ubi-with-scratch branch from d945e60 to f0a3009 Compare April 13, 2022 18:01
@naemono
Copy link
Contributor Author

naemono commented Apr 13, 2022

The additional ubi image build adds about 30 seconds to the full release.

### Start ###
13:46:52  # The following UBI build should already have the binary cached, and should
.......
13:47:14  #32 pushing layers
13:47:18  #32 pushing layers 3.3s done
13:47:18  #32 pushing manifest for docker.elastic.co/eck/eck-operator:2.3.0-rc0-ubi@sha256:7dd8d566fdcb6f0a20ad7b02f79de25131592be9b0fe85c1c3ea994e820b9157
13:47:20  #32 pushing manifest for docker.elastic.co/eck/eck-operator:2.3.0-rc0-ubi@sha256:7dd8d566fdcb6f0a20ad7b02f79de25131592be9b0fe85c1c3ea994e820b9157 1.9s done
13:47:22  #32 pushing layers 2.0s done
13:47:22  #32 pushing manifest for docker.io/elastic/eck-operator:2.3.0-rc0-ubi@sha256:7dd8d566fdcb6f0a20ad7b02f79de25131592be9b0fe85c1c3ea994e820b9157
13:47:22  #32 pushing manifest for docker.io/elastic/eck-operator:2.3.0-rc0-ubi@sha256:7dd8d566fdcb6f0a20ad7b02f79de25131592be9b0fe85c1c3ea994e820b9157 0.7s done
13:47:22  #32 DONE 11.0s
### end ###

@thbkrkr thbkrkr added the >enhancement Enhancement of existing functionality label Apr 14, 2022
@botelastic botelastic bot removed the triage label Apr 14, 2022
@pebrc
Copy link
Collaborator

pebrc commented Apr 14, 2022

I think we should adopt the same naming convention as Elasticsearch and the rest of the Elastic Stack: $REPO-ubi8:$VERSION. I know there are good arguments for putting it on the version as metadata as defined in semver, but just for consistency across Elastic's offerings we should follow the established pattern.

Dockerfile Outdated

# Copy the operator binary into a lighter image
FROM registry.access.redhat.com/ubi8/ubi-minimal:8.5
FROM scratch
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be distroless instead? We used to use that in the past. AFAIK one of the advantages is that it comes with CA certificates and supports tmp (not sure if we need the latter)

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 can move this to distroless. I had run through all e2e tests using scratch, and -ubi builds and hadn't hit a single failure.

Copy link
Collaborator

@pebrc pebrc Apr 14, 2022

Choose a reason for hiding this comment

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

I am trying to think of scenarios where the operator actually makes HTTP requests that would require root CA certs. The only one I can think of now is the external secrets feature we added recently. The operator makes a request to the linked Elastic Stack applications to determine their version, which would require those certs. We don't test this setup in e2e tests (we only approximate it with another Elasticsearch cluster in the same k8s cluster). @thbkrkr may have more insights here.

But I guess distroless puts us on the safe side wrt that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we are in a better place using distroless for the reasons you outlined.

@naemono naemono marked this pull request as ready for review April 20, 2022 19:14
@naemono
Copy link
Contributor Author

naemono commented Apr 20, 2022

After fighting with config/samples, and having my own data in there causing havoc in e2e tests, this has finally been verified, and ready for 👀

@naemono
Copy link
Contributor Author

naemono commented Apr 20, 2022

run/e2e-tests

@thbkrkr thbkrkr added the v2.3.0 label Apr 21, 2022
@naemono
Copy link
Contributor Author

naemono commented Apr 21, 2022

run/e2e-tests

@naemono
Copy link
Contributor Author

naemono commented Apr 21, 2022

run/e2e-tests

@naemono
Copy link
Contributor Author

naemono commented Apr 21, 2022

run/e2e-tests

Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

Almost good to go. We already have some UBI logic in the generator for the OLM versions of ECK. I would tie the image onto that so that we use it only for the certified operator.

hack/operatorhub/templates/csv.tpl Outdated Show resolved Hide resolved
test/e2e/samples_test.go Outdated Show resolved Hide resolved
@pebrc pebrc changed the title 4561 replace ubi with scratch Base ECK docker image on distroless instead of UBI by default Apr 27, 2022
Dockerfile Outdated
@@ -39,16 +39,11 @@ LABEL name="Elastic Cloud on Kubernetes" \
description="Elastic Cloud on Kubernetes automates the deployment, provisioning, management, and orchestration of Elasticsearch, Kibana, APM Server, Beats, and Enterprise Search on Kubernetes" \
io.k8s.description="Elastic Cloud on Kubernetes automates the deployment, provisioning, management, and orchestration of Elasticsearch, Kibana, APM Server, Beats, and Enterprise Search on Kubernetes"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I missed this initially. These label choices where motivated by the need to override existing labels in the UBI base image. I think now that we are moving back to distroless there is no need for these often redundant (description and io.k8s.description and summary, io.k8s.display-name and name) labels.

I wonder if we should use OCI standardised labels on this docker image instead (for the UBI one we have to stick with the current set of labels to keep the overrides)
https://github.com/opencontainers/image-spec/blob/main/annotations.md

Copy link
Contributor Author

@naemono naemono Apr 28, 2022

Choose a reason for hiding this comment

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

This is a good point. I've removed the overrides, and added OCI annotations as described in link, and grouped the k8s labels together alphabetically. lmk if I've covered them all.

Also verified that the golang:1.x image we base things from does not have any preexisting labels:

            "Labels": {}

Dockerfile Outdated Show resolved Hide resolved
Dockerfile.ubi Outdated Show resolved Hide resolved
naemono and others added 3 commits April 28, 2022 11:27
Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>
Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>
Copy link
Contributor

@thbkrkr thbkrkr left a comment

Choose a reason for hiding this comment

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

LGTM

Makefile Outdated Show resolved Hide resolved
hack/operatorhub/templates/csv.tpl Outdated Show resolved Hide resolved
hack/operatorhub/templates/csv.tpl Outdated Show resolved Hide resolved
hack/operatorhub/templates/csv.tpl Outdated Show resolved Hide resolved
naemono and others added 4 commits April 28, 2022 15:37
Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>
Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>
Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>
Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>
Dockerfile Outdated Show resolved Hide resolved
Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>
@naemono naemono merged commit 56d8edc into elastic:main Apr 29, 2022
@thbkrkr thbkrkr removed the :ci Things related to Continuous Integration, automation and releases label May 30, 2022
fantapsody pushed a commit to fantapsody/cloud-on-k8s that referenced this pull request Feb 7, 2023
…c#5580)

* Update default dockerfile to add scratch.
Update makefile to add new section for building ubi image.

Make spacing consistent in makefile
include '-ubi' suffix in image* when building CSV files for redhat.

Ensure UBI build references the ubi dockerfile

* move from scratch to distroless

* Update image name to include ubi suffix, not tag

* Move template to have suffix on image name, not tag

* Add without integration check to apmserver sample test builders

* Add newline to generate new git hash

* Attempt to use different machine type

* Remove user specification in dockerfile for distroless:nonroot, as it's already non-root.

* revert change to machine type

* Only use ubi image when certified-operator.

* removing WithoutIntegrationCheck() from this PR

* use UbiOnly instead of string comparison.

* Add OCI annotations to dockerfile, and remove legacy labels.

* Update Dockerfile label to correct base name

Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>

* Update Dockerfile.ubi for updated golang version

Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>

* Convert to ubi8 in Makefile

Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>

* Convert to ubi8 in  hack/operatorhub/templates/csv.tpl

Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>

* Convert to ubi8 again in hack/operatorhub/templates/csv.tpl

Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>

* Convert to ubi8 once more in hack/operatorhub/templates/csv.tpl

Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>

* Update license Label

Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>

Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>
Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement Enhancement of existing functionality v2.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace UBI with scratch image
3 participants