action-setup-postgres icon indicating copy to clipboard operation
action-setup-postgres copied to clipboard

Environment variables missing on Windows

Open karlhorky opened this issue 1 year ago • 6 comments

Hi @ikalnytskyi 👋 hope things are good with you!

We're running into a very confusing issue with the action, where environment variables which we set in our workflow are somehow disappearing:

name: CI
on: push

jobs:
  ci:
    name: CI
    runs-on: windows-latest
    timeout-minutes: 20
    env:
      PGHOST: localhost
      PGDATABASE: preflight_test_project_next_js_passing
      PGUSERNAME: preflight_test_project_next_js_passing
      # 💥 This environment variable is disappearing
      PGPASSWORD: preflight_test_project_next_js_passing
    steps:
      - uses: ikalnytskyi/action-setup-postgres@v5
        with:
          username: ${{ env.PGUSERNAME }}
          password: ${{ env.PGPASSWORD }}
          database: ${{ env.PGDATABASE }}
      - uses: actions/checkout@v4
      - uses: pnpm/action-setup@v3
        with:
          version: 'latest'

      # Use the official setup-node action (sets up Node.js):
      # https://github.com/actions/setup-node
      - name: Use Node.js
        uses: actions/setup-node@v4
        with:
          node-version: 'lts/*'
          cache: 'pnpm'

      - name: Install dependencies
        run: pnpm install

      - run: node db.js

Our db.js file:

import postgres from 'postgres';

// Connect to database using environment variables
// https://github.com/porsager/postgres#environmental-variables
const sql = postgres();

console.log(
  await sql`
    SELECT
      1
  `,
);

await sql.end();

This results in "password authentication failed for user" error messages:

Run node db.js
  node db.js
  shell: C:\Program Files\PowerShell\7\pwsh.EXE -command ". '{0}'"
  env:
    PGHOST: localhost
    PGDATABASE: preflight_test_project_next_js_passing
    PGUSERNAME: preflight_test_project_next_js_passing
    PGPASSWORD: 
    PQ_LIB_DIR: C:\Program Files\PostgreSQL\14\lib
    PGROOT: 
    PGDATA: 
    PGBIN: 
    PGUSER: 
    PGSERVICEFILE: D:\a\_temp/pgdata/pg_service.conf
    PNPM_HOME: C:\Users\runneradmin\setup-pnpm\node_modules\.bin
  
