(feat/statefulsets): MeshService API changes for Headless Services #4704

keithmattix · 2022-04-28T20:22:49Z

Description:
In order to provide statefulset support, introduce 2 new public
functions to the MeshService type to accomplish the following:

The ProviderKey function describes the mapping from a MeshService back to a provider
service (e.g. a Kubernetes service). This decouples the MeshService name
from being a foreign key between the provider's collection of services
and the MeshCatalog's set of services.
The Subdomain function returns the subdomain for a particular mesh service (e.g. the pod name for a particular pod within a headless service). This allows for proper matching of endpoints when building EDS and CDS responses.

Both of these functions rely on a new convention around MeshService.Name in that the name shall be formatted as ([subdomain.]providerKey). The values returned by these functions are calculated once and then memoized via unexported fields. The MeshService.Equals() function has been updated to account for these unexported fields.

Because of the semantics of statefulsets and headless services in Kubernetes, it is important to highlight the following guidance re: headless services:

Specific statefulset instances (e.g. mysql-0.mysql.namespace.svc.cluser.local) should only be
referenced for traffic routing (e.g. TrafficSplit) and not policy (e.g. TrafficTarget, Retry).
Conversely, the headless service (e.g. mysql.namespace.svc.cluster.local) should only be referenced
for policy and not traffic routing.

Signed-off-by: Keith Mattix II keithmattix2@gmail.com

Testing done:

Affected area:

Functional Area
New Functionality	[ ]
CI System	[ ]
CLI Tool	[ ]
Certificate Management	[ ]
Control Plane	[ ]
Demo	[ ]
Documentation	[ ]
Egress	[ ]
Ingress	[ ]
Install	[ ]
Networking	[ ]
Observability	[ ]
Performance	[ ]
SMI Policy	[ ]
Security	[ ]
Sidecar Injection	[ ]
Tests	[ ]
Upgrade	[ ]
Other	[ ]

Please answer the following questions with yes/no.

Does this change contain code from or inspired by another project?
- Did you notify the maintainers and provide attribution?
Is this a breaking change?
Has documentation corresponding to this change been updated in the osm-docs repo (if applicable)?

pkg/service/types.go

codecov-commenter · 2022-05-09T21:47:10Z

Codecov Report

Merging #4704 (7bf723a) into main (ec9b9f9) will increase coverage by 0.57%.
The diff coverage is 88.35%.

@@            Coverage Diff             @@
##             main    #4704      +/-   ##
==========================================
+ Coverage   68.55%   69.13%   +0.57%     
==========================================
  Files         228      227       -1     
  Lines       16289    16316      +27     
==========================================
+ Hits        11167    11280     +113     
+ Misses       5070     4984      -86     
  Partials       52       52

