Volcano's webhooks should be more respectful of other namespaces and RBAC permissions should be reduced

[jsolbrig]

What would you like to be added:

I would like to see Volcano default to managing an explicit list of namespaces rather than managing all namespaces by default. To me, this behavior is both unexpected and problematic. It blocks deployment of other applications. It also leads to the need for less secure RBAC configuration where Volcano can manage all Pods in the cluster.

Currently Volcano's webhooks are able to ignore specific namespaces by using the --ignored-namespaces flag when calling vc-webhook-manager. That is helpful, but I think that Volcano should be more restrictive in terms of what it manages. Rather than needing to list which namespaces to ignore, it should be possible to list the namespaces that should be managed.

I propose that there should be a --managed-namespaces flag. It should operate the same as the --ignored-namespaces flag, but instead of using the NotIn operator in the namespaceSelector it should use In. --managed-namespaces should be mutually exclusive with --ignored-namespaces.

Questions

Why does Volcano manage all namespaces by default?

• I'm sure there is a good reason for this, but is it something that can be addressed?

Would there be side effects of this in the Volcano system?

• If so, can they be addressed?
• For example, would this allow creating VCJobs outside of managed namespaces (probably not good)?
• Could this be protected against using additional webhooks to block creation of VCJobs in unmanaged namespaces?

Why is this needed:

Generally, I think that it is a bad idea to take over the entire kubernetes cluster by default. People use clusters for many purposes and won't expect Volcano to block deployment of other applications. Currently, once Volcano is deployed, it must be patched before a new application is deployed in a new namespace.

This would also allow more restrictive RBAC for Volcano. It would allow reducing the scope of the ClusterRoles to only manage Pods in the managed namespaces rather than requiring cluster-wide access to all Pods. This would be significantly more secure.

[hwdef]

--managed-namespaces sound reasonable. Would you like to add this feature?

[jsolbrig]

Sure, I'll give it a shot.

[Sharathmk99]

@jsolbrig is it better to read from namespace label to either ignore the namespace or not. Like how istio does, they use label to ignore the namespace

[jsolbrig]

@Sharathmk99 That could work well. I was just grabbing on to how the current functionality works with --ignore-namespaces. Personally, I'd prefer an opt-in system rather than opt-out. Maybe add a volcano.sh/monitored: "true" to namespaces where Volcano's webhooks should take effect?

As @hwdef pointed out in a PR that I put together, I didn't see the full scope of the problem when submitting this issue or the PR. Parts of the PR might still be useful but it's mostly junk.

When ignoring/monitoring specific namespaces, VCJobs can still get submitted to unmonitored namespaces. They do run, but they don't get handled by the webhooks. One option to handle this might be to block submission of VCJobs to unmonitored namespaces.

Thoughts?

[jsolbrig]

@Sharathmk99 Also, as mentioned in the PR after realizing that the PR is a mess, I do think that I have a solution that can easily achieve my basic goal for now.

The most basic goal is to just get Volcano to quit blocking admission for resources from other applications. VCJobs add specific labels to the Pods that they create. I've updated cmd/webhook-manager/app/util.go to generate webhooks that contain an objectSelector like below. This seems to work to allow non-Volcano Pods to be created.

- apiVersion: admissionregistration.k8s.io/v1
  kind: MutatingWebhookConfiguration
  metadata:
    name: volcano-admission-pods-mutate
  webhooks:
  - admissionReviewVersions:
    objectSelector:
      matchExpressions:
      - {key: volcano.sh/job-name, operator: Exists}

Should I submit an issue and a PR for the above change?

This doesn't block VCJobs from being created in ignored or unmanaged namespaces, though. VCJobs can still get created in the volcano-system namespace even if it is "ignored".

[Sharathmk99]

@Sharathmk99 Also, as mentioned in the PR after realizing that the PR is a mess, I do think that I have a solution that can easily achieve my basic goal for now.

The most basic goal is to just get Volcano to quit blocking admission for resources from other applications. VCJobs add specific labels to the Pods that they create. I've updated cmd/webhook-manager/app/util.go to generate webhooks that contain an objectSelector like below. This seems to work to allow non-Volcano Pods to be created.

- apiVersion: admissionregistration.k8s.io/v1
  kind: MutatingWebhookConfiguration
  metadata:
    name: volcano-admission-pods-mutate
  webhooks:
  - admissionReviewVersions:
    objectSelector:
      matchExpressions:
      - {key: volcano.sh/job-name, operator: Exists}

Should I submit an issue and a PR for the above change?

This doesn't block VCJobs from being created in ignored or unmanaged namespaces, though. VCJobs can still get created in the volcano-system namespace even if it is "ignored".

If we don’t label vcjob, things will not work. It’s not backward compatible.

[jsolbrig]

@Sharathmk99 Was the label added recently? Pods from VCJobs currently have a volcano.sh/job-name label applied so it was easy to just use that to determine if a Pod belongs to a VCJob.

If this is an issue for backwards compatibility, maybe it could be an option. Maybe a flag like --selective could enable this behavior.

[stale[bot]]

Hello 👋 Looks like there was no activity on this issue for last 90 days. Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗 If there will be no activity for 60 days, this issue will be closed (we can always reopen an issue if we need!).

[stale[bot]]

Closing for now as there was no activity for last 60 days after marked as stale, let us know if you need this to be reopened! 🤗