-
-
Notifications
You must be signed in to change notification settings - Fork 746
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
correct bcrypt to fix the known issue with 4.1.2 #6210
base: master
Are you sure you want to change the base?
Conversation
The fixed requirements file is input to the other requirements files, so you need to regenerate the top level and component requirements files. |
Can you add a changelog entry for the PR please? |
@nzlosh I have updated the changelog. Could you pls take a look. Thank you! |
@nzlosh One of the CI test failed, looking at logs , it doesn't seem to be related to this change. I noticed that st2api and st2auth failed to start as the ports are not available . Here is the log from the test run st2api.log st2auth Same CI check passed for the previous commit , before change log was added . Here is the older CI test result for Orquesta on py3.8 https://github.com/StackStorm/st2/actions/runs/9330762609/job/25684689576 Can someone retrigger the CI checks? Looks to be a transient issue. |
Looks good. I'm going to update the pantsbuild dependencies (the lockfile) here and then I'll merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking until I update the pants lockfile.
OK. Looking further into this:
So, I think the I'm researching the issue with passlib+bcrypt. The issue was introduced in bcrypt 4.1.0, which was yanked because of it. This PR moved some attributes around, and added a
Supposedly, the issue was resolved in bcrypt 4.1.1 by renaming But, the error you posted is still present in at least bcrypt 4.1.1 and 4.1.2. That error occurs because bcrypt 4.1.0+ removed
It should just be a logged warning message though, and the version var is only used for a debug log message. So, that error message should be a red herring. @sravs-dev are there any other logged error messages? |
Here's the upstream passlib issue: https://foss.heptapod.net/python-libs/passlib/-/issues/190 I'm still not sure why st2auth would fail when using bcrypt 4.1.* however since that warning is not fatal. |
Here's an upstream bcrypt issue about the passlib warning message: pyca/bcrypt#684 So, we need to find a different root cause for the reported "st2auth fails to startup" issue, and why downgrading bcrypt would ameliorate that. |
@cognifloyd Thank you so much for digging into this. I couldn't find any other error message in st2auth logs. Let me rerun st2 on rhel9 and double check. |
I was testing st23.9 dev on Rhel9, st2auth fails to startup with following error. Downgrading bcrypt to fix the known bug in 4.1.2