Skip to content
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

Change default cookie session affinity format #1989

Merged
merged 6 commits into from
Jan 11, 2023
Merged

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented Jan 5, 2023

Fixes #1980

This also addresses the doc issues for #1981, but some of those should be backported to the release/latest branch.

This adds two new cookie based session affinity policies, 'HashCookie' and 'ArrCookie' that use stable hashes rather than Data Protection, making it easier to deploy across multiple instances without additional configuration or external storage.

  • 'HashCookie' uses XxHash64 which is fast and compact. This is the new default policy.
  • 'ArrCookie' uses SHA-256 and matches the format IIS's ARR uses, allowing YARP and ARR to be used together in the same environment.

Breaking Change: This is considered a behavioral breaking change because existing session cookies will not be honored on upgrade unless the old 'Cookie' policy is specified in config. The request will be re load balanced and the old cookie will be replaced. This format also makes fewer security guarantees about the protection level of the key data, the destination id. Sensitive data should not be used for destination ids.

@Tratcher Tratcher added this to the YARP 2.0.0 milestone Jan 5, 2023
@Tratcher Tratcher self-assigned this Jan 5, 2023
@Tratcher Tratcher changed the title Change default cookie session affinity fromat Change default cookie session affinity format Jan 5, 2023
Copy link

@blowdart blowdart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that occurred to me just now is the actual algorithm. As you're not using SHA256 for anything cryptographically related, you may want to consider one the of the non-cryptographic hashes we added a couple of versions ago. This was done so it would be a clear indication the hash wasn't be used for security purposes, and to enable folks to stop getting warnings for old algorithm use like SHA1. While SHA256 is probably not going away anytime soon, using anything from System.IO.Hashing.NonCryptographicHashAlgorithm would head that sort of code review off at the pass, and they'd be faster to generate as well.

@eyalg-msft
Copy link

One thing that occurred to me just now is the actual algorithm. As you're not using SHA256 for anything cryptographically related, you may want to consider one the of the non-cryptographic hashes we added a couple of versions ago. This was done so it would be a clear indication the hash wasn't be used for security purposes, and to enable folks to stop getting warnings for old algorithm use like SHA1. While SHA256 is probably not going away anytime soon, using anything from System.IO.Hashing.NonCryptographicHashAlgorithm would head that sort of code review off at the pass, and they'd be faster to generate as well.

The SHA256 is required to be compatible with ARR. This is an important requirement for products migrating from ARR. If we really want, we can allow selecting different algorithm. However, I do suggest keeping it simple, and to keep the ARR backward compatible version the default behavior.

@blowdart
Copy link

blowdart commented Jan 6, 2023

The SHA256 is required to be compatible with ARR. This is an important requirement for products migrating from ARR. If we really want, we can allow selecting different algorithm. However, I do suggest keeping it simple, and to keep the ARR backward compatible version the default behavior.

Are there really mixed setups where something could hit ARR and Yarp at the same time and you want routing to work the same in both? Or is the concern that a yarp cookie would upset ARR, in which case changing the cookie name is better.

Avoiding unnecessary SDL compliance issues is a huge plus for not using SHA256, and the current, non-SHA approach isn't compatible with ARR either, so using something else puts you in the same incompatible format. The original issue did not talk about wanting strict ARR compatibility. Also using SHA256 doesn't ensure ARR compatibility at all, unless ARR works in the exact same way, hashing the exact same resource identifier, and that seems a fragile thing to base a decision on.

return new(null, AffinityStatus.AffinityKeyNotSet);
}

foreach (var d in destinations)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of this O(n) (I realize we already have a TODO for it in BaseSessionAffinityPolicy)

docs/docfx/articles/session-affinity.md Outdated Show resolved Hide resolved
@eyalg-msft
Copy link

The SHA256 is required to be compatible with ARR. This is an important requirement for products migrating from ARR. If we really want, we can allow selecting different algorithm. However, I do suggest keeping it simple, and to keep the ARR backward compatible version the default behavior.

