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

Fix "sidecar.opentelemetry.io/injected" label value sometimes being longer than 63 characters #1032

Conversation

KevinSnyderCodes
Copy link

@KevinSnyderCodes KevinSnyderCodes commented Aug 11, 2022

Fixes #1031

Use two new labels:

  • sidecar.opentelemetry.io/injected-namespace: namespace of the OpenTelemetryCollector
  • sidecar.opentelemetry.io/injected-name: name of the OpenTelemetryCollector

Since Kubernetes object names and label values have the same restrictions, we can safely use these two new labels with guarantee that it will not produce errors.

For backwards compatibility, we continue to set the sidecar.opentelemetry.io/injected label only if it does not exceed 63 characters. This ensures that it still exists when valid, but does not cause errors when invalid.

…onger than 63 characters

Use two new labels:

- `sidecar.opentelemetry.io/injected-namespace`: namespace of the `OpenTelemetryCollector`
- `sidecar.opentelemetry.io/injected-name`: name of the `OpenTelemetryCollector`

Since Kubernetes object names and label values have the same restrictions, we can safely use these two new labels with guarantee that it will not produce errors.

For backwards compatibility, we continue to set the `sidecar.opentelemetry.io/injected` label _only if_ it does not exceed 63 characters. This ensures that it still exists when valid, but does not cause errors when invalid.
@KevinSnyderCodes KevinSnyderCodes requested a review from a team August 11, 2022 01:43
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 11, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: KevinSnyderCodes (3702fbf)
  • ✅ login: jaronoff97 / name: Jacob Aronoff (11a19a5)

@pavolloffay
Copy link
Member

I would like to understand how the operator uses sidecar.opentelemetry.io/injected". The only reference to this label I found is

pod.Labels[label] = fmt.Sprintf("%s.%s", otelcol.Namespace, otelcol.Name)
- which just sets the label, but I could not find a reading of that label in the codebase.

@jpkrohling since you are the author, could you please take a look and explain what use-cases sidecar.opentelemetry.io/injected solves?

@KevinSnyderCodes
Copy link
Author

KevinSnyderCodes commented Aug 11, 2022

At the very least, can we make it so the label is not set if the value exceeds 63 characters? As it stands the injector can produce an invalid manifest, which it should never do.

@pavolloffay
Copy link
Member

pavolloffay commented Aug 15, 2022

If the label is not being used, we could stop setting it altogether.

@KevinSnyderCodes
Copy link
Author

KevinSnyderCodes commented Aug 15, 2022

Sure, but we don't know how long it's going to take to get an answer to that question. A fix to the bug that would get us unblocked without breaking backwards compatibility would be a nice first step IMO.

@pavolloffay
Copy link
Member

cc) @jpkrohling could you please take a look at the question above?

@KevinSnyderCodes
Copy link
Author

@pavolloffay How long are we willing to wait for a response before introducing a backwards-compatible fix? This continues to impact our Kubernetes environments.

@pavolloffay
Copy link
Member

I saw @jpkrohling doing some work in this repository recently, but maybe he missed this one.

@yuriolisa
Copy link
Contributor

@KevinSnyderCodes, did you have the opportunity to check it out?

@jaronoff97
Copy link
Contributor

@KevinSnyderCodes I just rebased your PR but was thinking you could actually just use our naming package's Truncate function seen here rather than only setting the label name. Let me know if you have time / want to work on that, if not I can take on this PR.

RoVernekar added a commit to RoVernekar/opentelemetry-operator that referenced this pull request Oct 19, 2023
RoVernekar added a commit to RoVernekar/opentelemetry-operator that referenced this pull request Oct 19, 2023
RoVernekar added a commit to RoVernekar/opentelemetry-operator that referenced this pull request Oct 20, 2023
RoVernekar added a commit to RoVernekar/opentelemetry-operator that referenced this pull request Oct 20, 2023
@KevinSnyderCodes
Copy link
Author

@jaronoff97 That sounds good to me. I no longer use this package in my work. Feel free to take over this PR 👍

RoVernekar added a commit to RoVernekar/opentelemetry-operator that referenced this pull request Oct 25, 2023
RoVernekar added a commit to RoVernekar/opentelemetry-operator that referenced this pull request Oct 30, 2023
RoVernekar added a commit to RoVernekar/opentelemetry-operator that referenced this pull request Nov 6, 2023
jaronoff97 added a commit that referenced this pull request Nov 8, 2023
@jaronoff97
Copy link
Contributor

I think was fiixed by this #2250

@jaronoff97 jaronoff97 closed this Nov 28, 2023
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Label "sidecar.opentelemetry.io/injected" value sometimes longer than 63 characters
4 participants