[GitHub] [airflow] ashb commented on pull request #8974: Add VaultHook to hashicorp provider

[GitBox <gi...@apache.org>]

ashb commented on pull request #8974:
URL: https://github.com/apache/airflow/pull/8974#issuecomment-644739992


   > I see a few options of using Backend <> Hook in this scenario:
   > 
   >     1. we build an artificial connection and pass it to hook (very bad idea IMHO)
   
   Agreed, that sounds bad.
   
   > 
   >     2. Get Secret to use Hook directly. For that we will have to change the Hook to accept the basic parameters (host/port etc.) not only from connection but also if there is a lack of connection it should have additional parameters for host, port, user, passwords (or maybe just url)  from the parameters passed. I believe this is what you propose @ashb ? Correct?
   
   I was picturing just a URL (given that is how we configure the secrets backend now. Well URL + kwargs)
   
   > 
   >     3. Implement intermediary used by both Hook and Secret - (Client in my case) which is library used by both - Hook and Backend. Client has no relation to Connection - it's Hook's job to provide all the parameters to the Client parsed from the connection.
   
   I think my reservation to 3 was that this is a new concept to Airflow and it hadn't been mentioned before, nor was the reasoning for having this new class made clear. I should have been better about asking for that reason than jumping straight to my -1 comment.
   
   Equally, there was nothing in the PR or (doc) comments explaining why which means I had to guess or come to my own (wrong) conclusions.
   
   > For 2), I think exposing password/URL/login via Hook constructor is a bad idea. The hook should encourage connection as the only source of the connectivity information - because it is encrypted and shared with other workers by design. 
   
   Yes I agree. (I don't even like the pattern where AWS hooks accept an optional "region_name" parameter to some functions. Separate thing, but related point.)
   
   > Is the problem with documentation duplication? I think the separation of concerns is fairly solid here.
   
   On reflection I hadn't thought through my suggestion fully, but my objection was based around not setting precedent -- that is to not add a new "concept" inside a provider. But your points make sense, and Hook should only operate from connection info, not accept H+U+P directly.
   
   How about calling it airflow.providers.hashicorp._internal.client.VaultClient? That would address my concerns about a new concept. And I think this concern only comes in to play with (some) Secrets backends, but not for most other providers, which would just have a hook and not need this extra client class. Could we perhaps also add this to a comment in where ever this class ends up living.
    
   I also think this "client" class should be excluded from the generated API docs -- that makes it not part of the public API.
   
   I'm really sorry for making you do the extra work of moving the class (not to mention the time you had to spend writing the comment.)


----------------------------------------------------------------
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