-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: add container and pod security context support for kubernetes workloads #3101
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2581,6 +2581,15 @@ def _container_templates(self): | |||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| pod_security_context = resources.get("pod_security_context", None) | ||||||||||||||||||||||||||||||||||||||||||
| _pod_security_context = {} | ||||||||||||||||||||||||||||||||||||||||||
| if pod_security_context is not None and len(pod_security_context) > 0: | ||||||||||||||||||||||||||||||||||||||||||
| _pod_security_context = { | ||||||||||||||||||||||||||||||||||||||||||
| "security_context": kubernetes_sdk.V1PodSecurityContext( | ||||||||||||||||||||||||||||||||||||||||||
| **pod_security_context | ||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+2584
to
+2591
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| # Create a ContainerTemplate for this node. Ideally, we would have | ||||||||||||||||||||||||||||||||||||||||||
| # liked to inline this ContainerTemplate and avoid scanning the workflow | ||||||||||||||||||||||||||||||||||||||||||
| # twice, but due to issues with variable substitution, we will have to | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -2639,6 +2648,7 @@ def _container_templates(self): | |||||||||||||||||||||||||||||||||||||||||
| port=port, | ||||||||||||||||||||||||||||||||||||||||||
| qos=resources["qos"], | ||||||||||||||||||||||||||||||||||||||||||
| security_context=security_context, | ||||||||||||||||||||||||||||||||||||||||||
| pod_security_context=pod_security_context, | ||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| for k, v in env.items(): | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -2804,6 +2814,16 @@ def _container_templates(self): | |||||||||||||||||||||||||||||||||||||||||
| if resources["image_pull_secrets"] | ||||||||||||||||||||||||||||||||||||||||||
| else None | ||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||
| # Set pod security context via pod_spec_patch | ||||||||||||||||||||||||||||||||||||||||||
| .pod_spec_patch( | ||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||
| "securityContext": kubernetes_sdk.V1PodSecurityContext( | ||||||||||||||||||||||||||||||||||||||||||
| **pod_security_context | ||||||||||||||||||||||||||||||||||||||||||
| ).to_dict() | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| if pod_security_context | ||||||||||||||||||||||||||||||||||||||||||
| else None | ||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+2818
to
+2826
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The existing code already imports
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
| # Set container | ||||||||||||||||||||||||||||||||||||||||||
| .container( | ||||||||||||||||||||||||||||||||||||||||||
| # TODO: Unify the logic with kubernetes.py | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -4503,7 +4523,12 @@ def pod_spec_patch(self, pod_spec_patch=None): | |||||||||||||||||||||||||||||||||||||||||
| if pod_spec_patch is None: | ||||||||||||||||||||||||||||||||||||||||||
| return self | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| self.payload["podSpecPatch"] = json.dumps(pod_spec_patch) | ||||||||||||||||||||||||||||||||||||||||||
| if "podSpecPatch" in self.payload: | ||||||||||||||||||||||||||||||||||||||||||
| existing = json.loads(self.payload["podSpecPatch"]) | ||||||||||||||||||||||||||||||||||||||||||
| existing.update(pod_spec_patch) | ||||||||||||||||||||||||||||||||||||||||||
| self.payload["podSpecPatch"] = json.dumps(existing) | ||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||
| self.payload["podSpecPatch"] = json.dumps(pod_spec_patch) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| return self | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,8 @@ | |
| KUBERNETES_NODE_SELECTOR, | ||
| KUBERNETES_PERSISTENT_VOLUME_CLAIMS, | ||
| KUBERNETES_PORT, | ||
| KUBERNETES_SECURITY_CONTEXT, | ||
| KUBERNETES_POD_SECURITY_CONTEXT, | ||
| KUBERNETES_SERVICE_ACCOUNT, | ||
| KUBERNETES_SHARED_MEMORY, | ||
| KUBERNETES_TOLERATIONS, | ||
|
|
@@ -136,6 +138,17 @@ class KubernetesDecorator(StepDecorator): | |
| - run_as_user: int, optional, default None | ||
| - run_as_group: int, optional, default None | ||
| - run_as_non_root: bool, optional, default None | ||
| - read_only_root_filesystem: bool, optional, default None | ||
| - capabilities: Dict[str, List[str]], optional, default None | ||
| Can also be set via METAFLOW_KUBERNETES_SECURITY_CONTEXT (JSON). | ||
| pod_security_context: Dict[str, Any], optional, default None | ||
| Pod-level security context. Applies to all containers in the pod. Allows the following keys: | ||
| - run_as_user: int, optional, default None | ||
| - run_as_group: int, optional, default None | ||
| - run_as_non_root: bool, optional, default None | ||
| - fs_group: int, optional, default None | ||
| - supplemental_groups: List[int], optional, default None | ||
| Can also be set via METAFLOW_KUBERNETES_POD_SECURITY_CONTEXT (JSON). | ||
| """ | ||
|
|
||
| name = "kubernetes" | ||
|
|
@@ -168,6 +181,7 @@ class KubernetesDecorator(StepDecorator): | |
| "hostname_resolution_timeout": 10 * 60, | ||
| "qos": KUBERNETES_QOS, | ||
| "security_context": None, | ||
| "pod_security_context": None, | ||
| } | ||
| package_metadata = None | ||
| package_url = None | ||
|
|
@@ -310,6 +324,19 @@ def init(self): | |
| if not self.attributes["port"]: | ||
| self.attributes["port"] = KUBERNETES_PORT | ||
|
|
||
| # Security context: decorator takes precedence over env var | ||
| if not self.attributes["security_context"] and KUBERNETES_SECURITY_CONTEXT: | ||
| self.attributes["security_context"] = json.loads( | ||
| KUBERNETES_SECURITY_CONTEXT | ||
| ) | ||
| if ( | ||
| not self.attributes["pod_security_context"] | ||
| and KUBERNETES_POD_SECURITY_CONTEXT | ||
| ): | ||
| self.attributes["pod_security_context"] = json.loads( | ||
| KUBERNETES_POD_SECURITY_CONTEXT | ||
| ) | ||
|
Comment on lines
+327
to
+338
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| # Refer https://github.com/Netflix/metaflow/blob/master/docs/lifecycle.png | ||
| def step_init(self, flow, graph, step, decos, environment, flow_datastore, logger): | ||
| # Executing Kubernetes jobs requires a non-local datastore. | ||
|
|
@@ -500,6 +527,7 @@ def runtime_step_cli( | |
| "labels", | ||
| "annotations", | ||
| "security_context", | ||
| "pod_security_context", | ||
| ]: | ||
| cli_args.command_options[k] = json.dumps(v) | ||
| else: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
""instead ofNoneAll adjacent config vars (
KUBERNETES_MEMORY,KUBERNETES_DISK, etc.) default toNone. Using""works because the falsiness check in the decorator handles it, but it's inconsistent and slightly harder to reason about. Consider defaulting toNoneto match the established pattern.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!