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/07 23:44:41 UTC
[GitHub] [airflow] EliMor opened a new issue #17489: Helm chart converts ' ' to '+' in passwords due to urlJoin helm function
EliMor opened a new issue #17489:
URL: https://github.com/apache/airflow/issues/17489
<!--
IMPORTANT!!!
PLEASE CHECK "SIMILAR TO X EXISTING ISSUES" OPTION IF VISIBLE
NEXT TO "SUBMIT NEW ISSUE" BUTTON!!!
PLEASE CHECK IF THIS ISSUE HAS BEEN REPORTED PREVIOUSLY USING SEARCH!!!
Please complete the next sections or the issue will be closed.
These questions are the first thing we need to know to understand the context.
-->
**Apache Airflow version**: 2.1.2
**Kubernetes version (if you are using kubernetes)** (use `kubectl version`): 1.19
**What happened**:
When using the helm chart with postgres or redis passwords that have a ' ', the chart will convert the space to a '+' due to the helm urlJoin function
**What you expected to happen**:
Passwords provided should not be modified by code.
<!-- What do you think went wrong? -->
**How to reproduce it**: write a dummy password in the values.yaml file that has a ' ' and see what the secret that gets generated looks like. You will see a '+' replaced.
<!---
As minimally and precisely as possible. Keep in mind we do not have access to your cluster or dags.
If you are using kubernetes, please attempt to recreate the issue using minikube or kind.
## Install minikube/kind
- Minikube https://minikube.sigs.k8s.io/docs/start/
- Kind https://kind.sigs.k8s.io/docs/user/quick-start/
If this is a UI bug, please provide a screenshot of the bug or a link to a youtube video of the bug in action
You can include images using the .md style of
To record a screencast, mac users can use QuickTime and then create an unlisted youtube video with the resulting .mov file.
--->
**Anything else we need to know**:
Location of error;
https://github.com/apache/airflow/blob/main/chart/templates/secrets/metadata-connection-secret.yaml#L44
https://github.com/apache/airflow/blob/main/chart/templates/secrets/redis-secrets.yaml#L77
<!--
How often does this problem occur? Once? Every time etc?
Any relevant logs to include? Put them here in side a detail tag:
<details><summary>x.log</summary> lots of stuff </details>
-->
--
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 issue #17489: Helm chart converts ' ' to '+' in passwords (and username)
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-899311549
> * **Whats the rationale for constructing and encoding the URL to be saved and putting it in the Secret?** This is as opposed to plopping the secret as provided by the user in the values.yaml and then building and encoding the URL inside the containers using some standard encoding & url construction strategy. Is it because this seemed like wasted work done on every pod reboot?
Because Airflow takes URL as parameter and it's standard way for airflow (and airflow does it because this is the way how sqlalchemy does it) on one hand, or because it makes a unified way of passing not only the host/user/password/ but also extra parameters. If we want to construct the URL from secrets to Airflow, we would need some extra code to do that on the flight, rather than mapping the secret. In Airflow you simply "speak" URL when it comes to connections. You have to remember that Helm Chart and K8S are one of several ways Airflow can be deployed and in our choice we are airflow-centric not K8S centric. We feel that it's K8S that should bend to airflow ways, not the other way. You can see more at our talk from this year's summit: https://airflowsummit.org/sessions/2021/airflow-loves-kubernetes/
> Do we think that a preconnect check should happen in the migration pod? I thought it was strange that it passed just fine and I didn't see connection errors until we were to the worker-pods this is why I initially assumed that some extra funky encoding was happening again behind the scenes and why using spaces raw seemed to work. I also thought it was strange that I did not see any of the other pods in a crashloop as well. Please correct me if I'm wrong about this.
Of course it should. And maybe you discovered an issue there. Maybe you can track it down and provide a PR fixing that? That would be fantastic contribution back that would "pave the way" for others who have similar problem. Airflow is built by community and it's super helpful if users who get it for free are contributing back those kind of things.
--
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] EliMor edited a comment on issue #17489: Helm chart converts ' ' to '+' in passwords (and username)
Posted by GitBox <gi...@apache.org>.
EliMor edited a comment on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-894863824
Apologies for my bad assumption(s)!
That's annoying! Small minefield for me to remember now.
To minimize confusion to potential readers, I've corrected the issue to reflect what y'all have discovered.
I'm sorry If doing so I committed a faux pas. I am open and grateful for criticism anywhere!
Thanks.
--
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] EliMor edited a comment on issue #17489: Helm chart converts ' ' to '+' in passwords (and username)
Posted by GitBox <gi...@apache.org>.
EliMor edited a comment on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-894841242
Hi people!
I thought it was strange that it was encoded as '+' as well!!
Here's a more clear example...
If I have in my values.yaml file:
```
metadataConnection:
user: airflow
pass: top secret password
protocol: postgresql
host: fakedb.us-west-2.rds.amazonaws.com
port: 5432
db: postgres
sslmode: disable
```
Here's how we get rendered in the Secret (base64 decoded)
```
postgresql://airflow:top+secret+password@fakedb.us-west-2.rds.amazonaws.com:5432/postgres?sslmode=disable
```
This will result in a connection error unless I manually update the secret to:
```
postgresql://airflow:top secret password@fakedb.us-west-2.rds.amazonaws.com:5432/postgres?sslmode=disable
```
Strange no?! ~~I don't understand how the second one works.~~ (I just had a chance to try this again and although it seems to get past the migration -- no apparent loud failures --, the workers go into a death spiral as they should!)
No big on my end to just not have spaces. :D
--
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] EliMor commented on issue #17489: Helm chart converts ' ' to '+' in passwords (and username)
Posted by GitBox <gi...@apache.org>.
EliMor commented on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-899289298
So...
Some questions I had lingering in my head, no response expected on my end. Please keep in mind I'm very naive/lacking any context and there are extra implicit assumptions here but...
- **Whats the rationale for constructing and encoding the URL to be saved and putting it in the Secret?** This is as opposed to plopping the secret as provided by the user in the values.yaml and then building and encoding the URL inside the containers using some standard encoding & url construction strategy. Is it because this seemed like wasted work done on every pod reboot?
- **Do we think that a preconnect check should happen in the migration pod?** I thought it was strange that it passed just fine and I didn't see connection errors until we were to the worker-pods this is why I initially assumed that some extra funky encoding was happening again behind the scenes and why using spaces raw seemed to work. I also thought it was strange that I did not see any of the other pods in a crashloop as well. Please correct me if I'm wrong about this.
Cheers.
--
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 edited a comment on issue #17489: Helm chart converts ' ' to '+' in passwords (and username)
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-899730815
> If the above are true and if the argument is that the use of the helm chart should be 'intuitively' or 'naturally' the way Airflow does things (I'm extrapolating from your statement that kube should bend to Airflow), then it seems that the chart should be structured to look as close to Airflow directly as possible.
You are extrapolating a bit more than what I wrote. Your assumptions do not take into account the audiences. I never said "human" providing the URL. I said "Secret" providing URL to Airflow. There is one more step between Human entering configuration and Secret Keeping it. The reason why you have it in helm chart, is because the Helm chart is for "Humans" to configure, while the "Secrets" is for "airflow" to consume. and for "Kubernetes" to store.
What the helm chart does - It takes "user" input and converts it (on the flight) to "URL" format that is stored in "Secret" if you look under the hood. I cannot imagine "human" by hand providing "Secret". This is established practice in K8S that you prepare Kubernetes Resources (including secrets) via some kind of templating engine (usually Go-templating based) to make it easier for "Human" to provide the right values. This is what Helm Chart is doing and it uses the filters to convert the values.
Similarly in Airflow we provide a tool: https://airflow.apache.org/docs/apache-airflow/stable/howto/connection.html#generating-a-connection-uri which allows Human to generate the URI to use by providing all the values separately.
Same princilple. pretty consistently applied across the board.
--
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 issue #17489: Helm chart converts ' ' to '+' in passwords due to urlJoin helm function
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-894850015
@jedcunningham @kaxil -> I think this one is pretty legit, if ' ' is encoded as '+' in the URI, this is pretty wrong, possibly we could use different encoding function in the 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] EliMor edited a comment on issue #17489: Helm chart converts ' ' to '+' in passwords due to urlJoin helm function
Posted by GitBox <gi...@apache.org>.
EliMor edited a comment on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-894841242
Hi people!
I thought it was strange that it was encoded as '+' as well!!
Here's a more clear example...
If I have in my values.yaml file:
```
metadataConnection:
user: airflow
pass: top secret password
protocol: postgresql
host: fakedb.us-west-2.rds.amazonaws.com
port: 5432
db: postgres
sslmode: disable
```
Here's how we get rendered in the Secret (base64 decoded)
```
postgresql://airflow:top+secret+password@fakedb.us-west-2.rds.amazonaws.com:5432/postgres?sslmode=disable
```
This will result in a connection error unless I manually update the secret to:
```
postgresql://airflow:top secret password@fakedb.us-west-2.rds.amazonaws.com:5432/postgres?sslmode=disable
```
Strange no?! I don't understand how the second one works.
No big on my end to just not have spaces. :D
--
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] EliMor edited a comment on issue #17489: Helm chart converts ' ' to '+' in passwords (and username)
Posted by GitBox <gi...@apache.org>.
EliMor edited a comment on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-899289298
So...
Some questions I had lingering in my head, no response expected on my end. Please keep in mind I'm very naive/lacking any context and there are extra implicit assumptions here but...
- **Whats the rationale for constructing and encoding the URL to be saved and putting it in the Secret?** This is as opposed to plopping the secret as provided by the user in the values.yaml and then building and encoding the URL inside the containers using some standard encoding & url construction strategy. Is it because this seemed like wasted work done on every pod reboot? Perhaps an extra benefit as a user is that if I want to make my own Secret, I could use the visual cue in the values.yaml to make the Secret and not have to worry about encoding myself.
- **Do we think that a preconnect check should happen in the migration pod?** I thought it was strange that it passed just fine and I didn't see connection errors until we were to the worker-pods this is why I initially assumed that some extra funky encoding was happening again behind the scenes and why using spaces raw seemed to work. I also thought it was strange that I did not see any of the other pods in a crashloop as well. Please correct me if I'm wrong about this.
Cheers.
--
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] EliMor edited a comment on issue #17489: Helm chart converts ' ' to '+' in passwords due to urlJoin helm function
Posted by GitBox <gi...@apache.org>.
EliMor edited a comment on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-894841242
Hi people!
I thought it was strange that it was encoded as '+' as well!!
Here's a more clear example...
If I have in my values.yaml file:
```
metadataConnection:
user: airflow
pass: top secret password
protocol: postgresql
host: fakedb.us-west-2.rds.amazonaws.com
port: 5432
db: postgres
sslmode: disable
```
Here's how we get rendered in the Secret (base64 decoded)
```
postgresql://airflow:top+secret+password@fakedb.us-west-2.rds.amazonaws.com:5432/postgres?sslmode=disable
```
This will result in a connection error unless I manually update the secret to:
```
postgresql://airflow:top secret password@fakedb.us-west-2.rds.amazonaws.com:5432/postgres?sslmode=disable
```
Strange no?!
No big on my end to just not have spaces. :D
--
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] EliMor edited a comment on issue #17489: Helm chart converts ' ' to '+' in passwords due to urlJoin helm function
Posted by GitBox <gi...@apache.org>.
EliMor edited a comment on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-894841242
Hi people!
I thought it was strange that it was encoded as '+' as well!!
Here's a more clear example...
If I have in my values.yaml file:
```
metadataConnection:
user: airflow
pass: top secret password
protocol: postgresql
host: fakedb.us-west-2.rds.amazonaws.com
port: 5432
db: postgres
sslmode: disable
```
Here's how we get rendered in the Secret (base64 decoded)
```
postgresql://airflow:top+secret+password@fakedb.us-west-2.rds.amazonaws.com:5432/postgres?sslmode=disable
```
This will result in a connection error unless I manually update the secret to:
```
postgresql://airflow:top secret password@fakedb.us-west-2.rds.amazonaws.com:5432/postgres?sslmode=disable
```
Strange no?! ~~I don't understand how the second one works.~~ (I just had a chance to try this again and although it seems to get past the migration, the workers go into a death spiral as they should!)
No big on my end to just not have spaces. :D
--
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 edited a comment on issue #17489: Helm chart converts ' ' to '+' in passwords due to urlJoin helm function
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-894825583
Yeah @mik-laj is right. You can also see examples of that explained in our documentation - https://airflow.apache.org/docs/apache-airflow/stable/howto/connection.html#handling-of-special-characters-in-connection-params according to https://datatracker.ietf.org/doc/html/rfc3986 both pasword and user should be percent-encoded.
However I am a bit surprised to see '+' in your issue rather than %20 (which should be used in this case).
The '+' replacing ' ' is part of another specification https://datatracker.ietf.org/doc/html/rfc1866 - there space might be replaced by '+' only in the context of form fields or form values - strictly when the HTML data is encoded with `application/x-www-form-urlencoded`. This sounds as completely different case and should never be used in URI's (though from what I know a lot of URI parsers will happily accept + as 'space' in other places, but this is rather a side-effect not specification-conformance.
Could you please @EliMor provide us examples of such secrets generated (with some bogus passwords?) was it part of the URI or form? Or some other secret content?
--
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 issue #17489: Helm chart converts ' ' to '+' in passwords (and username)
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-899492327
> So, in your opinion, do you think simply explicitly enforcing `AIRFLOW__CORE*` as env variables OR providing an "airflow.cfg" as a Secret in the helm chart should be the ONLY way to do things?
Strange statement. I do not understand what you are trying to ask and for what purpose. I simply do not understand what you are asking about? Could you elaborate and explain what you mean by "the ONLY way to do things?" - this is so general that I am not sure what you are asking about.
--
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] boring-cyborg[bot] commented on issue #17489: Helm chart converts ' ' to '+' in passwords due to urlJoin helm function
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-894719076
Thanks for opening your first issue here! Be sure to follow the issue template!
--
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] EliMor commented on issue #17489: Helm chart converts ' ' to '+' in passwords (and username)
Posted by GitBox <gi...@apache.org>.
EliMor commented on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-899322540
Thanks for the extra context.
I admit I most likely would have the impulse to want to build some bridge between 'the airflow way' things happen and what I assume is an intuitive way to engage with the helm chart. I realize 'intuitive' is subjective though.
... Maybe forcing the user to construct the string in the values.yaml/their own Secret, for example. Apologies if this is trodden terrain.
Of course I'd love to contribute back, I'll test it out and give that PR a go.
--
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] EliMor edited a comment on issue #17489: Helm chart converts ' ' to '+' in passwords due to urlJoin helm function
Posted by GitBox <gi...@apache.org>.
EliMor edited a comment on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-894841242
Hi people!
I thought it was strange that it was encoded as '+' as well!!
Here's a more clear example...
If I have in my values.yaml file:
```
metadataConnection:
user: airflow
pass: top secret password
protocol: postgresql
host: fakedb.us-west-2.rds.amazonaws.com
port: 5432
db: postgres
sslmode: disable
```
Here's how we get rendered in the Secret (base64 decoded)
```
postgresql://airflow:top+secret+password@fakedb.us-west-2.rds.amazonaws.com:5432/postgres?sslmode=disable
```
This will result in a connection error unless I manually update the secret to:
```
postgresql://airflow:top secret password@fakedb.us-west-2.rds.amazonaws.com:5432/postgres?sslmode=disable
```
Strange no?!
No big on my end to just not have spaces. :D
--
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 issue #17489: Helm chart converts ' ' to '+' in passwords due to urlJoin helm function
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-894825583
Yeah @mik-laj is right. You can also see examples of that explained in our documentation - https://airflow.apache.org/docs/apache-airflow/stable/howto/connection.html#handling-of-special-characters-in-connection-params according to https://datatracker.ietf.org/doc/html/rfc3986 both pasword and user should be percent-encoded.
However I am a bit surprised to see '+' in your issue rather than %20 (which should be used in this case).
The '+' replacing ' ' is part of another specification https://datatracker.ietf.org/doc/html/rfc1866 - there space might be replaced by '+' only in the context of form fields or form values - strictly when the HTML data is encoded with `application/x-www-form-urlencoded` - which sounds as completely different case and should never be used in URI's (though from what I know a lot of URI parsers will happily accept + as 'space' in other places, but this is rather a side-effect not specification-conformance.
Could you please @EliMor provide us examples of such secrets generated (with some bogus passwords?) was it part of the URI or form? Or some other secret content?
--
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] EliMor commented on issue #17489: Helm chart converts ' ' to '+' in passwords (and username)
Posted by GitBox <gi...@apache.org>.
EliMor commented on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-899346754
So, in your opinion, do you think simply explicitly enforcing ```AIRFLOW__CORE*``` as env variables OR providing an "airflow.cfg" as a Secret in the helm chart should be the ONLY way to do things?
--
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 issue #17489: Helm chart converts ' ' to '+' in passwords due to urlJoin helm function
Posted by GitBox <gi...@apache.org>.
mik-laj commented on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-894810340
Space is a character not allowed in the URI, so it should be percent-encoded. We used to have errors with the handling of special characters in passwords.
https://github.com/apache/airflow/issues/16000
Why do you think this character shouldn't be converted?
--
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] EliMor commented on issue #17489: Helm chart converts ' ' to '+' in passwords due to urlJoin helm function
Posted by GitBox <gi...@apache.org>.
EliMor commented on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-894863824
Apologies for my bad assumption!
That's annoying!
--
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] EliMor edited a comment on issue #17489: Helm chart converts ' ' to '+' in passwords due to urlJoin helm function
Posted by GitBox <gi...@apache.org>.
EliMor edited a comment on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-894841242
Hi people!
I thought it was strange that it was encoded as '+' as well!!
Here's a more clear example...
If I have in my values.yaml file:
```
metadataConnection:
user: airflow
pass: top secret password
protocol: postgresql
host: fakedb.us-west-2.rds.amazonaws.com
port: 5432
db: postgres
sslmode: disable
```
Here's how we get rendered in the Secret (base64 decoded)
```
postgresql://airflow:top+secret+password@fakedb.us-west-2.rds.amazonaws.com:5432/postgres?sslmode=disable
```
This will result in a connection error unless I manually update the secret to:
```
postgresql://airflow:top secret password@fakedb.us-west-2.rds.amazonaws.com:5432/postgres?sslmode=disable
```
Strange no?! Is it encoded properly somewhere/how else??
No big on my end to just not have spaces. :D
--
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 issue #17489: Helm chart converts ' ' to '+' in passwords (and username)
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-899730815
> If the above are true and if the argument is that the use of the helm chart should be 'intuitively' or 'naturally' the way Airflow does things (I'm extrapolating from your statement that kube should bend to Airflow), then it seems that the chart should be structured to look as close to Airflow directly as possible.
You are extrapolating a bit more than what I wrote. Your assumptions do not take into account the audiences. I never said "human" providing the URL. I said "Secret" providing URL to Airflow. There is one more step between Human entering configuration and Secret Keeping it. The reason why you have it in helm chart, is because the Helm chart is for "Humans" to configure, while the "Secrets" is for "airflow" to consume. and for "Kunernetes" to store.
What the helm chart does - It takes "user" input and converts it (on the flight) to "URL" format that is stored in "Secret" if you look under the hood. I cannot imagine "human" by hand providing "Secret". This is established practice in K8S that you prepare Kubernetes Resources (including secrets) via some kind of templating engine (usually Go-templating based) to make it easier for "Human" to provide the right values. This is what Helm Chart is doing and it provides the right filters to convert the values.
Similarly in Airflow we provide a tool: https://airflow.apache.org/docs/apache-airflow/stable/howto/connection.html#generating-a-connection-uri which allows Human to generate the URI to use by providing all the values separately.
Same princilple. pretty consistently applied across the board.
--
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 issue #17489: Helm chart converts ' ' to '+' in passwords due to urlJoin helm function
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-894851965
@jedcunningham @kaxil : from what I can see (and what seems to be confirmed by https://github.com/pallets/jinja/issues/17 - it seems that the "urlquery" filter we use is wrong. It should be more complex. The problem is that urlquery replaces ' ' with '+' (and / with %2F) but 'urlencode` replaces " " with %20 but it does not replace "/" with %2F. I looked it up and seems that however strange it is, it is how it is. Unfortunately both user and password require STRICT percent-encoding.
Seems that for Jinja, this is the only right way we can do it::
```
{{ value|urlencode|replace("/", "%2f") }}
```
--
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] EliMor edited a comment on issue #17489: Helm chart converts ' ' to '+' in passwords (and username)
Posted by GitBox <gi...@apache.org>.
EliMor edited a comment on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-899289298
So...
Some questions I had lingering in my head, no response expected on my end. Please keep in mind I'm very naive/lacking any context and there are extra implicit assumptions here but...
- **Whats the rationale for constructing and encoding the URI to be saved and putting it in the Secret?** This is as opposed to plopping the secret as provided by the user in the values.yaml and then building and encoding the URI inside the containers using some standard encoding & uri construction strategy. Is it because this seemed like wasted work done on every pod reboot? Perhaps an extra benefit as a user is that if I want to make my own Secret, I could use the visual cue in the values.yaml to make the Secret and not have to worry about encoding myself.
- **Do we think that a preconnect check should happen in the migration pod?** I thought it was strange that it passed just fine and I didn't see connection errors until we were to the worker-pods this is why I initially assumed that some extra funky encoding was happening again behind the scenes and why using spaces raw seemed to work. I also thought it was strange that I did not see any of the other pods in a crashloop as well. Please correct me if I'm wrong about this.
Cheers.
--
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] EliMor edited a comment on issue #17489: Helm chart converts ' ' to '+' in passwords due to urlJoin helm function
Posted by GitBox <gi...@apache.org>.
EliMor edited a comment on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-894863824
Apologies for my bad assumption!
That's annoying! Small minefield for me to remember 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.
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 issue #17489: Helm chart converts ' ' to '+' in passwords due to urlJoin helm function
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-894849733
Actually it should work (and is the only correct way!!!) if you replace it with:
```
postgresql://airflow:top%20secret%20password@fakedb.us-west-2.rds.amazonaws.com:5432/postgres?sslmode=disable
```
Neither ' ' nor '+' is correct.
--
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] EliMor commented on issue #17489: Helm chart converts ' ' to '+' in passwords (and username)
Posted by GitBox <gi...@apache.org>.
EliMor commented on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-899623808
Apologies. I'm a strange person and I struggle to communicate clearly at times.
This is what I am asking.
Is it correct that there are currently several ways of providing, in our case, the db connection details;
```
metadataConnection:
user: airflow
pass: top secret password
protocol: postgresql
host: fakedb.us-west-2.rds.amazonaws.com
port: 5432
db: postgres
sslmode: disable
```
And
```
extraEnv: AIRFLOW__CORE__SQL_ALCHEMY_CONN*
```
And
mounting ```airflow.cfg``` from a secret or (configmap?) directly
If the above are true and if the argument is that the use of the helm chart should be 'intuitively' or 'naturally' the way Airflow does things (I'm extrapolating from your statement that kube should bend to Airflow), then it seems that the chart should be structured to look as close to Airflow directly as possible.
Ie. perhaps instead of a 'metadataConnection,' etc., there should just be an area that engages with this behavior:
> The universal order of precedence for all configuration options is as follows:
> set as an environment variable (AIRFLOW__CORE__SQL_ALCHEMY_CONN)
> set as a command environment variable (AIRFLOW__CORE__SQL_ALCHEMY_CONN_CMD)
> set as a secret environment variable (AIRFLOW__CORE__SQL_ALCHEMY_CONN_SECRET)
> set in airflow.cfg
> command in airflow.cfg
> secret key in airflow.cfg
> Airflow’s built in defaults
Please let me know if I'm making more bad assumptions here.
--
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 issue #17489: Helm chart converts ' ' to '+' in passwords (and username)
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-899326972
> ... Maybe forcing the user to construct the string in the values.yaml/their own Secret, for example. Apologies if this is trodden terrain.
This is already possible for SQLalchemy connection if you want. https://airflow.apache.org/docs/apache-airflow/stable/howto/set-config.html#setting-configuration-options (see the _cmd option). You can write and execute your "glue" as a script (in python/bash or any other that is supported by your image)
--
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] EliMor commented on issue #17489: Helm chart converts ' ' to '+' in passwords due to urlJoin helm function
Posted by GitBox <gi...@apache.org>.
EliMor commented on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-894841242
Hi people!
I thought it was strange that it was encoded as '+' as well!!
Here's a more clear example...
If I have in my values.yaml file:
```
metadataConnection:
user: airflow
pass: top secret password
protocol: postgresql
host: fakedb.us-west-2.rds.amazonaws.com
port: 5432
db: postgres
sslmode: disable
```
Here's how we get rendered in the Secret
```
postgresql://airflow:top+secret+password@fakedb.us-west-2.rds.amazonaws.com:5432/postgres?sslmode=disable
```
This will result in a connection error unless I manually update the secret to:
```
postgresql://airflow:top secret password@fakedb.us-west-2.rds.amazonaws.com:5432/postgres?sslmode=disable
```
Strange no?!
No big on my end to just not have spaces. :D
--
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] EliMor commented on issue #17489: Helm chart converts ' ' to '+' in passwords (and username)
Posted by GitBox <gi...@apache.org>.
EliMor commented on issue #17489:
URL: https://github.com/apache/airflow/issues/17489#issuecomment-899790527
Thank you for your time and patience to help me understand a little more.
Cheers.
--
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