Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chart: Fix cluster-wide RBAC naming clash when using multiple multiNamespace releases with the same name #37197

Merged
merged 6 commits into from Mar 21, 2024

Conversation

mewa
Copy link
Contributor

@mewa mewa commented Feb 6, 2024

Currently, when deploying multiple releases that share the same release name in different namespaces deployment is failing due to a naming clash of the cluster-scoped RBAC resources.

This PR introduces changes that make sure that ClusterRoles and ClusterRoleBindings are uniquely named according to both the release name, as well as the release namespace, thus solving the naming clash.


Steps to reproduce current behavior:

  1. Create namespace airflow-a
  2. Install helm chart release named airflow into namespace airflow-a with multiNamespaceMode: true
  3. Create namespace airflow-b
  4. Install helm chart release named airflow into namespace airflow-b with multiNamespaceMode: true
  5. Installing the helm chart will fail due to a naming clash of the cluster-wide RBAC resources

Related: #31613 (closed, resubmitting with updated tests)


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@eladkal
Copy link
Contributor

eladkal commented Feb 6, 2024

Multinamespace is not yet fully supported. It should be added with #35639

@mewa
Copy link
Contributor Author

mewa commented Feb 6, 2024

Multinamespace is not yet fully supported. It should be added with #35639

I took a quick look at it and they seem to support different scenarios. I believe the above PR means to introduce fine-grained RBAC controls when using multiNamespaceMode, to have better control over security when deploying a single Airflow instance to run workloads across multiple namespaces.

Whereas, this PR fixes the behaviour with the existing multiNamespaceMode where it's only possible to have a single such deployment per cluster. The scenario here is to have multiple Airflow instances (in different namespaces), each of which runs workloads across multiple namespaces.

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

Looks good

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.

Can we add a default value in valyes.yaml for multiNamespaceMode? @mewa

@mewa
Copy link
Contributor Author

mewa commented Feb 12, 2024

@amoghrajesh It already defaults to false

airflow/chart/values.yaml

Lines 2389 to 2391 in 4c37328

# Whether Airflow can launch workers and/or pods in multiple namespaces
# If true, it creates ClusterRole/ClusterRolebinding (with access to entire cluster)
multiNamespaceMode: false

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.

Thanks for clarifying.
Looks good!

@eladkal
Copy link
Contributor

eladkal commented Feb 15, 2024

Static checks are failing

@mewa
Copy link
Contributor Author

mewa commented Feb 20, 2024

I updated the newsfragment to satisfy the checks. I also moved it to significant type since it involves renaming of the resources.

@eladkal eladkal added this to the Airflow Helm Chart 1.14.0 milestone Mar 9, 2024
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

LGTM. But would love also @jedcunningham 's pair of eyes on it.

@potiuk
Copy link
Member

potiuk commented Mar 14, 2024

Ah... We already had a lot of eyes I see..

@jedcunningham jedcunningham merged commit a0c1985 into apache:main Mar 21, 2024
59 checks passed
Copy link

boring-cyborg bot commented Mar 21, 2024

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

@jedcunningham
Copy link
Member

Thanks @mewa! Congrats on your first commit 🍺

@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Mar 23, 2024
utkarsharma2 pushed a commit to astronomer/airflow that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:helm-chart Airflow Helm Chart changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants