[YUNIKORN-1851] Don't allow similar app ids by different users

[manirajv06]

What is this PR for?

Detect same app ids being used by different users for app submission and don't allow when app is already running. By default, this check has been disabled (configurable).

What type of PR is it?

• - Bug Fix

Todos

• - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-1851

How should this be tested?

Screenshots (if appropriate)

Questions:

• - The licenses files need update.
• - There is breaking changes for older versions.
• - It needs documentation.

[codecov]

Codecov Report

Merging #628 (92fd1e7) into master (64eadca) will decrease coverage by 0.47%.
The diff coverage is 77.77%.

❗ Current head 92fd1e7 differs from pull request most recent head 466d1ef. Consider uploading reports for the commit 466d1ef to get more accurate results

@@            Coverage Diff             @@
##           master     #628      +/-   ##
==========================================
- Coverage   71.90%   71.43%   -0.47%     
==========================================
  Files          51       51              
  Lines        8076     8133      +57     
==========================================
+ Hits         5807     5810       +3     
- Misses       2073     2126      +53     
- Partials      196      197       +1     

Files Changed	Coverage Δ
pkg/conf/schedulerconf.go	75.68% <50.00%> (-0.73%)	⬇️
pkg/appmgmt/general/podevent_handler.go	89.81% <100.00%> (+0.92%)	⬆️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[craigcondit]

I think this might be a case of the cure being worse than the disease. Shared environments like this are somewhat cooperative by nature, and reusing app ids can happen either accidentally or even automatically (especially for the autogenerated app id). Rejecting a pod in this case is very heavy-handed and will result in unexpected failures. Tracking users by app instead of pod leads to this (IMO small) issue, but I don't think we should disallow behavior that Kubernetes allows -- we should strive to be more compatible, not less.

I don't see this issue being much worse than the ambiguity we have in the case of group quotas and multiple possible matches. It's not perfect, but probably good enough.

I'm -1 on this approach due to the pain it will likely cause.

[wilfred-s]

Also need to hook this into the one application ID per namespace setting. If you have one app ID per namespace this will probably break things.

[wilfred-s]

reusing app ids can happen either accidentally or even automatically (especially for the autogenerated app id)

This does not affect re-using application IDs. You can still do that without a problem. If the application is not currently known in the shim the user info will be set without question. For the auto generated app ID: I do think that is a case we need to handle and not reject.

Tracking users by app instead of pod leads to this (IMO small) issue, but I don't think we should disallow behavior that Kubernetes allows -- we should strive to be more compatible, not less.

K8s does not have the concept of applications and or users on a workload so we have no compatibility look at. Looking at a simple case: I could set a Spark application ID on a pod as a completely different user, in a different namespace with a different service account etc. That pod will then be run as part of the already running Spark job in a queue which I might not have access to under a different quota. That is a huge gap which will show up as a problem. I am surprised that it has not as yet.

I don't see this issue being much worse than the ambiguity we have in the case of group quotas and multiple possible matches. It's not perfect, but probably good enough.

The group resolution is defined and far less of a problem than this is.
The reason for adding this is that we have seen abuse similar to this on YARN years ago. We have to put mitigations in place. Even if they are off by default but we need to have them.

[pbacsko]

Found some smaller things.

[pbacsko]

+1 LGTM
I'll let Wilred take a look at it too.

[craigcondit]

We need to be careful about this one. https://issues.apache.org/jira/browse/YUNIKORN-1961 was filed a few days ago that indicates that Spark uses different auth info when submitting executor pods, so somehow we need to take that into acccount here.

[craigcondit]

Closing this for now. We can't make this change, as it will break a lot of things (including Spark) as executors are launched using different security credentials (the spark service account) than the original driver.