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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d3ea349
Decouple k8s namespace from run-as-user
DaoWen Apr 29, 2019
788b53d
feedback from shams
DaoWen Apr 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 34 additions & 25 deletions waiter/src/waiter/scheduler/kubernetes.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,9 @@
:pod-name pod-name
:restart-number restart-number}))

(defn- log-dir-path [namespace restart-count]
(defn- log-dir-path [user restart-count]
"Build log directory path string for a containter run."
(str "/home/" namespace "/r" restart-count))
(str "/home/" user "/r" restart-count))

(defn- killed-by-k8s?
"Determine whether a pod was killed (restarted) by its corresponding Kubernetes liveness checks."
Expand Down Expand Up @@ -185,7 +185,7 @@
:flags failure-flags
:healthy? false
:id newest-failure-id
:log-directory (log-dir-path (:k8s/namespace live-instance)
:log-directory (log-dir-path (:k8s/user live-instance)
(dec restart-count))
:started-at newest-failure-start-time)
;; To match the behavior of the marathon scheduler,
Expand All @@ -201,18 +201,21 @@
(try
(let [port0 (get-in pod [:spec :containers 0 :ports 0 :containerPort])
restart-count (get-in pod [:status :containerStatuses 0 :restartCount])
namespace (k8s-object->namespace pod)]
run-as-user (or (get-in pod [:metadata :labels :waiter/user])
;; falling back to namespace for legacy pods missing the waiter/user label
(k8s-object->namespace pod))]
(scheduler/make-ServiceInstance
{:extra-ports (->> (get-in pod [:metadata :annotations :waiter/port-count])
Integer/parseInt range next (mapv #(+ port0 %)))
:healthy? (get-in pod [:status :containerStatuses 0 :ready] false)
:host (get-in pod [:status :podIP])
:id (pod->instance-id pod)
:k8s/app-name (get-in pod [:metadata :labels :app])
:k8s/namespace namespace
:k8s/namespace (k8s-object->namespace pod)
:k8s/pod-name (k8s-object->id pod)
:k8s/restart-count restart-count
:log-directory (log-dir-path namespace restart-count)
:k8s/user run-as-user
:log-directory (log-dir-path run-as-user restart-count)
:port port0
:service-id (k8s-object->service-id pod)
:started-at (-> pod
Expand Down Expand Up @@ -280,9 +283,13 @@
(log/error "request to K8s API server failed: " url options body response)
(ss/throw+ response))))

(defn- service-description->namespace
[{:strs [run-as-user]}]
run-as-user)
(defn- retrieve-run-as-user
"Get the correspoinding run-as-user for a service-id,
looked up via the corresponding service-description."
[{:keys [service-id->service-description-fn]} service-id]
(-> service-id
service-id->service-description-fn
(get "run-as-user")))

(defn- get-services
"Get all Waiter Services (reified as ReplicaSets) running in this Kubernetes cluster."
Expand Down Expand Up @@ -446,11 +453,12 @@
{:cmd-type cmd-type
:service-description service-description
:service-id service-id}))))
(let [spec-json (replicaset-spec-builder-fn scheduler service-id service-description)
(let [rs-spec (replicaset-spec-builder-fn scheduler service-id service-description)
request-namespace (k8s-object->namespace rs-spec)
request-url (str api-server-url "/apis/" replicaset-api-version "/namespaces/"
(service-description->namespace service-description) "/replicasets")
request-namespace "/replicasets")
response-json (api-request request-url scheduler
:body (utils/clj->json spec-json)
:body (utils/clj->json rs-spec)
:request-method :post)
service (some-> response-json replicaset->Service)]
(when-not service
Expand Down Expand Up @@ -596,8 +604,8 @@
:message "Error while scaling waiter service"})))

