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

update docker to debian latest #40

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions Dockerfile
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
# BUILD STAGE ---------------------------------------
# split this stage to save time and reduce image size
# ---------------------------------------------------
FROM python:3.8-alpine3.11 as BuildStage
# update apk cache
RUN apk update
FROM python:3.8-slim-bullseye as BuildStage
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for slim in the BuildStage

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not Python 3.10 ?
(I mean you probably want it fast for Riskified, but would be good to upgrade in the near future)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should test our code for 3.10, I would be glad to know that we can move to 3.10 @asafc, can we?

# update apt
RUN apt-get update
# TODO: remove this when upgrading to a new alpine version
# more details: https://github.com/pyca/cryptography/issues/5771
ENV CRYPTOGRAPHY_DONT_BUILD_RUST=1
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you explain why?

Copy link
Contributor

Choose a reason for hiding this comment

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

CRYPTOGRAPHY_DONT_BUILD_RUST looks like an alpine thing

# install linux libraries necessary to compile some python packages
RUN apk add --update --no-cache --virtual .build-deps gcc git build-base alpine-sdk python3-dev musl-dev postgresql-dev libffi-dev libressl-dev
# TODO ask asaf about postgres 11
RUN apt-get install --fix-missing -y gcc git make python3-dev libpq-dev libffi-dev libssl-dev g++
Copy link
Contributor

Choose a reason for hiding this comment

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

Do this together with apt-get update

Copy link
Contributor

@roekatz roekatz Jun 23, 2022

Choose a reason for hiding this comment

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

  • & apt-get clean to reduce image size.
  • Also - if you take @orishavit 's advice and use python3.8 without the slim, it almost certainly already has you covered in terms of deps and no real need to install anything else (that's the purpose of that image, and that was my experience with opal) - That would speed up the image build which is always good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wouldn't using the non-slim image will make the image larger?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you use the slim image and then all install deps yourself, you'll get the same size but longer setup times. Slim might actually be better, but I would test this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Couple of things:

  • Actually Python's official rule of thumb is go with the regular. It's bigger in size but many images are using the underlying buildpack-deps - therefor it "compresses" better per machine (docker stores the layer only once for many images).
  • The BuildStage is a temporary thing just used to build the pkgs, the final image can be still based on the slim flavor.

# from now on, work in the /app directory
WORKDIR /app/
# Layer dependency install (for caching)
Expand All @@ -19,11 +20,18 @@ RUN pip install --upgrade pip && pip install --user -r requirements.txt
# MAIN IMAGE ----------------------------------------
# most of the time only this image should be built
# ---------------------------------------------------
FROM python:3.8-alpine3.11
FROM python:3.8-slim-bullseye
# setup optional testing repo for newer packages
COPY docker-files/testing.list /etc/apt/sources.list.d/
COPY docker-files/testing.prefs /etc/apt/preferences.d/
# update apt
RUN apt-get update
# bash is needed for ./start/sh script
RUN apk add --update --no-cache bash curl
RUN apt-get -y install curl
# needed for rookout
RUN apk add g++ python3-dev linux-headers
RUN apt-get -y install --fix-missing gcc g++ python3-dev
Copy link
Contributor

Choose a reason for hiding this comment

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

On my tests seems like rookout is installable & importable without that line. (wasn't the case on alpine)

# install newer pcre2 to resolve CVE-2022-1586
RUN apt-get -y install -t testing libpcre2-8-0
Copy link
Contributor

Choose a reason for hiding this comment

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

Merge all apt-gets to one step

# copy opa from official image (main binary and lib for web assembly)
RUN curl -L -o /opa https://openpolicyagent.org/downloads/latest/opa_linux_amd64_static && chmod 755 /opa
# copy libraries from build stage
Expand Down Expand Up @@ -72,4 +80,4 @@ EXPOSE 7000
# expose opa directly
EXPOSE 8181
# run gunicorn
CMD ["/start.sh"]
CMD ["/start.sh"]
Copy link
Contributor

Choose a reason for hiding this comment

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

(can't comment on lines that haven't change)
regarding wait-for-it.sh... I found that it uses nc which isn't installed by default. (apt-get install netcat).
I think u don't use it here anyway...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for that :)

1 change: 1 addition & 0 deletions docker-files/testing.list
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
deb http://deb.debian.org/debian bookworm main
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed? Might cause collisions between versions (not unlike what we have with tenacity). If debian bullseye has versions too old for what you need, consider using Ubuntu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a very new version, with a patch for critical vlun
https://security.snyk.io/vuln/SNYK-DEBIAN11-PCRE2-2808697
Check out the date

7 changes: 7 additions & 0 deletions docker-files/testing.prefs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# 100 <= P < 500: causes a version to be installed unless there is a
# version available belonging to some other distribution or the installed
# version is more recent

Package: *
Pin: release a=testing
Pin-Priority: 400
7 changes: 5 additions & 2 deletions horizon/startup/remote_config.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import requests
from typing import Optional

from tenacity import retry, wait, stop, retry_if_not_exception_type
# TODO: readd after we release the new version of opal (should be next week)
#from tenacity import retry, wait, stop, retry_if_not_exception_type
from tenacity import retry, wait, stop
from pydantic import ValidationError
from opal_common.logger import logger

Expand Down Expand Up @@ -50,7 +52,8 @@ class RemoteConfigFetcher:
organizations (which is not secure).
"""
DEFAULT_RETRY_CONFIG = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@orishavit please review it

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

'retry': retry_if_not_exception_type(InvalidPDPTokenException),
# TODO: readd after we release the new version of opal (should be next week)
# 'retry': retry_if_not_exception_type(InvalidPDPTokenException),
'wait': wait.wait_random_exponential(max=10),
'stop': stop.stop_after_attempt(10),
'reraise': True,
Expand Down
2 changes: 1 addition & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ pydantic[email]==1.8.2
starlette==0.14.2
uvicorn[standard]
requests==2.25.0
tenacity>=8.0.1
tenacity>=6.3.1
Jinja2==3.0.3
logzio-python-handler
rook
Expand Down