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/08/26 11:54:58 UTC
[GitHub] [airflow] potiuk opened a new pull request #17847: Sane detection of the host/port in entrypoint prod
potiuk opened a new pull request #17847:
URL: https://github.com/apache/airflow/pull/17847
The previous regexp parsing was well, not perfect closely following the
ancient Chinese proverb "If you have problem, introduce regexp - you
will have two problems".
This PR replaces regexp matching with python urlsplit method.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/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/main/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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #17847: Sane detection of the host/port in entrypoint prod
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #17847:
URL: https://github.com/apache/airflow/pull/17847#discussion_r696662423
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -83,24 +84,12 @@ function wait_for_connection {
# It tries `CONNECTION_CHECK_MAX_COUNT` times and sleeps `CONNECTION_CHECK_SLEEP_TIME` between checks
local connection_url
connection_url="${1}"
- local detected_backend=""
- local detected_host=""
- local detected_port=""
-
- # Auto-detect DB parameters
- # Examples:
- # postgres://YourUserName:password@YourHostname:5432/YourDatabaseName
- # postgres://YourUserName:password@YourHostname:5432/YourDatabaseName
- # postgres://YourUserName:@YourHostname:/YourDatabaseName
- # postgres://YourUserName@YourHostname/YourDatabaseName
- [[ ${connection_url} =~ ([^:]*)://([^:@]*):?([^@]*)@?([^/:]*):?([0-9]*)/([^\?]*)\??(.*) ]] && \
- detected_backend=${BASH_REMATCH[1]} &&
- # Not used USER match
- # Not used PASSWORD match
- detected_host=${BASH_REMATCH[4]} &&
- detected_port=${BASH_REMATCH[5]} &&
- # Not used SCHEMA match
- # Not used PARAMS match
+ local detected_backend
+ detected_backend=$(python -c "from urllib.parse import urlsplit; print(urlsplit('${connection_url}').scheme)")
Review comment:
```suggestion
detected_backend=$(python -c "from urllib.parse import urlsplit; import sys; print(urlsplit(sys.argv[1]).scheme)" "${connection_url}")
```
I am concerned that some values may lead to Command Injection and therefore incorrect script behavior.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #17847: Sane detection of the host/port in entrypoint prod
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #17847:
URL: https://github.com/apache/airflow/pull/17847
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #17847: Sane detection of the host/port in entrypoint prod
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #17847:
URL: https://github.com/apache/airflow/pull/17847#issuecomment-906812893
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #17847: Sane detection of the host/port in entrypoint prod
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17847:
URL: https://github.com/apache/airflow/pull/17847#discussion_r696977910
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -83,24 +84,12 @@ function wait_for_connection {
# It tries `CONNECTION_CHECK_MAX_COUNT` times and sleeps `CONNECTION_CHECK_SLEEP_TIME` between checks
local connection_url
connection_url="${1}"
- local detected_backend=""
- local detected_host=""
- local detected_port=""
-
- # Auto-detect DB parameters
- # Examples:
- # postgres://YourUserName:password@YourHostname:5432/YourDatabaseName
- # postgres://YourUserName:password@YourHostname:5432/YourDatabaseName
- # postgres://YourUserName:@YourHostname:/YourDatabaseName
- # postgres://YourUserName@YourHostname/YourDatabaseName
- [[ ${connection_url} =~ ([^:]*)://([^:@]*):?([^@]*)@?([^/:]*):?([0-9]*)/([^\?]*)\??(.*) ]] && \
- detected_backend=${BASH_REMATCH[1]} &&
- # Not used USER match
- # Not used PASSWORD match
- detected_host=${BASH_REMATCH[4]} &&
- detected_port=${BASH_REMATCH[5]} &&
- # Not used SCHEMA match
- # Not used PARAMS match
+ local detected_backend
+ detected_backend=$(python -c "from urllib.parse import urlsplit; print(urlsplit('${connection_url}').scheme)")
Review comment:
adding separate function is what I tried initially, and I REALLY found it unnecessary
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #17847: Sane detection of the host/port in entrypoint prod
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #17847:
URL: https://github.com/apache/airflow/pull/17847#discussion_r696666501
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -83,24 +84,12 @@ function wait_for_connection {
# It tries `CONNECTION_CHECK_MAX_COUNT` times and sleeps `CONNECTION_CHECK_SLEEP_TIME` between checks
local connection_url
connection_url="${1}"
- local detected_backend=""
- local detected_host=""
- local detected_port=""
-
- # Auto-detect DB parameters
- # Examples:
- # postgres://YourUserName:password@YourHostname:5432/YourDatabaseName
- # postgres://YourUserName:password@YourHostname:5432/YourDatabaseName
- # postgres://YourUserName:@YourHostname:/YourDatabaseName
- # postgres://YourUserName@YourHostname/YourDatabaseName
- [[ ${connection_url} =~ ([^:]*)://([^:@]*):?([^@]*)@?([^/:]*):?([0-9]*)/([^\?]*)\??(.*) ]] && \
- detected_backend=${BASH_REMATCH[1]} &&
- # Not used USER match
- # Not used PASSWORD match
- detected_host=${BASH_REMATCH[4]} &&
- detected_port=${BASH_REMATCH[5]} &&
- # Not used SCHEMA match
- # Not used PARAMS match
+ local detected_backend
+ detected_backend=$(python -c "from urllib.parse import urlsplit; print(urlsplit('${connection_url}').scheme)")
Review comment:
For readability, we can extraactt it to a separate function.
```
function get_url_part() {
url_part="$1";
connection_url="$2";
python -c "from urllib.parse import urlsplit; import sys; print(getattr(urlsplit(sys.argv[2]), sys.argv[1]))" "${url_part}" "${connection_url}"
}
get_url_part scheme redis://
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #17847: Sane detection of the host/port in entrypoint prod
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #17847:
URL: https://github.com/apache/airflow/pull/17847#discussion_r696666501
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -83,24 +84,12 @@ function wait_for_connection {
# It tries `CONNECTION_CHECK_MAX_COUNT` times and sleeps `CONNECTION_CHECK_SLEEP_TIME` between checks
local connection_url
connection_url="${1}"
- local detected_backend=""
- local detected_host=""
- local detected_port=""
-
- # Auto-detect DB parameters
- # Examples:
- # postgres://YourUserName:password@YourHostname:5432/YourDatabaseName
- # postgres://YourUserName:password@YourHostname:5432/YourDatabaseName
- # postgres://YourUserName:@YourHostname:/YourDatabaseName
- # postgres://YourUserName@YourHostname/YourDatabaseName
- [[ ${connection_url} =~ ([^:]*)://([^:@]*):?([^@]*)@?([^/:]*):?([0-9]*)/([^\?]*)\??(.*) ]] && \
- detected_backend=${BASH_REMATCH[1]} &&
- # Not used USER match
- # Not used PASSWORD match
- detected_host=${BASH_REMATCH[4]} &&
- detected_port=${BASH_REMATCH[5]} &&
- # Not used SCHEMA match
- # Not used PARAMS match
+ local detected_backend
+ detected_backend=$(python -c "from urllib.parse import urlsplit; print(urlsplit('${connection_url}').scheme)")
Review comment:
For readability, we can extraactt it to a separate function.
```bash
function get_url_part() {
url_part="$1";
connection_url="$2";
python -c "from urllib.parse import urlsplit; import sys; print(getattr(urlsplit(sys.argv[2]), sys.argv[1]))" "${url_part}" "${connection_url}"
}
get_url_part scheme redis://
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #17847: Sane detection of the host/port in entrypoint prod
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17847:
URL: https://github.com/apache/airflow/pull/17847#discussion_r696977515
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -83,24 +84,12 @@ function wait_for_connection {
# It tries `CONNECTION_CHECK_MAX_COUNT` times and sleeps `CONNECTION_CHECK_SLEEP_TIME` between checks
local connection_url
connection_url="${1}"
- local detected_backend=""
- local detected_host=""
- local detected_port=""
-
- # Auto-detect DB parameters
- # Examples:
- # postgres://YourUserName:password@YourHostname:5432/YourDatabaseName
- # postgres://YourUserName:password@YourHostname:5432/YourDatabaseName
- # postgres://YourUserName:@YourHostname:/YourDatabaseName
- # postgres://YourUserName@YourHostname/YourDatabaseName
- [[ ${connection_url} =~ ([^:]*)://([^:@]*):?([^@]*)@?([^/:]*):?([0-9]*)/([^\?]*)\??(.*) ]] && \
- detected_backend=${BASH_REMATCH[1]} &&
- # Not used USER match
- # Not used PASSWORD match
- detected_host=${BASH_REMATCH[4]} &&
- detected_port=${BASH_REMATCH[5]} &&
- # Not used SCHEMA match
- # Not used PARAMS match
+ local detected_backend
+ detected_backend=$(python -c "from urllib.parse import urlsplit; print(urlsplit('${connection_url}').scheme)")
Review comment:
I prefer to leave it explicit. I think it is far less readable with url_part parameter to be honest.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #17847: Sane detection of the host/port in entrypoint prod
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17847:
URL: https://github.com/apache/airflow/pull/17847#discussion_r696979652
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -83,24 +84,12 @@ function wait_for_connection {
# It tries `CONNECTION_CHECK_MAX_COUNT` times and sleeps `CONNECTION_CHECK_SLEEP_TIME` between checks
local connection_url
connection_url="${1}"
- local detected_backend=""
- local detected_host=""
- local detected_port=""
-
- # Auto-detect DB parameters
- # Examples:
- # postgres://YourUserName:password@YourHostname:5432/YourDatabaseName
- # postgres://YourUserName:password@YourHostname:5432/YourDatabaseName
- # postgres://YourUserName:@YourHostname:/YourDatabaseName
- # postgres://YourUserName@YourHostname/YourDatabaseName
- [[ ${connection_url} =~ ([^:]*)://([^:@]*):?([^@]*)@?([^/:]*):?([0-9]*)/([^\?]*)\??(.*) ]] && \
- detected_backend=${BASH_REMATCH[1]} &&
- # Not used USER match
- # Not used PASSWORD match
- detected_host=${BASH_REMATCH[4]} &&
- detected_port=${BASH_REMATCH[5]} &&
- # Not used SCHEMA match
- # Not used PARAMS match
+ local detected_backend
+ detected_backend=$(python -c "from urllib.parse import urlsplit; print(urlsplit('${connection_url}').scheme)")
Review comment:
> I am concerned that some inputs may lead to Command Injection and therefore incorrect script behavior.
Yep. Agree on that one.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #17847: Sane detection of the host/port in entrypoint prod
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #17847:
URL: https://github.com/apache/airflow/pull/17847#discussion_r696662423
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -83,24 +84,12 @@ function wait_for_connection {
# It tries `CONNECTION_CHECK_MAX_COUNT` times and sleeps `CONNECTION_CHECK_SLEEP_TIME` between checks
local connection_url
connection_url="${1}"
- local detected_backend=""
- local detected_host=""
- local detected_port=""
-
- # Auto-detect DB parameters
- # Examples:
- # postgres://YourUserName:password@YourHostname:5432/YourDatabaseName
- # postgres://YourUserName:password@YourHostname:5432/YourDatabaseName
- # postgres://YourUserName:@YourHostname:/YourDatabaseName
- # postgres://YourUserName@YourHostname/YourDatabaseName
- [[ ${connection_url} =~ ([^:]*)://([^:@]*):?([^@]*)@?([^/:]*):?([0-9]*)/([^\?]*)\??(.*) ]] && \
- detected_backend=${BASH_REMATCH[1]} &&
- # Not used USER match
- # Not used PASSWORD match
- detected_host=${BASH_REMATCH[4]} &&
- detected_port=${BASH_REMATCH[5]} &&
- # Not used SCHEMA match
- # Not used PARAMS match
+ local detected_backend
+ detected_backend=$(python -c "from urllib.parse import urlsplit; print(urlsplit('${connection_url}').scheme)")
Review comment:
```suggestion
detected_backend=$(python -c "from urllib.parse import urlsplit; import sys; print(urlsplit(sys.argv[1]).scheme)" "${connection_url}")
```
I am concerned that some inputs may lead to Command Injection and therefore incorrect script behavior.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org