Skip to content
This repository has been archived by the owner on Mar 22, 2023. It is now read-only.

Decouple k8s namespace from run-as-user #718

Merged
merged 2 commits into from
Apr 29, 2019
Merged

Decouple k8s namespace from run-as-user #718

merged 2 commits into from
Apr 29, 2019

Conversation

DaoWen
Copy link
Contributor

@DaoWen DaoWen commented Apr 29, 2019

Changes proposed in this PR

Decouple k8s objects' namespace from the Waiter run-as-user option.

Why are we making these changes?

We would like to make the namespace independently configurable from the run-as-user (e.g., via a new run-in-namespace Waiter service parameter). Separating the namespace logic from the run-as-user logic is the first step for supporting independent configuration. This also seems like an overall architectural improvement, since the concerns of run-as-user and k8s namespace are semantically distinct.

@DaoWen DaoWen added the wip label Apr 29, 2019
@DaoWen DaoWen requested a review from shamsimam April 29, 2019 15:25
@DaoWen DaoWen removed the wip label Apr 29, 2019
waiter/src/waiter/scheduler/kubernetes.clj Outdated Show resolved Hide resolved
waiter/src/waiter/scheduler/kubernetes.clj Outdated Show resolved Hide resolved
waiter/src/waiter/scheduler/kubernetes.clj Outdated Show resolved Hide resolved
:spec {:replicas min-instances
:selector {:matchLabels {:app k8s-name
:waiter-cluster cluster-name}}
:waiter-cluster cluster-name
:waiter/user run-as-user}}
Copy link
Contributor

Choose a reason for hiding this comment

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

ANy reason we chose waiter-cluster over waiter/cluster earlier? Does that same restriction still apply for waiter/user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I noticed that too. We should switch to waiter/cluster, but I was planning to open a separate PR for that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created issue #721 for this.

@shamsimam shamsimam merged commit 4766b44 into twosigma:master Apr 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants