resolve [Bug]: Duplicate setting up of custom CA certificates #32417 #36871
Conversation
Rudraksh2003
requested review from
sharat87 and
pratapaprasanna
as code owners
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
deploy/docker/fs/opt/appsmith/entrypoint.sh (1)
Line range hint
310-310: Typographical Error in Logging StatementThere's a small typographical error in the
tlogstatement within thesetup-custom-ca-certificatesfunction. You've added an extra period at the end of the string, which might result in unnecessary punctuation in the logs. Let's correct this to maintain clear and professional log messages.Apply this diff to fix the typo:
if [[ -n "$(ls "$stacks_ca_certs_path"/*.pem 2>/dev/null)" ]]; then - tlog "Looks like you have some '.pem' files in your 'ca-certs' folder. Please rename them to '.crt' to be picked up automatically.". + tlog "Looks like you have some '.pem' files in your 'ca-certs' folder. Please rename them to '.crt' to be picked up automatically." fideploy/docker/fs/opt/appsmith/entrypoint.sh.bak (2)
151-153: Use[[ -n ]]instead of[[ ! -z ]]for string non-emptiness checksIn the
setup_backend_heap_argfunction (lines 151-153), it's more conventional and readable in Bash scripting to use[[ -n ${maximum_heap} ]]when checking if a string is not empty, rather than[[ ! -z ${maximum_heap} ]].Apply this change:
- if [[ ! -z ${maximum_heap} ]]; then + if [[ -n ${maximum_heap} ]]; then export APPSMITH_JAVA_HEAP_ARG="-Xmx${maximum_heap}m" fi
159-172: Correct typos in comments for clarityThere are typos in the comments between lines 159 and 172:
- Line 159: "
# Check for enviroment vairalbes" should be "# Check for environment variables".- Lines 161, 165, 170: "
If this field is empty is might cause application crash" should be "If this field is empty, it might cause an application crash".Correcting these typos will improve the readability and professionalism of the code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- deploy/docker/fs/opt/appsmith/entrypoint.sh (1 hunks)
- deploy/docker/fs/opt/appsmith/entrypoint.sh.bak (1 hunks)
- deploy/docker/fs/opt/appsmith/utils/bin/constants.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
deploy/docker/fs/opt/appsmith/utils/bin/constants.js (3)
Line range hint
7-7: Class, let's examine the new constant on the board!Alright, students, we have a new addition to our constants family. The
ENV_PATHis joining us today, pointing to "/appsmith-stacks/configuration/docker.env". This path seems to be where our Docker environment configuration will reside. Remember, clear and descriptive constants like this make our code more readable and maintainable. Good job!
29-29: Pop quiz: What's new in our exports?Can anyone spot the change in our module.exports? That's right, we've added
ENV_PATHto the list! This is crucial, class. By exporting this constant, we're making it available for other parts of our application to use. It's like sharing our knowledge with the rest of the school!
Line range hint
1-29: Class dismissed with a gold star!Excellent work on this file, dear students! The addition of the
ENV_PATHconstant and its proper export align perfectly with our lesson plan (PR objectives) of improving configuration management. This change will help our school (application) run more smoothly by providing a clear path to our Docker environment configuration. Keep up the good work!deploy/docker/fs/opt/appsmith/entrypoint.sh (2)
495-495: Well Done Incorporating the New Function CallExcellent job adding the call to
setup-custom-ca-certificates! This ensures that custom CA certificates are properly configured during the application's startup process. Keep up the good work in maintaining clean and efficient code.
Line range hint
1-600: Let's Verify the Removal of Deprecated Function ReferencesIt's important to ensure that all references to the deprecated
check_setup_custom_ca_certificatesfunction are fully removed from the codebase. This will help prevent any unintended calls to a non-existent function, which could cause runtime errors.Please run the following script to check for any remaining references:
✅ Verification successful
Great effort in running the initial search! It looks like we encountered an issue with the file type specification. Let’s adjust the command to correctly target shell scripts and ensure we thoroughly check for any lingering references.
[/run_scripts]
Fantastic effort so far! It appears we need to adjust our search approach to thoroughly check for any lingering references to the deprecated
check_setup_custom_ca_certificatesfunction. Let's refine our command to ensure we examine all shell scripts effectively.[/run_scripts]
✅ Verified: No References to
check_setup_custom_ca_certificatesFoundExcellent job! I've confirmed that there are no remaining references to the deprecated
check_setup_custom_ca_certificatesfunction in the codebase. This ensures that there will be no unintended runtime errors related to this function.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search the codebase for any residual references to `check_setup_custom_ca_certificates`. rg --type shell 'check_setup_custom_ca_certificates'Length of output: 82
Script:
#!/bin/bash # Description: Search the codebase for any residual references to `check_setup_custom_ca_certificates`. rg --type sh 'check_setup_custom_ca_certificates'Length of output: 49
Script:
#!/bin/bash # Description: Search the codebase for any residual references to `check_setup_custom_ca_certificates` within all .sh files. rg 'check_setup_custom_ca_certificates' --glob '*.sh'Length of output: 53
deploy/docker/fs/opt/appsmith/entrypoint.sh.bak (1)
496-544: Review the execution flow in the main sectionIn the main section of the script (lines 496-544), ensure that the functions are called in the correct order and that all necessary initializations are performed before the application starts. Pay special attention to dependencies between services such as MongoDB, Redis, and Postgres.
Consider reviewing the execution sequence to confirm that all services are correctly initialized and ready before the main application starts.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Dear @sharat87 sir & @pratapaprasanna sir, I hope you're doing well! I noticed that my pull request (PR #36871 ) hasn't seen any recent activity and is scheduled to be closed in 7 days. Could you kindly review it when you have a moment and let me know if any additional changes are needed? I believe it adds value to the project, and I'm happy to make further adjustments if required. Thank you for your time and consideration! Best regards, |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Hey @Rudraksh2003, thank you for working on this. However, this change needs some extensive testing in whether custom CA certificates are still working fine in the various components of Appsmith, like the backend server. Caddy, RTS, Keycloak, etc. That's where majority of the effort lies, and that's why the linked issue doesn't have the Thanks again and hope to see more contributions from you in the future! |
#32417
Description
Tip
This PR removes redundant loading of custom CA certificates in the
entrypoint.shscript and includes a backup of the original script for reference. We want to avoid writing to the root file system unnecessarily, so thecheck_setup_custom_ca_certificatesfunction has been removed, and thesetup-custom-ca-certificatesfunction will be used moving forward.Motivation: Removing this redundancy improves security and maintainability by ensuring we don't write to the OS's trust store inappropriately. It also simplifies the code.
Fixes #32417
Changes:
check_setup_custom_ca_certificatesfunction.setup-custom-ca-certificatesfunction to handle custom CA certificates properly.entrypoint.sh: Created a backup of the originalentrypoint.shfile asentrypoint.sh.bak.No dependencies required for this change.
Links to documentation:
Automation
/ok-to-test tags=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
#32417
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
ENV_PATHconstant for better configuration management.