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

Allow k8s namespace with run-as-requestor #745

Merged
merged 2 commits into from May 14, 2019

Conversation

Projects
None yet
2 participants
@DaoWen
Copy link
Contributor

commented May 10, 2019

Changes proposed in this PR

Run run-as-requestor apps in the user's namespace when x-waiter-namespace=*.

Why are we making these changes?

The strict requirement that the namespace matches the run-as-user field (or is omitted) made it impossible to create run-as-requestor apps in the requestor's namespace.

See the original patch in #723.

@DaoWen DaoWen added the wip label May 10, 2019

@DaoWen DaoWen force-pushed the DaoWen:feature/requestor-namespace branch from 4633e64 to f1821c7 May 13, 2019

@DaoWen DaoWen added internal-green and removed wip labels May 13, 2019

@DaoWen DaoWen requested a review from shamsimam May 13, 2019

@@ -42,7 +42,7 @@ Additional (optional) parameters that can be set:
|`X-Waiter-Metadata-*`|n/a|any string|A metadata field. Metadata fields allow you to store arbitrary strings along with your service description.|Allows additional information to be stored with a service to help identify the service.|
|`X-Waiter-Metric-Group`|"other"|[A-Za-z0-9-_]+|Metric groups allow services to be grouped together for the purpose of metric collection.|If you have `:statsd` enabled in your [config file](../config-full.edn), then specify a metric group. If you have multiple services, choose the same metric group if you'd like combined metrics, otherwise choose independent metric groups.|
|`X-Waiter-Min-Instances`|1|1 or 2|The minimum number of instances that Waiter should keep running at all times.|Go with the default.|
|`X-Waiter-Namespace`|empty|user-name|The namespace in which to create back-end scheduler objects. Must match the run-as-user when specified.|Leave this field blank unless you need direct access to resources Waiter creates on your behalf.|
|`X-Waiter-Namespace`|empty|user-name|The namespace in which to create back-end scheduler objects. Must match the run-as-user when specified, or set to `*`.|Leave this field blank unless you need direct access to resources Waiter creates on your behalf.|

This comment has been minimized.

Copy link
@shamsimam

shamsimam May 13, 2019

Contributor
Suggested change
|`X-Waiter-Namespace`|empty|user-name|The namespace in which to create back-end scheduler objects. Must match the run-as-user when specified, or set to `*`.|Leave this field blank unless you need direct access to resources Waiter creates on your behalf.|
|`X-Waiter-Namespace`|empty|user-name|The namespace in which to create back-end scheduler objects. Must match the run-as-user when specified or set to `*`.|Leave this field blank unless you need direct access to resources Waiter creates on your behalf.|

This comment has been minimized.

Copy link
@DaoWen

DaoWen May 13, 2019

Author Contributor

Reworded.

(check-pod-namespace waiter-url current-user current-user))
(check-pod-namespace waiter-url {:x-waiter-namespace current-user} current-user))
(testing "Run-as-requestor namespace"
(check-pod-namespace waiter-url {:x-waiter-namespace "*" :x-waiter-run-as-user "*"} current-user))

This comment has been minimized.

Copy link
@shamsimam

shamsimam May 13, 2019

Contributor

We should add checks for:

  • when x-waiter-namespace is specified as a different value than the user and x-waiter-run-as-user: *
  • when x-waiter-namespace is specified as the same value as the user and x-waiter-run-as-user: *
  • when x-waiter-namespace is not specified and x-waiter-run-as-user: *

This comment has been minimized.

Copy link
@DaoWen

DaoWen May 13, 2019

Author Contributor

x-waiter-namespace is not specified and x-waiter-run-as-user: *

That's already covered by the default-namespace case in the unit tests.

Added integration and unit tests for the other cases.

@DaoWen DaoWen force-pushed the DaoWen:feature/requestor-namespace branch 2 times, most recently from 1f87087 to 77b2771 May 13, 2019

@DaoWen DaoWen force-pushed the DaoWen:feature/requestor-namespace branch from 77b2771 to 34de12a May 13, 2019

@DaoWen

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

@shamsimam - All green!

(check-pod-namespace waiter-url {:x-waiter-namespace "not-current-user"} current-user))))
(testing "Invalid namespace for run-as-requestor"
(is (thrown? Exception #"Cannot use run-as-requestor with a specific namespace"
(check-pod-namespace waiter-url {:x-waiter-namespace "not-current-user" :x-waiter-run-as-user "*"} current-user))))

This comment has been minimized.

Copy link
@shamsimam

shamsimam May 13, 2019

Contributor

Missing the case where the namespace is the same as the current running user.

This comment has been minimized.

Copy link
@DaoWen

DaoWen May 14, 2019

Author Contributor

All 12 cases are covered now.

Show resolved Hide resolved waiter/src/waiter/service_description.clj Outdated
Show resolved Hide resolved waiter/src/waiter/service_description.clj Outdated
Show resolved Hide resolved waiter/src/waiter/service_description.clj Outdated
Show resolved Hide resolved waiter/test/waiter/service_description_test.clj Outdated
(throw (ex-info "Cannot use namespace * with specific run-as-user"
{:namespace raw-namespace :run-as-user raw-run-as-user :status 400})))
_ (when (and (nil? sanitized-run-as-user) (some? raw-namespace) (not= "*" raw-namespace))
(throw (ex-info "Cannot use run-as-requestor with a specific namespace"

This comment has been minimized.

Copy link
@shamsimam

shamsimam May 13, 2019

Contributor

For an on-the-fly request, run-as-user will be missing, but it should be allowed to specify the namespace:requester. This scenario should not flag an error. I think we should be more explicit on our run-as-user checks with '*'

@shamsimam

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

I think the validation should have the following logic:

Assume requesting user is foo for the following table:

Run-As-User Namespace Validation
Missing Missing OK
Missing * OK
Missing foo OK
Missing bar FAIL
foo Missing OK
foo * OK
foo foo OK
foo bar FAIL
* Missing OK
* * OK
* foo FAIL
* bar FAIL

@DaoWen DaoWen force-pushed the DaoWen:feature/requestor-namespace branch from 668cd68 to c7d236d May 14, 2019

@shamsimam shamsimam merged commit 4de273c into twosigma:master May 14, 2019

2 checks passed

Mergeable Mergeable Run has been Completed!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.