node:internal/process/esm_loader:34
      internalBinding('errors').triggerUncaughtException(
                                ^
PostgresError: password authentication failed for user "preflight_test_project_next_js_passing"
    at ErrorResponse (file:///D:/a/preflight-test-project-next-js-passing/preflight-test-project-next-js-passing/node_modules/.pnpm/[email protected]/node_modules/postgres/src/connection.js:788:[2](https://github.com/upleveled/preflight-test-project-next-js-passing/actions/runs/8526965282/job/23357327414?pr=152#step:8:2)6)
    at handle (file:///D:/a/preflight-test-project-next-js-passing/preflight-test-project-next-js-passing/node_modules/.pnpm/postgres@[3](https://github.com/upleveled/preflight-test-project-next-js-passing/actions/runs/8526965282/job/23357327414?pr=152#step:8:3).4.4/node_modules/postgres/src/connection.js:474:6)
    at Socket.data (file:///D:/a/preflight-test-project-next-js-passing/preflight-test-project-next-js-passing/node_modules/.pnpm/postgres@3.[4](https://github.com/upleveled/preflight-test-project-next-js-passing/actions/runs/8526965282/job/23357327414?pr=152#step:8:4).4/node_modules/postgres/src/connection.js:31[5](https://github.com/upleveled/preflight-test-project-next-js-passing/actions/runs/8526965282/job/23357327414?pr=152#step:8:5):9)
    at Socket.emit (node:events:518:28)
    at addChunk (node:internal/streams/readable:559:12)
    at readableAddChunkPushByteMode (node:internal/streams/readable:510:3)
    at Readable.push (node:internal/streams/readable:390:5)
    at TCP.onStreamRead (node:internal/stream_base_commons:190:23)
    at cachedError (file:///D:/a/preflight-test-project-next-js-passing/preflight-test-project-next-js-passing/node_modules/.pnpm/[email protected]/node_modules/postgres/src/query.js:170:23)
    at new Query (file:///D:/a/preflight-test-project-next-js-passing/preflight-test-project-next-js-passing/node_modules/.pnpm/[email protected]/node_modules/postgres/src/query.js:3[6](https://github.com/upleveled/preflight-test-project-next-js-passing/actions/runs/8526965282/job/23357327414?pr=152#step:8:6):24)
    at sql (file:///D:/a/preflight-test-project-next-js-passing/preflight-test-project-next-js-passing/node_modules/.pnpm/[email protected]/node_modules/postgres/src/index.js:112:11)
    at file:///D:/a/preflight-test-project-next-js-passing/preflight-test-project-next-js-passing/db.js:10:12 {
  severity_local: 'FATAL',
  severity: 'FATAL',
  code: '2[8](https://github.com/upleveled/preflight-test-project-next-js-passing/actions/runs/8526965282/job/23357327414?pr=152#step:8:8)P01',
  file: 'auth.c',
  line: '338',
  routine: 'auth_failed'
}
Node.js v20.[11](https://github.com/upleveled/preflight-test-project-next-js-passing/actions/runs/8526965282/job/23357327414?pr=152#step:8:11).1
Error: Process completed with exit code 1.

Logging out the process.env shows that the environment variable PGPASSWORD is missing:

PGHOST localhost
PGUSERNAME preflight_test_project_next_js_passing
PGPASSWORD 
PGDATABASE preflight_test_project_next_js_passing

Not present on non-Windows OSes

This script runs fine on other non-Windows OSes 🤔

Not relying on environment variables works

Also, on Windows, when not relying on the environment variables, then our script runs through:

// This version works:
const sql = postgres(
 
 'postgres://preflight_test_project_next_js_passing:preflight_test_project_next_js_passing@localhost:5432/preflight_test_project_next_js_passing',
);

Is it possible that the action is actually manipulating environment variables that are being explicitly set by the user in the workflow? (but only on Windows?)

If so, this is very confusing and surprising behavior.

karlhorky avatar Apr 02 '24 18:04 karlhorky

Ohh, on Windows, some environment variables are indeed being unset by the action:

https://github.com/ikalnytskyi/action-setup-postgres/blob/ca880f61d266671e698058978c0d135c769df280/action.yml#L42-L47

@ikalnytskyi would you consider reverting this change? Or at least, making it so that user-specified environment variables are not discarded?

karlhorky avatar Apr 02 '24 18:04 karlhorky

Workaround

Manually re-add the cleared PGPASSWORD environment variable using an env configuration block (also any other environment variables in this list: PGROOT, PGDATA, PGBIN, PGUSER)

      - name: Run db.js

        # Manually add back PGPASSWORD which has been cleared by ikalnytskyi/action-setup-postgres
        #
        # TODO: Remove when the issue is resolved:
        # https://github.com/ikalnytskyi/action-setup-postgres/issues/32
        env:
          PGPASSWORD: preflight_test_project_next_js_passing

        run: node db.js

karlhorky avatar Apr 02 '24 21:04 karlhorky

Hey @karlhorky,

PGUSER, PGPASSWORD, PGHOST and other PG* environment variables are picked by any libpq based client, not only psql. I intentionally wanted them to be unset and trigger authentication issues because in most cases applications are configured via connection URI-s or some other application specific environment variables (e.g. MYAPP_DATABASE_PASSWORD). When using this action in CI, it'd be unfortunate if someone get their credentials propagation broken but won't see it since libpq based drivers auto magically read PG* envvars. Besides, it may also be a bit misleading if someone create multiple users for tests (via createuser CLI tool).

Can you please explain what you're trying to achieve? You aren't intended to repeat credentials or password. If you check the README file, there are two examples how to get credentials out from action. You can either retrieve the connection URI that most drivers can swallow, e.g.

https://github.com/ikalnytskyi/action-setup-postgres/blob/ca880f61d266671e698058978c0d135c769df280/README.md?plain=1#L60-L70

or you can activate the service, that most libpq based clients can swallow (psql included):

https://github.com/ikalnytskyi/action-setup-postgres/blob/ca880f61d266671e698058978c0d135c769df280/README.md?plain=1#L83-L97

I'd go ahead and remove those environment variables for good (since we don't have set on Linux and macOS runners), it's just github actions platform doesn't support unsetting them.

ikalnytskyi avatar Apr 02 '24 23:04 ikalnytskyi

PGUSER, PGPASSWORD, PGHOST and other PG* environment variables are picked by any libpq based client

Yes, this is the behavior that I want.

Check out the Postgres.js docs page I listed in the code comment above:

https://github.com/porsager/postgres#environmental-variables

Keeping the code simple and being able to rely on standardized environment variables for the connection information seems like a common use case.

This works on all other environments, so intentionally breaking this on Windows seems like a bug / large footgun in Windows support.

This is also what caused psql SQL queries to not work in a very confusing way (only on Windows):

  • https://github.com/ikalnytskyi/action-setup-postgres/issues/31#issuecomment-2032627624

karlhorky avatar Apr 03 '24 05:04 karlhorky

Workaround 2

Copy + paste this minimal replacement below instead of ikalnytskyi/action-setup-postgres for cross-platform PostgreSQL across Windows, macOS, Linux (without any clearing of environment variables):

name: CI
on: push

jobs:
  ci:
    name: CI
    runs-on: ${{ matrix.os }}
    strategy:
      matrix:
        os: [windows-latest, macos-latest, ubuntu-latest]
    timeout-minutes: 15
    env:
      PGHOST: localhost
      PGDATABASE: preflight_test_project_next_js_passing
      PGUSERNAME: preflight_test_project_next_js_passing
      PGPASSWORD: preflight_test_project_next_js_passing
    steps:
      - name: Add PostgreSQL binaries to PATH
        shell: bash
        run: |
          if [ "$RUNNER_OS" == "Windows" ]; then
            echo "$PGBIN" >> $GITHUB_PATH
          elif [ "$RUNNER_OS" == "Linux" ]; then
            echo "$(pg_config --bindir)" >> $GITHUB_PATH
          fi
      - name: Start preinstalled PostgreSQL
        shell: bash
        run: |
          echo "Initializing database cluster..."

          # Convert backslashes to forward slashes in RUNNER_TEMP for Windows Git Bash
          export PGHOST="${RUNNER_TEMP//\\//}/postgres"
          export PGDATA="$PGHOST/pgdata"
          mkdir -p "$PGDATA"

          # initdb requires file for password in non-interactive mode
          export PWFILE="$RUNNER_TEMP/pwfile"
          echo "postgres" > "$PWFILE"
          initdb --pgdata="$PGDATA" --username="postgres" --pwfile="$PWFILE"

          echo "Starting PostgreSQL..."
          echo "unix_socket_directories = '$PGHOST'" >> "$PGDATA/postgresql.conf"
          pg_ctl start

          echo "Creating user..."
          psql --host "$PGHOST" --username="postgres" --dbname="postgres" --command="CREATE USER $PGUSERNAME PASSWORD '$PGPASSWORD'" --command="\du"

          echo "Creating database..."
          createdb --owner="$PGUSERNAME" --username="postgres" "$PGDATABASE"

Also added to github-tricks: https://github.com/karlhorky/github-tricks/#github-actions-start-preinstalled-postgresql-database-on-windows-macos-and-linux

karlhorky avatar Apr 03 '24 15:04 karlhorky

I'm sorry for being away so long. It does make sense that you may want to set PG* environment variables on the job level and let it be picked up by the postgres.js. It's unfortunate that postgres.js doesn't support PGSERVICE, since this is the pattern that I envisioned for libpq-based applications:

- run: psql -c "SELECT 1"
  env:
    PGSERVICE: ${{ steps.postgres.outputs.service-name }}

I'll definitely revisit the unsetting part of this action for v7.

ikalnytskyi avatar Aug 19 '24 22:08 ikalnytskyi