fix(operator): truncate sidecar pod injected label to 63 chars #2250
Conversation
pkg/sidecar/pod.go
Outdated
| @@ -53,7 +53,7 @@ func add(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetryCo | |||
| if pod.Labels == nil { | |||
| pod.Labels = map[string]string{} | |||
| } | |||
| pod.Labels[label] = fmt.Sprintf("%s.%s", otelcol.Namespace, otelcol.Name) | |||
| pod.Labels[label] = naming.Truncate("%s.%s", 63, otelcol.Namespace, otelcol.Name) | |||
There was a problem hiding this comment.
suggestion: instead of truncating here, could make a method called naming.SidecarLabel in the naming package?
There was a problem hiding this comment.
Could you also rename the label variable which IMO is a very nondescript name 😓
There was a problem hiding this comment.
How about naming.PodInstanceLabel? The same logic is used in a few other places so I figured we could reuse this function but couldn't think of a great name.
Also I renamed label to injectedLabel. How's that?
|
@RoVernekar youll need to fill out the CLA before we can merge |
RoVernekar
force-pushed
the
truncate-pod-label
branch
2 times, most recently
from
c573d51 to
74211ad
Compare
Sorry for the delay, should be good now. |
internal/naming/main.go
Outdated
| @@ -124,3 +124,8 @@ func ServiceMonitor(otelcol string) string { | |||
| func TargetAllocatorServiceAccount(otelcol string) string { | |||
| return DNSName(Truncate("%s-targetallocator", 63, otelcol)) | |||
| } | |||
|
|
|||
| // PodInstanceLabel returns a label value containing the namespace and instance name. | |||
| func PodInstanceLabel(namespace string, otelcol string) string { | |||
There was a problem hiding this comment.
is Pod the right prefix we want to use? Shouldn't this be just InstanceLabel ?
There was a problem hiding this comment.
I would revert other changes in this PR . The naming/main.go is used for k8s object names.
Or expose this function in the labels.go
There was a problem hiding this comment.
Okay I went back to the original changes. Let me know if you would prefer I expose this in labels.go instead.
There was a problem hiding this comment.
@pavolloffay can you take another look at this when you get a chance?
pkg/sidecar/pod.go
Outdated
| @@ -53,7 +53,7 @@ func add(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetryCo | |||
| if pod.Labels == nil { | |||
| pod.Labels = map[string]string{} | |||
| } | |||
| pod.Labels[label] = fmt.Sprintf("%s.%s", otelcol.Namespace, otelcol.Name) | |||
| pod.Labels[injectedLabel] = naming.PodInstanceLabel(otelcol.Namespace, otelcol.Name) | |||
There was a problem hiding this comment.
review note: this is the fix of the issue
RoVernekar
force-pushed
the
truncate-pod-label
branch
from
146a9d5 to
e3f5ffc
Compare
RoVernekar
force-pushed
the
truncate-pod-label
branch
from
e3f5ffc to
b274bf1
Compare
RoVernekar
force-pushed
the
truncate-pod-label
branch
from
b274bf1 to
f2f3bd1
Compare
…telemetry#2250) Refs: open-telemetry#1032 Co-authored-by: Jacob Aronoff <[email protected]>
Description:
Truncate
sidecar.opentelemetry.io/injectedsidecar pod label to 63 characters since Kubernetes labels must be 63 characters or less.Link to tracking Issue:
resolves #1031
Testing:
Tested with a unit test in
pod_test.go.Documentation: