[GitHub] [airflow] dstandish commented on issue #6104: [AIRFLOW-4574] allow providing private_key in SSHHook
dstandish commented on issue #6104: [AIRFLOW-4574] allow providing private_key in SSHHook URL: https://github.com/apache/airflow/pull/6104#issuecomment-532965215 Sounds like you don't like the `private_key` hook init param. I don't care about this I was just adding it to be consistent with with existing hook, in which pretty much every attribute was overridable in init. There was already `password`, so an invitation to be a knucklehead already exists ;). If you want to remove this init param I have absolutely no objection. But concerning allowing private key to be provided directly in airflow connection, as opposed to only key file `path`, I assume you do not mean to object to that. There is absolutely no difference between providing a private key here and `keyfile_dict` in GCP, or `password` in _any_ other conn uri. The connection framework is fundamental to airflow and if we object to that, then we object to all hooks. Moreover I think the case is strong that to require distributing keys across all nodes -- in other words adding special extra processing relative to other connection types -- rather _increases_ likelihood of security lapses relative to just being able to lean on the connection framework alone. Perhaps it makes sense to make a new PR with your proposed change and we can discuss there? 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [airflow] dstandish commented on issue #6104: [AIRFLOW-4574] allow providing private_key in SSHHook
dstandish commented on issue #6104: [AIRFLOW-4574] allow providing private_key in SSHHook URL: https://github.com/apache/airflow/pull/6104#issuecomment-532817543 I did provide an example connection string in the documentation file, following the example with existing param `key_file`. I did not warn against including creds in code, though. On that, I am definitely sympathetic to your concern, but I guess my thought is it's a type of general advice that is not specific to this operator, or to airflow for that matter. I think providing connection string examples in docstings is not a terrible idea. In this case it's pretty straightforward to figure out because you see very cleary how it pulls the value from extras, along with the others. For some reason with GCP connections I remember being very confused trying to traceback how to get the keyfile info to the right place. I ended up writing a connection string generator that I use when adding new types of connections to our setup. Maybe including something like that, as a part of the hook, is a possibility. But could be confusing as well. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [airflow] dstandish commented on issue #6104: [AIRFLOW-4574] allow providing private_key in SSHHook
dstandish commented on issue #6104: [AIRFLOW-4574] allow providing private_key in SSHHook URL: https://github.com/apache/airflow/pull/6104#issuecomment-532440383 @pgagnon the same could be said of password, which some hooks have. The point of this in my view is that ssh is a connection like any other. Any other connection, we can supply creds through DB or env var alone; this one for some reason requires we also create a file on the instance, does not allow us to supply directly via connection string. I suppose we could remove the option to supply param at hook instantiation, so it is an extras-only kind of deal, but I don't personally see the value. WDYT? 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [airflow] dstandish commented on issue #6104: [AIRFLOW-4574] allow providing private_key in SSHHook
dstandish commented on issue #6104: [AIRFLOW-4574] allow providing private_key in SSHHook URL: https://github.com/apache/airflow/pull/6104#issuecomment-531580727 @mik-laj thank you -- did not realize there was this doc. I have updated. 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: us...@infra.apache.org With regards, Apache Git Services
