add missing read for K8S config file from conn in deferred `KubernetesPodOperator` #29498

hussein-awala · 2023-02-12T22:15:08Z

The async execute method of KubernetesPodOperator doesn't check if the config_path is provided in the connection extra, this PR fixes this by extracting the config path in order to read it and convert it to dictionary.

raphaelauv · 2023-02-13T14:56:12Z

airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py

@@ -565,7 +565,16 @@ def execute_async(self, context: Context):

    def convert_config_file_to_dict(self):
        """Converts passed config_file to dict format."""
-        config_file = self.config_file if self.config_file else os.environ.get(KUBE_CONFIG_ENV_VAR)
+        config_file = None


thanks @hussein-awala for proposing this fix.

why the async need the function convert_config_file_to_dictand not the sync ?

Look like the async was implemented not fully following this pattern -> #20578

your PR fix the problems for the extra config_path , there is a risk that another is missing or new in the future would need "manual" fix like this

I am not sure about the initial reason to convert the file into dictionary before creating the trigger, it may be to avoid copying the config file to the triggerer, where the pod is created on the worker using the sync hook and the waiting task is running on the triggerer and it uses the async hook.

here is a risk that another is missing or new in the future would need "manual" fix like this

With this fix, we cover all options currently available to provide the configuration file, and yes, if we add a new one in the future, we must add it on the sync hook and in this method.

@VladaZakharova can you please explain what was the motivation to convert the config file to a dictionary before creating the trigger?

Hi Team!
This was implemented to that config file was converted to dict to be passed to trigger and then hook to establish connection.

what do you mean by lighten the credential management ?

the hook is not re instantiate at every run of the trigger ?

We needed a way to pass config file to the trigger to create a client for kubernetes, but using file system to communicate with trigger was not a good solution. So then we added a possibility to pass all config file parameters as a dict.

To respect the pattern mentioned by @raphaelauv, I will try loading the config file in the async hook, this should work where the triggerer is initiated once.

Please mind that all FS operations are blocking side effects. It's violating asyncio contract and can cause additional error logs informing about blocking code.

potiuk · 2023-02-20T09:33:26Z

@hussein-awala I guess you will be still changing the config access pattern on that one ? Do I understand correctly?

hussein-awala · 2023-02-20T09:56:03Z

I guess you will be still changing the config access pattern on that one ? Do I understand correctly?

Yes, I'm testing loading the config file in the triggerer instead of loading it in the worker and pass it as a dict.

I convert the PR to draft until I finish testing

VladaZakharova · 2023-02-20T10:24:04Z

Hi!
May i ask in which format you will pass the config file to trigger? So it will be just a file passed as a parameter to trigger? Or how?

…rker and pass it as a dict

hussein-awala · 2023-02-21T00:38:04Z

Hi! May i ask in which format you will pass the config file to trigger? So it will be just a file passed as a parameter to trigger? Or how?

@VladaZakharova - Yes, I pass the file path and let the triggerer loads it. Can you check my last commit?

BTW, I am not sure if loading the config file from the env var KUBECONFIG is a good idea or not, because it's difficult to decide when we need to load it and when we don't.

raphaelauv · 2023-02-21T08:30:27Z

Loading the config file from the env KUBECONFIG is deprecated in latest provider version

raphaelauv

LGTM

github-actions · 2023-04-11T00:11:35Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

raphaelauv · 2023-04-13T09:17:45Z

@hussein-awala the PR have conflicts , could you rebase on main , thank you 👍

eladkal · 2023-04-15T12:42:15Z

airflow/providers/cncf/kubernetes/operators/pod.py

-    def convert_config_file_to_dict(self):
-        """Converts passed config_file to dict format."""
-        config_file = self.config_file if self.config_file else os.environ.get(KUBE_CONFIG_ENV_VAR)
-        if config_file:
-            with open(config_file) as f:
-                self._config_dict = yaml.safe_load(f)
-        else:
-            self._config_dict = None


is removing this function considered a breaking change?

In my opinion, this method is used as a private method since it only updates some attributes in the class instances without returning any value. However, it's possible that someone could extend the operator class and use it. Should we deprecate it and remove it in the next major release, or should we add a breaking change note?

So lets deprecate first. Just to be on the safe side.

potiuk · 2023-04-22T17:22:16Z

LGTM. @eladkal ?

add missing read for conf file from connection

72a1913

hussein-awala requested a review from jedcunningham as a code owner February 12, 2023 22:15

boring-cyborg bot added provider:cncf-kubernetes Kubernetes provider related issues area:providers labels Feb 12, 2023

add a test for the different methods to provide the config file path

8ffa8eb

hussein-awala changed the title ~~[WIP] add missing read for K8S config file from conn in deferred KubernetesPodOperator~~ add missing read for K8S config file from conn in deferred KubernetesPodOperator Feb 13, 2023

change loading order to config_file, connection then env var

8f40fbb

raphaelauv reviewed Feb 13, 2023

View reviewed changes

potiuk requested a review from dstandish February 20, 2023 09:32

hussein-awala marked this pull request as draft February 20, 2023 09:56

load the config file in the triggerer instead of loading it in the wo…

e7b0e39

…rker and pass it as a dict

fix deferrable mode tests

a007a76

hussein-awala marked this pull request as ready for review February 22, 2023 00:55

raphaelauv approved these changes Feb 23, 2023

View reviewed changes

raphaelauv mentioned this pull request Mar 14, 2023

KPO (async) log full config_dict in triggerer #30097

Closed

2 tasks

github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Apr 11, 2023

github-actions bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Apr 14, 2023

hussein-awala force-pushed the fix/deferrable_k8s_pod_op branch 4 times, most recently from 839b3c1 to 59d76b8 Compare April 14, 2023 23:40

Merge branch 'main' into fix/deferrable_k8s_pod_op

24885c6

hussein-awala force-pushed the fix/deferrable_k8s_pod_op branch from 59d76b8 to 24885c6 Compare April 15, 2023 00:28

eladkal reviewed Apr 15, 2023

View reviewed changes

hussein-awala mentioned this pull request Apr 15, 2023

Add provider for Apache Kafka #30175

Merged

hussein-awala force-pushed the fix/deferrable_k8s_pod_op branch from bdef821 to 1a87f6e Compare April 18, 2023 00:04

restore convert_config_file_to_dict method and deprecate it

5bc37f2

hussein-awala force-pushed the fix/deferrable_k8s_pod_op branch from 1a87f6e to 5bc37f2 Compare April 18, 2023 00:07

potiuk approved these changes Apr 22, 2023

View reviewed changes

eladkal approved these changes Apr 22, 2023

View reviewed changes

potiuk merged commit b5296b7 into apache:main Apr 22, 2023

eladkal mentioned this pull request May 16, 2023

Status of testing Providers that were prepared on May 19, 2023 #31322

Closed

80 tasks

bjankie1 mentioned this pull request Dec 29, 2023

Invalid kube-config file. Expected key current-context in kube-config when using deferrable=True #34644

Closed

2 tasks

GoVulnBot mentioned this pull request Jan 24, 2024

x/vulndb: potential Go vuln in github.com/apache/airflow: CVE-2023-51702 golang/vulndb#2475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add missing read for K8S config file from conn in deferred `KubernetesPodOperator` #29498

add missing read for K8S config file from conn in deferred `KubernetesPodOperator` #29498

hussein-awala commented Feb 12, 2023

raphaelauv Feb 13, 2023

hussein-awala Feb 14, 2023

VladaZakharova Feb 14, 2023 •

edited

Loading

raphaelauv Feb 14, 2023

VladaZakharova Feb 14, 2023

hussein-awala Feb 14, 2023

bjankie1 Feb 24, 2023

potiuk commented Feb 20, 2023

hussein-awala commented Feb 20, 2023

VladaZakharova commented Feb 20, 2023 •

edited

Loading

hussein-awala commented Feb 21, 2023

raphaelauv commented Feb 21, 2023

raphaelauv left a comment

github-actions bot commented Apr 11, 2023

raphaelauv commented Apr 13, 2023

eladkal Apr 15, 2023

hussein-awala Apr 15, 2023

eladkal Apr 15, 2023

potiuk commented Apr 22, 2023

add missing read for K8S config file from conn in deferred KubernetesPodOperator #29498

add missing read for K8S config file from conn in deferred KubernetesPodOperator #29498

Conversation

hussein-awala commented Feb 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VladaZakharova Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk commented Feb 20, 2023

hussein-awala commented Feb 20, 2023

VladaZakharova commented Feb 20, 2023 • edited Loading

hussein-awala commented Feb 21, 2023

raphaelauv commented Feb 21, 2023

raphaelauv left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 11, 2023

raphaelauv commented Apr 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk commented Apr 22, 2023

add missing read for K8S config file from conn in deferred `KubernetesPodOperator` #29498

add missing read for K8S config file from conn in deferred `KubernetesPodOperator` #29498

VladaZakharova Feb 14, 2023 •

edited

Loading

VladaZakharova commented Feb 20, 2023 •

edited

Loading