ingress/model: Support multiple certs based on SNI

[sayboras]

Description

As per cert-selection docs, only the first certificate listed is used, this causes the bug in which the wrong certs are used in either Ingress or Gateway API.

This commit is to add serverNames, which are having values same as host name, in TLS filter chain.

Fixes: #22668

Reported-by: Nikhil Jha hi@nikhiljha.com
Signed-off-by: Tam Mach tam.mach@cilium.io

ingress/model: Support multiple certs based on SNI

Testing

Testing was done before and after changes with below manifest

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: tls-ingress
  namespace: default
spec:
  ingressClassName: cilium
  rules:
  - host: hipstershop.cilium.rocks
    http:
      paths:
      - backend:
          service:
            name: details
            port:
              number: 9080
        path: /details
        pathType: Prefix
  - host: bookinfo.tam.rocks
    http:
      paths:
      - backend:
          service:
            name: productpage
            port:
              number: 9080
        path: /
        pathType: Prefix
  tls:
  - hosts:
    - hipstershop.cilium.rocks
    secretName: demo-cert
  - hosts:
    - bookinfo.tam.rocks
    secretName: demo-tam-cert

Before

# Error in cilium agent
2022-12-11T11:08:45.476453454Z level=warning msg="Failed to update CiliumEnvoyConfig" ciliumEnvoyConfigName=cilium-ingress error="NACK received: Error adding/updating listener(s) kube-system/cilium-ingress/listener: Failed to load certificate chain from <inline>, at most one certificate of a given type may be specified\n" k8sApiVersion= k8sNamespace=kube-system k8sUID=74722a46-0962-4cf9-9983-06501a9fa0d0 subsys=k8s-watcher

# Generated cilium envoy configuration
...
      transportSocket:
        name: envoy.transport_sockets.tls
        typedConfig:
          '@type': type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.DownstreamTlsContext
          commonTlsContext:
            tlsCertificateSdsSecretConfigs:
            - name: cilium-secrets/default-demo-tam-cert
              sdsConfig:
                apiConfigSource:
                  apiType: GRPC
                  grpcServices:
                  - envoyGrpc:
                      clusterName: xds-grpc-cilium
                  transportApiVersion: V3
                resourceApiVersion: V3
            - name: cilium-secrets/default-demo-cert
              sdsConfig:
                apiConfigSource:
                  apiType: GRPC
                  grpcServices:
                  - envoyGrpc:
                      clusterName: xds-grpc-cilium
                  transportApiVersion: V3
                resourceApiVersion: V3
...

After

...
# Generated Cilium Envoy config
    - filterChainMatch:
        serverNames:
        - bookinfo.tam.rocks
        transportProtocol: tls
      filters:
      - name: envoy.filters.network.http_connection_manager
        typedConfig:
          '@type': type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
          httpFilters:
          - name: envoy.filters.http.router
          rds:
            routeConfigName: listener-secure
          statPrefix: listener-secure
          upgradeConfigs:
          - upgradeType: websocket
      transportSocket:
        name: envoy.transport_sockets.tls
        typedConfig:
          '@type': type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.DownstreamTlsContext
          commonTlsContext:
            tlsCertificateSdsSecretConfigs:
            - name: cilium-secrets/default-demo-tam-cert
              sdsConfig:
                apiConfigSource:
                  apiType: GRPC
                  grpcServices:
                  - envoyGrpc:
                      clusterName: xds-grpc-cilium
                  transportApiVersion: V3
                resourceApiVersion: V3
    - filterChainMatch:
        serverNames:
        - hipstershop.cilium.rocks
        transportProtocol: tls
      filters:
      - name: envoy.filters.network.http_connection_manager
        typedConfig:
          '@type': type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
          httpFilters:
          - name: envoy.filters.http.router
          rds:
            routeConfigName: listener-secure
          statPrefix: listener-secure
          upgradeConfigs:
          - upgradeType: websocket
      transportSocket:
        name: envoy.transport_sockets.tls
        typedConfig:
          '@type': type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.DownstreamTlsContext
          commonTlsContext:
            tlsCertificateSdsSecretConfigs:
            - name: cilium-secrets/default-demo-cert
              sdsConfig:
                apiConfigSource:
                  apiType: GRPC
                  grpcServices:
                  - envoyGrpc:
                      clusterName: xds-grpc-cilium
                  transportApiVersion: V3
                resourceApiVersion: V3
...


$ curl -s --cacert minica.pem https://hipstershop.cilium.rocks/details/1
{"id":1,"author":"William Shakespeare","year":1595,"type":"paperback","pages":200,"publisher":"PublisherA","language":"English","ISBN-10":"1234567890","ISBN-13":"123-1234567890"}

$ curl -s --cacert minica.pem https://bookinfo.tam.rocks                
<!DOCTYPE html>
<html>
  <head>
    <title>Simple Bookstore App</title>
...

[nikhiljha]

We're now running this commit (in production lol)-- appears to work :)

Thanks @sayboras !

[sayboras]

We're now running this commit (in production lol)-- appears to work :)

Thanks a lot for your issue, as well as the effort in testing the change. This got overlooked as part of another work item in #19698.

Support multiple TLSs in envoy downstream context, see #18867 (comment).

[sayboras]

/test

[tklauser]

I got assigned to review this on behalf of @cilium/operator. The change itself (and the package it touches) looks servicemesh-specific, so I've sent #22683 such that these kinds of changes get reviewed by @cilium/sig-servicemesh in the future as well.

[sayboras]

/test

[pippolo84]

Left a comment inline regarding the unit test.
Apart from that, LGTM 🚀

[pippolo84]

Since we have just one config at a time, we can avoid the sorting here and at line 63.

[sayboras]

good point, let me clean this up in subsequent PR, just to avoid re-run all tests.

[joestringer]

/test

Job 'Cilium-PR-K8s-1.24-kernel-5.4' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With VXLAN

Failure Output

FAIL: Failed to reach 192.168.56.11:80 from testserver-host-jx9pp

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.24-kernel-5.4 so I can create one.

[joestringer]

codeowners for this PR seem to include @cilium/operator , but the code itself doesn't look very relevant to that code owner. Perhaps we could drop that codeowner from these files. For this particular PR, I'm OK to accept the change with just servicemesh review (which was already provided by Fabio above, thanks!)

[sayboras]

/test-1.24-5.4

[sayboras]

/test-1.26-net-next

[sayboras]

/test-runtime

[joestringer]

Per Slack, the previous failures were:

• CI: Host firewall: unable to upgrade connection #15455
• CI: RuntimeDatapathPrivilegedUnitTests: TestFutureWatcher failure in pkg/crypto/certloader #22750
• Provisioning failure

Tam, if you re-run tests, please provide an explanation like the above that demonstrates why the PR is not at cause for the failures. We have had too many cases recently where we've merged PRs that have blatant issues in them, because PR authors have not triaged the issues and we've ignored the CI in order to merge the PRs.

[joestringer]

I've taken a stab at one step forward on #15455, at least some further investigation. It looks like #22750 has recent activity pushing the issue forwards, so I'm not as worried about that one. I'm not sure where to look to improve the reliability of the "provisioning error" cases but if it comes up more frequently then we should spent some time looking deeper into it.

Given that this is a bugfix and a release-blocker, I'll merge this now.