cmd: add --managed-namespaces flag to vc-webhook-manager

[jsolbrig]

As described in #2286 this adds the --managed-namespaces flag to the vc-webhook-manager binary. The flag is mutually exclusive with the --ingored-namespaces flag.

As a note, I personally think that --ignored-namespaces is a dangerous idea. It essentially leans towards "most privileged" rather than "least privileged".

Important notes

Syntax question

I'm not really a Go programmer (mostly Python) but am learning. I struggled with one thing. On line 61 of cmd/webhook-manager/app/util.go I used var selectorOperator metav1.LabelSelectorOperator. I'm not sure if this is the preferred syntax for this project but struggled to get any other syntax to work here.

Removal of default ingored namespaces

In adding this flag, it was significantly simpler to remove the defaultIgnoredNamespaces. I also think that setting the list of ignored/managed namespaces should be explicit.

This became an issue for me while working on putting together a Helm chart for Volcano and found that having a default set of ignored namespaces made developing a functional Helm chart difficult. For example, Helm has the convention of specifying which namespace a chart will be deployed do using helm install --namespace. With defaultIgnoredNamespaces set, it was non-obvious why the chart was managing the wrong namespace.

RBAC

While #2286 mentions the issue of over-broad RBAC, in order to keep this PR small, I haven't done anything to address the RBAC issues. I will, however, attempt to address those issues in the helm chart mentioned above. I can open a new issue for the RBAC issues if needed.

Fixes #2286

[volcano-sh-bot]

Welcome @jsolbrig!

It looks like this is your first PR to volcano-sh/volcano 馃帀.

Thank you, and welcome to Volcano. :smiley:

[volcano-sh-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: To complete the pull request process, please assign shinytang6 You can assign the PR to them by writing /assign @shinytang6 in a comment when ready.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[jsolbrig]

/assign @shinytang6

[jsolbrig]

I'm confused about the issue that the bot is reporting with my commit message. I followed the instructions here. Should I remove the part that says Fixes: #2286? Do the instructions or the bot need to be updated?

[hwdef]

git commit --amend --signoff --no-edit
git push --force-with-lease origin managed-namespaces

[jsolbrig]

Thanks @hwdef I signed the commits.

[hwdef]

please rebase your code. Merge 2 commits into 1.

[jsolbrig]

Thanks @hwdef, I've squashed the two commits into one and removed the "Fixes: #2286" portion to get the bot to quit complaining.

[jsolbrig]

Yes, you're right. I didn't do this completely. I guess that I got a little excited.

I'm looking more closely at what I did and what needs to be done. I think I should probably either retract this PR or set it to "draft" and do some more work before resubmitting. Do you have a preference?

Before I keep working on this, if done correctly, is this functionality that you're interested in? Personally, without it, I will probably need to find a different batch processing solution since Volcano currently interferes with deploying other applications by blocking the deployment of Pods.

[jsolbrig]

@hwdef Sorry for wasting your time with this PR. I took on something too complicated for my first issue in this project and the work that I did is largely junk since it doesn't begin to get into the actual webhook code.

I do think that I see a solution that can easily achieve my basic goal for now. Is there a way that we can discuss how this can work?

The 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. Maybe simply adding an ObjectSelector to the Pod webhooks would be enough to avoid blocking pods from other applications. Something like this:

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

[hwdef]

@jsolbrig I'm not sure if it's feasible to use objectSelector. It's feasible to filter vcjobs through it, but there may be no way to filter deployments, or pods created through deployments. You're doing a great job now, we just need to refine the scenario. Is it possible to combine namespaceSelector and matchPolicy to filter by namespace and ensure that vcjobs must go through webhook?

[jsolbrig]

@hwdef

Sorry for the delay on this. I'll get back to it sometime soon. I've needed to move on for now but it is still a priority for me to get this working correctly.

[hwdef]

I think after https://github.com/volcano-sh/volcano/pull/2346 merged. We do not need this PR.

[jsolbrig]

@hwdef I think you're right but I'll give it a good look sometime in the next week or so.

Sorry it's taking me so long to get back to this. I've had other things climb up my priority list...

[stale[bot]]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.