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

Bad naming convention for service name #1360

Closed
lujiajing1126 opened this issue Jan 12, 2021 · 7 comments · Fixed by #2101
Closed

Bad naming convention for service name #1360

lujiajing1126 opened this issue Jan 12, 2021 · 7 comments · Fixed by #2101
Labels
enhancement New feature or request

Comments

@lujiajing1126
Copy link
Contributor

lujiajing1126 commented Jan 12, 2021

Currently the naming convention for the collector service is determined in this function,

func GetPortNameForGRPC(jaeger *v1.Jaeger) string {

which results in some name like grpc-http(s).

However, this will cause confusing for istio as documented in the protocol-selection section, from which I excerpt as follows,

  • By the name of the port: name: <protocol>[-<suffix>].
  • In Kubernetes 1.18+, by the appProtocol field: appProtocol: <protocol>.

The result of this bad name is severe, the istio-sidecar will automatically convert grpc protocol to http one which cannot be correctly processed by collector service.

Thus, I suggest changing the naming convention to follow the istio rules. I would like to submit a PR to resolve this issue.

@github-actions github-actions bot added the needs-triage New issues, in need of classification label Jan 12, 2021
@jpkrohling jpkrohling added enhancement New feature or request and removed needs-triage New issues, in need of classification labels Jan 12, 2021
@jpkrohling
Copy link
Contributor

Could you provide a test for the case you described? In any case, a PR would certainly be appreciated.

@lujiajing1126
Copy link
Contributor Author

Attached there are two captured tcpdump logs for the scenarios with or without istio-sidecar injected to the application pod,
(while in the same time, we do not inject istio-sidecar for jaeger-collector pod by default)

As we can see, if the istio-sidecar is injected, the grpc stream is transcoded to http request automatically.

I suppose I can use this function to reproduce the issue? Or we need a full example?

https://github.com/istio/istio/blob/68ebdc1f1eb9fe832e6ddc26962c0630f9275de1/pkg/config/kube/conversion.go#L47-L79

@jpkrohling
Copy link
Contributor

So, this is a problem only for pods with both Jaeger and Istio sidecars? @jkandasa do we have tests for this scenario?

@lujiajing1126
Copy link
Contributor Author

So, this is a problem only for pods with both Jaeger and Istio sidecars? @jkandasa do we have tests for this scenario?

Exactly. Specifically,

| Pod: Application --(spans)--> Jaeger Agent --(network intercepted by)--> Istio Sidecar | 
                    |
                    |
                    |
                    |
                    |
                    |
                    v
| Pod: (without Istio Sidecar) Jaeger Collector |

I haven't tested the case with both istio sidecar injected.

@jpkrohling
Copy link
Contributor

Was this closed as part of #1368?

@lujiajing1126
Copy link
Contributor Author

Was this closed as part of #1368?

It should have been closed. I don't know what happened to the Mergify bot.

@Stevmann
Copy link

Stevmann commented Feb 1, 2022

Hello,

I have an issue described in #1122 with port name set to "grcp-https":

"authentication handshake failed: tls: first record does not look like a TLS handshake"

If I set the port name to string which was changed in this issue; "https-grpc" then it works.


jaeger-operator.v1.29.1
OpenShift 4.6.43
Istio version OSSM_2.1.1-1.el8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants