Migration path for Service Identity #3170

draychev · 2021-04-15T02:06:02Z

This PR replaces the usage of K8sServiceAccount with ServiceIdentity in most functions in pkg/catalog and pkg/envoy.

As part of #2218 we determined that the ServiceAccount primitive is something specific to Kubernetes, and neither Mash Catalog, nor Envoy xDS should be Kubernetes specific.

With #3179 we created K8sServiceAccount.ToServiceIdentity() and ServiceIdentity.ToK8sServiceAccount(), which allow for a service account to be converted to a service identity and vice versa. This is not a problem in the current state of OSM - there is only one kind of ServiceIdentity - a Kubernetes ServiceAccount.

This PR uses ToK8sServiceAccount() and ToServiceIdentity() as crutches to avoid making too many changes - it is already very difficult to review these large changes. Both of these functions are annotated with a // TODO and a link to #2218. These should be used sparingly and perhaps even completely removed before #2218 is completed.

This is part of:

Use ServiceIdentity type in catalog APIs instead of service.K8sServiceAccount Use ServiceIdentity type in catalog APIs instead of service.K8sServiceAccount #2218

Signed-off-by: Delyan Raychev <delyan.raychev@microsoft.com>

shashankram

Overall this is my understanding from the change:

XDS extracts k8s service account from XDS cert
While making calls to catalog, XDS convert k8s service account to the service identity FQDN (trust domain hardcoded to cluster.local)
Catalog implementation performs conversions between the 2 types (k8s service account and service identity FQDN) as necessary

I was wondering what you had in mind for the following:

Directly retrieve the ServiceIdentity from the proxy XDS cert, so this would skip 1 above
Converting from ServiceAccount -> ServiceIdentity is not precise because the trust domain information is lost. Do you see this to work differently for multicluster?

shashankram · 2021-04-16T16:19:29Z

pkg/catalog/inbound_traffic_policies.go

@@ -61,15 +63,17 @@ func (mc *MeshCatalog) listInboundPoliciesFromTrafficTargets(upstreamIdentity id
 // listInboundPoliciesForTrafficSplits loops through all SMI TrafficTarget resources and returns inbound policies for apex services based on the following conditions:
 // 1. the given upstream identity matches the destination specified in a TrafficTarget resource
 // 2. the given list of upstream services are backends specified in a TrafficSplit resource
-func (mc *MeshCatalog) listInboundPoliciesForTrafficSplits(upstreamIdentity identity.K8sServiceAccount, upstreamServices []service.MeshService) []*trafficpolicy.InboundTrafficPolicy {
+func (mc *MeshCatalog) listInboundPoliciesForTrafficSplits(upstreamIdentity identity.ServiceIdentity, upstreamServices []service.MeshService) []*trafficpolicy.InboundTrafficPolicy {


Could you add in comments what upstreamIdentity should look like: foo.bar vs foo.bar.cluster.local.

This comment applies to any function that takes a ServiceIdentity type as an argument.

@shashankram I do not agree that there should be such comment. We created ServiceIdentity as a type to give us the flexibility to define these things in the type itself.

I comment with the sample string would only be needed because ServiceIdentity is still just a string alias. When we design and implement something more complex type ServiceIdentity struct we would have this information encoded in the struct.

So in the short term I could go around sprinkling comments, but these would be relevant for a short period of time while ServiceIdentity is a string.

I think there needs to be a comment, otherwise authors not familiar with the concept of ServiceIdentity are going to wonder what should be passed as an argument to functions that accept a ServiceIdentity type (foo.bar vs foo.bar.cluster.local). Even just adding this to the interface will suffice for now, so its clear what the catalog interface expects at this moment.

@shashankram I'll add the comment and look forward to sophisticating ServiceIdentity from a string to a struct, which would be explicit about the components of this string (now implied and relying on comments). We can discuss and tackle this going forward with the following GitHub Issue:

identity: Change (sophisticate) ServiceIdentity from a string to a struct{} so it can capture more context identity: Change (sophisticate) ServiceIdentity from a string to a struct{} so it can capture more context #3188

Thanks @draychev, the new issues make sense.

draychev · 2021-04-16T17:42:40Z

@shashankram, thank you so much for taking the time to go through this monster PR. Sincere apologies for the size of it - it is tough to chop it up into pieces.

My responses are inline:

Overall this is my understanding from the change:

XDS extracts k8s service account from XDS cert

This is mostly to preserve the current state of the system. I think of it as 0.5 steps forward. I want to avoid making drastic changes all at once. The Envoy xDS cert points to a K8s Service Account. I want to leave that as is and debate/design it separatel. So decided to preserve the flow of cert -> CN -> K8sServiceAccount -> ServiceIdentity -- just to make a small step forward to overhauling the types.

While making calls to catalog, XDS convert k8s service account to the service identity FQDN (trust domain hardcoded to cluster.local)

I believe this preserves the current state of the system as of release-v0.8. Everything is based on a concatenation of.

Service Account Name
Service Account Namespace
cluster.local

Catalog implementation performs conversions between the 2 types (k8s service account and service identity FQDN) as necessary

And the point of this is just to provide a bridge between the two types co-existing. (I suspect they will continue to co-exist in some shape). This is just to allow for as small PR as possible to change the types of the Mesh Catalog interfaces, which we agree should not be Kubernetes aware.

I was wondering what you had in mind for the following:

Directly retrieve the ServiceIdentity from the proxy XDS cert, so this would skip 1 above

I agree this needs to happen at some point. I made this new GitHub issue, and summarized this there:

certificates: Envoy xDS Certs should have a ServiceIdentity in the CN; not ServiceAccount certificates: Envoy xDS Certs should have a ServiceIdentity in the CN; not ServiceAccount #3186
My goal would be to make this in a separate PR to avoid bloating this here, which is already large and tough to review.

Converting from ServiceAccount -> ServiceIdentity is not precise because the trust domain information is lost. Do you see this to work differently for multicluster?

I see the hard-coding of cluster.local as temporary to get us a step further. I created this task to follow-up on this:

identity: Remove hard-coded cluster.local trust domain and make flexible identity: Remove hard-coded cluster.local trust domain and make flexible #3187

Signed-off-by: Delyan Raychev <delyan.raychev@microsoft.com>

codecov-commenter · 2021-04-16T18:07:22Z

Codecov Report

Merging #3170 (558a992) into main (8457eab) will increase coverage by 0.06%.
The diff coverage is 86.84%.

@@            Coverage Diff             @@
##             main    #3170      +/-   ##
==========================================
+ Coverage   66.29%   66.36%   +0.06%     
==========================================
  Files         161      161              
  Lines        7390     7405      +15     
==========================================
+ Hits         4899     4914      +15     
  Misses       2464     2464              
  Partials       27       27

Flag	Coverage Δ
unittests	`66.36% <86.84%> (+0.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/catalog/mock_catalog_generated.go	`0.00% <0.00%> (ø)`
pkg/endpoint/mock_provider_generated.go	`0.00% <ø> (ø)`
pkg/endpoint/providers/kube/fake.go	`0.00% <0.00%> (ø)`
pkg/endpoint/types.go	`100.00% <ø> (ø)`
pkg/envoy/cds/cluster.go	`99.00% <ø> (ø)`
pkg/kubernetes/types.go	`100.00% <ø> (ø)`
pkg/envoy/lds/inmesh.go	`78.62% <57.14%> (ø)`
pkg/catalog/endpoint.go	`67.56% <66.66%> (ø)`
pkg/envoy/lds/rbac.go	`82.97% <66.66%> (ø)`
pkg/envoy/sds/response.go	`89.71% <91.66%> (-0.19%)`	⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8457eab...558a992. Read the comment docs.

michelleN · 2021-04-16T18:16:48Z

pkg/catalog/endpoint.go

@@ -53,11 +54,11 @@ func (mc *MeshCatalog) ListAllowedEndpointsForService(downstreamIdentity identit
 		return nil, err
 	}

-	// allowedEndpoints comprises of only those endpoints from outboundEndpoints that matches the endpoints from listEndpointsforIdentity
+	// allowedEndpoints comprises of only those endpoints from outboundEndpoints that matches the endpoints from listEndpointsForServiceIdentity


we should go back and clarify this comment. Was a bit difficult for me to understand.

🤔 This might be the wrong abstraction... Thanks for challenging this! I'll dig into it.

@michelleN I started by merely trying to change the type here so it does not reference K8sServiceAccount and uses ServiceIdentity.
I see that we have bigger problems with ListAllowedEndpointsforService. I recorded these in:

catalog: Simplify ListAllowedEndpointsForService catalog: Simplify ListAllowedEndpointsForService #3190

Would you agree it would be better to tackle simplifying this function in a separate PR (as part of #3190) - not to muddy the waters here?

Alternatively - I could revert the changes to ListAllowedEndpointsForService and keep it in its original form (by using the .ToK8sServiceAccount() and .ToServiceIdentity() temporary helpers), but not sure this would be any different.

I propose we work on this as a fast-follow to this PR here.

pkg/endpoint/types.go

michelleN · 2021-04-16T19:08:37Z

pkg/envoy/ads/secrets.go

@@ -67,7 +67,7 @@ func makeRequestForAllSecrets(proxy *envoy.Proxy, meshCatalog catalog.MeshCatalo

 	// Create an SDS validation cert corresponding to each upstream service that this proxy can connect to.
 	// Each cert is used to validate the certificate presented by the corresponding upstream service.
-	upstreamServices := meshCatalog.ListAllowedOutboundServicesForIdentity(proxyIdentity)
+	upstreamServices := meshCatalog.ListAllowedOutboundServicesForIdentity(proxyIdentity.ToServiceIdentity())


we might want to rename proxyIdentity to serviceAccount to make this more clear.

@michelleN I renamed proxyIdentity to serviceAccount. Also left a comment - we need to continue work on this as part of:

certificates: Envoy xDS Certs should have a ServiceIdentity in the CN; not ServiceAccount certificates: Envoy xDS Certs should have a ServiceIdentity in the CN; not ServiceAccount #3186

…uture improvements w/ GitHub Issue 3186

draychev force-pushed the migration_path_for_ServiceIdentity branch 11 times, most recently from 9713f3e to 7e80190 Compare April 15, 2021 21:34

draychev changed the title ~~Migration path for service identity~~ Migration path for Service Identity Apr 15, 2021

identity: Adding helpers to convert K8sServiceAccount to ServiceIdentity

61df7bc

draychev force-pushed the migration_path_for_ServiceIdentity branch 7 times, most recently from 186ccad to 5581dc0 Compare April 16, 2021 00:12

identity.ServiceIdentity: Step Forward

6ba1e6a

Signed-off-by: Delyan Raychev <delyan.raychev@microsoft.com>

draychev force-pushed the migration_path_for_ServiceIdentity branch from 5581dc0 to 6ba1e6a Compare April 16, 2021 00:13

draychev marked this pull request as ready for review April 16, 2021 00:22

draychev requested a review from a team as a code owner April 16, 2021 00:22

shashankram reviewed Apr 16, 2021

View reviewed changes

This was referenced Apr 16, 2021

certificates: Envoy xDS Certs should have a ServiceIdentity in the CN; not ServiceAccount #3186

Closed

identity: Remove hard-coded cluster.local trust domain and make flexible #3187

Closed

draychev mentioned this pull request Apr 16, 2021

identity: Change (sophisticate) ServiceIdentity from a string to a struct{} so it can capture more context #3188

Closed

shashankram approved these changes Apr 16, 2021

View reviewed changes

draychev added 2 commits April 16, 2021 10:49

Merge branch 'main' into migration_path_for_ServiceIdentity

1dbab2a

Add a comment on what the identity.ServiceIdentity string is

35a7b69

Signed-off-by: Delyan Raychev <delyan.raychev@microsoft.com>

draychev force-pushed the migration_path_for_ServiceIdentity branch from edd1114 to 35a7b69 Compare April 16, 2021 18:03

michelleN suggested changes Apr 16, 2021

View reviewed changes

draychev added 2 commits April 16, 2021 12:44

Remove variable name from Interface definition

06b09da

Rename proxyIdentity to serviceAccount for correctness; comment for f…

558a992

…uture improvements w/ GitHub Issue 3186

draychev requested a review from michelleN April 16, 2021 19:49

michelleN approved these changes Apr 20, 2021

View reviewed changes

draychev merged commit 06e70ae into openservicemesh:main Apr 20, 2021

draychev deleted the migration_path_for_ServiceIdentity branch April 20, 2021 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migration path for Service Identity #3170

Migration path for Service Identity #3170

draychev commented Apr 15, 2021 •

edited

Loading

shashankram left a comment

shashankram Apr 16, 2021

draychev Apr 16, 2021

shashankram Apr 16, 2021

draychev Apr 16, 2021

shashankram Apr 16, 2021

draychev commented Apr 16, 2021 •

edited

Loading

codecov-commenter commented Apr 16, 2021 •

edited

Loading

michelleN Apr 16, 2021

draychev Apr 16, 2021

draychev Apr 16, 2021 •

edited

Loading

michelleN Apr 16, 2021

draychev Apr 16, 2021

Migration path for Service Identity #3170

Migration path for Service Identity #3170

Conversation

draychev commented Apr 15, 2021 • edited Loading

shashankram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

draychev commented Apr 16, 2021 • edited Loading

codecov-commenter commented Apr 16, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

draychev Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

draychev commented Apr 15, 2021 •

edited

Loading

draychev commented Apr 16, 2021 •

edited

Loading

codecov-commenter commented Apr 16, 2021 •

edited

Loading

draychev Apr 16, 2021 •

edited

Loading