Skip to content

Conversation

@yyvess
Copy link
Contributor

@yyvess yyvess commented Aug 4, 2022

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 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 :-(

@yyvess yyvess requested a review from jedcunningham as a code owner August 4, 2022 09:44
@boring-cyborg boring-cyborg bot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:providers labels Aug 4, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Aug 4, 2022

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 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 Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment 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 for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
    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

@yyvess yyvess requested a review from uranusjr August 5, 2022 15:59
@yyvess yyvess requested review from potiuk and uranusjr and removed request for jedcunningham, potiuk and uranusjr August 18, 2022 09:19
@yyvess
Copy link
Contributor Author

yyvess commented Aug 26, 2022

@uranusjr please ☀️

@uranusjr
Copy link
Member

You didn’t answer why you changed the security_context values in tests and how it affects compatibility.

@uranusjr uranusjr removed their request for review August 30, 2022 07:54
@yyvess
Copy link
Contributor Author

yyvess commented Aug 30, 2022

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

@yyvess yyvess requested review from potiuk and uranusjr August 30, 2022 18:19
@uranusjr
Copy link
Member

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.

@potiuk potiuk merged commit 4b26c8c into apache:main Sep 9, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Sep 9, 2022

Awesome work, congrats on your first merged pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:cncf-kubernetes Kubernetes (k8s) provider related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants