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: title: Default sslmode 'prefer' unsupported by Grafana #1082

Merged

Conversation

andriisoldatenko
Copy link
Contributor

@andriisoldatenko andriisoldatenko commented May 3, 2021

@@ -7,3 +7,12 @@ tests:
asserts:
- isKind:
of: Deployment
- it: should work with ssl mode
set:
global:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danielhoherd ☝️

Copy link
Member

Choose a reason for hiding this comment

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

Globals are broken with the version of helm-unittest that we are using. There is a ticket open to move to a newer version (https://github.com/astronomer/issues/issues/2701) or we could switch to using pytest like FOSS airflow chart does (https://github.com/astronomer/airflow/blob/master/chart/tests/test_dags_persistent_volume_claim.py)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah it's a good idea, we should invest some time since we are use a lot of helm-template logic.

@andriisoldatenko andriisoldatenko changed the title layout PR Fix: title: Default sslmode 'prefer' unsupported by Grafana May 4, 2021
@danielhoherd
Copy link
Member

I think we should get some feedback from @astronomer/customer-support about this change. Will this break existing installations? Also what kind of docs changes need to go along with this? @jwitz

@jwitz
Copy link
Contributor

jwitz commented May 4, 2021

@danielhoherd If it's any kind of breaking change, I'll document it in the "Upgrade to v0.25" doc with pre-upgrade considerations. Otherwise, I was planning to mention this only in release notes. Do you think this would have any implications beyond upgrade, as in should we update any of our existing install/build recommendations now that this is supported?

@andriisoldatenko
Copy link
Contributor Author

@danielhoherd @vishwas-astro i tested locally this change works well with grafana 7.5.4:

cat docker-compose.yml
version: '3.1'

services:
  grafana:
    image: grafana/grafana:7.5.4
    container_name: grafana
    ports:
      - 3000:3000
    environment:
      - GF_DATABASE_URL=postgres://postgres:postgres@postgres:5432/postgres?sslmode=require
      - GF_DATABASE_SSL_MODE=require
    links:
      - postgres
  # we will use postgres as datasource.
  postgres:
    image: postgres:12-alpine
    command: >
      -c ssl=on
      -c ssl_cert_file=/var/lib/postgresql/server.crt
      -c ssl_key_file=/var/lib/postgresql/server.key
    environment:
      # POSTGRES_HOST_AUTH_METHOD: trust
      POSTGRES_USER: postgres     # define credentials
      POSTGRES_PASSWORD: postgres # define credentials
      POSTGRES_DB: postgres       # define database
    volumes:
      - ./server.crt:/var/lib/postgresql/server.crt:ro
      - ./server.key:/var/lib/postgresql/server.key:ro
    ports:
      - 5432:5432                 # Postgres port
dco ps
         Name                       Command               State           Ports
----------------------------------------------------------------------------------------
grafana                  /run.sh                          Up      0.0.0.0:3000->3000/tcp
grafana_lab_postgres_1   docker-entrypoint.sh -c ss ...   Up      0.0.0.0:5432->5432/tcp
grafana     | t=2021-05-05T20:25:51+0000 lvl=info msg="Config overridden from command line" logger=settings arg="default.log.mode=cons[34/1812]grafana     | t=2021-05-05T20:25:51+0000 lvl=info msg="Config overridden from Environment variable" logger=settings var="GF_PATHS_DATA=/var/lib
/grafana"                                                                                                                                      grafana     | t=2021-05-05T20:25:51+0000 lvl=info msg="Config overridden from Environment variable" logger=settings var="GF_PATHS_LOGS=/var/log
/grafana"                                                                                                                                      grafana     | t=2021-05-05T20:25:51+0000 lvl=info msg="Config overridden from Environment variable" logger=settings var="GF_PATHS_PLUGINS=/var/
lib/grafana/plugins"                                                                                                                           grafana     | t=2021-05-05T20:25:51+0000 lvl=info msg="Config overridden from Environment variable" logger=settings var="GF_PATHS_PROVISIONING=
/etc/grafana/provisioning"                                                                                                                     grafana     | t=2021-05-05T20:25:51+0000 lvl=info msg="Config overridden from Environment variable" logger=settings var="GF_DATABASE_URL=postgr
es://postgres:-redacted-@postgres:5432/postgres?sslmode=disable"                                                                               grafana     | t=2021-05-05T20:25:51+0000 lvl=info msg="Config overridden from Environment variable" logger=settings var="GF_DATABASE_SSL_MODE=d
isable"                                                                                                                                        grafana     | t=2021-05-05T20:25:51+0000 lvl=info msg="Path Home" logger=settings path=/usr/share/grafana
grafana     | t=2021-05-05T20:25:51+0000 lvl=info msg="Path Data" logger=settings path=/var/lib/grafana                                        grafana     | t=2021-05-05T20:25:51+0000 lvl=info msg="Path Logs" logger=settings path=/var/log/grafana
grafana     | t=2021-05-05T20:25:51+0000 lvl=info msg="Path Plugins" logger=settings path=/var/lib/grafana/plugins                             grafana     | t=2021-05-05T20:25:51+0000 lvl=info msg="Path Provisioning" logger=settings path=/etc/grafana/provisioning
grafana     | t=2021-05-05T20:25:51+0000 lvl=info msg="App mode production" logger=settings                                                    grafana     | t=2021-05-05T20:25:51+0000 lvl=info msg="Connecting to DB" logger=sqlstore dbtype=postgres
grafana     | t=2021-05-05T20:25:51+0000 lvl=info msg="Starting DB migrations" logger=migrator                                                 grafana     | t=2021-05-05T20:25:51+0000 lvl=info msg="migrations completed" logger=migrator performed=0 skipped=279 duration=718.578µs
grafana     | t=2021-05-05T20:25:51+0000 lvl=info msg="Starting plugin search" logger=plugins                                                  grafana     | t=2021-05-05T20:25:51+0000 lvl=info msg="Registering plugin" logger=plugins id=input
postgres@localhost:postgres> SELECT datname,usename, ssl, client_addr
   FROM pg_stat_ssl
   JOIN pg_stat_activity
     ON pg_stat_ssl.pid = pg_stat_activity.pid;
+-----------+-----------+-------+---------------+
| datname   | usename   | ssl   | client_addr   |
|-----------+-----------+-------+---------------|
| <null>    | <null>    | False | <null>        |
| <null>    | postgres  | False | <null>        |
| postgres  | postgres  | False | 172.24.0.3    |
| postgres  | postgres  | True  | 172.24.0.1    |
| postgres  | postgres  | False | 172.24.0.3    |
| <null>    | <null>    | False | <null>        |
| <null>    | <null>    | False | <null>        |
| <null>    | <null>    | False | <null>        |
+-----------+-----------+-------+---------------+
SELECT 8
Time: 0.019s

values.yaml Outdated Show resolved Hide resolved
@andriisoldatenko
Copy link
Contributor Author

@danielhoherd PTAL

@andriisoldatenko andriisoldatenko merged commit b4862eb into master May 7, 2021
@andriisoldatenko andriisoldatenko deleted the add-ability-to-configure-sslmode-for-grafana branch May 7, 2021 17:36
andriisoldatenko added a commit that referenced this pull request May 8, 2021
* layout PR

* remove test

* change default value for grafana sslmode from disable to require

* add more clear comment for grafana ssl
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.

None yet

3 participants