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

NE-1815: Activate dynamic cookies for dynamic server slots (DCM) #636

Merged

Conversation

alebedev87
Copy link
Contributor

@alebedev87 alebedev87 commented Nov 5, 2024

  • Introduced a "dynamic" option for "cookie" directive used on plain HTTP, edge and re-encrypt routes. This option activates the dynamic cookie creation per server, applicable only for dynamic server slots (static server slots set cookie values explicitly). Without this option, dynamic servers will not set a cookie value.

  • Added the "dynamic-cookie-key" directive to re-ecrypt routes to align the dynamic cookie handling across all route types (plain HTTP and edge already have "dynamic-cookie-key").This directive is used to generate a unique cookie value for a each route.

Set-Cookie header sent from dynamic servers before this change:

set-cookie: a84fef2b01006bb7036c768d4a819999=; Expires=Thu, 01-Jan-1970 00:00:01 GMT; path=/; HttpOnly

and after:

set-cookie: a84fef2b01006bb7036c768d4a819999=424f3944a7ac32ec; path=/; HttpOnly

Note: the dynamic cookie value is a 16 chars long hash which is different from the cookie set on the static servers (32 chars).

Previous PR on the same topic (dynamic cookies for DCM slots): openshift/origin#20557. This PR introduced "dynamic-cookie-key" directive for plain http/edge and passthrough backends in order to be able to disable the dynamic cookies for blueprint routes buy default and re-enable after a reload.

- Introduced a "dynamic" option for "cookie" directive used on plain HTTP,
  edge and re-encrypt routes. This option activates the dynamic cookie creation
  per server, applicable only for dynamic server slots (static server
  slots set cookie values explicitly). Without this option,
  dynamic servers will not set a cookie value.

- Added the "dynamic-cookie-key" directive to re-ecrypt routes to align the
  dynamic cookie handling across all route types (plain HTTP and edge
  already have "dynamic-cookie-key").This directive is used to generate
  a unique cookie value for a each route.
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 5, 2024
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 5, 2024

@alebedev87: This pull request references NE-1815 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

  • Introduced a "dynamic" option for "cookie" directive used on plain HTTP, edge and re-encrypt routes. This option activates the dynamic cookie creation per server, applicable only for dynamic server slots (static server slots set cookie values explicitly). Without this option, dynamic servers will not set a cookie value.

  • Added the "dynamic-cookie-key" directive to re-ecrypt routes to align the dynamic cookie handling across all route types (plain HTTP and edge already have "dynamic-cookie-key").This directive is used to generate a unique cookie value for a each route.

Set-Cookie header sent from dynamic servers before this change:

set-cookie: a84fef2b01006bb7036c768d4a819999=; Expires=Thu, 01-Jan-1970 00:00:01 GMT; path=/; HttpOnly

and after:

set-cookie: a84fef2b01006bb7036c768d4a819999=424f3944a7ac32ec; path=/; HttpOnly

Note: the dynamic cookie value is a 16 chars long hash which is different from the cookie set on the static servers (32 chars).

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot requested review from frobware and knobunc November 5, 2024 17:07
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 5, 2024

@alebedev87: This pull request references NE-1815 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

  • Introduced a "dynamic" option for "cookie" directive used on plain HTTP, edge and re-encrypt routes. This option activates the dynamic cookie creation per server, applicable only for dynamic server slots (static server slots set cookie values explicitly). Without this option, dynamic servers will not set a cookie value.

  • Added the "dynamic-cookie-key" directive to re-ecrypt routes to align the dynamic cookie handling across all route types (plain HTTP and edge already have "dynamic-cookie-key").This directive is used to generate a unique cookie value for a each route.

Set-Cookie header sent from dynamic servers before this change:

set-cookie: a84fef2b01006bb7036c768d4a819999=; Expires=Thu, 01-Jan-1970 00:00:01 GMT; path=/; HttpOnly

and after:

set-cookie: a84fef2b01006bb7036c768d4a819999=424f3944a7ac32ec; path=/; HttpOnly

Note: the dynamic cookie value is a 16 chars long hash which is different from the cookie set on the static servers (32 chars).

Previous PR on the same topic (dynamic cookies for DCM slots): openshift/origin#20557. This PR introduced "dynamic-cookie-key" directive for plain http/edge and passthrough backends in order to be able to disable the dynamic cookies for blueprint routes buy default and re-enable after a reload.

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 openshift-eng/jira-lifecycle-plugin repository.

Copy link
Contributor

openshift-ci bot commented Nov 5, 2024

@alebedev87: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 6, 2024

@alebedev87: This pull request references NE-1815 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

  • Introduced a "dynamic" option for "cookie" directive used on plain HTTP, edge and re-encrypt routes. This option activates the dynamic cookie creation per server, applicable only for dynamic server slots (static server slots set cookie values explicitly). Without this option, dynamic servers will not set a cookie value.

  • Added the "dynamic-cookie-key" directive to re-ecrypt routes to align the dynamic cookie handling across all route types (plain HTTP and edge already have "dynamic-cookie-key").This directive is used to generate a unique cookie value for a each route.

Set-Cookie header sent from dynamic servers before this change:

set-cookie: a84fef2b01006bb7036c768d4a819999=; Expires=Thu, 01-Jan-1970 00:00:01 GMT; path=/; HttpOnly

and after:

set-cookie: a84fef2b01006bb7036c768d4a819999=424f3944a7ac32ec; path=/; HttpOnly

Note: the dynamic cookie value is a 16 chars long hash which is different from the cookie set on the static servers (32 chars).

Previous PR on the same topic (dynamic cookies for DCM slots): openshift/origin#20557. This PR introduced "dynamic-cookie-key" directive for plain http/edge and passthrough backends in order to be able to disable the dynamic cookies for blueprint routes buy default and re-enable after a reload.

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 openshift-eng/jira-lifecycle-plugin repository.

@gcs278
Copy link
Contributor

gcs278 commented Nov 7, 2024

/assign

@gcs278
Copy link
Contributor

gcs278 commented Nov 7, 2024

@alebedev87 Generally everything looks fine to me. Tested it out too.

But do you know why they put dynamic-cookie-key in the passthrough backend section? Passthrough operates on layer 4 (except for reading SNI...), so it can't use cookies. What's the point of having that config in the backend for passthrough?

@alebedev87
Copy link
Contributor Author

alebedev87 commented Nov 8, 2024

But do you know why they put dynamic-cookie-key in the passthrough backend section? Passthrough operates on layer 4 (except for reading SNI...), so it can't use cookies. What's the point of having that config in the backend for passthrough?

No idea. I agree, I don't think it makes any sense on passthrough routes. I didn't want to remove anything though, just to make the PR easier and less "destructive".

Upd: may be, this was related to the implementation details of the route blueprints. They had a problem with route blueprints changing the backend name after a reload which impacted the cookie value. For, this reason they dynamically disabled the dynamic cookie for the route blueprint backends. May be, to do so, dynamic-cookie-key had to be present in the config (TBC).

@gcs278
Copy link
Contributor

gcs278 commented Nov 8, 2024

I didn't want to remove anything though, just to make the PR easier and less "destructive".

That's fair. We can clean up later, but it isn't hurting anything right now.

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2024
Copy link
Contributor

openshift-ci bot commented Nov 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gcs278

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 6c988a5 into openshift:master Nov 8, 2024
8 checks passed
@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

Distgit: ose-haproxy-router-base
This PR has been included in build ose-haproxy-router-base-container-v4.18.0-202411081737.p0.g6c988a5.assembly.stream.el9.
All builds following this will include this PR.

@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

Distgit: openshift-enterprise-haproxy-router
This PR has been included in build openshift-enterprise-haproxy-router-container-v4.18.0-202411081737.p0.g6c988a5.assembly.stream.el9.
All builds following this will include this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants