[processor/k8sattributes] Support name:tag@digest image name format #36145
Conversation
github-actions
bot
requested review from
dmitryax,
fatsheep9146 and
TylerHelmuth
spiffyy99
force-pushed
the
fix-k8s-image-parsing
branch
2 times, most recently
from
8a9c1bd to
90d3a08
Compare
spiffyy99
force-pushed
the
fix-k8s-image-parsing
branch
4 times, most recently
from
09323ed to
828a728
Compare
There was a problem hiding this comment.
Thank's for working on this @spiffyy99!
I think the fix is in the right direction but I left some concerns.
|
thanks for the review christos. |
There was a problem hiding this comment.
Overall looks good. Could we add a section in the docs describing the logic from https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/36145/files#r1832085419?
Already referenced it here: https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/36145/files#diff-f9652f665e3a05f0768f863df6d7543c56b7cf671e4d2be1d7086c5d73e7cc3dR12 any other section you think it would be good to mention this? |
spiffyy99
force-pushed
the
fix-k8s-image-parsing
branch
from
a51981f to
6b95457
Compare
spiffyy99
force-pushed
the
fix-k8s-image-parsing
branch
from
6b95457 to
3859c70
Compare
spiffyy99
force-pushed
the
fix-k8s-image-parsing
branch
from
e09c207 to
25d7678
Compare
spiffyy99
force-pushed
the
fix-k8s-image-parsing
branch
from
d9ee59f to
35b2a0b
Compare
|
@dmitryax @fatsheep9146 @TylerHelmuth could one of you take a look at this? thanks! |
| // parseAttributesFromImage parses the image name and tag for differently-formatted image names. | ||
| // returns "latest" as the default if tag not present. also checks if the image contains a digest. | ||
| // if it does, no latest tag is assumed. | ||
| func parseNameAndTagFromImage(image string) (name, tag string, err error) { |
There was a problem hiding this comment.
Should we use the already available internal docker.ParseImageName function instead?
I would prefer to all components use the same strategy for common functionalities. Note that the shared function cannot parse images with digest at the moment and should be fixed beforehand: #36279
There was a problem hiding this comment.
Having a single implementation makes sense.
Also I think the package should be renamed to container and not docker since it doesn't only cover docker.
There was a problem hiding this comment.
Since this is a private implementation I am ok moving forward with this approach for now, but I agree it should used the shared function. Created #36418
There was a problem hiding this comment.
@spiffyy99 I was gonna merge this before the release but now I want to make our test coverage is still handling when no digest is set. If you get to using docker.ParseImageName before this is merged I'll close the issue.
There was a problem hiding this comment.
sure, I can update.
There was a problem hiding this comment.
actually, looking at this further, wouldn't it be better to use the official docker api code for things like this? i'm sure this regex is correct as well but I'd think that the official one is a better practice (reference.Parse).
another option is we can update docker.ParseImageName to use reference.Parse, but it seems like that function is used in a bunch
i'm not the one maintaining this though, so I don't feel strongly. just pointing it out
There was a problem hiding this comment.
@spiffyy99 your updates to the test give me confidence that we set the new attribute correctly, but are there still tests for latest or v0.112.0 (tests for when no digest is used)?
@TylerHelmuth yes, there are tests with only the version, and no version. this is also in the e2e tests. we have cases with no tag or digest, tag only, digest only, and both. |
spiffyy99
force-pushed
the
fix-k8s-image-parsing
branch
from
f9ecf48 to
115ec54
Compare
|
@spiffyy99 thanks for the fix! |
…pen-telemetry#36145) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Fixed issue with `k8sattributesprocessor` where digest is not properly separated from tag if both are present. used official docker library to perform parsing. <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes open-telemetry#36131 <!--Describe what testing was performed and which tests were added.--> #### Testing unit tests integration/e2e tests <!--Describe the documentation added.--> #### Documentation N/A. Fields are already described correctly, this is simply fixing parsing logic <!--Please delete paragraphs that you did not use before submitting.-->
…pen-telemetry#36145) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Fixed issue with `k8sattributesprocessor` where digest is not properly separated from tag if both are present. used official docker library to perform parsing. <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes open-telemetry#36131 <!--Describe what testing was performed and which tests were added.--> #### Testing unit tests integration/e2e tests <!--Describe the documentation added.--> #### Documentation N/A. Fields are already described correctly, this is simply fixing parsing logic <!--Please delete paragraphs that you did not use before submitting.-->
…pen-telemetry#36145) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Fixed issue with `k8sattributesprocessor` where digest is not properly separated from tag if both are present. used official docker library to perform parsing. <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes open-telemetry#36131 <!--Describe what testing was performed and which tests were added.--> #### Testing unit tests integration/e2e tests <!--Describe the documentation added.--> #### Documentation N/A. Fields are already described correctly, this is simply fixing parsing logic <!--Please delete paragraphs that you did not use before submitting.-->
Description
Fixed issue with
k8sattributesprocessorwhere digest is not properly separated from tag if both are present. used official docker library to perform parsing.Link to tracking issue
Fixes #36131
Testing
unit tests
integration/e2e tests
Documentation
N/A. Fields are already described correctly, this is simply fixing parsing logic