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

fix: Add password based auth for postgres #37068

Open
wants to merge 16 commits into
base: release
Choose a base branch
from

Conversation

abhvsn
Copy link
Contributor

@abhvsn abhvsn commented Oct 24, 2024

Description

  • Introduced a custom PostgreSQL configuration file to enhance database access control.

Manual testing for the auth mechanism with appsmith and postgres user:

> psql -h 127.0.0.1 -p 5432 -U postgres

psql: error: connection to server at "127.0.0.1", port 5432 failed: FATAL:  no pg_hba.conf entry for host "127.0.0.1", user "postgres", database "postgres", no encryption

Comments: With host(-h) on postgres user fails to perform any op as the entry is not added in the conf file


> psql -p 5432 -U postgres
psql (14.13 (Ubuntu 14.13-1.pgdg20.04+1))
Type "help" for help.

postgres=#

Comments: Without host(-h) postgres user can perform any op  


> psql -h 127.0.0.1 -p 5432 -U appsmith
Password for user appsmith:

Comments: With host(-h) on appsmith user asks for password which is present in docker.env

> For incorrect password we land in following situation:
psql -h 127.0.0.1 -p 5432 -U appsmith
Password for user appsmith:
psql: error: connection to server at "127.0.0.1", port 5432 failed: FATAL:  password authentication failed for user "appsmith"


> psql -p 5432 -U appsmith
psql: error: connection to server on socket "/var/run/postgresql/.s.PGSQL.5432" failed: FATAL:  no pg_hba.conf entry for host "[local]", user "appsmith", database "appsmith", no encryption

Comments: Without host appsmith user can`t talk to postgres DB at all

> su appsmith -c "pg_isready -h 127.0.0.1"
su: user appsmith does not exist

Comments: pg_isready can`t be checked with the appsmith user, as it does not exists

> su postgres -c "pg_isready -h 127.0.0.1"
127.0.0.1:5432 - accepting connections

Comments: With postgres user one can check the pg_isready signal

Fixes https://github.com/appsmithorg/appsmith-ee/issues/5261

/test Sanity

🔍 Cypress test results

Caution

🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11814765103
Commit: 6fe0e0a
Cypress dashboard.
Tags: @tag.Sanity
Spec:
The following are new failures, please fix them before merging the PR:

  1. cypress/e2e/Sanity/Datasources/RestApiDatasource_spec.js
List of identified flaky tests.
Wed, 13 Nov 2024 10:25:28 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • No

Summary by CodeRabbit

  • New Features

    • Introduced a custom PostgreSQL configuration file for enhanced authentication methods.
    • Added a new script to automate PostgreSQL authentication testing after upgrades.
    • Implemented a script to facilitate the deployment of Appsmith using Docker Compose.
  • Improvements

    • Enhanced PostgreSQL initialization process for better security and reliability.
    • Updated local testing script to allow specification of Docker image tags.
    • Streamlined command-line argument handling in local testing.
  • Bug Fixes

    • Improved readiness checks for PostgreSQL connections.
  • Chores

    • Added a new Docker Compose file for managing test services.

Copy link
Contributor

coderabbitai bot commented Oct 24, 2024

Walkthrough

The changes in this pull request enhance the PostgreSQL initialization process within the Appsmith deployment. Key modifications include the addition of a custom appsmith_hba.conf file to enforce password-based authentication, updates to the entrypoint.sh script for ownership and permission settings, and improvements to the readiness checks. New testing scripts have also been introduced to automate PostgreSQL authentication checks, while adjustments to existing scripts streamline local testing functionality.

Changes

File Path Change Summary
deploy/docker/fs/opt/appsmith/entrypoint.sh - Added copying of appsmith_hba.conf to PostgreSQL data directory.
- Set ownership and permissions for pg_hba.conf.
- Modified readiness check to use 127.0.0.1.
deploy/docker/fs/opt/appsmith/postgres/appsmith_hba.conf - Introduced appsmith_hba.conf for custom PostgreSQL authentication settings.
deploy/docker/fs/opt/appsmith/pg-utils.sh - Renamed postgres_admin_user to POSTGRES_ADMIN_USER.
- Updated function name and removed host parameter for local connections.
deploy/docker/tests/.gitignore - Added docker-compose.yml for managing multi-container Docker applications.
deploy/docker/tests/test-pg-auth.sh - Introduced functions for testing PostgreSQL authentication post-upgrade.
scripts/local_testing.sh - Updated command-line syntax to include a tag parameter for Docker image naming.
deploy/docker/tests/composes.sh - Added functions for generating and managing Docker Compose files for Appsmith deployment.

Assessment against linked issues

Objective Addressed Explanation
Enforce password-based authentication for roles in Postgres (5261)

Possibly related PRs

Suggested labels

High, Production, Needs Triaging, Move to Postgres, DB Infrastructure Pod, skip-changelog

Suggested reviewers

  • sharat87
  • pratapaprasanna

🎉 In the land of code where the databases play,
PostgreSQL's security has found its way!
With hba.conf in place, passwords now reign,
Appsmith's deployment won't be in vain.
Scripts for testing, oh what a delight,
Making sure everything runs just right! 🚀


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the Bug Something isn't working label Oct 24, 2024
@abhvsn abhvsn marked this pull request as ready for review October 25, 2024 03:13
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (1)
deploy/docker/fs/opt/appsmith/entrypoint.sh (1)

Add connection test for appsmith user before database shutdown

Add a verification step before stopping Postgres to ensure the appsmith user can connect with password authentication:

su postgres -c "env PATH='$PATH' psql -h 127.0.0.1 -U appsmith -d ${APPSMITH_PG_DATABASE} -c '\q'"
  • Location: deploy/docker/fs/opt/appsmith/entrypoint.sh in create_appsmith_pg_db function, before the pg_ctl stop command
🔗 Analysis chain

Line range hint 437-483: Verify appsmith user connection after setup

Add a connection test for the appsmith user after database setup to ensure the authentication changes work as expected.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify if appsmith_hba.conf exists and contains proper authentication rules

# Test: Check if appsmith_hba.conf exists
if ! rg -q "appsmith_hba.conf" .; then
    echo "Error: appsmith_hba.conf not found in the repository"
    exit 1
fi

# Test: Check authentication rules in appsmith_hba.conf
rg -A 5 "^host.*appsmith.*password" 

Length of output: 65


Script:

#!/bin/bash
# Let's check the actual content of the postgres configuration files
fd "appsmith_hba.conf" . -x cat {}

# Also check if there's any connection verification logic in the entrypoint script
rg -B 2 -A 2 "psql.*appsmith" deploy/docker/fs/opt/appsmith/entrypoint.sh

Length of output: 1423


Script:

#!/bin/bash
# Let's check if there's any connection test after database creation
rg -B 5 -A 10 "create_appsmith_pg_db" deploy/docker/fs/opt/appsmith/entrypoint.sh

# Also check if there's any psql command execution after database setup
rg -B 2 -A 5 "su postgres.*psql" deploy/docker/fs/opt/appsmith/entrypoint.sh

Length of output: 1648

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 3d9d08a and 3517cd9.

📒 Files selected for processing (2)
  • deploy/docker/fs/opt/appsmith/entrypoint.sh (2 hunks)
  • deploy/docker/fs/opt/appsmith/postgres/appsmith_hba.conf (1 hunks)
🧰 Additional context used
📓 Learnings (1)
deploy/docker/fs/opt/appsmith/entrypoint.sh (2)
Learnt from: abhvsn
PR: appsmithorg/appsmith#36670
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:465-483
Timestamp: 2024-10-07T09:13:14.613Z
Learning: In the `deploy/docker/fs/opt/appsmith/entrypoint.sh` script, `APPSMITH_PG_DATABASE` is a constant with the value `appsmith`, so we can assume it remains constant and doesn't require additional quoting in SQL commands.
Learnt from: abhvsn
PR: appsmithorg/appsmith#36670
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:465-483
Timestamp: 2024-10-08T15:32:34.115Z
Learning: In the `deploy/docker/fs/opt/appsmith/entrypoint.sh` script, `APPSMITH_PG_DATABASE` is a constant with the value `appsmith`, so we can assume it remains constant and doesn't require additional quoting in SQL commands.
🔇 Additional comments (5)
deploy/docker/fs/opt/appsmith/postgres/appsmith_hba.conf (5)

1-10: Documentation is clear and helpful.

The comments effectively explain the purpose and implications of the configuration.


15-16: Verify IPv4 configuration aligns with deployment architecture.

The configuration correctly implements password authentication for the appsmith user. However, limiting connections to only localhost (127.0.0.1) might be restrictive if the application needs to accept connections from other containers or hosts.

Consider if you need to allow connections from other network ranges based on your container networking setup.


18-19: IPv6 configuration mirrors IPv4 settings.

The configuration maintains consistency between IPv4 and IPv6 settings.


13-13: Review security implications of trust authentication for postgres user.

While using trust method for local connections is common in containerized environments, consider if this presents any security risks in your deployment context.

#!/bin/bash
# Check if there are any other configuration files that might override these settings
fd -t f -e conf -e config . | grep -i "pg.*hba"

22-24: 🛠️ Refactor suggestion

Review necessity of replication configuration.

The replication configuration might be unnecessary unless you're planning to implement PostgreSQL replication.

Consider removing the replication configuration if it's not needed for your use case:

-# Allow replication connections from localhost, by a user with the
-# replication privilege.
-local   replication     all                                     md5
-host    replication     all             127.0.0.1/32            md5
-host    replication     all             ::1/128                 md5

deploy/docker/fs/opt/appsmith/postgres/appsmith_hba.conf Outdated Show resolved Hide resolved
deploy/docker/fs/opt/appsmith/entrypoint.sh Outdated Show resolved Hide resolved
deploy/docker/fs/opt/appsmith/entrypoint.sh Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (2)
deploy/docker/fs/opt/appsmith/pg-utils.sh (1)

110-114: Add error handling for database creation.

The database creation looks good, but consider capturing and handling potential errors.

- psql -p "$PG_DB_PORT" -U "$POSTGRES_ADMIN_USER" -c "CREATE DATABASE $PG_DB_NAME;"
+ if ! psql -p "$PG_DB_PORT" -U "$POSTGRES_ADMIN_USER" -c "CREATE DATABASE $PG_DB_NAME;"; then
+   echo "Failed to create database $PG_DB_NAME"
+   exit 1
+ fi
deploy/docker/fs/opt/appsmith/entrypoint.sh (1)

479-479: Add timeout to pg_isready command.

While the explicit host is good, the command should include a timeout to prevent hanging. Consider adding the -t (timeout) parameter.

- until su postgres -c "env PATH='$PATH' pg_isready -h 127.0.0.1"; do
+ until su postgres -c "env PATH='$PATH' pg_isready -h 127.0.0.1 -t 5"; do
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 3517cd9 and bdc0719.

📒 Files selected for processing (3)
  • deploy/docker/fs/opt/appsmith/entrypoint.sh (2 hunks)
  • deploy/docker/fs/opt/appsmith/pg-utils.sh (3 hunks)
  • deploy/docker/fs/opt/appsmith/postgres/appsmith_hba.conf (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • deploy/docker/fs/opt/appsmith/postgres/appsmith_hba.conf
🧰 Additional context used
📓 Learnings (1)
deploy/docker/fs/opt/appsmith/entrypoint.sh (3)
Learnt from: abhvsn
PR: appsmithorg/appsmith#37068
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:475-475
Timestamp: 2024-10-25T08:05:20.269Z
Learning: In `deploy/docker/fs/opt/appsmith/entrypoint.sh`, `pg_isready` does not check authentication, so even if there is no `pg_hba.conf` entry for host connections for the `postgres` user, we don't need to worry.
Learnt from: abhvsn
PR: appsmithorg/appsmith#36670
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:465-483
Timestamp: 2024-10-07T09:13:14.613Z
Learning: In the `deploy/docker/fs/opt/appsmith/entrypoint.sh` script, `APPSMITH_PG_DATABASE` is a constant with the value `appsmith`, so we can assume it remains constant and doesn't require additional quoting in SQL commands.
Learnt from: abhvsn
PR: appsmithorg/appsmith#36670
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:465-483
Timestamp: 2024-10-08T15:32:34.115Z
Learning: In the `deploy/docker/fs/opt/appsmith/entrypoint.sh` script, `APPSMITH_PG_DATABASE` is a constant with the value `appsmith`, so we can assume it remains constant and doesn't require additional quoting in SQL commands.
🔇 Additional comments (3)
deploy/docker/fs/opt/appsmith/pg-utils.sh (2)

9-9: LGTM! Variable naming follows shell conventions.


133-135: LGTM! Clear function naming and proper parameter passing.

deploy/docker/fs/opt/appsmith/entrypoint.sh (1)

436-440: LGTM! PostgreSQL configuration setup looks secure.

The implementation follows PostgreSQL security best practices with proper file ownership and permissions.

deploy/docker/fs/opt/appsmith/pg-utils.sh Show resolved Hide resolved
deploy/docker/fs/opt/appsmith/pg-utils.sh Show resolved Hide resolved
@abhvsn
Copy link
Contributor Author

abhvsn commented Oct 25, 2024

/build-deploy-preview skip-tests=true

Copy link

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11514664672.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 37068.
recreate: .

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
deploy/docker/fs/opt/appsmith/entrypoint.sh (1)

436-440: Add error handling for configuration file operations.

The file operations look good, with correct permissions (600) and ownership (postgres:postgres) for PostgreSQL security. However, we should add error handling to ensure the configuration file exists and operations succeed.

Apply this diff to add error handling:

+    if [[ ! -f /opt/appsmith/postgres/appsmith_hba.conf ]]; then
+        echo "Error: PostgreSQL HBA configuration file not found"
+        return 1
+    fi
     cp /opt/appsmith/postgres/appsmith_hba.conf "$POSTGRES_DB_PATH/pg_hba.conf"
+    if [[ $? -ne 0 ]]; then
+        echo "Error: Failed to copy PostgreSQL HBA configuration"
+        return 1
+    fi
     # PostgreSQL requires strict file permissions for the pg_hba.conf file. Add file permission settings after copying the configuration file.
     # 600 is the recommended permission for pg_hba.conf file for read and write access to the owner only.
     chown postgres:postgres "$POSTGRES_DB_PATH/pg_hba.conf"
     chmod 600 "$POSTGRES_DB_PATH/pg_hba.conf"
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between bdc0719 and a6b2797.

📒 Files selected for processing (1)
  • deploy/docker/fs/opt/appsmith/entrypoint.sh (1 hunks)
🧰 Additional context used
📓 Learnings (1)
deploy/docker/fs/opt/appsmith/entrypoint.sh (3)
Learnt from: abhvsn
PR: appsmithorg/appsmith#37068
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:475-475
Timestamp: 2024-10-25T08:05:20.269Z
Learning: In `deploy/docker/fs/opt/appsmith/entrypoint.sh`, `pg_isready` does not check authentication, so even if there is no `pg_hba.conf` entry for host connections for the `postgres` user, we don't need to worry.
Learnt from: abhvsn
PR: appsmithorg/appsmith#36670
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:465-483
Timestamp: 2024-10-07T09:13:14.613Z
Learning: In the `deploy/docker/fs/opt/appsmith/entrypoint.sh` script, `APPSMITH_PG_DATABASE` is a constant with the value `appsmith`, so we can assume it remains constant and doesn't require additional quoting in SQL commands.
Learnt from: abhvsn
PR: appsmithorg/appsmith#36670
File: deploy/docker/fs/opt/appsmith/entrypoint.sh:465-483
Timestamp: 2024-10-08T15:32:34.115Z
Learning: In the `deploy/docker/fs/opt/appsmith/entrypoint.sh` script, `APPSMITH_PG_DATABASE` is a constant with the value `appsmith`, so we can assume it remains constant and doesn't require additional quoting in SQL commands.
🔇 Additional comments (1)
deploy/docker/fs/opt/appsmith/entrypoint.sh (1)

436-436: Verify the HBA configuration content.

Let's verify the content of the new HBA configuration file to ensure it properly configures password-based authentication for the appsmith user.

✅ Verification successful

HBA configuration is properly configured for secure authentication

The PostgreSQL HBA configuration file correctly implements:

  • Password-based authentication (scram-sha-256) for the appsmith user
  • Restricted access to localhost only (127.0.0.1/32 and ::1/128)
  • Proper access control for both appsmith and postgres databases
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the content of the PostgreSQL HBA configuration file
# Expected: Should find entries for password authentication

if [[ -f /opt/appsmith/postgres/appsmith_hba.conf ]]; then
    echo "=== PostgreSQL HBA Configuration ==="
    cat /opt/appsmith/postgres/appsmith_hba.conf
else
    echo "Error: PostgreSQL HBA configuration file not found"
    exit 1
fi

Length of output: 171


Script:

#!/bin/bash
# Let's check the actual content in the repository
fd appsmith_hba.conf

# If found, let's see its content
fd appsmith_hba.conf --exec cat {}

Length of output: 1500

Copy link

Deploy-Preview-URL: https://ce-37068.dp.appsmith.com

@abhvsn abhvsn requested a review from srix October 25, 2024 12:13
Copy link
Contributor

@srix srix left a comment

Choose a reason for hiding this comment

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

please add test cases under docker/test/

@abhvsn abhvsn added the ok-to-test Required label for CI label Oct 25, 2024
@abhvsn abhvsn requested a review from srix October 29, 2024 08:27
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 13

🧹 Outside diff range and nitpick comments (1)
deploy/docker/tests/test-pg-auth.sh (1)

2-2: Consider removing the commented-out debug option

The line # set -x is commented out. If debugging is not needed, consider removing it to keep the script clean.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between a6b2797 and 85ab70f.

📒 Files selected for processing (4)
  • deploy/docker/tests/.gitignore (1 hunks)
  • deploy/docker/tests/pg-upgrade/composes.sh (1 hunks)
  • deploy/docker/tests/test-pg-auth.sh (1 hunks)
  • scripts/local_testing.sh (4 hunks)
✅ Files skipped from review due to trivial changes (1)
  • deploy/docker/tests/.gitignore
🔇 Additional comments (7)
deploy/docker/tests/pg-upgrade/composes.sh (1)

49-68: Apply the same refactoring pattern here.

This function can use the same refactoring pattern suggested for compose_appsmith_latest.

scripts/local_testing.sh (3)

13-13: LGTM: Help message accurately reflects new parameters.

The help message has been updated to correctly document the new tag parameter in the command syntax.


78-80: LGTM: Proper default tag handling.

Good practice to set a default tag value when none is provided.


116-120: Verify Docker base image compatibility.

The Docker build command now uses the tag parameter correctly, but we should verify that all referenced base images exist.

✅ Verification successful

Since Docker commands cannot be executed in the sandbox environment, let me try to verify the base image references in the codebase to understand their usage and configuration.


Base images are correctly referenced and used in the codebase

The base images appsmith/base-ce:release and appsmith/base-ee:release are properly configured in the build system:

  • The main Dockerfile uses an ARG BASE parameter which is correctly passed from the build command
  • The base images are consistently used across the codebase, as seen in route-tests configuration
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the base images are available
docker pull appsmith/base-ce:release
docker pull appsmith/base-ee:release

Length of output: 225


Script:

#!/bin/bash
# Search for references to base images in Docker files and scripts
rg -g '!node_modules' -g '!dist' -g '!build' "appsmith/base-[ce|ee]" -A 2

# Look for Dockerfile configurations
fd Dockerfile --exec cat {}

Length of output: 3529

deploy/docker/tests/test-pg-auth.sh (3)

5-5: Verify the existence of 'composes.sh'

Ensure that the ./pg-upgrade/composes.sh script exists and is correctly sourced to avoid runtime errors.


51-52: Handle possible authentication failures

The command may fail if local connections require a password. Ensure the authentication configuration allows passwordless local connections if intended.


116-119: Ensure 'pg_isready' is available

Verify that pg_isready exists within the 'appsmith' container to prevent command not found errors.

deploy/docker/tests/pg-upgrade/composes.sh Outdated Show resolved Hide resolved
deploy/docker/tests/pg-upgrade/composes.sh Outdated Show resolved Hide resolved
deploy/docker/tests/pg-upgrade/composes.sh Outdated Show resolved Hide resolved
deploy/docker/tests/pg-upgrade/composes.sh Outdated Show resolved Hide resolved
deploy/docker/tests/pg-upgrade/composes.sh Outdated Show resolved Hide resolved
deploy/docker/tests/test-pg-auth.sh Outdated Show resolved Hide resolved
deploy/docker/tests/test-pg-auth.sh Outdated Show resolved Hide resolved
deploy/docker/tests/test-pg-auth.sh Show resolved Hide resolved
deploy/docker/tests/test-pg-auth.sh Outdated Show resolved Hide resolved
deploy/docker/tests/test-pg-auth.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (3)
deploy/docker/tests/test-pg-auth.sh (3)

18-30: Improve retry mechanism configuration

Consider these enhancements:

  1. Move magic numbers to configurable variables
  2. Add exponential backoff to prevent overwhelming the system
-  local max_retries=200
+  local max_retries=${MAX_HEALTH_RETRIES:-200}
+  local retry_delay=${HEALTH_RETRY_DELAY:-1}
   while [ $retry_count -lt $max_retries ]; do
     response_code=$(curl -s -o /dev/null -w "%{http_code}" http://localhost/health)
     if [[ $response_code -eq 200 ]]; then
       return 0
     fi
     echo "Waiting for Appsmith instance to be ready... (Attempt: $((retry_count + 1)))"
     retry_count=$((retry_count + 1))
-    sleep 1
+    sleep $((retry_delay * retry_count))

111-111: Enhance cleanup process

The cleanup process should be more thorough and handle potential failures.

+cleanup() {
+  echo "Cleaning up containers and volumes..."
+  docker compose down --timeout 30 || true
+  docker compose rm -fsv appsmith || true
+  # Optionally clean volumes if needed
+  # docker volume prune -f
+}
+
+trap cleanup EXIT

Also applies to: 140-141


181-182: Add command-line options for test configuration

Consider adding options to control test behavior (e.g., cleanup, timeouts, verbosity).

+# Parse command line options
+while getopts "cv:t:" opt; do
+  case $opt in
+    c) SKIP_CLEANUP=true ;;
+    v) VERBOSE=$OPTARG ;;
+    t) TIMEOUT=$OPTARG ;;
+    \?) echo "Invalid option: -$OPTARG" >&2; exit 1 ;;
+  esac
+done
+
 test_postgres_auth_enabled_upgrade_from_147tolocal
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 85ab70f and 527dd5c.

