Fix a race condition with initial secret_key.py creation #6775
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SpecLad
force-pushed
the
atomic-secret-key
branch
from
a71e990 to
65a042f
Compare
Codecov Report
@@ Coverage Diff @@
## develop #6775 +/- ##
===========================================
+ Coverage 82.43% 82.45% +0.01%
===========================================
Files 366 366
Lines 39833 39761 -72
Branches 3545 3545
===========================================
- Hits 32835 32783 -52
+ Misses 6998 6978 -20
|
|
@SpecLad , let's fix the conflict and simplify the code as discussed. Ping me in slack when it is ready. |
Currently, it's possible (even if unlikely) for multiple backend processes to create and use different Django `SECRET_KEY` values, because the following scenario is possible: * process 1: fail to import secret_key.py * process 2: fail to import secret_key.py * process 1: generate a new secret_key.py * process 1: import secret_key.py * process 2: generate a new secret_key.py * process 2: import secret_key.py Fix this by making it so that `secret_key.py` is created atomically, and never overwritten if it already exists. In addition, only generate the secret key if the import fails due to the module not being found, since other failure reasons suggest incorrect configuration or data corruption, and so require administrator attention.
SpecLad
force-pushed
the
atomic-secret-key
branch
2 times, most recently
from
f5a1d32 to
dc999b3
Compare
|
@Keramblock Thanks for the report. What filesystem are you using? |
@SpecLad that is default azure RWX volume: here is pvc spec: |
|
Hrm. Looks like Azure Files doesn't support hard links when mounted over SMB. Could you try mounting over NFS instead? |
|
We dont have NFS server there(and I am not sure if Azure provides production ready NFS storageclass for k8s at all),but is it really necessary to use hardlink here? P.S. for now I just copied files from other server and it is working fine |
Merged
nmanovic
added a commit
that referenced
this pull request
Sep 6, 2023
### Added - Gamma correcton filter (<#6771>) - Introduced the feature to hide or show objects in review mode (<#6808>) ### Changed - \[Helm\] Database migrations are now executed as a separate job, rather than in the server pod, to mitigate the risk of data corruption when using multiple server replicas (<#6780>) - Clicking multiple times on icons in the left sidebar now toggles the corresponding popovers open and closed (<#6817>) - Transitioned to using KeyDB with FLASH for data chunk caching, replacing diskcache (<#6773>) ### Removed - Removed outdated use of hostnames when accessing Git, OpenCV, or analytics via the UI (<#6799>) - Removed the Feedback/Share component (<#6805>) ### Fixed - Resolved the issue of the canvas zooming while scrolling through the comments list in an issue (<#6758>) - Addressed the bug that allowed for multiple issue creations upon initial submission (<#6758>) - Fixed the issue of running deep learning models on non-JPEG compressed TIFF images (<#6789>) - Adjusted padding on the tasks, projects, and models pages (<#6778>) - Corrected hotkey handlers to avoid overriding default behavior when modal windows are open (<#6800>) - Resolved the need to move the mouse to activate brush or eraser effects; a single click is now sufficient (<#6800>) - Fixed a memory leak issue in the logging system (<#6804>) - Addressed a race condition that occurred during the initial creation of `secret_key.py` (<#6775>) - Eliminated duplicate log entries generated by the CVAT server (<#6766>)
mikhail-treskin
pushed a commit
to retailnext/cvat
that referenced
this pull request
Oct 25, 2023
Currently, it's possible (even if unlikely) for multiple backend processes to create and use different Django `SECRET_KEY` values, because the following scenario can happen: * process 1: fail to import `secret_key.py` * process 2: fail to import `secret_key.py` * process 1: generate a new `secret_key.py` * process 1: import `secret_key.py` * process 2: generate a new `secret_key.py` * process 2: import `secret_key.py` Fix this by making it so that `secret_key.py` is created atomically, and never overwritten if it already exists. In addition, only generate the secret key if the import fails due to the module not being found, since other failure reasons suggest incorrect configuration or data corruption, and so require administrator attention.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and context
Currently, it's possible (even if unlikely) for multiple backend processes to create and use different Django
SECRET_KEYvalues, because the following scenario can happen:secret_key.pysecret_key.pysecret_key.pysecret_key.pysecret_key.pysecret_key.pyFix this by making it so that
secret_key.pyis created atomically, and never overwritten if it already exists.In addition, only generate the secret key if the import fails due to the module not being found, since other failure reasons suggest incorrect configuration or data corruption, and so require administrator attention.
How has this been tested?
Manual testing.
Checklist
developbranch[ ] I have updated the documentation accordingly[ ] I have added tests to cover my changes[ ] I have linked related issues (see GitHub docs)[ ] I have increased versions of npm packages if it is necessary(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.