fix(xds): Avoid generating duplicate subsets in ingress

[lahabana]

In the ingress generator multiple TrafficRoutes with the same destinations
would generate duplicated subsets in the edsClusterConfig.
As ingress are generated for all meshes this could generate unecessarily complex configuration.
This adds uniqueness on the matches

Signed-off-by: Charly Molter charly@koyeb.com

[lahabana]

As a side note I've noticed the same code in OutboudProxyGenerator. I'm wondering if it might be worth adding this dedupe in envoy_clusters.LbSubset to avoid making the same mistake again.

[lobkovilya]

Hey @lahabana, yes I agree - duplicated subsets make no sense for Envoy, this is a good idea to move dedup in envoy_clusters.LbSubset

[lahabana]

Updated @lobkovilya

[lobkovilya]

I really like the direction you take with this PR, this kind of refactoring was awaited. I want to share some improvement points for this PR, it's not a strict requirement, just a discussion.

1. I think the signature of LbSubset function should be more self-explained, ideally with a single parameter:

func LbSubset(tagSets []envoy.TagKeys, removedTags []string, addedTags []string) {
...
}

I'd replace it with:

func LbSubset(tagSets []envoy.TagKeys) {
...
}

But then we have to call LbSubset already taking into account removedTags and addedTags tags. Here I'd propose to create auxiliary methods for []envoy.TagKeys type:

type TagKeysSlice []envoy.TagKeys

func (list TagKeysSlice) Map(f func(item envoy.TagKeys) envoy.TagKeys) TagKeysSlice {
    // applies `f` to every item of the list and returns new `list`
}

// With add a list of keys (avoids duplicates)
func With(keys ...string) func(envoy.TagKeys) envoy.TagKeys {
	return func(t envoy.TagKeys) envoy.TagKeys {
		all := map[string]bool{}
		for _, v := range t {
			all[v] = true
		}
		for _, v := range keys {
			all[v] = true
		}
		res := envoy.TagKeys{}
		for k := range all {
			res = append(res, k)
		}
		sort.Strings(res)
		return res
	}
}

// Without removes a list of keys
func Without(keys ...string) func(envoy.TagKeys) envoy.TagKeys {
	return func(t envoy.TagKeys) envoy.TagKeys {
		all := map[string]bool{}
		for _, t := range keys {
			all[t] = true
		}
		res := envoy.TagKeys{}
		for _, r := range t {
			if !all[r] {
				res = append(res, r)
			}
		}
		return res
	}
}

So now you can call LbSubset like:

LbSubset(destinationTags.Map(With("mesh")).Map(Without(mesh_proto.ServiceTag)))

2. I think name TagKeys should be replaced with SelectorKeys because this is Envoy terminology and I believe it's better to use it here. I know it wasn't introduced by you, but I think it's better to rename it since we started this refactoring.

What do you think?

[lahabana]

1. Sounds great. I'd actually go with Transform instead of map as map is usually more of a a->b this appends, removes elements. What do you think of: LbSubset(destinationTags.Transform(With("mesh), Without(mesh_proto.ServiceTag), Unique()) (Unique is probably systematically applied dup tag keys don't make sense.
2. I'm not opposed but does that imply also renaming Tags in ClusterSubset? If that's the I probably will open a first PR with this rename to avoid blowing up changesets.

[lahabana]

Regarding 2. After looking some more at the code there are logical usages of these as Tags and not Selectors so I'm not convinced the rename is justified

[lobkovilya]

1. Yeah, I don't mind using Transform naming if you'd like to modify each item in the destinationTags rather than generate a new destinationTags slice.
2. Okay, I see, let's keep these names as it is

[lahabana]

@lobkovilya updated as we've talked about.

[lobkovilya]

Nice! Transformers looks cool 👍 I have a couple of small comments and 1 comments regarding hashing

[lobkovilya]

nit: "to each TagKeys"

[lobkovilya]

I guess this variable should be named tagKeys or keys rather than slice, right?

[lobkovilya]

This FNV hash algorithm has some collision rate, even though it's quite low, I'm still concerned about just rewriting the values with the same hash. If it shoots one day we will never debug it. I see 2 options here:

• classic equal and buckets
• build hashes without collisions, tagKeys are not huge it will be fine to convert them to string and name it a hash

I like the second approach more since it simpler

[lobkovilya]

nit: ToTagKeysSlice()

[lahabana]

@lobkovilya updated thanks!

[lobkovilya]

Great work! Thank you for fixing all the comments. Just a little request - please don't force push commits, it's quite complicated to review the difference after review.

[lahabana]

Thanks for the review and I won't force push in the future :)

[lahabana]

@lobkovilya can I merge this?

[lobkovilya]

Sorry, I was about to merge it myself, but it slipped out of my mind. So yes, please, go ahead and merge it 👍