Skip to content

services/horizon: Fix docker-compose Postgres' auth #2301

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

Merged
merged 2 commits into from
Feb 21, 2020

Conversation

2opremio
Copy link
Contributor

@2opremio 2opremio commented Feb 20, 2020

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    semver, or if it's mainly a patch change. The PR is targeted at the next
    release branch if it's not a patch change.

What

Fix the authentication of the Horizon container against the Postgres Horizon container at
services/horizon/docker/docker-compose.yml.

Why/How

By not pinning the full Postgres version, authentication stopped working since now it requires either setting the Postgres password or an explicit POSTGRES_HOST_AUTH_METHOD=trust:

Error: Database is uninitialized and superuser password is not specified.
       You must specify POSTGRES_PASSWORD for the superuser. Use
       "-e POSTGRES_PASSWORD=password" to set it in "docker run".

       You may also use POSTGRES_HOST_AUTH_METHOD=trust to allow all connections
       without a password. This is *not* recommended. See PostgreSQL
       documentation about "trust":
       https://www.postgresql.org/docs/current/auth-trust.html

Making this breaking change on a patch release is arguably a mistake on Postgres' end (see docker-library/postgres#681 (comment) ) but ... it is what it is.

This PR sets POSTGRES_HOST_AUTH_METHOD=trust and also pins the patch number of Postgres to make it future-proof.

Known limitations

N/A

@cla-bot

This comment has been minimized.

@2opremio 2opremio changed the title services/horizon: Fix docker-compose postgress auth services/horizon: Fix docker-compose Postgres' auth Feb 20, 2020
@cla-bot

This comment has been minimized.

@2opremio

This comment has been minimized.

@cla-bot

This comment has been minimized.

@cla-bot

This comment has been minimized.

@leighmcculloch
Copy link
Member

Hi @2opremio! Thanks for bringing this problem to our attention and opening a fix! Please bare with me a moment while I update our clabot-config.


In terms of how we address this breaking change in Postgres, I think we should set POSTGRES_HOST_AUTH_METHOD=trust in the docker-compose.yml. While pinning to the previous version works, I think if that will only serve to pin us into the past. It is a simple enough change to make to add the environment variable.

@2opremio
Copy link
Contributor Author

In terms of how we address this breaking change in Postgres, I think we should set POSTGRES_HOST_AUTH_METHOD=trust in the docker-compose.yml

Yes, that's what the PR does.

@2opremio
Copy link
Contributor Author

2opremio commented Feb 20, 2020

While pinning to the previous version works, I think if that will only serve to pin us into the past

It pins the current version, not the past one. Do you mean you prefer to have the patch number unpinned? I can definitely do that if you prefer.

@leighmcculloch

This comment has been minimized.

@cla-bot cla-bot bot added the cla: yes label Feb 20, 2020
@cla-bot

This comment has been minimized.

By not pinning the Postgres version, authentication stopped working since now it
requires either setting the Postgres password or an explicit
`POSTGRES_HOST_AUTH_METHOD=trust`:

```
Error: Database is uninitialized and superuser password is not specified.
       You must specify POSTGRES_PASSWORD for the superuser. Use
       "-e POSTGRES_PASSWORD=password" to set it in "docker run".

       You may also use POSTGRES_HOST_AUTH_METHOD=trust to allow all connections
       without a password. This is *not* recommended. See PostgreSQL
       documentation about "trust":
       https://www.postgresql.org/docs/current/auth-trust.html
```

Making this breaking change on a patch release is arguably a mistake on
Postgres' end (see
docker-library/postgres#681 (comment) )
but ... it is what it is. This commit sets `POSTGRES_HOST_AUTH_METHOD=trust` and
also pins the patch number of Postgres to make it future-proof.
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

Oops, indeed you're already using the new environment variable. Sorry!

@@ -1,7 +1,7 @@
version: '3'
services:
core-postgres:
image: postgres:9.6-alpine
image: postgres:9.6.17-alpine
Copy link
Member

Choose a reason for hiding this comment

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

I think we should leave this change out and continue to use the latest 9.6 patch version. It's valuable for us to continue to use the latest security patches and to not need to update this manually on each patch release. Normally a patch release won't include breaking changes and this seems like an exception, so the pain of a new version breaking use is unlikely to be frequent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's valuable for us to continue to use the latest security patches and to not need to update this manually on each patch release

As far as I understand this is only used for local development, so I am not sure security is more important than stability but, happy to make the change if you want.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, but nobody wants their developer environment exploitable.

Copy link
Member

@leighmcculloch leighmcculloch Feb 20, 2020

Choose a reason for hiding this comment

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

It's worth noting that this is only pinning to a version of Postgres, not to a version of the Docker image. The version number is the version of Postgres, and the Docker images can change independent of that with any change to the official Dockerfiles.

Specifying the patch version number wouldn't have prevented this breakage from occurring because the breaking change was a change to the Docker image (docker-library/postgres@42ce743), and not Postgres. See this PR for more information that is adding a warning to their docs about this as a result of this break: docker-library/postgres#689.

If we really want to ensure 100% stability this would need to pin to the image digest, but I think this is unnecessary as breaking changes are rare.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your point doesn't hold, because tags with the patch number are not rebuilt, and thus can be considered immutable for all they are worth
.

Copy link
Member

Choose a reason for hiding this comment

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

Oh nice, TIL. Thanks!

Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

🎉

@tamirms tamirms merged commit b7aebe6 into stellar:master Feb 21, 2020
@2opremio 2opremio deleted the fix-compose-pg-pwd branch February 21, 2020 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants