-
Notifications
You must be signed in to change notification settings - Fork 411
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Israel Blancas <[email protected]>
@@ -0,0 +1,16 @@ | |||
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | |||
change_type: bug_fix |
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.
@@ -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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The 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 /<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