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

Eng 487 change ruby base images from debian to alpine #364

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

HauwaHakimi
Copy link
Contributor

No description provided.

Copy link
Contributor

@heathd heathd left a comment

Choose a reason for hiding this comment

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

need more info about alpine linux change

Dockerfile Outdated
RUN apt update -y \
&& apt -y install nano \
&& cp /usr/bin/nano /usr/local/bin/
FROM ruby:3.3.5-alpine as rubybuilder
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the switch to -alpine intentional here?

alpine linux is a lightweight linux distribution designed for running in containers. It uses a different 'libc' than debian (which is what the -slim) version uses.

There may be compatibility issues arising from pulling in from a different linux distribution.

It would probably be safest stick with the -slim variant for any additional builder layers that are pulled in

Copy link
Contributor

Choose a reason for hiding this comment

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

We're looking at a switch to alpine as the ruby:3.3.5 and ruby:3.3.5-slim images are based on debian stable, which appears to have a critical CVE associated with it. It looks like Hauwa has been able to get the app running correctly with alpine and the gcompat library, which handles the glibc/musl conversion.

I think this is a really good change if it works as it gives us a much smaller image and a more up-to-date set of packages, with a smaller attack surface and therefore fewer vulnerabilities.

Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved

group :test do
gem 'rack_session_access', '~> 0.2.0'
end

gem "listen", "~> 3.9", :group => :development
gem "listen", "~> 3.9"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we need to do this? Looks like the listen gem is only used in the development environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when we specify it with :group => :development in the Gemfile, our Docker build fails with the error:

"listen is not part of the bundle. Add it to your Gemfile."

Copy link
Contributor

@heathd heathd Sep 26, 2024

Choose a reason for hiding this comment

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

hmm, I think this is the wrong solution. I added a comment on the Jira ticket about this

removing the "group" constrain on the "listen" gem will mean that it gets included even when the app runs in "production" mode, when it is not needed.

There are already other steps in the dockerfile (the bundle config and rake assets:precompile) which work towards making this dockerfile a "production mode only" docker file, which I think is the right thing to do.

What we should do instead is specify RAILS_ENV as production in the final CMD line in the docker file

a nice way of doing that would be to add an ENV statement which defaults RAILS_ENV to production

@HauwaHakimi
Copy link
Contributor Author

need more info about alpine linux change

Reason for Change

The main driver for this change is to address the critical vulnerability CVE-2023-45853 in MiniZip (part of zlib) that was present in the Debian-based images. This vulnerability, which could lead to integer overflow and heap-based buffer overflow, was not resolved in the Debian images we were using. Alpine Linux has resolved this vulnerability in their zlib packages, along with several other CVEs. Changing this image to alpine eliminates the CVE-2023-45853 and reduces the overall image size.

Updated Dockerfile to use Alpine-based images

  • Changed base images from Debian-based to Alpine-based for Node.js and Ruby
  • Updated installation commands to use Alpine's package manager (apk)
  • Added necessary build dependencies for Alpine
  • Adjusted user creation command for Alpine

Testing

  • The image has been built and scanned locally. The container runs successfully locally with the new Alpine-based images.
  • Docker Scout scan shows no vulnerabilities in the new image.

Next Steps

  • Deploy to the test environment
  • Conduct a full vulnerability scan in ECR to ensure all issues are resolved

C-gyorfi
C-gyorfi previously approved these changes Sep 26, 2024
Copy link

@C-gyorfi C-gyorfi left a comment

Choose a reason for hiding this comment

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

It looks good to me! I wonder how the build time has changed now that we are using a smaller base image but installing more dependencies.

* Explicit error message if env file is not set
* doesn't load fake data unless running in development
*errors out if unable to get csv file from s3
if Rails.env.development?
return File.read(File.join(Rails.root, 'config', 'cost_centre_fixture.csv'))
# dont load the cost centers from s3 if were not running the server or we're not in RAILS_ENV != production
if Rails.env.development? or !defined?(Rails::Server)
Copy link
Contributor

Choose a reason for hiding this comment

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

seems reasonable .. I would maybe double down on making the logic explicit by extracting !defined?(Rails::Server) into a named method eg.

def running_inside_a_rake_task?
   @defined(Rails::Server)
end

...
def get_cost_centre_data()
...
 if Rails.env.development? or running_inside_a_rake_task?
   ...
 end
...

* initializers are always run after version 4 of rails. In this instance the initializer was being run by rake during the docker build process. However, the necessary env vars are not present at build time so this step was failing. Line 36 stops this.
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.

5 participants