Skip to content

Warning for using listing conns & vars with secrets backend - #24679

Closed
RNHTTR wants to merge 0 commit into
apache:mainfrom
RNHTTR:main
Closed

RNHTTR wants to merge 0 commit into
apache:mainfrom
RNHTTR:main

Conversation

@RNHTTR

@RNHTTR RNHTTR commented Jun 27, 2022 •

Copy link
Copy Markdown
Contributor

The Airflow CLI and UI don't show Variables or Connections that are stored in a secrets backend. This can be really confusing if you don't already know that. This PR adds a warning to CLI commands and a warning banner to the list connections and list variables views in the UI when a user has enabled a secrets backend. The warning just explains that connections and variables stored in secrets backends won't be displayed using the CLI or UI.

Close: #10867


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragement file, named {pr_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg Bot added area:CLI area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Jun 27, 2022
Comment on lines 65 to 73

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cli airflow connections get <connection> will return connection from secrets backends if it exists because it use BaseHook.get_connection(args.conn_id) if it won't return but it should that mean you didn't configure cli to use alternative secrets backend.

Comment on lines 90 to 86

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At that moment airflow grab credentials/variables from SomeAlternateSecretsBackend, EnvironmentVariablesBackend and MetastoreBackend.

And actually it return list only from MetastoreBackend, and ignore all other backends.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My fault, actually for listing connection and variable Airflow doesn't use MetastoreBackend rather than it make direct query to Metastore

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah -- connections_list and variables_list doesn't capture secrets backend conns/vars:

@suppress_logs_and_warning
def connections_list(args):
    """Lists all connections at the command line"""
    with create_session() as session:
        query = session.query(Connection)
        if args.conn_id:
            query = query.filter(Connection.conn_id == args.conn_id)
        conns = query.all()

        AirflowConsole().print_as(
            data=conns,
            output=args.output,
            mapper=_connection_mapper,
        )

Comment on lines 38 to 43

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as connections_list

Comment on lines 54 to 61

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as connections_get

@mik-laj

mik-laj commented Jun 27, 2022

Copy link
Copy Markdown
Member

#10867 is it related?

@RNHTTR

RNHTTR commented Jun 27, 2022

Copy link
Copy Markdown
Contributor Author

#10867 is it related?

Yes, I think so.

@blag

blag commented Jun 27, 2022

Copy link
Copy Markdown
Contributor

Yep, this solves #10867. Thank you for fixing this!

Comment on lines 69 to 70

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be helpful to include examples of what are considered "alternative" backends. Really, just listing the backends that are distributed with Airflow, but are ignored by this command.

Comment on lines 92 to 93

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is the same warning message as connections_get, it might be useful to store it in a variable and reference it everywhere it's used - in the connections and variables commands.

Maybe put it in a new airflow.cli.commands._warnings module?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea; I'll update

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List those alternative backends distributed with Airflow here too.

And if you want extra credit, maybe count the number of connections/variables not shown, only show the warning if there are include that number in the warning message:

WARNING:
{% trans count=num_variables %}
The Airflow UI will not display the {{ num_variables }} variable stored
in alternative secrets backends (like EnvironmentVariablesBackend
and SecretsBackend).
{% pluralize %}
The Airflow UI will not display the {{ num_variables }} variables stored
in alternative secrets backends (like EnvironmentVariablesBackend
and SecretsBackend).
{% endtrans %}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes that we use the i18n extension of Jinja, and as far as I can tell, we don't yet.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: It seems a little verbose to mention "connections or variables" when the page is only dedicated to showing connections. I might just mention connections here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went back and forth on this. I figured that this way it wouldn't give a user the impression that one would display in the UI but not the other. I don't feel strongly either way.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd lean towards only referring to connections in the connections page and vice versa.

Comment thread airflow/cli/commands/connection_command.py Outdated
@RNHTTR
RNHTTR requested review from Taragolis, blag and uranusjr and removed request for Taragolis and blag August 12, 2022 15:48
@ashb

ashb commented Aug 17, 2022

Copy link
Copy Markdown
Member

@Kinceo1 Are you trying to help out your reviews or are you just trolling?

@potiuk

potiuk commented Aug 17, 2022

Copy link
Copy Markdown
Member

@Kinceo1 Are you trying to help out your reviews or are you just trolling?

BTW. We are going to block your account if you do not respond and show whether you have good intentions @Kinceo1 - so far it does not look like we understand your intentions, and if you won't respond shortly, we will assume they are not good and block you.

@blag blag left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good; just about ready.

The Jinja+HTML templates could use more consistent indentation (see my inline suggestions).

It's a nitpick, but I'm not a huge fan of the cli_or_ui and connection_or_variable parameter names to warn_list_secrets_alterative_backend Perhaps context and backend_type, respectively, might be better? The "blast radius" of this is rather small, and the current X_or_Y names do a good job of indicating the mutually exclusive nature of the parameter values though, so if you're happy with those names we can go with them.

Comment thread airflow/www/templates/airflow/connection_list.html Outdated
Comment thread airflow/www/templates/airflow/variable_list.html Outdated
Comment thread airflow/www/templates/airflow/variable_list.html Outdated
Comment thread airflow/www/templates/airflow/variable_list.html Outdated
@github-actions

github-actions Bot commented Oct 4, 2022

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions Bot added stale Stale PRs per the .github/workflows/stale.yml policy file and removed stale Stale PRs per the .github/workflows/stale.yml policy file labels Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:CLI area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Warning about invisible connections/variable in the Web UI

8 participants