Are there really mixed setups where something could hit ARR and Yarp at the same time and you want routing to work the same in both? Or is the concern that a yarp cookie would upset ARR, in which case changing the cookie name is better.

Avoiding unnecessary SDL compliance issues is a huge plus for not using SHA256, and the current, non-SHA approach isn't compatible with ARR either, so using something else puts you in the same incompatible format. The original issue did not talk about wanting strict ARR compatibility. Also using SHA256 doesn't ensure ARR compatibility at all, unless ARR works in the exact same way, hashing the exact same resource identifier, and that seems a fragile thing to base a decision on.

We had to check ARR code to validate, and we did already validate its working well it in FastFood.

@blowdart
Copy link

blowdart commented Jan 6, 2023

We had to check ARR code to validate, and we did already validate its working well it in FastFood.

That doesn't answer my question about what you mean by ARR compatible, and why it's needed.

@Tratcher
Copy link
Member Author

Rebased, cleaned up. Still working through the Sha vs Hash discussion offline.

@Tratcher
Copy link
Member Author

Updated plan (see the edited description above): Ship both XxHash64 and SHA-256 implementations, with XxHash64 as the default.

…Policy.cs

Co-authored-by: David Fowler <davidfowl@gmail.com>
docs/docfx/articles/session-affinity.md Outdated Show resolved Hide resolved
docs/docfx/articles/session-affinity.md Outdated Show resolved Hide resolved
docs/docfx/articles/session-affinity.md Outdated Show resolved Hide resolved
@@ -71,7 +72,7 @@ new ClusterConfig
```

## Affinity Key
Request to destination affinity is established via the affinity key identifying the target destination. That key can be stored on different request parts depending on the given session affinity implementation, but each request cannot have more than one such key. The exact key semantics is implementation dependent, in example the both of built-in `CookieSessionAffinityPolicy` and `CustomHeaderAffinityPolicy` are currently use `DestinationId` as the affinity key.
Request to destination affinity is established via the affinity key identifying the target destination. That key can be stored on different request parts depending on the given session affinity implementation, but each request cannot have more than one such key. The exact key semantics is implementation dependent, but the built-in policies currently use `DestinationId` as the affinity key.

The current design doesn't require a key to uniquely identify the single affinitized destination. It's allowed to establish affinity to a destination group. In this case, the exact destination to handle the given request will be determined by the load balancer.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I misunderstanding this or is this just plain wrong? If the key is shared by more than 1 destination, we'll pick whichever one we happen to find first.

Copy link
Member Author

@Tratcher Tratcher Jan 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current policy implementations would pick the first one since we do a linear search, but the abstraction allows us to create keys that are not specific to a single destination. E.g. if you had sets of destinations a-1, a-2, b-1, b-2... you could make a policy that used the key a and returned both a-1 and a-2, then load balancing would choose between them.

Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
@Tratcher Tratcher enabled auto-merge (squash) January 11, 2023 20:18
@Tratcher Tratcher merged commit b311ca7 into main Jan 11, 2023
@Tratcher Tratcher deleted the tratcher/hashaffinity branch January 11, 2023 20:47
@davidfowl
Copy link
Contributor

This is an excellent change!

@davidfowl
Copy link
Contributor

davidfowl commented Jan 14, 2023

@Tratcher do we need to hardcode the ARR affinity cookie name for it to be compatible? Or should we document that if the name isn't set appropriately, it won't be compatible?

@Tratcher
Copy link
Member Author

@Tratcher Chris Ross FTE do we need to hardcode the ARR affinity cookie name for it to be compatible? Or should we document that if the name isn't set appropriately, it won't be compatible?

That's at least worth documenting, what was the name?

YARP doesn't have a default cookie name for session affinity, it must be specified in config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Affinity is broken with multiple YARP instances
6 participants