Skip to content

Conversation

@hussein-awala
Copy link
Member

For security reasons, using "0.0.0.0" as a host is not a good practice, even for a health check server.

This PR creates a new config to configure the host of the scheduler health check server and uses the existing one as the default value.

related: #35615

@hussein-awala hussein-awala added the type:new-feature Changelog: New Features label Nov 13, 2023
version_added: 2.8.0
type: string
example: ~
default: "0.0.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Should the default continue to be 0.0.0.0 ? For me it looks like we might choose no default and use IP reverse-dnsed from hostname callable: https://airflow.apache.org/docs/apache-airflow/stable/configurations-ref.html#hostname-callable maybe?). That would be quite a bit more secure default and only slightly risky when it comes to backwards compatibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about it, but I think it's a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

A bit. But we do allow breaking changes "because security" ....

Copy link
Member

Choose a reason for hiding this comment

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

(just saying :D) . Not very strong on it

@potiuk
Copy link
Member

potiuk commented Nov 13, 2023

(and nice bandit to tell us so - BTW :)

Copy link
Contributor

@amoghrajesh amoghrajesh left a comment

Choose a reason for hiding this comment

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

@hussein-awala generally the changes look fine, can you have some test coverage here: test_scheduler_command.py?

@hussein-awala
Copy link
Member Author

Static checks failure is not related to this PR; I will try to fix it in a separate PR.

@hussein-awala hussein-awala merged commit 454e63f into apache:main Nov 25, 2023
@eladkal eladkal added this to the Airflow 2.8.0 milestone Nov 25, 2023
ephraimbuddy pushed a commit that referenced this pull request Nov 26, 2023
…server (#35616)

* Add a new config to configure the host of the scheduler healthcheck server

* Add a test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:new-feature Changelog: New Features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants