-
Notifications
You must be signed in to change notification settings - Fork 35
Handle zero weights in cluster and non-cluster mappings, add new tests #241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- previously zero weights were interpreted as being 100 - added tests verifying the partial weights with 0 entries - deprecated `GetWeight` method in favor of directly using the `Weight` field (this is the function that made 0 -> 100) Signed-off-by: Alberto Ricart <[email protected]>
Pull Request Test Coverage Report for Build 19439588169Details
💛 - Coveralls |
|
This is actually not going to work - the previous implementation relied on the fact that if the weight was not specified - defaulted to |
…ork propery - new ones created by the API will ALWAYS serialize the weight - so if created with no weight expecting the old behaviour, it will set to 0 (which will not route traffic). Signed-off-by: Alberto Ricart <[email protected]>
|
Changed the implementation do do custom unmarshalling - if the weight option is not present the value will be 100 - this makes existing JWTs work as they did. However, if new code is written that doesn't explicitly set the mapping value, the value will be 0, which means no traffic. |
|
@kozlovic I am going to do a test to make sure that self-mappings actually work. |
Signed-off-by: Alberto Ricart <[email protected]>
kozlovic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
it does! |
reworked. |
philpennock
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone has JSON exports to re-create a setup, this should work fine because either the weight is present and correct, or missing and 100, the old exports won't include a 0 weight because of the older omitempty semantics in Go.
If someone is creating weighted mappings from some other tooling, and they explicitly say 0 because it has previously been interpreted as 100%, that would now fail. So this is fine if there's a major number bump for a breaking change, but not as a patch-level change. I could succumb in a moment of weakness and accept a minor level change because it's a weird corner case.
Is there any way to look at the set of weighted mappings and say "if there's only one mapping, and it's 0%, then we interpret that as 100% unless and until there's a second mapping"? Or would that break because the server has different semantics and this is purely around how nsc models things?
Change itself looks sane, so approving.
|
That would invalidate the ability to self map to 0. It would require self map + one more perhaps at zero. While the risk is there we can annotate. Or simply wait for server 3.0 |
GetWeightmethod in favor of directly using theWeightfield (this is the function that made 0 -> 100)Fix nats-io/nsc#775