Fix Service object remove from resource map from route controller #549

chauhanshubham · 2022-10-12T09:16:51Z

This commit fetches the services that were referenced by the route which has now been deleted, and removes those objects from the resource map. In order to facilitate this, the commit introduces a provider cache for kubernetes that stores mappings between kubernetes objects - for now it stores the map between [tls/http]route -> backend service references.
So that once the [tls/http]route is deleted we have the services that it referenced.
Fixes: #536

Signed-off-by: Shubham Chauhan shubham@tetrate.io

This commit fetches the services that were referenced by the route which has now been deleted, and removes those objects from the resource map. In order to facilitate this, the commit introduces a provider cache for kubernetes that stores mappings between kubernetes objects - for now it stores the map between [tls/http]route -> backend service references. So that once the [tls/http]route is deleted we have the services that it referenced. Signed-off-by: Shubham Chauhan <shubham@tetrate.io>

codecov-commenter · 2022-10-12T09:20:42Z

Codecov Report

Merging #549 (fb48c25) into main (2fb3ca8) will decrease coverage by 0.36%.
The diff coverage is 74.68%.

@@            Coverage Diff             @@
##             main     #549      +/-   ##
==========================================
- Coverage   60.98%   60.61%   -0.37%     
==========================================
  Files          45       47       +2     
  Lines        5564     5721     +157     
==========================================
+ Hits         3393     3468      +75     
- Misses       1968     2034      +66     
- Partials      203      219      +16

Impacted Files	Coverage Δ
internal/provider/kubernetes/kubernetes.go	`50.94% <50.00%> (+2.94%)`	⬆️
internal/provider/kubernetes/tlsroute.go	`51.98% <54.54%> (-14.69%)`	⬇️
internal/provider/kubernetes/httproute.go	`57.61% <55.55%> (-8.48%)`	⬇️
internal/provider/kubernetes/store.go	`100.00% <100.00%> (ø)`
internal/infrastructure/kubernetes/configmap.go	`75.86% <0.00%> (-6.60%)`	⬇️
...ternal/infrastructure/kubernetes/serviceaccount.go	`77.96% <0.00%> (-4.80%)`	⬇️
internal/xds/translator/listener.go	`81.42% <0.00%> (-4.40%)`	⬇️
internal/gatewayapi/contexts.go	`76.02% <0.00%> (-4.12%)`	⬇️
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

arkodg · 2022-10-13T17:57:14Z

thanks for working on this !
this functionality will most likely not be needed once #413 is in since it will always build out SoTW from scratch, using the controller runtime cache, but it might make sense to introduce this PR if we want to support RefereneGrant in 0.2.0 since #539 (a much more severe consequence of not removing the resource) will leverage the same library introduced in this PR

chauhanshubham · 2022-10-13T18:22:42Z

Will there not be a need to store Service objects any more in the resource map post 413?
Could we try fetching the Services from the controller cache right now itself? doesn't seem to depend on 413. Maybe I'm missing something.
We'll have to pass on the client to the gatewayapi/translator for fetching these objects.

I'm looking at

gateway/internal/gatewayapi/translator.go

Line 66 in 0be1aef

func (r *Resources) GetService(namespace, name string) *v1.Service {

The GetNamespace, GetService, and GetSecret functions here, that try fetching it from the Resources struct, instead we could read it from the controller cache perhaps? We can add a similar GetReferenceGrant too.

arkodg · 2022-10-13T18:54:53Z

Will there not be a need to store Service objects any more in the resource map post 413? Could we try fetching the Services from the controller cache right now itself? doesn't seem to depend on 413. Maybe I'm missing something. We'll have to pass on the client to the gatewayapi/translator for fetching these objects.

I'm looking at

gateway/internal/gatewayapi/translator.go

Line 66 in 0be1aef

func (r *Resources) GetService(namespace, name string) *v1.Service {

The GetNamespace, GetService, and GetSecret functions here, that try fetching it from the Resources struct, instead we could read it from the controller cache perhaps? We can add a similar GetReferenceGrant too.

I was referring to the fact that we won't need to handle delete logic anymore with #413 .

Any changes to gatewayclasses, gateways, httproutes, services .... will trigger a reconcile
the function would build all the resource state from scratch starting from the accepted GatewayClass

type resource struct {
GatewayClass string
Gateways []*v1b1.Gateway
HTTPRoutes []*v1b1.HTTPRoute
....

and update the watchable map with the same key, so we dont need to deal with deletes in the provider anymore

if we want to add watches for specific services, namespaces etc. we might need to handle delete logic there

arkodg

🚀

LukeShu

There are some things I think should be changed, but if we want to get this in to 0.2.0, I'm OK merging as-is and cleaning it up later.

LukeShu · 2022-10-17T20:59:12Z

internal/provider/kubernetes/cache.go

+	// routeToServicesMappings maintains a mapping of a Route object,
+	// and the Services it references. For instance
+	// HTTPRoute/ns1/route1	-> { ns1/svc1, ns1/svc2, ns2/svc1 }
+	// TLSRoute/ns1/route1	-> { ns1/svc1, ns2/svc2 }
+	routeToServicesMappings map[string]sets.String


Can we use typed structs, instead of having to wrangle strings?

type routeIdentifier struct { Kind string Namespace string Name string } routeToServicesMappings map[routeIdentifier]Set[types.NamespacedName]

(it's easy enough to write a type-parametarized Set[T] (example) but if we don't want to do that, map[types.NamespacedName]struct{} would do in a pinch)

LukeShu · 2022-10-17T21:02:45Z

internal/provider/kubernetes/cache.go

+// providerCache maintains additional mappings related to Kubernetes provider
+// resources. The mappings are regularly updated from the reconcilers based
+// on the existence of the object in the Kubernetes datastore.
+type providerCache struct {


Calling this a "cache" is misleading. It isn't really a cache, it's a reference-counting machine.

LukeShu · 2022-10-17T21:04:01Z

internal/provider/kubernetes/cache.go

+func (p *providerCache) isServiceReferredByRoutes(service string) bool {
+	p.mu.Lock()
+	defer p.mu.Unlock()
+
+	for _, svcs := range p.routeToServicesMappings {
+		if svcs.Has(service) {
+			return true
+		}
+	}
+	return false
+}


This is O(n), and that makes me uneasy. Would it make sense to have the primary key be the service, rather than the route?

It will probably get uglier if we use the reverse mapping with the service as P.K.
As we need to query this primarily - Find all services for a Route r1 which was deleted, while reconciling the deleted Route r1. With service as the P.K. we'll have to traverse all the Services (and find a route match) which will be more than the number of TLS/HTTP Routes. (Note that with route as the P.K we are filtering based on kind too)

I started off by adding multiple mappings like route-to-services and service-to-routes, but seemed like we're optimizing at the cost of double the memory, so stuck with the route-to-services mapping only.

Signed-off-by: Shubham Chauhan <shubham@tetrate.io>

arkodg · 2022-10-18T16:06:37Z

hey @chauhanshubham can you rm the docs/user/tls-passthrough.md changes from this PR :)

Signed-off-by: Shubham Chauhan <shubham@tetrate.io>

LukeShu

Thanks!

…voyproxy#549) * Fix Service object remove from resource map from route controller This commit fetches the services that were referenced by the route which has now been deleted, and removes those objects from the resource map. In order to facilitate this, the commit introduces a provider cache for kubernetes that stores mappings between kubernetes objects - for now it stores the map between [tls/http]route -> backend service references. So that once the [tls/http]route is deleted we have the services that it referenced. Signed-off-by: Shubham Chauhan <shubham@tetrate.io>

chauhanshubham requested a review from a team as a code owner October 12, 2022 09:16

chauhanshubham marked this pull request as draft October 12, 2022 09:17

chauhanshubham marked this pull request as ready for review October 12, 2022 10:25

danehans added the provider/kubernetes Issues related to the Kubernetes provider label Oct 12, 2022

danehans added this to the 0.2.0 milestone Oct 12, 2022

danehans added the kind/bug Something isn't working label Oct 13, 2022

arkodg previously approved these changes Oct 13, 2022

View reviewed changes

LukeShu previously approved these changes Oct 17, 2022

View reviewed changes

minor

b9f6843

Signed-off-by: Shubham Chauhan <shubham@tetrate.io>

chauhanshubham dismissed stale reviews from LukeShu and arkodg via b9f6843 October 18, 2022 08:41

refactor - review comments

3731205

Signed-off-by: Shubham Chauhan <shubham@tetrate.io>

chauhanshubham added 4 commits October 18, 2022 21:38

revert bad commit

425213c

Signed-off-by: Shubham Chauhan <shubham@tetrate.io>

more refactor

83ac836

Signed-off-by: Shubham Chauhan <shubham@tetrate.io>

test-lint-fix

1114aaa

Signed-off-by: Shubham Chauhan <shubham@tetrate.io>

sigh

fb48c25

Signed-off-by: Shubham Chauhan <shubham@tetrate.io>

LukeShu self-requested a review October 18, 2022 17:07

LukeShu approved these changes Oct 18, 2022

View reviewed changes

arkodg merged commit 9ab00f2 into envoyproxy:main Oct 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Service object remove from resource map from route controller #549

Fix Service object remove from resource map from route controller #549

chauhanshubham commented Oct 12, 2022

codecov-commenter commented Oct 12, 2022 •

edited

Loading

arkodg commented Oct 13, 2022

chauhanshubham commented Oct 13, 2022 •

edited

Loading

arkodg commented Oct 13, 2022

arkodg left a comment

LukeShu left a comment

LukeShu Oct 17, 2022

LukeShu Oct 17, 2022

LukeShu Oct 17, 2022

chauhanshubham Oct 18, 2022 •

edited

Loading

arkodg commented Oct 18, 2022

LukeShu left a comment

Fix Service object remove from resource map from route controller #549

Fix Service object remove from resource map from route controller #549

Conversation

chauhanshubham commented Oct 12, 2022

codecov-commenter commented Oct 12, 2022 • edited Loading

Codecov Report

arkodg commented Oct 13, 2022

chauhanshubham commented Oct 13, 2022 • edited Loading

arkodg commented Oct 13, 2022

arkodg left a comment

Choose a reason for hiding this comment

LukeShu left a comment

Choose a reason for hiding this comment

LukeShu Oct 17, 2022

Choose a reason for hiding this comment

LukeShu Oct 17, 2022

Choose a reason for hiding this comment

LukeShu Oct 17, 2022

Choose a reason for hiding this comment

chauhanshubham Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

arkodg commented Oct 18, 2022

LukeShu left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 12, 2022 •

edited

Loading

chauhanshubham commented Oct 13, 2022 •

edited

Loading

chauhanshubham Oct 18, 2022 •

edited

Loading