You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/08/04 09:44:30 UTC
[GitHub] [airflow] yyvess opened a new pull request, #25530: feat(KubernetesPodOperator): Add support of container_security_context
yyvess opened a new pull request, #25530:
URL: https://github.com/apache/airflow/pull/25530
Allow to define a container security context on KubernetesPodOperator.
Why:
On clusters restricted with strong security policy, pods cannot be executed without disable privilege escalation.
About test :
I run successfully run locally there tests : _tests/providers/cncf/kubernetes/operators/test_kubernetes_pod.py_
But I was not able to run tests _kubernetes_tests/test_kubernetes_pod_operator.py_ :-(
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #25530: feat(KubernetesPodOperator): Add support of container_security_context
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25530:
URL: https://github.com/apache/airflow/pull/25530#discussion_r939248997
##########
kubernetes_tests/test_kubernetes_pod_operator.py:
##########
@@ -472,10 +472,28 @@ def test_volume_mount(self):
assert self.expected_pod == actual_pod
def test_run_as_user_root(self):
+ security_context = {'runAsUser': 0}
+ k = KubernetesPodOperator(
+ namespace='default',
+ image="ubuntu:16.04",
+ cmds=["bash", "-cx"],
+ arguments=["echo 10"],
+ labels={"foo": "bar"},
+ name="test-" + str(random.randint(0, 1000000)),
+ task_id="task" + self.get_current_task_name(),
+ in_cluster=False,
+ do_xcom_push=False,
+ security_context=security_context,
+ )
+ context = create_context(k)
+ k.execute(context)
+ actual_pod = self.api_client.sanitize_for_serialization(k.pod)
+ self.expected_pod['spec']['securityContext'] = security_context
+ assert self.expected_pod == actual_pod
+
+ def test_run_as_user_non_root(self):
security_context = {
- 'securityContext': {
- 'runAsUser': 0,
- }
+ 'runAsUser': 1000,
Review Comment:
Is this related, and why?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #25530: feat(KubernetesPodOperator): Add support of container_security_context
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #25530:
URL: https://github.com/apache/airflow/pull/25530#issuecomment-1232506488
> It's seem that main issue is how the test is write. I am sure that on this test you can set security_context = {'hello': 'world'} and the test still green.
This is the context I was looking for.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #25530: feat(KubernetesPodOperator): Add support of container_security_context
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #25530:
URL: https://github.com/apache/airflow/pull/25530#issuecomment-1231286033
You didn’t answer why you changed the `security_context` values in tests and how it affects compatibility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #25530: feat(KubernetesPodOperator): Add support of container_security_context
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #25530:
URL: https://github.com/apache/airflow/pull/25530#issuecomment-1205018087
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #25530: feat(KubernetesPodOperator): Add support of container_security_context
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #25530:
URL: https://github.com/apache/airflow/pull/25530#discussion_r938502022
##########
kubernetes_tests/test_kubernetes_pod_operator.py:
##########
@@ -495,12 +513,11 @@ def test_run_as_user_root(self):
self.expected_pod['spec']['securityContext'] = security_context
assert self.expected_pod == actual_pod
- def test_run_as_user_non_root(self):
+ def test_disable_privilege_escalation(self):
security_context = {
- 'securityContext': {
- 'runAsUser': 1000,
- }
+ 'runAsUser': 1000,
}
+ container_security_context = {'allowPrivilegeEscalation': false}
Review Comment:
```suggestion
container_security_context = {'allowPrivilegeEscalation': False}
```
?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] yyvess commented on pull request #25530: feat(KubernetesPodOperator): Add support of container_security_context
Posted by GitBox <gi...@apache.org>.
yyvess commented on PR #25530:
URL: https://github.com/apache/airflow/pull/25530#issuecomment-1228078438
@uranusjr please ☀️
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #25530: feat(KubernetesPodOperator): Add support of container_security_context
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #25530:
URL: https://github.com/apache/airflow/pull/25530#discussion_r939641159
##########
kubernetes_tests/test_kubernetes_pod_operator.py:
##########
@@ -472,10 +472,28 @@ def test_volume_mount(self):
assert self.expected_pod == actual_pod
def test_run_as_user_root(self):
+ security_context = {'runAsUser': 0}
+ k = KubernetesPodOperator(
+ namespace='default',
+ image="ubuntu:16.04",
+ cmds=["bash", "-cx"],
+ arguments=["echo 10"],
+ labels={"foo": "bar"},
+ name="test-" + str(random.randint(0, 1000000)),
+ task_id="task" + self.get_current_task_name(),
+ in_cluster=False,
+ do_xcom_push=False,
+ security_context=security_context,
+ )
+ context = create_context(k)
+ k.execute(context)
+ actual_pod = self.api_client.sanitize_for_serialization(k.pod)
+ self.expected_pod['spec']['securityContext'] = security_context
+ assert self.expected_pod == actual_pod
+
+ def test_run_as_user_non_root(self):
security_context = {
- 'securityContext': {
- 'runAsUser': 0,
- }
+ 'runAsUser': 1000,
Review Comment:
And this looks like a breaking change. Why do we change it ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] yyvess commented on pull request #25530: feat(KubernetesPodOperator): Add support of container_security_context
Posted by GitBox <gi...@apache.org>.
yyvess commented on PR #25530:
URL: https://github.com/apache/airflow/pull/25530#issuecomment-1232005181
> You didn’t answer why you changed the `security_context` values in tests and how it affects compatibility.
@uranusjr I was answed you .....
Changing only tests cannot affect compatibility 🙄
And as you can see all tests still green
I re-copy here my previous answer, please read it =>
@uranusjr Why => Because for me that was incorrect but isn't related to my main change as I write previously.
@potiuk I don't thinks that break anythings, the question is : why that test was not failed before my change ...
The variable security_context is injected on k8s.V1PodSpec =>
spec=k8s.V1PodSpec(..., security_context=self.security_context,
Look API documentation of K8s and you can see that security_context don't have a property securityContext =>
https://github.com/kubernetes-client/python/blob/master/kubernetes/docs/V1PodSpec.md
https://github.com/kubernetes-client/python/blob/master/kubernetes/docs/V1PodSecurityContext.md
It's seem that main issue is how the test is write. I am sure that on this test you can set security_context = {'hello': 'world'} and the test still green.
Perhaps we must change the type of security_context & container_security_context on class KubernetesPodOperator that is actually defined as Dict
security_context: Optional[Dict] = None, container_security_context: Optional[Dict] = None,
Should be more safer to declare it as is
security_context: Optional[k8s.V1PodSecurityContext] = None, container_security_context: Optional[k8s.V1SecurityContext] = None,
But should make on a different PR is you approved to change types
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #25530: feat(KubernetesPodOperator): Add support of container_security_context
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #25530:
URL: https://github.com/apache/airflow/pull/25530#issuecomment-1241392835
Awesome work, congrats on your first merged pull request!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #25530: feat(KubernetesPodOperator): Add support of container_security_context
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #25530:
URL: https://github.com/apache/airflow/pull/25530
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org