Fix Ingress for HTTP OTLP receiver #2450
Conversation
Signed-off-by: Israel Blancas <iblancasa@gmail.com>
| @@ -0,0 +1,16 @@ | |||
| # One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | |||
| change_type: bug_fix | |||
There was a problem hiding this comment.
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.
I don't think so. Because the Ingress doesn't work otherwise.
There was a problem hiding this comment.
Take a look to #2449
There was a problem hiding this comment.
The assumption was that users setup URL rewrite annotations
There was a problem hiding this comment.
Where is that documented? Because I was not able to find anything related to that.
There was a problem hiding this comment.
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.
OK. I can revert the annotation, but we still need the other change.
There was a problem hiding this comment.
The current implementation is broken as enabling ingress results in 404 for all paths.
| @@ -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.
Can you explain why the path changes?
This will also result in a breaking change.
There was a problem hiding this comment.
The traces, for instance, are reported to <<URL>>/v1/traces. Without this, you can only access to <<URL>>. Everything you send to <URL>/<subpath> will be redirected to <URL>. Exposing the collector doesn't work right now.
There was a problem hiding this comment.
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 /<port-name> is already accepted and this regex doesn't change that because the current options are captured within the regex. ex:
/<port-name> is matched by /<port-name>(/|$)(.*)
/<port-name>/somethingelse is matched by /<port-name>(/|$)(.*)
Description:
Fixes #2449