You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/10/09 17:44:39 UTC
[GitHub] [airflow] potiuk opened a new pull request #11384: Mutual SSL added in PGBouncer configuration in the Chart
potiuk opened a new pull request #11384:
URL: https://github.com/apache/airflow/pull/11384
Adds SSL configuration for PGBouncer in the Helm Chart. PGBouncer
is crucial to handle the big number of connections that airflow
opens for the database, but often the database is outside of the
Kubernetes Cluster or generally the environment where Airflow is
installed and PGBouncer needs to connect securely to such database.
This PR adds capability of seting CA/Certificate and Private Key
in the PGBouncer configuration that allows for mTLS authentication
(both client and server are authenticated) and secure connection
even over public network.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11384: Mutual SSL added in PGBouncer configuration in the Chart
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11384:
URL: https://github.com/apache/airflow/pull/11384#discussion_r502952325
##########
File path: chart/templates/secrets/pgbouncer-certificates-secret.yaml
##########
@@ -0,0 +1,44 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+################################
+## Pgbouncer Certificate Secret
+#################################
+{{- if .Values.pgbouncer.enabled }}
Review comment:
Not an expert in go templating but I think or on all certs/keys will work much better here. Seems that or on empty/null values works as expected.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #11384: Mutual SSL added in PGBouncer configuration in the Chart
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11384:
URL: https://github.com/apache/airflow/pull/11384#issuecomment-706594794
Seems like all the tests are passing :) and we tested it with the customer project.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11384: Mutual SSL added in PGBouncer configuration in the Chart
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11384:
URL: https://github.com/apache/airflow/pull/11384#discussion_r502696980
##########
File path: chart/templates/_helpers.yaml
##########
@@ -262,6 +266,13 @@ max_client_conn = {{ .Values.pgbouncer.maxClientConn }}
verbose = {{ .Values.pgbouncer.verbose }}
log_disconnections = {{ .Values.pgbouncer.logDisconnections }}
log_connections = {{ .Values.pgbouncer.logConnections }}
+
+server_tls_sslmode = {{ .Values.pgbouncer.sslmode }}
+server_tls_ca_file = /etc/pgbouncer/root.crt
Review comment:
I was waiting for this comment :) and I believe we should not guard it.
There is rather complex logic determining if those values should be used or not: https://www.pgbouncer.org/config.html
The default setting now is "prefer" which requests the TLS from the server and if it is ok, it proceeds, but the server cert is not validated (so not used), similar case is in "require" mode (surprisingly). If SSL is not accepted, the connection is rejected, but if it is accepted, neither the cA nor cert are validated (so they are not used).
Only in case of "verify-ca" or "verify-full" one or both are validated (respectively).
Additionally - If the server does not request client certificate, it's not presented at all (thus not read). So the client cert is only actually used if the server requests it.
So both server/client certs are not used by default even in "prefer" mode. And having them "empty" makes no difference whatsoever.
From my tests, the connection will work just fine in "prefer" mode if those are empty, and I'd rather leave it to the pgbouncer to use it as needed using its logic rather than trying to duplicate the logic in the helm chart.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #11384: Mutual SSL added in PGBouncer configuration in the Chart
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11384:
URL: https://github.com/apache/airflow/pull/11384#issuecomment-706317232
cc: @jaketf
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11384: Mutual SSL added in PGBouncer configuration in the Chart
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11384:
URL: https://github.com/apache/airflow/pull/11384#discussion_r502686520
##########
File path: chart/templates/_helpers.yaml
##########
@@ -262,6 +266,13 @@ max_client_conn = {{ .Values.pgbouncer.maxClientConn }}
verbose = {{ .Values.pgbouncer.verbose }}
log_disconnections = {{ .Values.pgbouncer.logDisconnections }}
log_connections = {{ .Values.pgbouncer.logConnections }}
+
+server_tls_sslmode = {{ .Values.pgbouncer.sslmode }}
+server_tls_ca_file = /etc/pgbouncer/root.crt
Review comment:
Do these not need to be guarded so we don't set out if the options are empty?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11384: Mutual SSL added in PGBouncer configuration in the Chart
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11384:
URL: https://github.com/apache/airflow/pull/11384#discussion_r502865530
##########
File path: chart/templates/_helpers.yaml
##########
@@ -262,6 +266,13 @@ max_client_conn = {{ .Values.pgbouncer.maxClientConn }}
verbose = {{ .Values.pgbouncer.verbose }}
log_disconnections = {{ .Values.pgbouncer.logDisconnections }}
log_connections = {{ .Values.pgbouncer.logConnections }}
+
+server_tls_sslmode = {{ .Values.pgbouncer.sslmode }}
+server_tls_ca_file = /etc/pgbouncer/root.crt
Review comment:
Actualy I rethought it. I added missing schema and implemented it in a "proper" way -each of the files is separately guarded now - both in the secret and in the .ini file.
Should be good to go now :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11384: Mutual SSL added in PGBouncer configuration in the Chart
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11384:
URL: https://github.com/apache/airflow/pull/11384#discussion_r502951011
##########
File path: chart/templates/secrets/pgbouncer-certificates-secret.yaml
##########
@@ -0,0 +1,44 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+################################
+## Pgbouncer Certificate Secret
+#################################
+{{- if .Values.pgbouncer.enabled }}
Review comment:
True. Let me see how I can get rid of the secret entirely.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #11384: Mutual SSL added in PGBouncer configuration in the Chart
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #11384:
URL: https://github.com/apache/airflow/pull/11384
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11384: Mutual SSL added in PGBouncer configuration in the Chart
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11384:
URL: https://github.com/apache/airflow/pull/11384#discussion_r502949854
##########
File path: chart/templates/secrets/pgbouncer-certificates-secret.yaml
##########
@@ -0,0 +1,44 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+################################
+## Pgbouncer Certificate Secret
+#################################
+{{- if .Values.pgbouncer.enabled }}
Review comment:
This would create an empty secret if the ssl file options aren't set - would be nice to avoid that
##########
File path: chart/values.schema.json
##########
@@ -764,6 +764,33 @@
"logConnections": {
"description": "Log successful logins.",
"type": "integer"
+ },
+ "sslmode": {
+ "description": "SSL mode for pgbouncer.",
+ "type": "string",
+ "enum": ["disable", "allow", "prefer", "require", "verify-ca", "verify-full"]
+ },
+ "ciphers": {
+ "description": "The allowed ciphers, might be 'fast', 'normal' or list ciphers separated with :.",
Review comment:
```suggestion
"description": "The allowed ciphers, might be 'fast', 'normal' or list ciphers separated with ':'.",
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11384: Mutual SSL added in PGBouncer configuration in the Chart
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11384:
URL: https://github.com/apache/airflow/pull/11384#discussion_r502696980
##########
File path: chart/templates/_helpers.yaml
##########
@@ -262,6 +266,13 @@ max_client_conn = {{ .Values.pgbouncer.maxClientConn }}
verbose = {{ .Values.pgbouncer.verbose }}
log_disconnections = {{ .Values.pgbouncer.logDisconnections }}
log_connections = {{ .Values.pgbouncer.logConnections }}
+
+server_tls_sslmode = {{ .Values.pgbouncer.sslmode }}
+server_tls_ca_file = /etc/pgbouncer/root.crt
Review comment:
I was waiting for this comment :) and I believe we should not guard it.
There is rather complex logic determining if those values should be used or not: https://www.pgbouncer.org/config.html
The default setting now is "prefer" which requests the TLS from the server and if it is ok, it proceeds, but the server cert is not validated (so not used), similar case is in "require" mode (surprisingly). If SSL is not accepted, the connection is rejected, but if it is accepted, neither the cA nor cert are validated (soi they are not used).
Only in case of "verify-ca" or "verify-full" one or both are validated (respectively).
Additionally - If the server does not request client certificate, it's not presented at all (thus not read). So the client cert is only actually used if the server requests it.
So both server/client certs are not used by default even in "prefer" mode. And having them "empty" makes no difference whatsoever.
From my tests, the connection will work just fine in "prefer" mode if those are empty, and I'd rather leave it to the pgbouncer to use it as needed using its logic rather than trying to duplicate the logic in the helm chart.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org