Adding initial Provisional GEP-3798 for client ip based session persistence

[arihantg]

/kind gep

First iteration of provisional GEP - focused on "What?"/"Why?"/"Who?"

Relates to #3798

Does this PR introduce a user-facing change?:

NONE

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: arihantg / name: Arihant Bachhawat (020660f, 7f7e51c, df3a4ee, 246e56a, f677ed9)
• ✅ login: shaneutt / name: Shane Utt (7396511)

[k8s-ci-robot]

Welcome @arihantg!

It looks like this is your first PR to kubernetes-sigs/gateway-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/gateway-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @arihantg. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[aojea]

I never seen this use case, can you elaborate how it works?

[arihantg]

This will be a mask to be applied on client IP. This may be used to persist clients from a subnet to the same server.
for a request originating from 100.20.30.40 with ipMask as 24 it would mean requests from all ip in range 100.20.30.40/24 would be persisted to same backend

[aojea]

is there any implementation already that does this? why would you like to make session affinity per subnet for a loadbalancer? theoretically session affinity is to guarantee the connection sticks to the same backend but in a subnet this will be different connections, is there any share session property based on connection use cases?

[arihantg]

AVI supports this property with ipmask as optional field.

[DamianSawicki]

theoretically session affinity is to guarantee the connection sticks to the same backend but in a subnet this will be different connections, is there any share session property based on connection use cases?

+1

A fixed-size mask only makes sense under the assumption that the entire IP address space of sources of the incoming traffic is neatly split into fixed-size subnets. I guess it excludes the public internet, but is it even common for internal networks?

[DamianSawicki]

theoretically session affinity is to guarantee the connection sticks to the same backend but in a subnet this will be different connections, is there any share session property based on connection use cases?

Another thought: perhaps the proposed feature should not be part of the sessionPersistence field, but rather of some new sharding field?

[shaneutt]

Situations like this are why we advise focusing on the "What?", "Why?" and "Who?" and not adding any "How?" in the first iteration as it can make that first PR get swamped. I see the contentious bits have been removed now.

@arihantg please add this comment thread to the references section of the GEP Metadata, noting that we need to re-review this feedback before this transitions to Experimental and pull in these stakeholders. Then we can consider this comment resolved.

[aojea]

is there any max value?
what the zero value mean? it sticks forever?

[arihantg]

Max value could be 128( max prefix-length in IPv6 ) to support IPv6 addresses. When set to 0, all requests will be sent to the same server for the period of absoluteTimeout

[aojea]

I'm referring to the absoluteTimeout field :)

[arihantg]

Sorry misunderstood the line number. I think timeout of 0 should not be applicable to any persistence type unless it is documented 0 means to stick forever. Max timeout could be implementation dependent since currently there is no CEL on absoluteTimeout

[sunjayBhatia]

gateway-api/apis/v1/shared_types.go

Lines 818 to 825 in c3dd6c3

	// AbsoluteTimeout defines the absolute timeout of the persistent
	// session. Once the AbsoluteTimeout duration has elapsed, the
	// session becomes invalid.
	//
	// Support: Extended
	//
	// +optional
	AbsoluteTimeout *Duration `json:"absoluteTimeout,omitempty"`

(yep, for reference no existing max value for the field)

[aojea]

it is documented 0 means to stick forever

yeah, you should document this in the API, if 0 will mean forever or nothing, but since is a pointer, a nil value means already nothing ...

[DamianSawicki]

Well, I think the zero value makes no sense here and we should have included validation duration > 0 in GEP-1619. However, the nil value does makes sense. In GEP-1619, we have validation requiring absoluteTimeout to be non-nil for Permament cookie lifetimeType, but it can be nil for Session cookie lifetimeType as such cookies are anyway deleted at the end of a session.

For context, cookies with unset Max-Age or Expires attributes are interpreted as session cookies, while those with one of these set are called permanent cookies, so the mapping nil -> forever would be consistent with that.

[shaneutt]

@arihantg same thing here: #3844 (comment)

[DamianSawicki]

It should be clarified whether we aim for session affinity or session persistence here. Probably, more justification for the subnet mask is needed.

[DamianSawicki]

theoretically session affinity is to guarantee the connection sticks to the same backend but in a subnet this will be different connections, is there any share session property based on connection use cases?

+1

A fixed-size mask only makes sense under the assumption that the entire IP address space of sources of the incoming traffic is neatly split into fixed-size subnets. I guess it excludes the public internet, but is it even common for internal networks?

[DamianSawicki]

GEP-1619 provides an overview of implementations supporting session persistence in different forms and refrains from including in the specification low-level details, e.g. cookie attributes. Could you please include a similar overview in the present GEP? If AVI is the only implementation supporting the subnet mask, I would be against including this field in Gateway API.

[DamianSawicki]

The ring hash algorithm makes session affinity more robust, but such a session affinity can still be lost when a server is added to or deleted from the serving pool, so it does not qualify as session persistence:

With the ring partitioned appropriately, the addition or removal of one host from a set of N hosts will affect only 1/N requests.

