-
Notifications
You must be signed in to change notification settings - Fork 414
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
[proposal] Use golang based cp tool rather than busybox in autoinstrumentation images. #1727
Conversation
args := os.Args[1:] | ||
|
||
if len(args) < 2 { | ||
return errors.New("Not enough arguments given.") |
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.
can we make this more user-friendly? How many arguments are expected?
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.
Still working on this and nearly done. I did upload another commit that is unrelated to this.
@@ -25,6 +25,7 @@ jobs: | |||
grep -v '\#' versions.txt | grep opentelemetry-collector | awk -F= '{print "OTELCOL_VERSION="$2}' >> $GITHUB_ENV | |||
grep -v '\#' versions.txt | grep targetallocator | awk -F= '{print "TARGETALLOCATOR_VERSION="$2}' >> $GITHUB_ENV | |||
grep -v '\#' versions.txt | grep operator-opamp-bridge | awk -F= '{print "OPERATOR_OPAMP_BRIDGE_VERSION="$2}' >> $GITHUB_ENV | |||
grep -v '\#' versions.txt | grep autoinstrumentation-utils | awk -F= '{print "AUTO_INSTRUMENTATION_UTILS_VERSION="$2}' >> $GITHUB_ENV |
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 am not sure why this is needed the utils version is not need to be listed in the root versions.txt
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.
We probably don't need this, I was doing my best trying to understand the publish logic. WiIl fix later.
autoinstrumentation/utils/Dockerfile
Outdated
@@ -0,0 +1,11 @@ | |||
# This utils image is intended to be used as a base image for OpenTelemetry Operator |
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.
It's not a base image. The docker image is used just to get the tool into the base image.
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.
Good point. After some more thought, I think we could rework this image and use it as a base image and cut down on lines of code in several areas. Checking...
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 was able to get change the utils image name to cp. I was able to make it a base image which I believe makes understandability and adoptability much easier. See: autoinstrumentation/cp/Dockerfil
Related Code Changes
autoinstrumentation/utils/Dockerfile
Outdated
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 we rename the directory to cp
?
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.
Started the changes for this, still need to add more soon.
id: meta | ||
uses: docker/metadata-action@v4 | ||
with: | ||
images: ghcr.io/open-telemetry/opentelemetry-operator/autoinstrumentation-utils |
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 would prefer to be more explicit here e.g. ghcr.io/open-telemetry/opentelemetry-operator/autoinstrumentation-cp
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.
Depending on where the final code for this lives, this can vary. If we keep the cp code here in this repo, we will use the proposed name.
autoinstrumentation/utils/Dockerfile
Outdated
# This utils image is intended to be used as a base image for OpenTelemetry Operator | ||
# autoinstrumentation images. The utils will allow autoinstrumentation packages to be | ||
# copied (via a go based cp command) from the init container to the final destination volume. | ||
FROM golang as utils |
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.
This Dockerfile still results in an image that is distributed with the golang
base image right? What is golang
built on top of? I think this would need to be a multistage build to avoid this where the last stage is FROM scratch
.
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.
You are correct. I refactored the code and it now lives in autoinstrumentation/cp/Dockerfile.
I would really like us to follow a similar pattern shown in this file whether or not the code is based off of rust or golang. This way downstream auto-instrumentation vendors don't have to worry about the implementation because it is abstracted enough for them not to care about it.
Let me know your thoughts if you have them.
Related Code Changes
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.
Only as a reference: #1698 (comment)
|
Attaching CVE (vulnerability info):
|
@jvoravong what is the status of this PR? |
I haven't been able to commit more time to this work. Going to close this PR for now. Will open a new PR in the future if I can work on it later. |
Updates: #1600
As mentioned in previous SIG meeting and related issues, we would like to propose a replacement to the busybox base image used in the auto-instrumentation images. More work would still have to be done, but I think we have enough here start discussions. Happy to answer questions or take improvements.
This PR Proposal: