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.

Sign up for GitHub

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

Jump to bottom

fix(authproxy): cookie secret creation and reference using env valueFrom #180

Merged
merged 5 commits into from
Aug 12, 2024
Merged

fix(authproxy): cookie secret creation and reference using env valueFrom #180

merged 5 commits into from
Aug 12, 2024

Conversation

elias-gb
Copy link
Contributor

@elias-gb elias-gb commented Aug 7, 2024 •
edited
Loading

Description

Fix

  • Added relevant cookie secret.
  • Added test for cookie secret.

Motivations

There was no cookie secret to be looked up in subsequent helm upgrade.

andrewazores
andrewazores reviewed Aug 7, 2024
View reviewed changes
Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

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

Hi,

Thanks for the contribution! Could you detail how to reproduce the bug you ran into? It sounds like you did an install and then an upgrade, and that's when you hit a problem with the Secret?

.gitignore Outdated Show resolved Hide resolved
@elias-gb
Copy link
Contributor Author

elias-gb commented Aug 7, 2024

Hi,

Thanks for the contribution! Could you detail how to reproduce the bug you ran into? It sounds like you did an install and then an upgrade, and that's when you hit a problem with the Secret?

There is no secrets created for cookies during both the initial install and upgrades. We noticed the issue as you described when we tried to install and upgrade. Each time it tried to recreate the cookie secrets on authproxy.

@andrewazores
Copy link
Member

andrewazores commented Aug 7, 2024 •
edited
Loading

@elias-gb could you test the latest commit I pushed and ensure it works on your setup?

The Secret you've defined should get properly created on installation still, but it will get recreated with a new secret value on upgrade. This shouldn't be too much of a problem, it only means that after upgrade any users' browsers will have an invalid session and will need to re-login. That was already the case before this PR anyway since the Secret was never actually created. At least now the Secret is properly created and referenced in a better way.

@andrewazores andrewazores changed the title fix(secret): cookie secret fix(authproxy): cookie secret creation and reference using env valueFrom Aug 7, 2024
@elias-gb
Copy link
Contributor Author

elias-gb commented Aug 7, 2024

@elias-gb could you test the latest commit I pushed and ensure it works on your setup?

The Secret you've defined should get properly created on installation still, but it will get recreated with a new secret value on upgrade. This shouldn't be too much of a problem, it only means that after upgrade any users' browsers will have an invalid session and will need to re-login. That was already the case before this PR anyway since the Secret was never actually created. At least now the Secret is properly created and referenced in a better way.

Tested it now. Looks fine :)

tthvo
tthvo reviewed Aug 7, 2024
View reviewed changes
Copy link
Member

@tthvo tthvo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested both on k8s (minikube) and OpenShift Local. Looks good^^ And nice catch indeed!!

ebaron
ebaron approved these changes Aug 12, 2024
View reviewed changes
Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you for your contribution @elias-gb!

@ebaron ebaron merged commit 61cf6a0 into cryostatio:main Aug 12, 2024
6 checks passed
mergify bot pushed a commit that referenced this pull request Aug 12, 2024
@elias-gb @mergify
fix(authproxy): cookie secret creation and reference using env valueF…
6df78f5 
…rom (#180)

* Added a cookie secret.

* Added test for creation of cookie secret.

* fix(authproxy): reference cookie secret using env valueFrom

* don't recreate secret on upgrade

* restore helper

---------

Co-authored-by: Andrew Azores <aazores@redhat.com>
(cherry picked from commit 61cf6a0)
@mergify mergify bot mentioned this pull request Aug 12, 2024
Merged
@elias-gb elias-gb deleted the cookie-secret branch August 13, 2024 13:54
andrewazores pushed a commit that referenced this pull request Aug 13, 2024
@mergify @elias-gb
fix(authproxy): cookie secret creation and reference using env valueF…
2edf42d 
…rom (#180) (#183)

Co-authored-by: Andrew Azores <aazores@redhat.com>
(cherry picked from commit 61cf6a0)

Co-authored-by: elias-gb <elias@ardoq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tthvo tthvo tthvo left review comments

@ebaron ebaron ebaron approved these changes

@andrewazores andrewazores Awaiting requested review from andrewazores

Assignees
No one assigned
Labels
backport fix safe-to-test
Projects
4.0.0 release
Status: Done
3.0.1 release
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@elias-gb @andrewazores @ebaron @tthvo