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

Adding Stickiness option for Target Groups #711

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

drmudgett
Copy link

Adds the option to enable Sticky Sessions to the ALB, defaults to false (off).
Closes #267

@szuecs
Copy link
Member

szuecs commented Aug 19, 2024

All golden files tests would have a CF stack change because of the new parameter. I am not sure if you can do this such that default is nil instead of "false", then you don't need to update tests.

drewmudgett and others added 3 commits August 19, 2024 16:01
Signed-off-by: drmudgett <[email protected]>
Signed-off-by: Drew Mudgett <[email protected]>
Signed-off-by: Drew Mudgett <[email protected]>
Signed-off-by: Drew Mudgett <[email protected]>
@drmudgett
Copy link
Author

All golden files tests would have a CF stack change because of the new parameter. I am not sure if you can do this such that default is nil instead of "false", then you don't need to update tests.

I've updated the tests and they're passing locally now, thanks.

@szuecs
Copy link
Member

szuecs commented Aug 20, 2024

All golden files tests would have a CF stack change because of the new parameter. I am not sure if you can do this such that default is nil instead of "false", then you don't need to update tests.

I've updated the tests and they're passing locally now, thanks.

We have to check if we can allow this.
Best would be if "false" that there is no parameter change in the CF stack.

The problem that might happen is that cloud load balancers could be forced to be new created.
We had a similar problem in the past that lead to downtime of some production applications.

@drmudgett
Copy link
Author

All golden files tests would have a CF stack change because of the new parameter. I am not sure if you can do this such that default is nil instead of "false", then you don't need to update tests.

I've updated the tests and they're passing locally now, thanks.

We have to check if we can allow this. Best would be if "false" that there is no parameter change in the CF stack.

The problem that might happen is that cloud load balancers could be forced to be new created. We had a similar problem in the past that lead to downtime of some production applications.

Ok, I think I've got it working the way you'd like now.

},
{
"parameterKey": "Stickiness",
"parameterValue": "false"
Copy link
Member

Choose a reason for hiding this comment

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

I guess these fields should not be there if we have not sticky configuration.

Copy link
Author

Choose a reason for hiding this comment

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

I've removed the line that the linter was complaining about, but I still see this error locally:

Error: kubernetes/pods.go:103:9: S1009: should omit nil check; len() for []k8s.io/api/core/v1.ContainerStatus is defined as zero (gosimple)
	return p.Status.ContainerStatuses != nil &&

But it looks like that error was there before my commits (i locally tested against main).

Copy link
Member

@szuecs szuecs Aug 21, 2024

Choose a reason for hiding this comment

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

that's a new staticcheck version, so would be great if you just fix it :)
https://staticcheck.dev/docs/checks#S1009

Copy link
Author

Choose a reason for hiding this comment

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

no problem, this is fixed in the latest commit

Copy link
Member

@szuecs szuecs Aug 21, 2024

Choose a reason for hiding this comment

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

but the result params should not change and as far as I read the code.
This should not be there in the PR:

},
  {
    "parameterKey": "Stickiness",
    "parameterValue": "false"

Ah and please add a test that actually tests the feature with "true" and "false" set :)

Drew Mudgett added 2 commits August 21, 2024 11:30
Signed-off-by: Drew Mudgett <[email protected]>
Signed-off-by: Drew Mudgett <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sticky Session
3 participants