pkg/*: use Kubernetes ServiceAccount as service identity #1990
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1990 +/- ##
==========================================
- Coverage 56.64% 56.62% -0.03%
==========================================
Files 139 140 +1
Lines 5589 5611 +22
==========================================
+ Hits 3166 3177 +11
- Misses 2420 2431 +11
Partials 3 3
Continue to review full report at Codecov.
|
This change does the following: - Uses kubernetes ServiceAccount as service identity for proxy service certificates. The service certificate CN/SAN will be of the form <svc-account>.<namespace>.<domain>. - Refactors SDS to make it easier to share code in its internal implementation. - Removes the overloaded usage of the certificate's CN for SNI. SNI (Server Name Identifier) is based on the service FQDN, while the service certificate is derived from the ServiceIdentity being used. - Refactors sds/response_test.go to make it possible to use table-driven testing to test different unit testing scenarios. Signed-off-by: Shashank Ram <shashank08@gmail.com>
Signed-off-by: Shashank Ram <shashank08@gmail.com>
| @@ -243,6 +243,7 @@ github.com/envoyproxy/go-control-plane v0.9.0/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymF | |||
| github.com/envoyproxy/go-control-plane v0.9.1-0.20191026205805-5f8ba28d4473/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4= | |||
| github.com/envoyproxy/go-control-plane v0.9.6 h1:GgblEiDzxf5ajlAZY4aC8xp7DwkrGfauFNMGdB2bBv0= | |||
| github.com/envoyproxy/go-control-plane v0.9.6/go.mod h1:GFqM7v0B62MraO4PWRedIbhThr/Rf7ev6aHOOPXeaDA= | |||
| github.com/envoyproxy/go-control-plane v0.9.7 h1:EARl0OvqMoxq/UMgMSCLnXzkaXbxzskluEBlMQCJPms= | |||
| return nil, err | ||
| } | ||
|
|
||
| // Iterate over the list of tasks and create response structs to be | ||
| // sent to the proxy that made the discovery request | ||
| sdsImpl := newSDSImpl(proxy, meshCatalog, certManager, cfg, svcList, svcAccount) |
There was a problem hiding this comment.
Not sure what impl means here or anywhere below really. I would prefer to use something that reads easily in plain english and helps you understand what's behind this variable.
No need to change this PR - but eventually would be good to add clarity so it is easy to read.
There was a problem hiding this comment.
I left a comment on what sdsImpl refers to in sds/types.go.
| if serviceForProxy != sdsCert.MeshService { | ||
| log.Error().Msgf("Proxy %s (service %s) requested service certificate %s; this is not allowed", proxy.GetCommonName(), serviceForProxy, sdsCert.MeshService) | ||
| if proxyService != sdsCert.MeshService { | ||
| log.Debug().Msgf("Proxy %s (service %s) requested service certificate %s; this is not allowed", s.proxy.GetCommonName(), proxyService, sdsCert.MeshService) |
There was a problem hiding this comment.
Debug makes sense, except I worry that it would hide this message, when I believe it should be emitted with a higher level of urgency. If error seems to high - I'd say warn.
There was a problem hiding this comment.
Warn seems appropriate for this use case, will address!
Thanks for the review, will address the nits in a follow-up coming shortly. |
Description:
This change does the following:
Uses kubernetes ServiceAccount as service identity for
proxy service certificates. The service certificate
CN/SAN will be of the form
<svc-account>.<namespace>.<domain>.Refactors SDS to make it easier to share code in its internal
implementation.
Removes the overloaded usage of the certificate's CN for SNI.
SNI (Server Name Identifier) is based on the service FQDN,
while the service certificate is derived from the ServiceIdentity
being used.
Removes SAN matching for the downstream/upstream service
in permissive mode, where there are no SMI TrafficTarget policies.
In permissive mode, every service within the mesh is allowed to
reach every other service as long as they use the correct certificate.
Refactors sds/response_test.go to make it possible to use
table-driven testing to test different unit testing scenarios.
Part of #1965
Affected area:
Please answer the following questions with yes/no.
No