(retrieve-directory-content
[{:keys [http-client log-bucket-url service-id->service-description-fn watch-state]
{:keys [port scheme]} :fileserver}
[{:keys [http-client log-bucket-url watch-state]
{:keys [port scheme]} :fileserver :as scheduler}
service-id instance-id host browse-path]
(let [{:keys [_ pod-name restart-number]} (unpack-instance-id instance-id)
instance-base-dir (str "r" restart-number)
Expand All @@ -608,9 +616,7 @@
(str "/")
(not (string/starts-with? browse-path "/"))
(->> (str "/")))
run-as-user (-> service-id
service-id->service-description-fn
service-description->namespace)
run-as-user (retrieve-run-as-user scheduler service-id)
pod (get-in @watch-state [:service-id->pod-id->pod service-id pod-name])]
(ss/try+
(if (pod-logs-live? pod)
Expand Down Expand Up @@ -666,10 +672,8 @@
:syncer (retrieve-syncer-state-fn)
:watch-state @watch-state})

(validate-service [_ service-id]
(let [run-as-user (-> service-id
service-id->service-description-fn
service-description->namespace)]
(validate-service [this service-id]
(let [run-as-user (retrieve-run-as-user this service-id)]
(authz/check-user authorizer run-as-user service-id))))

(defn compute-image
Expand Down Expand Up @@ -727,15 +731,20 @@
:apiVersion replicaset-api-version
:metadata {:annotations {:waiter/service-id service-id}
:labels {:app k8s-name
:waiter-cluster cluster-name}
:name k8s-name}
:waiter-cluster cluster-name
:waiter/user run-as-user}
:name k8s-name
;; TODO - set namespace via x-waiter-namespace service parameter
:namespace run-as-user}
: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.

:template {:metadata {:annotations {:waiter/port-count (str ports)
:waiter/service-id service-id}
:labels {:app k8s-name
:waiter-cluster cluster-name}}
:waiter-cluster cluster-name
:waiter/user run-as-user}}
:spec {:containers [{:command (conj (vec container-init-commands) cmd)
:env env
:image (compute-image image default-container-image image-aliases)
Expand Down
28 changes: 15 additions & 13 deletions waiter/test/waiter/scheduler/kubernetes_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@
(instance? Service x)
(dissoc x :k8s/app-name :k8s/namespace)
(instance? ServiceInstance x)
(dissoc x :k8s/app-name :k8s/namespace :k8s/pod-name :k8s/restart-count)
(dissoc x :k8s/app-name :k8s/namespace :k8s/pod-name :k8s/restart-count :k8s/user)
:else x))
walkable-collection))

Expand Down Expand Up @@ -317,11 +317,10 @@
:items [{:metadata {:name "test-app-1234"
:namespace "myself"
:labels {:app "test-app-1234"
:waiter-cluster "waiter"}
:waiter-cluster "waiter"
:waiter/user "myself"}
:annotations {:waiter/service-id "test-app-1234"}}
:spec {:replicas 2
:selector {:matchLabels {:app "test-app-1234"
:waiter-cluster "waiter"}}}
:spec {:replicas 2}
:status {:replicas 2
:readyReplicas 2
:availableReplicas 2}}
Expand All @@ -330,9 +329,7 @@
:labels {:app "test-app-6789"
:waiter-cluster "waiter"}
:annotations {:waiter/service-id "test-app-6789"}}
:spec {:replicas 3
:selector {:matchLabels {:app "test-app-6789"
:waiter-cluster "waiter"}}}
:spec {:replicas 3}
:status {:replicas 3
:readyReplicas 1
:availableReplicas 2
Expand All @@ -344,7 +341,8 @@
:items [{:metadata {:name "test-app-1234-abcd1"
:namespace "myself"
:labels {:app "test-app-1234"
:waiter-cluster "waiter"}
:waiter-cluster "waiter"
:waiter/user "myself"}
:annotations {:waiter/port-count "1"
:waiter/service-id "test-app-1234"}}
:spec {:containers [{:ports [{:containerPort 8080 :protocol "TCP"}]}]}
Expand All @@ -356,7 +354,8 @@
{:metadata {:name "test-app-1234-abcd2"
:namespace "myself"
:labels {:app "test-app-1234"
:waiter-cluster "waiter"}
:waiter-cluster "waiter"
:waiter/user "myself"}
:annotations {:waiter/port-count "1"
:waiter/service-id "test-app-1234"}}
:spec {:containers [{:ports [{:containerPort 8080 :protocol "TCP"}]}]}
Expand All @@ -368,7 +367,8 @@
{:metadata {:name "test-app-6789-abcd1"
:namespace "myself"
:labels {:app "test-app-6789"
:waiter-cluster "waiter"}
:waiter-cluster "waiter"
:waiter/user "myself"}
:annotations {:waiter/port-count "1"
:waiter/service-id "test-app-6789"}}
:spec {:containers [{:ports [{:containerPort 8080 :protocol "TCP"}]}]}
Expand All @@ -380,7 +380,8 @@
{:metadata {:name "test-app-6789-abcd2"
:namespace "myself"
:labels {:app "test-app-6789"
:waiter-cluster "waiter"}
:waiter-cluster "waiter"
:waiter/user "myself"}
:annotations {:waiter/port-count "1"
:waiter/service-id "test-app-6789"}}
:spec {:containers [{:ports [{:containerPort 8080 :protocol "TCP"}]}]}
Expand All @@ -394,7 +395,8 @@
{:metadata {:name "test-app-6789-abcd3"
:namespace "myself"
:labels {:app "test-app-6789"
:waiter-cluster "waiter"}
:waiter-cluster "waiter"
:waiter/user "myself"}
:annotations {:waiter/port-count "1"
:waiter/service-id "test-app-6789"}}
:spec {:containers [{:ports [{:containerPort 8080 :protocol "TCP"}]}]}
Expand Down