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

Operator configured service port names do not match istio convention #1306

Open
frzifus opened this issue Dec 7, 2022 · 8 comments
Open
Labels
area:collector Issues for deploying collector help wanted Extra attention is needed

Comments

@frzifus
Copy link
Member

frzifus commented Dec 7, 2022

Additional information

ServicePort names generated by the operator are not compatible with the istio naming convention. This leads to problems when using gRPC. Envoy (istio sidecar) uses the port name to determine the protocol used. For example if TLS traffic is transmitted or not. According to convention, a port handling tls traffic should have the prefix tls- or https-. Alternatively, since kubernetes 1.20 a new field appProtocol in the servicePort can be used to provide protocol information.

What is the problem?

Envoy as well as HAProxy [#902] fail the tls handshake if it is unclear what type of traffic it is. We saw the same issue on the jaeger-operator#2101 project.
authentication handshake failed: tls: first record does not look like a TLS handshake

How are port names generated?

Currently, these are generated based on the collector configuration (${receiver}-${suffix}).

# in this case, we have two ports, named: examplereceiver and examplereceiver-settings
receivers:
  examplereceiver:
    endpoint: 0.0.0.0:12345
  examplereceiver/settings:
    endpoint: 0.0.0.0:12346

In the case of otlp, it is only possible to use tls or grpc as prefix or appProtocol with a work around. Therefore the desired port name or protocol must be specified in the CR.

Example

apiVersion: opentelemetry.io/v1alpha1
kind: OpenTelemetryCollector
metadata:
  name: simplest
spec:
  mode: "deployment"
  ports:
    - name: grpc-otlp
      appProtocol: grpc
      port: 4317
  config: |
    receivers:
      otlp:
        protocols:
          grpc:
    exporters:
      logging:
    service:
      pipelines:
        traces:
          receivers: [otlp]
          processors: []
          exporters: [logging]

Possible solutions

One of the easiest ways to do this would be to reverse the orientation of the generated names.
E.g.: ${receiver}-${info} => ${info}-${receiver}
However, this would be a breaking change. Since manually created ingress entries or similar would no longer work.

In the case of otlp, the configuration provides all the necessary information to use the appProtocol field. If the insecure=false it is tls traffic. In case of insecure=true the specified protocol can be used (e.g. gRPC or HTTP). However, I am not sure that all receivers provide these configurations.

@frzifus frzifus added the help wanted Extra attention is needed label Jan 19, 2023
@JaredTan95
Copy link
Member

same isssue meet in open-telemetry/opentelemetry-helm-charts#589, and I'd like to support it in otel-operator, pls assign it to me.

@pavolloffay
Copy link
Member

@frzifus was this resolved?

@pavolloffay pavolloffay added the area:collector Issues for deploying collector label Mar 13, 2023
@frzifus
Copy link
Member Author

frzifus commented Mar 13, 2023

nope - might be a topic for the SIG call.

@jaronoff97
Copy link
Contributor

@frzifus should we do this with v2?

@jaronoff97 jaronoff97 added this to the v1alpha2 CRD release milestone Nov 28, 2023
@frzifus
Copy link
Member Author

frzifus commented Nov 29, 2023

Ive to check if that is still an issue. Since we dropped v1.19 support as far as I understand appProtocol should be enough.

https://kubernetes.io/docs/concepts/services-networking/service/#application-protocol

@jaronoff97
Copy link
Contributor

oh so can i close this?

@fujin
Copy link

fujin commented Dec 7, 2023

I just hit this and used appProtocols/custom names as described in the issue via the ports spec to workaround

forgive me, but it would be nice if the port names were istio naming convention/protocol detection compatible without needing to :-)

@pavolloffay pavolloffay removed this from the Collector v1beta1 CRD release milestone Apr 5, 2024
@GeniyX
Copy link

GeniyX commented May 2, 2024

I think that changing the port name may cause backward compatibility problems.
However, you can leave the port name unchanged and add the appProtocol field to the generator.

According to the documentation https://istio.io/latest/docs/ops/configuration/traffic-management/protocol-selection/#explicit-protocol-selection if the appProtocol value is present, the port name is ignored.

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 help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

6 participants