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

The "otlp-http-legacy" port breaks ingresses #1954

Closed
acramsay opened this issue Jul 21, 2023 · 8 comments · Fixed by #1971
Closed

The "otlp-http-legacy" port breaks ingresses #1954

acramsay opened this issue Jul 21, 2023 · 8 comments · Fixed by #1971
Labels
area:collector Issues for deploying collector bug Something isn't working

Comments

@acramsay
Copy link

I'm trying to use the otel operator to manage a collector ingress automatically. Unfortunately, the ingress does not work. At least, it does not work when backed by ALB controller in AWS. Here's a relevant part of my otelcol resource:

apiVersion: opentelemetry.io/v1alpha1
kind: OpenTelemetryCollector
metadata: ...
spec:
  ingress:
    hostname: gateway.redacted.com
    type: ingress
  config: |
    receivers:
      otlp:
        protocols:
          grpc:
          http:
            cors:
              allowed_origins:
                - "https://*"

And here are snippets of the service and ingress resources created by the operator.

apiVersion: v1
kind: Service
metadata: ...
spec:
  ...
  ports:
  - appProtocol: grpc
    name: otlp-grpc
    port: 4317
    protocol: TCP
    targetPort: 4317
  - appProtocol: http
    name: otlp-http
    port: 4318
    protocol: TCP
    targetPort: 4318
  - appProtocol: http
    name: otlp-http-legacy
    port: 55681
    protocol: TCP
    targetPort: 4318
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata: ...
spec:
  rules:
  - host: gateway.redacted.com
    http:
      paths:
      - backend:
          service:
            name: opentelemetry-deployment-collector
            port:
              name: otlp-grpc
        path: /otlp-grpc
        pathType: Prefix
      - backend:
          service:
            name: opentelemetry-deployment-collector
            port:
              name: otlp-http
        path: /otlp-http
        pathType: Prefix
      - backend:
          service:
            name: opentelemetry-deployment-collector
            port:
              name: otlp-http-legac
        path: /otlp-http-legacy
        pathType: Prefix

Notice that the service's .spec.ports[2].name is otlp-http-legacy while the ingress' .spec.rules[0].http.paths[2].backend.service.port.name is otlp-http-legac. In other words, the name is truncated in the ingress, missing the final y character.

I believe that I found the source of this issue in collector/reconcile/ingress.go, which limits the name to 15 characters. However, the service is parsed and limited to 63 characters.

Is there a reason that the ingress string is limited to 15 characters? Can we simply change it to 63 characters? Alternatively, can we limit the string in the service resource to the same size?

@acramsay
Copy link
Author

Note issue 4565 where it was decided to remove support for the legacy port. Maybe it makes more sense to continue PR 4916 and just remove this port from the ingress and service resources?

@TylerHelmuth TylerHelmuth added bug Something isn't working area:collector Issues for deploying collector labels Jul 24, 2023
@TylerHelmuth
Copy link
Member

We should remove the legacy http port stuff.

@pavolloffay
Copy link
Member

hi @lodrus did you manage to use Ingress created by the operator in your setup? For example did you mange to report to that ingress OTLP or OTLP HTTP data? If so you could please share your config and any additional config that you applied?

@acramsay
Copy link
Author

acramsay commented Aug 2, 2023

I did not find a way to use the operator managed ingress.

I have a tiny helm chart to deploy the otel collector resource. I simply added an ingress to that chart as a short term solution until we upgrade to a new release.

Edit: A little clarification. I took this approach because after looking through the operator code, I didn't think it was possible to work around the issue without changing the operator code.

@pavolloffay
Copy link
Member

@lodrus could you please point out what changes you needed on the ingress managed by the operator?

@acramsay
Copy link
Author

acramsay commented Aug 3, 2023

Here's the same yaml I originally posted but I removed ~90% of the content.

apiVersion: v1
kind: Service
spec:
  ports:
  - name: otlp-http-legacy

---

piVersion: networking.k8s.io/v1
kind: Ingress
spec:
  rules:
  - http:
      paths:
      - backend:
          service:
            port:
              name: otlp-http-legac

Note that both objects are created by the otel operator. There are two basic problems here. First is the existence of code to handle legacy ports which is solved by #1971. Unfortunately, this only masks the second issue: the two strings highlighted in this yaml need to match exactly but they do not.

In the service resource, the spec.ports[].name string is limited to 63 characters. But that same string in the ingress resource (spec.rules[].http.paths[[.backend.service.port.name) is truncated by the operator to 15 characters. If the service port name can be as long as 63 characters, then the ingress should allow this as well.

@pavolloffay
Copy link
Member

I think the port name in the ingress should not be used, it should rather use the port number to match with the service. WDYT?

@acramsay
Copy link
Author

acramsay commented Aug 3, 2023

Personally, I think either solution would be fine. I suppose the named port (e.g. otlp-http-legacy) is a bit more of a clear connection to the service. Is either a numbered or named port generally preferred over the other?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:collector Issues for deploying collector bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants