Hello,
It is indicated here that it fixes the bug of "duplicate mappings" when there are several analog mappings with different weights.
But... would the same problem occur when, for example, there are several analog mappings with different prededence?

In other words, is it intended to fix something more generic than weights?

@juanjoku Thanks for your response. I have tried with precedence configuration also.
When setting different precedences for 2 mappings (with same host and path) with different weights (for canary purpose), then 100% weight applied for both mappings in econf.json/envoy.json. so traffic always sent to mapping with higher precedence.

@juanjoku added fix for this bug. can you please review this PR #5705 ?

Hello @sekar-saravanan. Yes I have looked at PR, but I'm not the appropriate person to evaluate it properly (I've been using Emissary for years, but until now I didn't need to look at the source code :-( )

Thx

OK, I was able to talk over this with @sekar-saravanan (thanks! 🙂) and wow, I'm kind of amazed that this duplicate-route didn't show up before this year. 😐 Nice catch figuring out what's happening, @sekar-saravanan!

What's happening is that the weight-normalization code has been assuming that the different Mappings it's working with are different Mappings. If we have multiple Mappings that are identical except for their weights, though, we should treat that differently. I think the most straightforward solution is to allow IRBaseMappingGroup.normalize_weights_in_mappings to actually delete the duplicates, leaving only the Mapping with the largest weight.

(If a Mapping in the duplicated set has a missing weight, I think it should be treated as having a weight of infinity. For example, if your Group has

• Mapping 1: prefix /foo/ weight 10
• Mapping 2: prefix /foo/ weight missing
• Mapping 3: prefix /foo/ weight 30

then that'll become

• Mapping 2: prefix /foo/ weight missing

which would then be forced to 100%, which is fine. But if your Group has

• Mapping 1: prefix /foo/ weight 10
• Mapping 2: prefix /foo/ weight missing
• Mapping 3: prefix /foo/ weight 30
• Mapping 4: prefix /bar/ weight 50

then I think it makes more sense to end up with

• Mapping 2: prefix /foo/ weight missing
• Mapping 4: prefix /bar/ weight 50

than anything else. Opinions welcome.)

@sekar-saravanan, I'd be delighted if you could tweak your PR for this! thanks for jumping in to help. 🙂

@kflynn It looks, modifying inside IRBaseMappingGroup.normalize_weights_in_mappings method will be bit critical, because this method will be called finally after processing all the mappings with respected groups. https://github.com/emissary-ingress/emissary/blob/master/python/ambassador/ir/ir.py#L464

It can be more better, if we could avoid generating groups with duplicate mappings while processing itself, because we will have more flexibility to avoid the creation of duplicate mapping while processing. https://github.com/emissary-ingress/emissary/blob/master/python/ambassador/ir/ir.py#L459

Let's take same example, Each mapping can point to different route (cluster) for canary release purpose.

• Mapping 1: prefix /foo/ weight 10 - app1
• Mapping 2: prefix /foo/ weight missing - app2
• Mapping 3: prefix /foo/ weight 30 - app3

In this case, I need to maintain all the mappings with respected weights for canary purpose.

PR: 5705 - Here, we can make sure that, it will only add the mapping into the group only if already does not exists. (we are checking the mapping with rkey, and I hope it will be always unique <mapping_name>.)

Looking forward to your opinions / suggestions.

I was initially thinking that you might not have the information you need while the groups are being built, but the more I think about it, the more you likely will, so go for it!

I also suspect that the rkey is probably not the way to go about duplicate checking, as I think about it. All the Mappings in an IRHTTPMappingGroup should be "duplicates" in that they should all have the same routing information; what can vary is the target and the weight. So coalescing Mappings with the same mapping.cluster_key is, I think, likely to work better (different cluster_keys are, by definition, not the same target). Does that make sense? Happy to kick that around further...

Hey @kflynn - I understand what you are trying to achieve.

Fix 1:

If more than 1 mappings has same target (cluster_key) in a group (same host, path, etc.. - link), needs to remove the duplicates with the least weights.
Have added fix for this. I hope this is what you are trying to achieve. 173d98f

For example, if your Group has

• Mapping 1: prefix /foo/ - target A - weight 10
• Mapping 2: prefix /foo/ - target A - weight missing
• Mapping 3: prefix /foo/ - target B - weight 30

then that'll become

• Mapping 2: prefix /foo/ - target A - weight missing
• Mapping 3: prefix /foo/ - target B - weight 30

There is no impact on envoy behaviour having these duplicate mappings. This behaviour will occurred, when we are creating more than 1 mapping resource with same host, path, target intentionally / unintentionally.

Actually, this is not a bug. If we are maintaining correct mapping resources, this will never happen.

Fix 2:

But, this fix is used for avoid creating duplicate mappings based on same mapping resource.

If we are creating 2 mappings with same host, path, different target and different weight, it creates duplicate mappings multiple time of same mapping resource.

Steps to reproduce.

1. Create mapping A with Host_1, Path_1, Target_A, Weight_A

apiVersion: getambassador.io/v2
kind: Mapping
metadata:
  labels:
    app: test-app-emissariy
  name: test-app-emissariy-external-origin-test-app-emissariy2
  namespace: infrastructure
spec:
  bypass_auth: true
  host: origin-test-app-emissariy.spike-pp.test-labs.com
  prefix: /
  resolver: endpoint
  rewrite: ""
  service: test-app-emissariy-default.infrastructure:80
  timeout_ms: 3000

2. Create mapping B with Host_1, Path_1, Target_B, Weight_B

apiVersion: getambassador.io/v2
kind: Mapping
metadata:
  labels:
    app: canary-test-app-emissariy
  name: canary-test-app-emissariy-external-origin-canary-test-app-emissariy2
  namespace: infrastructure
spec:
  bypass_auth: true
  host: origin-test-app-emissariy.spike-pp.test-labs.com
  prefix: /
  resolver: endpoint
  rewrite: ""
  service: canary-test-app-emissariy-default.infrastructure:80
  timeout_ms: 3000
  weight: 10

3. Check ir.json. Group will have 2 mappings what we created in Step 1 and 2.
4. Edit/Update any other existing mapping. Check ir.json again. Now the same Group will have 3 mappings. Mapping A will get added 2 times (duplicate) and Mapping B will get added 1 time in the group.
5. Every time updating any other exiting mappings, same Group will get increased continuously with same duplicate mapping.
6. Some particular point of time or Editing/Updating Mapping A or Mapping B, Group will get regenerated and will have just 2 mapping again as same as Step 3.

Even if we are maintaining correct mapping resource, this behaviour will occur. This is a bug.
With Fix 1 itself, it will get fixed. But adding separate fix for this issue will be more effective.

Looking forward to your opinions / suggestions.

@sekar-saravanan That's a really superb rundown of this issue, nice catch!

Branch flynn/v4-5715 is a branch off Emissary 4 that has your fix for #5714 and also includes a Go unit test to catch the bug you've described here: look at cmd/entrypoint/unrelated_mappings_test.go. I would encourage you to rebase your fix here onto that branch, and to make sure that test passes! The minimal way to run that test:

make python-virtual-environment
source venv/bin/activate
rm -rf /tmp/mock
go test -count=1 -v -run '^TestUnrelatedMappings$' ./cmd/entrypoint

and after the test runs, you'll find all the configuration files produced during the test in /tmp/mock.

@AliceProxy, @tenshinhigashi, @the-wondersmith, there's a cmd/entrypoint/irtype.go in here too, because I needed it for the unit test. It is very far from complete but it's a starting point. And, no, I haven't really looked at backporting to Emissary 3, but it's likely not impossible.

@kflynn I have executed the test case as you described and test has been passed after adding the fix mentioned in the PR.
As discussed, updated PR to use _cache_key for comparing duplicate mappings instead of rkey.
Attached the testcase results too. testcase-result.zip