-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add CA certificates to collector/query images #598
Conversation
@pavolloffay wdyt? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you write a summary of the issue solved (#485) in the commit message so that people can quickly gain context when looking at git log
?
cmd/collector/Dockerfile
Outdated
@@ -1,5 +1,11 @@ | |||
FROM alpine as certs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this to be apline:latest
, or pinned to a version?
Which version of alpine
does this command use currently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does it matter? It's a throw-away layer anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it makes it harder to debug if/when it fails.
Why use an arbitrary version of a dependency when we can control it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we pin it, we also pin the version of certificates, which is worse imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand your argument.
Are you saying that it is better to use an arbitrary version instead of using apline:latest
, or pinning to a version (as I mentioned earlier)?
Or are you saying that the command FROM alpine as certs
doesn't pull in an arbitrary version and gets the latest certificates? If so, which version of alpine
does your invocation bring in?
cmd/collector/Dockerfile
Outdated
FROM scratch | ||
|
||
COPY --from=certs /usr/share/ca-certificates/ /usr/share/ca-certificates/ | ||
COPY --from=certs /etc/ssl/ /etc/ssl/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these directories need to be copied entirely?
Can't we get away by copying just /etc/ssl/certs/ca-certificates.crt
instead, which contains a single file version of the CA certificates?
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Resolves #485
When connecting query/collector to storage backends that use TLS with certificates signed by public root CAs, it's useful to have those CA certificates in the Docker image.
On the other hand, the certificates can always be mounted from the host filesystem, which is necessary anyway when connecting to servers using self-signed certificates.
I think this is a nice to have so that people don't have to do extra work. For collector it increases the image size from 24.6MB to 25.3MB.