Flag	Coverage Δ
unittests	`69.13% <88.35%> (+0.57%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/k8s/types.go	`100.00% <ø> (ø)`
pkg/validator/validators.go	`80.51% <ø> (+0.90%)`	⬆️
pkg/cli/verifier/envoy_config.go	`65.56% <63.63%> (-1.06%)`	⬇️
pkg/catalog/inbound_traffic_policies.go	`94.17% <71.42%> (+0.73%)`	⬆️
pkg/service/types.go	`66.66% <71.42%> (+4.16%)`	⬆️
pkg/k8s/client.go	`93.05% <93.75%> (-0.04%)`	⬇️
pkg/k8s/util.go	`96.03% <96.29%> (-0.20%)`	⬇️
pkg/catalog/ingress.go	`96.52% <100.00%> (+0.09%)`	⬆️
pkg/catalog/outbound_traffic_policies.go	`94.67% <100.00%> (ø)`
pkg/catalog/retry.go	`92.59% <100.00%> (+0.92%)`	⬆️
... and 22 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 ec9b9f9...7bf723a. Read the comment docs.

shashankram

Thanks for this change. The overall idea looks good, left some comments to enable the reuse of code and readability.

pkg/cli/verifier/envoy_config.go

pkg/k8s/client.go

pkg/k8s/types.go

pkg/service/types.go

pkg/k8s/util.go

steeling

Can we add an E2E test in this PR as well?

pkg/catalog/inbound_traffic_policies.go

pkg/envoy/registry/services.go

pkg/k8s/client.go

pkg/envoy/registry/services.go

keithmattix · 2022-05-12T22:29:28Z

@steeling An E2E test for all statefulset functionality will be added in subsequent PRs. This PR is only for the MeshService API changes and their downstream effects

steeling

Instead of all the init/partial stuff, if the issue is unexported fields, it's likely a code smell that we're using the wrong data structure, ie: instead of a hashset let's just use a map

pkg/service/service.go

pkg/k8s/util.go

As a stepping stone for statefulset support (openservicemesh#3477), introduce a new interface describing the ability to map an entity back to a provider service (e.g. a Kubernetes service). This decouples the MeshService name from being a foreign key between the provider's collection of services and the MeshCatalog's set of services Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

When retrieving MeshServices in order to create local clusters for a pod, exclude MeshServices whose subdomains don't match the pod's name Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

Tracking the unexported MeshService fields is difficult, and there are several hidden bugs that can occur depending on what fields are accessed. So, we create the NewMeshService and NewPartialMeshService functions to aid in correct usage Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

pkg/service/types.go

pkg/service/types_test.go

pkg/service/types.go

pkg/envoy/registry/services.go

pkg/tests/fixtures.go

nojnhuh · 2022-05-18T20:23:48Z

pkg/envoy/registry/services_test.go

+					{
+						Addresses: []v1.EndpointAddress{
+							{
+								IP:       "8.8.8.9",


If the IPs here aren't relevant to the test, I'd remove them.

The IPs are also used down the callstack in ServiceToMeshServices

More info: #4704 (comment)

pkg/envoy/registry/services_test.go

nojnhuh · 2022-05-18T20:29:24Z

pkg/envoy/registry/services_test.go

+					Namespace: namespace,
+					Name:      service1Name,
+				},
+				Subsets: []v1.EndpointSubset{


Like above, if these aren't headless services, then it seems like Subsets doesn't matter here.

Ah, I see what you mean now; yes they technically aren't needed. I don't know how I feel about using unrealistic resources in tests though....feels like as soon as that thing needs to be used, things break

My personal rule of thumb is "if I can comment it out and the test still passes, remove it." I find trying to make things as realistic as possible in unit tests is more work than it's worth and makes the tests harder to reason about later. If Subsets does come into play later for non-headless services, then the tests will almost certainly need to be updated to account for that anyway.

I get where you're coming from, and I certainly think there's a middle ground there. Spending a ton of time making tests hyper-realistic is wasteful, but at the same time, stripping test resources too far down can lead to unexpected behavior. Personally, I find unrealistic objects harder to reason about (e.g. "what does it mean for an Endpoints resource to have a nil subset"). I actually ran into this quite a bit while developing this feature: headless services are essentially a special case, but because a lot of our test objects don't have ClusterIPs, I had to go update unrelated tests when I built out headless service support. This is the primary reason why I started to add ClusterIPs to most of my test service objects. Basically, I'm just trying to say there's a balance. For this test in particular, I think need to add a headless service anyway for the test, but I did want to chat about the general point for posterity's sake 😄

pkg/cli/verifier/envoy_config.go

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

keithmattix force-pushed the meshservice-providerKey branch from b13c1a9 to 9be15b3 Compare April 28, 2022 20:39

steeling reviewed Apr 28, 2022

View reviewed changes

pkg/service/types.go Outdated Show resolved Hide resolved

steeling reviewed Apr 28, 2022

View reviewed changes

pkg/service/types.go Outdated Show resolved Hide resolved

keithmattix force-pushed the meshservice-providerKey branch from 161cb93 to 0099ad1 Compare May 2, 2022 18:44

keithmattix requested a review from steeling May 2, 2022 18:53

steeling reviewed May 2, 2022

View reviewed changes

pkg/service/types.go Outdated Show resolved Hide resolved

keithmattix requested a review from steeling May 2, 2022 20:06

keithmattix changed the title ~~Introduce service.ProviderMapper to MeshService~~ (feat/statefulsets): MeshService API changes May 3, 2022

keithmattix force-pushed the meshservice-providerKey branch 2 times, most recently from 1ac81e4 to cb6a880 Compare May 9, 2022 17:27

keithmattix changed the title ~~(feat/statefulsets): MeshService API changes~~ (feat/statefulsets): MeshService API changes for Headless Services May 9, 2022

keithmattix force-pushed the meshservice-providerKey branch from e513dc8 to 20e538b Compare May 9, 2022 21:38

keithmattix marked this pull request as ready for review May 9, 2022 21:38

keithmattix requested review from shashankram, snehachhabria, nojnhuh, draychev, jaellio and trstringer as code owners May 9, 2022 21:38

shashankram suggested changes May 10, 2022

View reviewed changes

shashankram approved these changes May 10, 2022

View reviewed changes

nojnhuh reviewed May 11, 2022

View reviewed changes

pkg/k8s/util.go Outdated Show resolved Hide resolved

shashankram added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 11, 2022

steeling reviewed May 11, 2022

View reviewed changes

pkg/catalog/inbound_traffic_policies.go Outdated Show resolved Hide resolved

pkg/envoy/registry/services.go Outdated Show resolved Hide resolved

pkg/k8s/client.go Outdated Show resolved Hide resolved

pkg/envoy/registry/services.go Outdated Show resolved Hide resolved

keithmattix force-pushed the meshservice-providerKey branch 2 times, most recently from 582b40c to 6ea3490 Compare May 12, 2022 22:08

steeling suggested changes May 16, 2022

View reviewed changes

pkg/service/service.go Outdated Show resolved Hide resolved

steeling reviewed May 16, 2022

View reviewed changes

pkg/k8s/util.go Outdated Show resolved Hide resolved

keithmattix requested a review from steeling May 16, 2022 21:32

keithmattix added 12 commits May 16, 2022 17:57

Add subdomain field to Meshservice

cc73ac1

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

Write tests for headless service functionality

7dfa9ce

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

Address PR comments

27f9b84

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

Filter MeshServices from headless service based on subdomain

e27e109

When retrieving MeshServices in order to create local clusters for a pod, exclude MeshServices whose subdomains don't match the pod's name Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

Fix arg order

79f3526

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

Remove unnecessary function & newline

89ef301

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

Re-duplicate svc to meshsvc code and de-memoize MeshService

6440855

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

Remove unneccesary Equals function

7100f51

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

Remove unneeded newline

4fe8697

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

Add mock expectation

c7a568e

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

keithmattix force-pushed the meshservice-providerKey branch from 20a60a6 to c7a568e Compare May 16, 2022 22:59

keithmattix added 3 commits May 17, 2022 09:37

Add kubecontroller to splitHostname functions

5436e1f

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

Add mock test expectation

b76ec42

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

Fix tests

3b038a5

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

keithmattix requested a review from nojnhuh May 17, 2022 18:47

Fix tests again

b3d0c7f

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

keithmattix removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 18, 2022

nojnhuh reviewed May 18, 2022

View reviewed changes

keithmattix added 2 commits May 18, 2022 17:42

Address PR comments

01161be

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

Remove unneded function

7bf723a

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

keithmattix requested a review from nojnhuh May 18, 2022 22:48

keithmattix added 3 commits May 18, 2022 18:17

Comment exported function

191a9cf

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

Additional PR fixes

0806320

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

Simplify tests

e7f8731

Signed-off-by: Keith Mattix II <keithmattix2@gmail.com>

nojnhuh approved these changes May 19, 2022

View reviewed changes

nojnhuh merged commit 0af42df into openservicemesh:main May 19, 2022

keithmattix deleted the meshservice-providerKey branch May 19, 2022 20:50

(feat/statefulsets): MeshService API changes for Headless Services #4704

(feat/statefulsets): MeshService API changes for Headless Services #4704

Uh oh!

Conversation

keithmattix commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

shashankram left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steeling left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keithmattix commented May 12, 2022

Uh oh!

steeling left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nojnhuh May 18, 2022

Choose a reason for hiding this comment

Uh oh!

keithmattix May 18, 2022

Choose a reason for hiding this comment

Uh oh!

keithmattix May 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nojnhuh May 18, 2022

Choose a reason for hiding this comment

Uh oh!

keithmattix May 18, 2022

Choose a reason for hiding this comment

Uh oh!

nojnhuh May 19, 2022

Choose a reason for hiding this comment

Uh oh!

keithmattix May 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

keithmattix commented Apr 28, 2022 •

edited

Loading

codecov-commenter commented May 9, 2022 •

edited

Loading