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

support multiple namespaces #1955

Merged
merged 2 commits into from
Apr 11, 2024
Merged

Conversation

AndrewChubatiuk
Copy link
Contributor

@AndrewChubatiuk AndrewChubatiuk commented Apr 7, 2024

replaced sparkJobNamespace with sparkJobNamespaces, added creation of spark role, serviceaccount and rolebinding for every spark job namespace

Copy link
Contributor

@yuchaoran2011 yuchaoran2011 left a comment

Choose a reason for hiding this comment

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

I'm not sure if changing Roles and RoleBindings to ClusterRoles and ClusterRoleBindings respectively is a good idea. RBAC permissions should be as tight as possible for security reasons. It's better to create a role and role binding for each application namespace

@AndrewChubatiuk AndrewChubatiuk force-pushed the multi-ns branch 4 times, most recently from 4955e24 to d8b454e Compare April 8, 2024 04:26
@AndrewChubatiuk
Copy link
Contributor Author

I'm not sure if changing Roles and RoleBindings to ClusterRoles and ClusterRoleBindings respectively is a good idea. RBAC permissions should be as tight as possible for security reasons. It's better to create a role and role binding for each application namespace

Changed to Role and RoleBinding loop

Copy link
Contributor

@yuchaoran2011 yuchaoran2011 left a comment

Choose a reason for hiding this comment

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

Could you add more details to how this PR was tested? Specifically, it will be good to verify that webhook still works as expected. Some web hook resources are namespace specific. I don't remember which ones off the top of my head, but the webhook service and MutatingWebhookConfiguration objects are good to validate

@AndrewChubatiuk AndrewChubatiuk force-pushed the multi-ns branch 2 times, most recently from 06d2428 to ba9a08d Compare April 8, 2024 07:28
Signed-off-by: Andrew Chubatiuk <andrew.chubatiuk@gmail.com>
Signed-off-by: Andrew Chubatiuk <andrew.chubatiuk@gmail.com>
@AndrewChubatiuk
Copy link
Contributor Author

@yuchaoran2011 I've beed running it in my project for several month in such setup. I see mutatingwebhooks for pods only and it monitors namespaces depending on operator's -namespace

@AndrewChubatiuk
Copy link
Contributor Author

it makes sense to migrate operator to kubebuilder v4, it simplifies setup and management a lot and also reduces amount of code lines

@AndrewChubatiuk
Copy link
Contributor Author

@yuchaoran2011 do you expect any other changes here?

Copy link
Contributor

@yuchaoran2011 yuchaoran2011 left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AndrewChubatiuk, yuchaoran2011

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit 835281d into kubeflow:master Apr 11, 2024
8 checks passed
peter-mcclonski pushed a commit to TechnologyBrewery/spark-on-k8s-operator that referenced this pull request Apr 16, 2024
* support multiple namespaces

Signed-off-by: Andrew Chubatiuk <andrew.chubatiuk@gmail.com>

* bump helm chart version

Signed-off-by: Andrew Chubatiuk <andrew.chubatiuk@gmail.com>

---------

Signed-off-by: Andrew Chubatiuk <andrew.chubatiuk@gmail.com>
Signed-off-by: Peter McClonski <mcclonski_peter@bah.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants