Skip to content

add session persistence support in destination rule #3502

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
327 changes: 327 additions & 0 deletions kubernetes/customresourcedefinitions.gen.yaml

Large diffs are not rendered by default.

15 changes: 15 additions & 0 deletions networking/v1/destination_rule_alias.gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

425 changes: 311 additions & 114 deletions networking/v1alpha3/destination_rule.pb.go

Large diffs are not rendered by default.

105 changes: 104 additions & 1 deletion networking/v1alpha3/destination_rule.pb.html

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 30 additions & 0 deletions networking/v1alpha3/destination_rule.proto
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,35 @@ message LoadBalancerSettings {
uint64 minimum_ring_size = 4 [deprecated=true];
};

// Session persistence settings for the destination. Use this for hard session affinity.
Copy link
Member

Choose a reason for hiding this comment

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

we need very clear docs on the difference between this and consist hash

Copy link
Contributor

Choose a reason for hiding this comment

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

I think both 'hard affinity' and 'consistent hash' are well defined - but what we do ( 'by encoding the IP:port of the service in a cookie/header' ) needs to be clearly understood by users.

message SessionPersistence {
// Type defines the type of session persistence such as through
// the use of a header or cookie. Default is COOKIE.
enum Type {
// Use a cookie to store the session affinity information.
COOKIE = 0;
// Use a header to store the session affinity information.
HEADER = 1;
Copy link
Member

Choose a reason for hiding this comment

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

how can this possibly be used? how would a client know the opaque value to put here?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not that opaque unfortunately - just an IP:port in a proto. I believe Envoy returns the header on the first request, and user is expected to set it in the next request.

Agree with your comment: it needs to be super clear in the comments, and also the case where we can't connect to the IP:port - don't remember if we return an error or just go to another endpoint and return the new updated header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Envoy returns the header with ip:port encoded and the client has to repeatedly send the header for every request.

The behaviour of Envoy when the ip:port is not found is dependent on the strict flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update the docs to make all of this clear.

Copy link
Member

Choose a reason for hiding this comment

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

This is weak feature, mostly we istio should be transparent of applications

}

// The type of session persistence to use.
Type type = 1;

// The name of the header or cookie to use for session persistence.
Copy link
Member

Choose a reason for hiding this comment

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

Ah, see it. It is reusable field. Why donot we make header and cookie oneof

string name = 2;

// The cookie settings for session persistence.
Cookie cookie = 3;

message Cookie {
Copy link
Member

Choose a reason for hiding this comment

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

no cookie name needed?

// Lifetime of the cookie. If specified, a cookie with the TTL will be
// generated if the cookie is not present. If the TTL is present and zero,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember exactly - but there were some concerns with non-session cookies ( around privacy regulations - I think you are required to show the consent screen and all kind of other things ).

I would very much prefer if we start without this setting and only with session cookies, it's a very complicated problem, best to avoid Istio starting to set persistent cookies and dealing with privacy.

// the generated cookie will be a session cookie.
// +protoc-gen-crd:duration-validation:none
google.protobuf.Duration ttl = 3;
}
}

// (-- TODO: Enable Subset load balancing after moving to v2 API Also
// look into enabling Priotity based load balancing for spilling over
// from one priority pool to another. --)
Expand All @@ -503,6 +532,7 @@ message LoadBalancerSettings {
oneof lb_policy {
SimpleLB simple = 1;
ConsistentHashLB consistent_hash = 2;
SessionPersistence session_persistence = 6;
}

// Locality load balancer settings, this will override mesh-wide settings in entirety, meaning no merging would be performed
Expand Down
42 changes: 42 additions & 0 deletions networking/v1alpha3/destination_rule_deepcopy.gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 22 additions & 0 deletions networking/v1alpha3/destination_rule_json.gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions networking/v1beta1/destination_rule_alias.gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.