(https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/upstream/load_balancing/load_balancers#ring-hash)

[sunjayBhatia]

I missed that the previous session persistence GEP was so detailed about the difference between session affinity and persistence

we can maybe update the Envoy example here to point to example APIs which can be used to implement session persistence more strictly, albeit likely requiring some creative configuration of Envoy, e.g. load balancer subsets to enable selecting specific upstream endpoints using metadata: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/upstream/load_balancing/subsets

(Envoy does not actually support downstream IP based session affinity natively: https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/stateful_session_filter#config-http-filters-stateful-session and I'm struggling to see how to implement it with the APIs as-is, will likely need to add a significant feature to Envoy)

[DamianSawicki]

I'm wondering, what's your intended use case / motivation for Client IP-based session persistence? Is it to reflect Client IP Persistence from AVI?

Speaking about Envoy, from the links you sent, Envoy subsets could be a building block of such a solution, but it does not exist currently. Stateful sessions OTOH rely on headers or cookies, similarly to what we have in GEP-1619. Is it important to you for the solution to be header-free and cookie-free?

I suppose so because if the solution relies on storing server identity on the client side inside a cookie or header, it won't be feasible to route other clients with the same IP / subnet to the same server. So I think you probably need a new sessionPersistence.type that I'll tentatively call LoadBalancer (this is what AVI documentation calls "locally stored persistence methods" and NGINX Plus calls "sticky learn" method relying on storing sessions in a shared memory zone). With that, we could specify that the key needed to find the server is the client IP.

I guess it could be configured via a config like the following.

sessionPersistence:
  type: "LoadBalancer"
  ...
  key:
    type: "ClientIP"
    clientIPMask: 24

The question would be what's the actual support for such a feature across various implementations.

[arihantg]

I do want to reflect client ip persistence from avi .Idea is also to keep the method of persistence header and cookie-free so that' why a new SessionPersistenceType: "ClientIP" is being defined. clientIPMask can be an optional field or omitted from API if not used by many implementations.

I prefer sessionPersistence.type to be "ClientIP" since it is much descriptive for the type of persistence we are trying to implement.

Netscaler , Azure , F5 are some examples which does support client or source IP persistence.

[DamianSawicki]

I do want to reflect client ip persistence from avi .Idea is also to keep the method of persistence header and cookie-free so that' why a new SessionPersistenceType: "ClientIP" is being defined. clientIPMask can be an optional field or omitted from API if not used by many implementations.

Great to understand it; thanks so much for the clarification!

Netscaler , Azure , F5 are some examples which does support client or source IP persistence.

Azure has a warning on the page you linked saying that this feature only offers session affinity:

When Load Balancer backend pool members change either by removing or adding a virtual machine, the distribution of client requests is recomputed. You can't depend on new connections from existing clients to end up at the same server.

I prefer sessionPersistence.type to be "ClientIP" since it is much descriptive for the type of persistence we are trying to implement.

Yeah, I see your point. But what if someone wants to have "ClientIPPort" or "ClientIPPortProtocol" in the future? Or perhaps destination IP and/or port as well? Or some extra information from various headers (including cookies)? IMHO such granular configuration options (possibly even allowing free text as in NGINX Plus Sticky learn) would fit better inside a dedicated field other than the existing sessionPersistence.type field, but let's wait for others to chime in.

[arihantg]

I think this properties ("ClientIPPort" or "ClientIPPortProtocol") along with the ipmask can go in a struct like ClientIPConfig similar to Cookieconfig in SessionPersistence struct.

[DamianSawicki]

Same here. ip_hash from NGINX offers session affinity, but not session persistence, which is only offered by NGINX Plus.

The documentation for the hash directive say:

Note that adding or removing a server from the group may result in remapping most of the keys to different servers. ... If the consistent parameter is specified, the ketama consistent hashing method will be used instead. The method ensures that only a few keys will be remapped to different servers when a server is added to or removed from the group.

AFAIK, the same applies to the ip_hash directive:

If one of the servers needs to be temporarily removed, it should be marked with the down parameter in order to preserve the current hashing of client IP addresses.

[DamianSawicki]

As per the nomenclature established in #1619, this feature is being referred as Session Affinity .

GEP-1619 makes a clear distinction between (weak) session affinity and session persistence. The former is explicitly outside the scope of GEP-1619 and accordingly we shouldn't add session affinity support to the sessionPersistence field via the present GEP-3798. What's the intention of the present GEP? Add Client IP session persistence to Gateway API or introduce session affinity of type Client IP?

[sunjayBhatia]

I think this is just an error with copied context from a previous copy of the session persistence GEP, the goal of this is strictly session persistence as the current form of GEP #1619 lays it out today, we can remove the last sentence's reference to Session Affinity

[DamianSawicki]

the goal of this is strictly session persistence as the current form of GEP #1619 lays it out today, we can remove the last sentence's reference to Session Affinity

Thanks so much for the clarification and updating the text!

[LiorLieberman]

/ok-to-test

[arihantg]

/test pull-gateway-api-verify

[robscott]

Thanks @arihantg!

[DamianSawicki]

Given the tight deadline, it's a great idea to focus on "What" and defer the "How" for later.

[DamianSawicki]

@aojea Are there any guarantees that k8s service's .spec.sessionAffinity is persistent are required by GEP-1619, i.e. the affinity is not lost when the number of serving pods changes?

[sunjayBhatia]

it looks like we have general agreement on the What here, Im inclined to approve, given the outstanding comments will be resolved

[robscott]

@arihantg For the purposes of Gateway API, we're really looking for multiple Gateway API implementations that would implement this feature. As far as I can tell, only 1 of the implementations in this list meets that criteria. If we can't find more, I don't think we'll be able to accept this to Gateway API.

As a reminder, we expect all new Gateway API features to be implementable by at least 3 implementations, and strongly prefer that those represent at least 2 distinct underlying dataplanes.

[arihantg]

Is this specific to subnet Mask ? I am ok to omit subnet mask from GEP since it is not widely implemented.
I think we can still go ahead with ClientIP as sessionPersistence type since this server persistence mode is supported across multiple implementations via different methods, Examples : GKE, Emissary, GlOO

[DamianSawicki]

this server persistence mode is supported across multiple implementations via different methods, Examples : GKE, Emissary, GlOO

Similarly to Envoy and NGINX that we discussed above, unfortunately the CLIENT_IP session affinity type in GKE is not session persistence in the sense of GEP-1619, it's (weak) session affinity. The linked documentation for Emissary and GIOO explains that these features are also hash-based methods (utilizing Maglev of Ring Hash algorithms), and as such they must also be affinity rather than persistence.

[shaneutt]

I understand Rob's concerns. I think if we move forward with this into provisional, it will be prudent NOT to go straight into trying to design this but rather finding some collaborators who would be interested in implementing this which can help assist in the design to cover more implementations.

[howardjohn]

+1 I have concerns with this GEP that it doesn't seem to be implementable. Or maybe it is but the GEP in its current form hasn't laid this out.

[shaneutt]

I think this one may end up in a bit of a tough spot. We ended up doing too much discussion on the "How?" during this cycle which lost us time, and it's not entirely clear that there is consensus on the "What?" and "Why?" yet.

In my own review, I do think the "What?" and "Why?" are fairly reasonable. I have a couple of comments about planning and housekeeping.

Ultimately I agree with @robscott's point that the "Who?" seems a bit unclear. This was also difficult with GEP-1619. If we move forward with this into Provisional, I think going straight into implementation details would not be advisable, but rather prioritize reaching out within the community for other representatives of implementations which can support this and help find the common denominators in terms of "How?" this gets implemented.

[shaneutt]

I understand Rob's concerns. I think if we move forward with this into provisional, it will be prudent NOT to go straight into trying to design this but rather finding some collaborators who would be interested in implementing this which can help assist in the design to cover more implementations.

[robscott]

@arihantg Thanks for all your work on this! Unfortunately as reviewers pointed out on this PR, the form of Client IP Session Persistence described in this GEP is meaningfully different from the Client IP Session Affinity supported by many dataplanes underlying Gateway API implementations. I don't have a lot of visibility into implementations that would support this mode, but definitely let us know if you can find any additional ones.

Until we can find a viable path to implementability here for 3+ Gateway API implementations, and preferably 2+ underlying dataplanes, I don't think we can move this one forward in the v1.4 release. Please let us know if you're able to find any that meet this criteria and we can pick this back up.

[shaneutt]

@arihantg Thanks for all your work on this! Unfortunately as reviewers pointed out on this PR, the form of Client IP Session Persistence described in this GEP is meaningfully different from the Client IP Session Affinity supported by many dataplanes underlying Gateway API implementations. I don't have a lot of visibility into implementations that would support this mode, but definitely let us know if you can find any additional ones.

Until we can find a viable path to implementability here for 3+ Gateway API implementations, and preferably 2+ underlying dataplanes, I don't think we can move this one forward in the v1.4 release. Please let us know if you're able to find any that meet this criteria and we can pick this back up.

@arihantg we (maintainers) got together and discussed this one today. There is general agreement with Rob's point above, where we all feel its fairly unclear if there are multiple implementations out there that can support this mode. We concluded that for now, we will merge this as provisional so as to allow you some time to work on that problem, and we'd like to support you in that. Please reach out to me on Kubernetes Slack (I'm @shane), I would love to discuss this with you in more depth and see if together we can't find some interested parties who can support the effort.

Please note that I added a note and disclaimer to the GEP however, highlighting that we struggled to see interest from multiple implementations on the initial PR. As such we just want to be clear that if we can't navigate that within the timeframe laid out in the v1.4.0 milestone, it may ultimately become appropriate to mark this GEP as Deferred and park it until a later time.

Again, please do reach out to me on Slack and let's chat and see what we can come up with. But for now, we'll get the initial "What?" and "Why?" merged so we can iterate from there:

/approve
/lgtm
/unhold

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arihantg, shaneutt, sunjayBhatia

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• geps/OWNERS [shaneutt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment