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 2021/05/27 13:44:42 UTC
[GitHub] [airflow] Junnplus opened a new pull request #16117: Hard-coded container port
Junnplus opened a new pull request #16117:
URL: https://github.com/apache/airflow/pull/16117
closes: https://github.com/apache/airflow/issues/16039
---
**^ 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] kaxil commented on a change in pull request #16117: Hard-coded container port
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #16117:
URL: https://github.com/apache/airflow/pull/16117#discussion_r641528381
##########
File path: chart/values.yaml
##########
@@ -846,8 +846,18 @@ elasticsearch:
# port: ~
connection: {}
+# All container ports used by chart, hard code here.
+_ports:
Review comment:
Needs a different name than `_ports`
cc @jedcunningham
--
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] Junnplus commented on pull request #16117: Hard-coded container port
Posted by GitBox <gi...@apache.org>.
Junnplus commented on pull request #16117:
URL: https://github.com/apache/airflow/pull/16117#issuecomment-850861580
> It'd be easy to block attempted changes via config.webserver.web_server_port, but env vars are harder to comprehensively block. Another plus, our values is big enough as it is.
You're right. This issue needs to be discussed.
I will close this and make other PR simple fix for set AirflowUI port to 80.
--
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] Junnplus edited a comment on pull request #16117: Hard-coded container port
Posted by GitBox <gi...@apache.org>.
Junnplus edited a comment on pull request #16117:
URL: https://github.com/apache/airflow/pull/16117#issuecomment-850861580
> It'd be easy to block attempted changes via config.webserver.web_server_port, but env vars are harder to comprehensively block. Another plus, our values is big enough as it is.
You're right. This issue needs to be discussed.
I will close this and make other PR simple fix for set AirflowUI port to 80.
--
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] jedcunningham commented on a change in pull request #16117: Hard-coded container port
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #16117:
URL: https://github.com/apache/airflow/pull/16117#discussion_r641596500
##########
File path: chart/values.yaml
##########
@@ -919,19 +929,22 @@ config:
colored_console_log: 'False'
metrics:
statsd_on: '{{ ternary "True" "False" .Values.statsd.enabled }}'
- statsd_port: 9125
+ statsd_port: '{{ .Values._ports.statsdIngest }}'
Review comment:
```suggestion
statsd_port: '{{ .Values.ports.statsdIngest }}'
```
This should be the service port as Airflow will go through the service.
##########
File path: chart/values.schema.json
##########
@@ -2153,8 +2153,64 @@
}
}
},
+ "_ports": {
+ "description": "All container ports used by chart.",
+ "type": "object",
+ "x-docsSection": "Ports",
+ "additionalProperties": false,
+ "properties": {
+ "flowerUI": {
+ "description": "Flower UI port.",
+ "type": "integer",
+ "default": 5555,
+ "const": 5555
+ },
+ "airflowUI": {
+ "description": "Airflow UI port.",
+ "type": "integer",
+ "default": 8080,
+ "const": 8080
Review comment:
I know this will block changes to the port when values.yaml is validated with values.schema.yaml, but having these "hard coded" in values.yaml where people are expecting to find changeable parameters would definitely be confusing.
If we do want to force them to use a certain container port, we should just hard code it in the templates instead (and in most cases we are talking about 1, maybe 2 places if we use named ports in services). It'd be easy to block attempted changes via `config.webserver.web_server_port`, but env vars are harder to comprehensively block. Another plus, our values is big enough as it is.
##########
File path: chart/values.yaml
##########
@@ -919,19 +929,22 @@ config:
colored_console_log: 'False'
metrics:
statsd_on: '{{ ternary "True" "False" .Values.statsd.enabled }}'
- statsd_port: 9125
+ statsd_port: '{{ .Values._ports.statsdIngest }}'
statsd_prefix: airflow
statsd_host: '{{ printf "%s-statsd" .Release.Name }}'
webserver:
+ web_server_port: '{{ .Values._ports.airflowUI }}'
enable_proxy_fix: 'True'
# For Airflow 1.10
rbac: 'True'
celery:
+ worker_log_server_port: '{{ .Values._ports.workerLogs }}'
+ flower_port: '{{ .Values._ports.flowerUI }}'
worker_concurrency: 16
scheduler:
# statsd params included for Airflow 1.10 backward compatibility; moved to [metrics] in 2.0
statsd_on: '{{ ternary "True" "False" .Values.statsd.enabled }}'
- statsd_port: 9125
+ statsd_port: '{{ .Values._ports.statsdIngest }}'
Review comment:
```suggestion
statsd_port: '{{ .Values.ports.statsdIngest }}'
```
--
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] mrbaguvix commented on pull request #16117: Hard-coded container port
Posted by GitBox <gi...@apache.org>.
mrbaguvix commented on pull request #16117:
URL: https://github.com/apache/airflow/pull/16117#issuecomment-850853005
Please pull `apache:master` to your branch
--
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] Brett37490 commented on pull request #16117: Hard-coded container port
Posted by GitBox <gi...@apache.org>.
Brett37490 commented on pull request #16117:
URL: https://github.com/apache/airflow/pull/16117#issuecomment-850328169
```
```
--
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] Junnplus closed pull request #16117: Hard-coded container port
Posted by GitBox <gi...@apache.org>.
Junnplus closed pull request #16117:
URL: https://github.com/apache/airflow/pull/16117
--
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] jedcunningham commented on a change in pull request #16117: Hard-coded container port
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #16117:
URL: https://github.com/apache/airflow/pull/16117#discussion_r641596016
##########
File path: chart/values.yaml
##########
@@ -846,8 +846,18 @@ elasticsearch:
# port: ~
connection: {}
+# All container ports used by chart, hard code here.
+_ports:
Review comment:
Yeah, I agree. Maybe `containerPorts`?
It might also make sense to only expose ports in values.yaml that can actually be changed (e.g. airflowUI, flowerUI and workerLogs), and actively prevent others from being changed if we can (e.g. pgbouncer).
--
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