-
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
Fix Ingress for HTTP OTLP receiver #2450
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
change_type: bug_fix | ||
|
||
# The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action) | ||
component: operator | ||
|
||
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
note: Fix the path and annotations needed for the Ingress to expose the OTLP HTTP receiver. | ||
|
||
# One or more tracking issues related to the change | ||
issues: [2449] | ||
|
||
# (Optional) One or more lines of additional information to render under the primary note. | ||
# These lines will be padded with 2 spaces and then inserted directly into the document. | ||
# Use pipe (|) for multiline entries. | ||
subtext: |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,11 +53,20 @@ func Ingress(params manifests.Params) (*networkingv1.Ingress, error) { | |
rules = createSubdomainIngressRules(params.OtelCol.Name, params.OtelCol.Spec.Ingress.Hostname, ports) | ||
} | ||
|
||
ingressAnnotations := params.OtelCol.Spec.Ingress.Annotations | ||
if ingressAnnotations == nil { | ||
ingressAnnotations = make(map[string]string) | ||
} | ||
|
||
if _, ok := ingressAnnotations["nginx.ingress.kubernetes.io/rewrite-target"]; !ok { | ||
ingressAnnotations["nginx.ingress.kubernetes.io/rewrite-target"] = "/$2" | ||
} | ||
|
||
return &networkingv1.Ingress{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: naming.Ingress(params.OtelCol.Name), | ||
Namespace: params.OtelCol.Namespace, | ||
Annotations: params.OtelCol.Spec.Ingress.Annotations, | ||
Annotations: ingressAnnotations, | ||
Labels: map[string]string{ | ||
"app.kubernetes.io/name": naming.Ingress(params.OtelCol.Name), | ||
"app.kubernetes.io/instance": fmt.Sprintf("%s.%s", params.OtelCol.Namespace, params.OtelCol.Name), | ||
|
@@ -78,7 +87,7 @@ func createPathIngressRules(otelcol string, hostname string, ports []corev1.Serv | |
for i, port := range ports { | ||
portName := naming.PortName(port.Name, port.Port) | ||
paths[i] = networkingv1.HTTPIngressPath{ | ||
Path: "/" + port.Name, | ||
Path: "/" + port.Name + "(/|$)(.*)", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain why the path changes? This will also result in a breaking change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The traces, for instance, are reported to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this fix will only work for nginx right? for anything using a default ingress provider this will have no effect? @pavolloffay this technically isn't a breaking change because we choose prefix currently, so anything after
|
||
PathType: &pathType, | ||
Backend: networkingv1.IngressBackend{ | ||
Service: &networkingv1.IngressServiceBackend{ | ||
|
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 is a correct behaviour of the operator.
Automatically putting ingress specific annotation should be a new feature
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 don't think so. Because the Ingress doesn't work otherwise.
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.
Take a look to #2449
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.
The assumption was that users setup URL rewrite annotations
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.
Where is that documented? Because I was not able to find anything related to that.
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.
Unfortunately, It's not documented well. But that is why the annotations field is in the ingress spec.
I think we should try to make this work as non breaking change. Note that the ingress can as well expose the receivers in "subdomain" mode. We should make sure the annotation will not break it.
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.
OK. I can revert the annotation, but we still need the other change.
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.
The current implementation is broken as enabling ingress results in 404 for all paths.