Bugfix: Invalid name when trimmed `pod_id` ends with hyphen in ``Kube… #15443

kaxil · 2021-04-19T22:00:43Z

…rnetesPodOperator``

When a pod name is more than MAX_LABEL_LEN (63 characters), it is trimmed to 63 chars

airflow/airflow/kubernetes/pod_generator.py

Lines 470 to 472 in 8711f90

    
           safe_uuid = uuid.uuid4().hex  # safe uuid will always be less than 63 chars 
        
           trimmed_pod_id = pod_id[:MAX_LABEL_LEN] 
        
           safe_pod_id = f"{trimmed_pod_id}.{safe_uuid}"

and we add a safe UUID to the pod_id joined by a dot .. However the regex
for Kubernetes name does not allow - followed by a ..

Valid Regex:

^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$

This commit strips any hypens at the end of the trimmed pod_id.

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

…rnetesPodOperator`` When a pod name is more than `MAX_LABEL_LEN` (63 characters), it is trimmed to 63 chars https://github.com/apache/airflow/blob/8711f90ab820ed420ef317b931e933a2062c891f/airflow/kubernetes/pod_generator.py#L470-L472 and we add a safe UUID to the `pod_id` joined by a dot `.`. However the regex for Kubernetes name does not allow `-` followed by a `.`. Valid Regex: ``` ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ ``` This commit strips any hypens at the end of the trimmed `pod_id`.

houqp

LGTM.

On top of this, I wonder if we should keep the name sanitizing logic consistent between executor and operator by passing the task name through _strip_unsafe_kubernetes_special_chars instead of using re.sub(r'[^a-z0-9.-]+', '-', name.lower()) in KubernetesPodOperator._set_name.

github-actions · 2021-04-19T22:40:40Z

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

jedcunningham · 2021-04-19T23:09:13Z

tests/kubernetes/test_pod_generator.py

+            ("pod-name-with-hyphen-", "pod-name-with-hyphen"),
+            ("pod-name-with-double-hyphen--", "pod-name-with-double-hyphen"),
+            ("pod0-name", "pod0-name"),
+            ("simple", "simple"),


Suggested change

("simple", "simple"),

("simple", "simple"),

("pod-name-with-dot.", "pod-name-with-dot"),

("pod-name-with-double-dot..", "pod-name-with-double-dot"),

("pod-name-with-hyphen-dot-.", "pod-name-with-hyphen-dot"),

jedcunningham · 2021-04-19T23:10:37Z

airflow/kubernetes/pod_generator.py

+
+        # Since we use '.' as separator we need to remove all the occurences of '-' if any
+        # in the trimmed_pod_id as the regex does not allow '-' followed by '.'.
+        safe_pod_id = f"{trimmed_pod_id.rstrip('-')}.{safe_uuid}"


Suggested change

# Since we use '.' as separator we need to remove all the occurences of '-' if any

# in the trimmed_pod_id as the regex does not allow '-' followed by '.'.

safe_pod_id = f"{trimmed_pod_id.rstrip('-')}.{safe_uuid}"

trimmed_pod_id = pod_id[:MAX_LABEL_LEN].rstrip('-') # Strip trailing '-' as it cant be followed by '.'

safe_pod_id = f"{trimmed_pod_id}.{safe_uuid}"

Maybe? Either way, your comment is wrong as it only removes trailing dashes, and occurrences has a typo.

What about the '..' case (and more, maybe trimming until the last char is a-z0-9)?

Created #15445 to address it

jedcunningham · 2021-04-19T23:13:29Z

Oops, too slow, guess we can address these in a follow up PR.

Missed a case in (apache#15443) where `.` can be followed by another `.`.

Missed a case in (#15443) where `.` can be followed by another `.`.

…rnetesPodOperator`` (apache#15443) When a pod name is more than `MAX_LABEL_LEN` (63 characters), it is trimmed to 63 chars https://github.com/apache/airflow/blob/8711f90ab820ed420ef317b931e933a2062c891f/airflow/kubernetes/pod_generator.py#L470-L472 and we add a safe UUID to the `pod_id` joined by a dot `.`. However the regex for Kubernetes name does not allow `-` followed by a `.`. Valid Regex: ``` ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ ``` This commit strips any hypens at the end of the trimmed `pod_id`. (cherry picked from commit 130f9e3)

Missed a case in (apache#15443) where `.` can be followed by another `.`. (cherry picked from commit 1e66ce8)

…rnetesPodOperator`` (apache#15443) When a pod name is more than `MAX_LABEL_LEN` (63 characters), it is trimmed to 63 chars https://github.com/apache/airflow/blob/8711f90ab820ed420ef317b931e933a2062c891f/airflow/kubernetes/pod_generator.py#L470-L472 and we add a safe UUID to the `pod_id` joined by a dot `.`. However the regex for Kubernetes name does not allow `-` followed by a `.`. Valid Regex: ``` ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ ``` This commit strips any hypens at the end of the trimmed `pod_id`. (cherry picked from commit 130f9e3)

Missed a case in (apache#15443) where `.` can be followed by another `.`. (cherry picked from commit 1e66ce8)

…rnetesPodOperator`` (#15443) When a pod name is more than `MAX_LABEL_LEN` (63 characters), it is trimmed to 63 chars https://github.com/apache/airflow/blob/8711f90ab820ed420ef317b931e933a2062c891f/airflow/kubernetes/pod_generator.py#L470-L472 and we add a safe UUID to the `pod_id` joined by a dot `.`. However the regex for Kubernetes name does not allow `-` followed by a `.`. Valid Regex: ``` ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ ``` This commit strips any hypens at the end of the trimmed `pod_id`. (cherry picked from commit 130f9e3)

Missed a case in (#15443) where `.` can be followed by another `.`. (cherry picked from commit 1e66ce8)

kaxil requested review from houqp, dimberman, ephraimbuddy and jedcunningham April 19, 2021 22:00

boring-cyborg bot added the provider:cncf-kubernetes Kubernetes provider related issues label Apr 19, 2021

kaxil added this to the Airflow 2.0.3 milestone Apr 19, 2021

houqp approved these changes Apr 19, 2021

View reviewed changes

github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Apr 19, 2021

kaxil merged commit 130f9e3 into apache:master Apr 19, 2021

kaxil deleted the fix-pod-name branch April 19, 2021 23:02

jedcunningham requested changes Apr 19, 2021

View reviewed changes

kaxil added a commit to astronomer/airflow that referenced this pull request Apr 20, 2021

Further fix trimmed pod_id for KubernetesPodOperator

63d5e2a

Missed a case in (apache#15443) where `.` can be followed by another `.`.

kaxil mentioned this pull request Apr 20, 2021

Further fix trimmed pod_id for KubernetesPodOperator #15445

Merged

kaxil added a commit that referenced this pull request Apr 20, 2021

Further fix trimmed pod_id for KubernetesPodOperator (#15445)

1e66ce8

Missed a case in (#15443) where `.` can be followed by another `.`.

kaxil added a commit to astronomer/airflow that referenced this pull request Apr 20, 2021

Further fix trimmed pod_id for KubernetesPodOperator (apache#15445)

4fef616

Missed a case in (apache#15443) where `.` can be followed by another `.`. (cherry picked from commit 1e66ce8)

kaxil added a commit to astronomer/airflow that referenced this pull request Apr 26, 2021

Further fix trimmed pod_id for KubernetesPodOperator (apache#15445)

085bfc3

Missed a case in (apache#15443) where `.` can be followed by another `.`. (cherry picked from commit 1e66ce8)

potiuk pushed a commit that referenced this pull request May 9, 2021

Further fix trimmed pod_id for KubernetesPodOperator (#15445)

5acd2d8

Missed a case in (#15443) where `.` can be followed by another `.`. (cherry picked from commit 1e66ce8)

ashb removed this from the Airflow 2.0.3 milestone May 18, 2021

ashb added this to the Airflow 2.1 milestone May 18, 2021

kaxil mentioned this pull request Jun 25, 2021

Added dashes to pod naming #16636

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix: Invalid name when trimmed `pod_id` ends with hyphen in ``Kube… #15443

Bugfix: Invalid name when trimmed `pod_id` ends with hyphen in ``Kube… #15443

kaxil commented Apr 19, 2021 •

edited

houqp left a comment

github-actions bot commented Apr 19, 2021

jedcunningham Apr 19, 2021

jedcunningham Apr 19, 2021

kaxil Apr 20, 2021

jedcunningham commented Apr 19, 2021

	safe_uuid = uuid.uuid4().hex # safe uuid will always be less than 63 chars
	trimmed_pod_id = pod_id[:MAX_LABEL_LEN]
	safe_pod_id = f"{trimmed_pod_id}.{safe_uuid}"

-            ("simple", "simple"),
+            ("simple", "simple"),
+            ("pod-name-with-dot.", "pod-name-with-dot"),
+            ("pod-name-with-double-dot..", "pod-name-with-double-dot"),
+            ("pod-name-with-hyphen-dot-.", "pod-name-with-hyphen-dot"),

Bugfix: Invalid name when trimmed pod_id ends with hyphen in ``Kube… #15443

Bugfix: Invalid name when trimmed pod_id ends with hyphen in ``Kube… #15443

Conversation

kaxil commented Apr 19, 2021 • edited

houqp left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 19, 2021

jedcunningham Apr 19, 2021

Choose a reason for hiding this comment

jedcunningham Apr 19, 2021

Choose a reason for hiding this comment

kaxil Apr 20, 2021

Choose a reason for hiding this comment

jedcunningham commented Apr 19, 2021

Bugfix: Invalid name when trimmed `pod_id` ends with hyphen in ``Kube… #15443

Bugfix: Invalid name when trimmed `pod_id` ends with hyphen in ``Kube… #15443

kaxil commented Apr 19, 2021 •

edited