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.

Sign up for GitHub

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

Jump to bottom

[processor/k8sattributes] Support name:tag@digest image name format #36145

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

[processor/k8sattributes] Support name:tag@digest image name format #36145

wants to merge 2 commits into from
+141 −44

Conversation

spiffyy99
Copy link
Contributor

@spiffyy99 spiffyy99 commented Nov 2, 2024 •
edited
Loading

Description

Fixed issue with k8sattributesprocessor where 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

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Nov 2, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot added the processor/k8sattributes k8s Attributes processor label Nov 2, 2024
@spiffyy99 spiffyy99 force-pushed the fix-k8s-image-parsing branch 2 times, most recently from 8a9c1bd to 90d3a08 Compare November 2, 2024 03:33
@spiffyy99 spiffyy99 marked this pull request as ready for review November 2, 2024 03:33
@spiffyy99 spiffyy99 requested a review from a team as a code owner November 2, 2024 03:33
@spiffyy99 spiffyy99 force-pushed the fix-k8s-image-parsing branch 4 times, most recently from 09323ed to 828a728 Compare November 2, 2024 17:10
ChrsMark
ChrsMark reviewed Nov 5, 2024
View reviewed changes
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

Thank's for working on this @spiffyy99!

I think the fix is in the right direction but I left some concerns.

processor/k8sattributesprocessor/e2e_test.go Outdated Show resolved Hide resolved
processor/k8sattributesprocessor/e2e_test.go Show resolved Hide resolved
processor/k8sattributesprocessor/internal/kube/client.go Show resolved Hide resolved
processor/k8sattributesprocessor/internal/kube/client.go Show resolved Hide resolved
@spiffyy99
Copy link
Contributor Author

thanks for the review christos.

ChrsMark
ChrsMark reviewed Nov 7, 2024
View reviewed changes
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

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?

processor/k8sattributesprocessor/e2e_test.go Show resolved Hide resolved
processor/k8sattributesprocessor/e2e_test.go Outdated Show resolved Hide resolved
@spiffyy99
Copy link
Contributor Author

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?

ChrsMark
ChrsMark reviewed Nov 7, 2024
View reviewed changes
processor/k8sattributesprocessor/documentation.md Outdated
@@ -4,33 +4,33 @@

## Resource Attributes

| Name | Description | Values | Enabled |
| ---- | ----------- | ------ | ------- |
| Name | Description | Values | Enabled |
Copy link
Member

Choose a reason for hiding this comment

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

That file is automatically generated by the metadata.yaml. Please apply the edits at

opentelemetry-collector-contrib/processor/k8sattributesprocessor/metadata.yaml

Line 109 in 3f9f88b

container.image.tag:
and run make generate to update the documentation.md properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

ChrsMark
ChrsMark approved these changes Nov 8, 2024
View reviewed changes
Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

LGTM. Thank's @spiffyy99!

@TylerHelmuth please take a look

processor/k8sattributesprocessor/e2e_test.go Outdated Show resolved Hide resolved
@spiffyy99
Copy link
Contributor Author

@dmitryax @fatsheep9146 @TylerHelmuth could one of you take a look at this? thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrsMark ChrsMark ChrsMark approved these changes

@dmitryax dmitryax Awaiting requested review from dmitryax dmitryax is a code owner

@fatsheep9146 fatsheep9146 Awaiting requested review from fatsheep9146 fatsheep9146 is a code owner

@TylerHelmuth TylerHelmuth Awaiting requested review from TylerHelmuth TylerHelmuth is a code owner

Assignees

@jpkrohling jpkrohling

Labels
processor/k8sattributes k8s Attributes processor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[processor/k8sattributes] Support name:tag@digest image name format
3 participants
@spiffyy99 @ChrsMark @jpkrohling