I do not habe a strong opinion on the topic but I am mostly with Jarek. The API must be multi tenant and mask all internal DB access - where we are close-by with AIP-44 and the other efforts of multi tenancy (missing a list of all planned efforts, would be cool to have a mata AIP keeping the completed and planned individual efforts)
But in contrast I see the CLI only partly being a tool on tenant but most parts on the internal perimeter admin side. Might be a bit „gray zone“. If we habe overlap to REST API (do we have a list of rhings like trigger sag or so?) we should remove and consolidate redundancy to API logic. But there is for sure some „black magic“ which must not be migrated and needs native DB access. E.g. resetting DB or triggering DB migration. For REST API as well as internal API you need a web endpoint incl. minimal authentication. Some base admin tooling is needed for bootstrap and base admin. Like creating the first admin user is impossible if no user. So before a lengthy discussion I‘d propose to document details and make an AIP to discuss, that might be easier then a reply-to-all thred to define the scope of cleanup. Also and especially for API cöient we need to check for impact if breaking changes appear. Jens Sent from Outlook for iOS<https://aka.ms/o0ukef> ________________________________ From: Andrey Anshin <andrey.ans...@taragol.is> Sent: Monday, February 19, 2024 3:54 PM To: dev@airflow.apache.org <dev@airflow.apache.org> Subject: Re: [DISCUSS] Deprecate cli options in Airflow Configurations and airflow.api.client >Actually - it's not at all part of AIP-44 :). Airflow CLI was (consciously and deliberately) not part of it. Then better to check whether or not it accidentally uses it, add to my checklist > So I would be very much for removing that whole part - even in Airflow 2.9. +1 for removal if it won't break our promises On Mon, 19 Feb 2024 at 16:01, Jarek Potiuk <ja...@potiuk.com> wrote: > Actually - it's not at all part of AIP-44 :). Airflow CLI was (consciously > and deliberately) not part of it. > > CLI is specifically treated as a "local" tool that is executed inside the > security perimeter where direct database access is needed (i.e. it falls in > the same category as 'scheduler', 'webserver`, `internal-api` component - > all of which stil has direct DB access and AIP-44 does not change that. > AIP-44 is specifically about Worker, Triggerrer and Dag File Processor > direct DB access only. > > I think if we want to allow CLI to be used without direct DB access, a > better solution would be to make it use the public REST API of ours, not > the internal api that is supposed to be used by worker/triggerer/dag file > processor. There will be different authentication and authorisation methods > than the ones that airflow CLI would - potentially use. I think if we want > to make airflow CLI use authorised/remote access - that needs a separate > AIP - where we define actors that should be able to use the CLI, > authorisation mechanisms etc. (could be the same as in the REST API, but it > needs to be clarified - currently the authorisation of CLI is based > exclusively on having access to the system/UNIX user that airflow is run > with and where DB configuration to access DB is present). > > But coming back to the main subject - yes, I think that old API is > experimental, and for a long time we have good replacement for it (i.e. the > official REST API that has been battle-tested and is pretty feature-full), > so It is a good idea to remove the old API (and CLI configuration option). > It's not likely widely used, and it's also had experimental status for a > very long time (since the beginning of Airflow 2.0) - so similarly as with > the MSSQL support, we are free to decide to remove it and still keep our > SemVer promises - it never made it on the Public Interface of Airflow > > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fairflow.apache.org%2Fdocs%2Fapache-airflow%2Fstable%2Fpublic-airflow-interface.html&data=05%7C02%7CJens.Scheffler%40de.bosch.com%7C7183c980c1634387b9c808dc315ab031%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C0%7C638439512771163568%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=hJSW%2FH7qQHQIH0YFHjxAoHWBNSejl2yUBpMx1Q9PQAo%3D&reserved=0<https://airflow.apache.org/docs/apache-airflow/stable/public-airflow-interface.html> > - and since the very beginning we did not have an intention for it to be > public API. > > So I would be very much for removing that whole part - even in Airflow 2.9. > > J. > > On Mon, Feb 19, 2024 at 11:53 AM Andrey Anshin <andrey.ans...@taragol.is> > wrote: > > > > the underlying question is what are we going to do with direct DB > access > > from the cli > > > > Good point! > > I guess it should be covered by AIP-44, so if there are no methods which > > implement these actions by AIP-44 it is better to create it instead of > just > > moving old code. > > > > > > > > On Mon, 19 Feb 2024 at 14:35, Bolke de Bruin <bdbr...@gmail.com> wrote: > > > > > with regards to the api client: the intention was indeed to remove > direct > > > access to the DB, which I think is still a valid thing to do. In my > > opinion > > > the underlying question is what are we going to do with direct DB > access > > > from the cli. Is that something we want to keep or do we want a > different > > > design? Just dropping the implementation seems a bit 'lazy' to me, > while > > I > > > understand that it hasn't been touched in a while. > > > > > > B. > > > > > > On Sun, 18 Feb 2024 at 12:17, Andrey Anshin <andrey.ans...@taragol.is> > > > wrote: > > > > > > > Greetings guys! > > > > > > > > I want to start a discussion about deprecation cli configuration > > options > > > > > > > > > > > > > > https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fairflow.apache.org%2Fdocs%2Fapache-airflow%2Fstable%2Fconfigurations-ref.html%23cli&data=05%7C02%7CJens.Scheffler%40de.bosch.com%7C7183c980c1634387b9c808dc315ab031%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C0%7C638439512771174117%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=dQ3MPz8rgbWo%2FIezHpvAuJ4fxDAe%2FT9hx3WRloR3ZRQ%3D&reserved=0<https://airflow.apache.org/docs/apache-airflow/stable/configurations-ref.html#cli> > > > > as well as `airflow/api/client` in upcoming minor/feature release of > > > > Airflow (2.9 or 2.10 depends on). > > > > > > > > This options control how end users retrieve information from the > > > Database, > > > > there is two kind provided classes > > > `airflow.api.client.api_client.Client` - > > > > direct access to DB, `airflow.api.client.json_client.Client` - access > > > > through Experimental API > > > > > > > > However internal clients cover only this cli commands: > > > > - trigger_dag: airflow dags trigger > > > > - delete_dag: airflow dags delete > > > > - get_pool: airflow pool get > > > > - get_pools: airflow pool list and airflow pool export > > > > - create_pool: airflow pool set and airflow pool import > > > > - pool_delete: airflow pool delete > > > > - get_lineage: doesn't use anywhere in CLI and Airflow codebase > > > > > > > > As a result, implementation of CLI spread across different modules / > > > > subpackages and in some cases could use experimental API. Seems like > it > > > is > > > > an abandoned implementation which comes from pre Airflow 2 and we > have > > to > > > > support it nowadays. > > > > > > > > *My proposal*: > > > > 1. Copy implementation from `airflow.api.client.local_client.Client` > > into > > > > the the code from appropriate modules into the airflow/cli/commands > > > > 2. Set default value for `[cli] api_client` to None. In case if > values > > > set > > > > in airflow.cfg (e.g. from the previous version of Airflow), raise > > future > > > > warning and update it to None if it set to > > > > `airflow.api.client.local_client.Client` in other cases do not touch > > > > 3. Set default value for `[cli] endpoint_url` to None. In case if > > values > > > > set in airflow.cfg raise future warning, and do not update value. > > > > 4. `airflow.api.client.get_current_api_client` should able to return > > > None, > > > > if it return None, then it should use implementation from the > > > > airflow/cli/commands, otherwise use deprecated client, with raising > > > > RemovedInAirflow3Warning > > > > > > > > WDYT? > > > > > > > > > > > > > -- > > > > > > -- > > > Bolke de Bruin > > > bdbr...@gmail.com > > > > > >
