Add tcp_tls protocol to Istio traffic management#6280
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vadimeisenbergibm The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report
@@ Coverage Diff @@
## master #6280 +/- ##
=======================================
- Coverage 68% 67% -<1%
=======================================
Files 345 345
Lines 30569 30589 +20
=======================================
- Hits 20499 20493 -6
- Misses 9250 9279 +29
+ Partials 820 817 -3
Continue to review full report at Codecov.
|
|
/retest |
| Protocol: string(model.ProtocolHTTPS), | ||
| Name: fmt.Sprintf("https-443-ingress-%s-%s", ingress.Name, ingress.Namespace), | ||
| Protocol: string(model.ProtocolTLSWithSNI), | ||
| Name: fmt.Sprintf("tls-with-sni-443-ingress-%s-%s", ingress.Name, ingress.Namespace), |
There was a problem hiding this comment.
? why change this? gateway is tls terminating entity.
pilot/pkg/model/service.go
Outdated
| // This is the default protocol for a service port. | ||
| ProtocolTCP Protocol = "TCP" | ||
| // ProtocolTLSWithSNI declares that the port carries TLS traffic with SNI | ||
| ProtocolTLSWithSNI Protocol = "TLS_with_SNI" |
pilot/pkg/model/service.go
Outdated
| case "https": | ||
| return ProtocolHTTPS | ||
| case "tls_with_sni": | ||
| return ProtocolTLSWithSNI |
| if !listenerTypeMap[listenerMapKey].IsTCP() || servicePort.Protocol != model.ProtocolHTTPS || !service.MeshExternal { | ||
| if !listenerTypeMap[listenerMapKey].IsTCP() || | ||
| (servicePort.Protocol != model.ProtocolHTTPS && | ||
| servicePort.Protocol != model.ProtocolTLSWithSNI) || |
There was a problem hiding this comment.
create a convenience function port.IsTLS() and this becoems shorter
rshriram
left a comment
There was a problem hiding this comment.
This is nice. thanks for picking up on the TODO.
TLSWithSNI is a bit clunky. TCP_TLS i feel is short and sweet.
You need to add logic to service entry to add a new protocol TCP_TLS and update the service entry conversion code to reflect this protocol.
|
Btw, we already have TCP-TLS as a protocol in the service entry (https://github.com/istio/api/blob/master/networking/v1alpha3/gateway.proto#L301). this just needs to be parsed. |
|
@rshriram |
it is assumed that this protocol contains SNI as part of the TLS handshake
|
/retest |
|
@vadimeisenbergibm: The following tests failed, say
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Almost all clients use SNI. And TCP_TLS is no different from plain TCP. In other words, TLS implies SNI for us and nothing else. We are not terminating TLS connections inside the mesh, on behalf of the application. Its all Istio CA stuff. |
Add
tls_with_snias a protocol different fromhttpsandtcp. It should be handled by Istio as opaque TLS with SNI, for which no TLS termination is performed. Note that if no TLS termination is performed, the protocol on top of TLS cannot be known - the communication is encrypted. For a proxy, it does not matter if the protocol on top TLS is HTTP or Mongo, it is opaque for the proxy since the proxy cannot decrypt it. Without TLS termination, treatment of HTTPS or Mongo/TLS is exactly the same.Note the distinction of TLS without SNI and
tls_with_sni. For TLS without SNI, the proxy can probably do nothing other than to treat it as opaque TCP.The proposed classification and treatment of the protocols on top of TLS:
httpsis known to be HTTP on top of TLS with SNI. Optional TLS termination by Istio and then treating the protocol as HTTP.tls_with_sniis unknown protocol on top of TLS with SNI. Treated as an opaque TCP with known SNI, including matching by SNI for routing.tcp, an opaque TCP without SNI and without matching by SNI.