📒 Files selected for processing (2)
  • deploy/docker/tests/pg-upgrade/composes.sh (1 hunks)
  • deploy/docker/tests/test-pg-auth.sh (1 hunks)
🧰 Additional context used
📓 Learnings (1)
deploy/docker/tests/pg-upgrade/composes.sh (2)
Learnt from: abhvsn
PR: appsmithorg/appsmith#37068
File: deploy/docker/tests/pg-upgrade/composes.sh:1-4
Timestamp: 2024-10-29T08:52:10.043Z
Learning: In the `deploy/docker/tests/pg-upgrade/composes.sh` script, it's acceptable to leave `set -o errexit` commented out because the script is used for testing purposes and errors are handled in the consumer script.
Learnt from: abhvsn
PR: appsmithorg/appsmith#37068
File: deploy/docker/tests/pg-upgrade/composes.sh:70-74
Timestamp: 2024-10-29T08:52:09.822Z
Learning: In the testing scripts, the `cleanup` function is called before running tests, so additional safety checks are not necessary.
🔇 Additional comments (5)
deploy/docker/tests/test-pg-auth.sh (1)

11-14: Add synchronization check before URL update

The URL update assumes the configuration file is ready. Add a check to ensure the file exists and is accessible before modification.

deploy/docker/tests/pg-upgrade/composes.sh (4)

5-24: LGTM!

The generate_compose_file function looks good. It centralizes the Docker Compose configuration generation, making the code more DRY and maintainable.


32-39: Verify error handling.

Similar to the previous comment, the compose_appsmith_latest function should handle potential errors from docker compose pull and docker compose up -d. Consider adding error checks and logging.


63-67: ****

Based on the learnings, the cleanup function is called before running tests, so additional safety checks are not necessary.


69-74: LGTM!

The check_appsmith_edition function correctly determines the Appsmith edition based on the Git remote URL.

deploy/docker/tests/test-pg-auth.sh Outdated Show resolved Hide resolved
deploy/docker/tests/test-pg-auth.sh Outdated Show resolved Hide resolved
deploy/docker/tests/pg-upgrade/composes.sh Outdated Show resolved Hide resolved
deploy/docker/tests/pg-upgrade/composes.sh Outdated Show resolved Hide resolved
@abhvsn
Copy link
Contributor Author

abhvsn commented Nov 6, 2024

/build-deploy-preview skip-tests=true

Copy link

github-actions bot commented Nov 6, 2024

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11703065186.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 37068.
recreate: .

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (4)
deploy/docker/tests/test-pg-auth.sh (4)

1-6: Enhance script robustness with additional shell options.

Consider adding these shell options for better error handling and debugging:

 #!/bin/bash
 set -o errexit
+set -o nounset
+set -o pipefail
 # set -x
+
+# shellcheck disable=SC1091
 source ./composes.sh

17-32: Parameterize the health check URL.

Consider making the health check URL configurable to support different environments and ports.

+readonly HEALTH_CHECK_URL=${HEALTH_CHECK_URL:-"http://localhost/health"}
+readonly MAX_RETRIES=${MAX_RETRIES:-200}
+
 is_appsmith_instance_ready() {
-  local max_retries=200
   local retry_count=0
   local response_code
 
-  while [ $retry_count -lt $max_retries ]; do
-    response_code=$(curl -s -o /dev/null -w "%{http_code}" http://localhost/health)
+  while [ $retry_count -lt $MAX_RETRIES ]; do
+    response_code=$(curl -s -o /dev/null -w "%{http_code}" "${HEALTH_CHECK_URL}")

41-89: Standardize error messages and logging format.

The error messages use inconsistent symbols (✅, ❌) and formats. Consider standardizing the output format for better parsing and readability.

+readonly SUCCESS="✅"
+readonly FAILURE="❌"
+
+log_status() {
+  local test_name="$1"
+  local status="$2"
+  echo "${test_name}: ${status}"
+}
+
 check_user_datasource_access_with_auth() {
   # ... existing code ...
   if [[ $appsmith_user_local_access -ne 0 && $appsmith_user_remote_access -eq 0 ]]; then
-    echo "appsmith user does not have read access to databases with local unix socket: ✅"
-    echo "appsmith user has read access to databases with local tcp socket: ✅"
+    log_status "appsmith user local unix socket access denied" "${SUCCESS}"
+    log_status "appsmith user local tcp socket access granted" "${SUCCESS}"

184-186: Add script documentation.

Consider adding a header comment block describing the script's purpose, usage, and requirements.

+#!/bin/bash
+# Description: Tests PostgreSQL authentication after upgrading from Appsmith v1.47
+# Usage: ./test-pg-auth.sh
+# Requirements:
+#   - Docker
+#   - Docker Compose
+#   - curl
+#   - PostgreSQL client tools
+
 container_name="appsmith-docker-test"
 
 test_postgres_auth_enabled_upgrade_from_147tolocal
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 527dd5c and bd7d5a4.

📒 Files selected for processing (1)
  • deploy/docker/tests/test-pg-auth.sh (1 hunks)
🧰 Additional context used
🪛 Shellcheck
deploy/docker/tests/test-pg-auth.sh

[warning] 144-144: The surrounding quotes actually unquote this. Remove or escape them.

(SC2027)

deploy/docker/tests/test-pg-auth.sh Outdated Show resolved Hide resolved
deploy/docker/tests/test-pg-auth.sh Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Nov 6, 2024

Deploy-Preview-URL: https://ce-37068.dp.appsmith.com

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 8

🧹 Outside diff range and nitpick comments (2)
deploy/docker/tests/composes.sh (1)

78-81: Improve variable declarations and validate paths.

Move variables to the top and add path validation:

+# Configuration
+container_name="appsmith-docker-test"
+stacks_path="/tmp/$container_name-stacks"
+docker_compose_path="docker-compose.yml"
+
+# Validate paths
+if [[ ! -w "$(dirname "$stacks_path")" ]]; then
+    echo "Error: Directory $(dirname "$stacks_path") is not writable"
+    exit 1
+fi
+
+if [[ ! -w "$(dirname "$docker_compose_path")" ]]; then
+    echo "Error: Directory $(dirname "$docker_compose_path") is not writable"
+    exit 1
+fi
+
-container_name="appsmith-docker-test"
-# mkdir -p /tmp/$container_name
-stacks_path="/tmp/$container_name-stacks"
-docker_compose_path="docker-compose.yml"
deploy/docker/tests/test-pg-auth.sh (1)

17-32: Parameterize health check URL and add timeout

The health check URL and curl timeout should be configurable:

+readonly HEALTH_CHECK_URL=${HEALTH_CHECK_URL:-"http://localhost/health"}
+readonly CURL_TIMEOUT=${CURL_TIMEOUT:-5}
+
 is_appsmith_instance_ready() {
   local max_retries=200
   local retry_count=0
   local response_code
 
   while [ $retry_count -lt $max_retries ]; do
-    response_code=$(curl -s -o /dev/null -w "%{http_code}" http://localhost/health)
+    response_code=$(curl -s -o /dev/null -w "%{http_code}" --max-time "$CURL_TIMEOUT" "$HEALTH_CHECK_URL")
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between bd7d5a4 and 78e72a1.

📒 Files selected for processing (2)
  • deploy/docker/tests/composes.sh (1 hunks)
  • deploy/docker/tests/test-pg-auth.sh (1 hunks)
🧰 Additional context used
🪛 Shellcheck
deploy/docker/tests/test-pg-auth.sh

[warning] 144-144: The surrounding quotes actually unquote this. Remove or escape them.

(SC2027)

deploy/docker/tests/composes.sh Show resolved Hide resolved
deploy/docker/tests/composes.sh Show resolved Hide resolved
deploy/docker/tests/composes.sh Show resolved Hide resolved
deploy/docker/tests/composes.sh Show resolved Hide resolved
deploy/docker/tests/test-pg-auth.sh Show resolved Hide resolved
deploy/docker/tests/test-pg-auth.sh Show resolved Hide resolved
deploy/docker/tests/test-pg-auth.sh Show resolved Hide resolved
deploy/docker/tests/test-pg-auth.sh Show resolved Hide resolved
psql -p ${port} -U ${POSTGRES_ADMIN_USER} -d ${db} -c "GRANT ALL PRIVILEGES ON SCHEMA ${schema} TO ${user};"
psql -p ${port} -U ${POSTGRES_ADMIN_USER} -d ${db} -c "GRANT ALL PRIVILEGES ON ALL TABLES IN SCHEMA ${schema} TO ${user};"
psql -p ${port} -U ${POSTGRES_ADMIN_USER} -d ${db} -c "ALTER DEFAULT PRIVILEGES IN SCHEMA ${schema} GRANT ALL PRIVILEGES ON TABLES TO ${user};"
psql -p ${port} -U ${POSTGRES_ADMIN_USER} -d ${db} -c "GRANT CONNECT ON DATABASE ${db} TO ${user};"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add "-h /var/.sql" check the default unix socket path. let's be explicit that we are forcing unix socket use here

deploy/docker/tests/test-pg-auth.sh Show resolved Hide resolved
}

container_name="appsmith-docker-test"

Copy link
Contributor

Choose a reason for hiding this comment

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

test for restart of local

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working ok-to-test Required label for CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants