Skip to content

Commit

Permalink
Mount DAGs read only when using gitsync (apache#15953)
Browse files Browse the repository at this point in the history
We will mount the DAGs as read only when we are using gitsync, but not
when we are only using persistence. This also moves the whole mount definition
into helpers instead of just the mount path, making it easier to modify
the DAGs mount everywhere it is used in the future.
  • Loading branch information
jedcunningham authored May 19, 2021
1 parent 6fd6712 commit e01b4e6
Show file tree
Hide file tree
Showing 9 changed files with 126 additions and 47 deletions.
3 changes: 3 additions & 0 deletions chart/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,6 @@ repository

# Chart dependencies
**/charts/*.tgz

# Never check in tmpcharts
tmpcharts
4 changes: 1 addition & 3 deletions chart/files/pod-template-file.kubernetes-helm-yaml
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,7 @@ spec:
subPath: ssh
{{- end }}
{{- if or .Values.dags.gitSync.enabled .Values.dags.persistence.enabled }}
- mountPath: {{ include "airflow_dags_mount_path" . }}
name: dags
readOnly: true
{{- include "airflow_dags_mount" . | nindent 8 }}
{{- end }}
{{- if .Values.workers.extraVolumeMounts }}
{{ toYaml .Values.workers.extraVolumeMounts | indent 8 }}
Expand Down
8 changes: 5 additions & 3 deletions chart/templates/_helpers.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -377,9 +377,11 @@ server_tls_key_file = /etc/pgbouncer/server.key
{{- end -}}
{{- end -}}

{{ define "airflow_dags_mount_path" -}}
{{ (printf "%s/dags" .Values.airflowHome) }}
{{- end }}
{{ define "airflow_dags_mount" -}}
- name: dags
mountPath: {{ (printf "%s/dags" .Values.airflowHome) }}
readOnly: {{ .Values.dags.gitSync.enabled | ternary "True" "False" }}
{{- end -}}

{{ define "airflow_config_path" -}}
{{ (printf "%s/airflow.cfg" .Values.airflowHome) | quote }}
Expand Down
11 changes: 5 additions & 6 deletions chart/templates/scheduler/scheduler-deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -162,16 +162,15 @@ spec:
mountPath: {{ template "airflow_config_path" . }}
subPath: airflow.cfg
readOnly: true
{{- if .Values.airflowLocalSettings }}
{{- if .Values.airflowLocalSettings }}
- name: config
mountPath: {{ template "airflow_local_setting_path" . }}
subPath: airflow_local_settings.py
readOnly: true
{{- end }}
{{- if or .Values.dags.gitSync.enabled .Values.dags.persistence.enabled }}
- name: dags
mountPath: {{ template "airflow_dags_mount_path" . }}
{{- end }}
{{- end }}
{{- if or .Values.dags.persistence.enabled .Values.dags.gitSync.enabled }}
{{- include "airflow_dags_mount" . | nindent 12 }}
{{- end }}
{{- if .Values.scheduler.extraVolumeMounts }}
{{ toYaml .Values.scheduler.extraVolumeMounts | indent 12 }}
{{- end }}
Expand Down
9 changes: 4 additions & 5 deletions chart/templates/webserver/webserver-deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,6 @@ spec:
{{- include "custom_airflow_environment" . | indent 10 }}
{{- include "standard_airflow_environment" . | indent 10 }}
containers:
{{- if and (.Values.dags.gitSync.enabled) (not .Values.dags.persistence.enabled) (semverCompare "<2.0.0" .Values.airflowVersion) }}
{{- include "git_sync_container" . | indent 8 }}
{{- end }}
- name: webserver
image: {{ template "airflow_image" . }}
imagePullPolicy: {{ .Values.images.airflow.pullPolicy }}
Expand All @@ -135,8 +132,7 @@ spec:
readOnly: true
{{- end }}
{{- if or (and .Values.dags.gitSync.enabled (semverCompare "<2.0.0" .Values.airflowVersion)) .Values.dags.persistence.enabled }}
- name: dags
mountPath: {{ template "airflow_dags_mount_path" . }}
{{- include "airflow_dags_mount" . | nindent 12 }}
{{- end }}
{{- if .Values.logs.persistence.enabled }}
- name: logs
Expand Down Expand Up @@ -179,6 +175,9 @@ spec:
env:
{{- include "custom_airflow_environment" . | indent 10 }}
{{- include "standard_airflow_environment" . | indent 10 }}
{{- if and (.Values.dags.gitSync.enabled) (not .Values.dags.persistence.enabled) (semverCompare "<2.0.0" .Values.airflowVersion) }}
{{- include "git_sync_container" . | indent 8 }}
{{- end }}
{{- if .Values.webserver.extraContainers }}
{{- toYaml .Values.webserver.extraContainers | nindent 8 }}
{{- end }}
Expand Down
11 changes: 5 additions & 6 deletions chart/templates/workers/worker-deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -156,16 +156,15 @@ spec:
mountPath: {{ .Values.kerberos.ccacheMountPath | quote }}
readOnly: true
{{- end }}
{{- if .Values.airflowLocalSettings }}
{{- if .Values.airflowLocalSettings }}
- name: config
mountPath: {{ template "airflow_local_setting_path" . }}
subPath: airflow_local_settings.py
readOnly: true
{{- end }}
{{- if or .Values.dags.persistence.enabled .Values.dags.gitSync.enabled }}
- name: dags
mountPath: {{ template "airflow_dags_mount_path" . }}
{{- end }}
{{- end }}
{{- if or .Values.dags.persistence.enabled .Values.dags.gitSync.enabled }}
{{- include "airflow_dags_mount" . | nindent 12 }}
{{- end }}
envFrom:
{{- include "custom_airflow_environment_from" . | default "\n []" | indent 10 }}
env:
Expand Down
92 changes: 92 additions & 0 deletions chart/tests/test_airflow_common.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.

import unittest

import jmespath
from parameterized import parameterized

from tests.helm_template_generator import render_chart


class AirflowCommon(unittest.TestCase):
"""
This class holds tests that apply to more than 1 Airflow component so
we don't have to repeat tests everywhere
The one general exception will be the KubernetesExecutor PodTemplateFile,
as it requires extra test setup.
"""

@parameterized.expand(
[
({"gitSync": {"enabled": True}}, True),
({"persistence": {"enabled": True}}, False),
(
{
"gitSync": {"enabled": True},
"persistence": {"enabled": True},
},
True,
),
]
)
def test_dags_mount(self, dag_values, expected_read_only):
docs = render_chart(
values={
"dags": dag_values,
"airflowVersion": "1.10.15",
}, # airflowVersion is present so webserver gets the mount
show_only=[
"templates/scheduler/scheduler-deployment.yaml",
"templates/workers/worker-deployment.yaml",
"templates/webserver/webserver-deployment.yaml",
],
)

assert 3 == len(docs)
for doc in docs:
expected_mount = {
"mountPath": "/opt/airflow/dags",
"name": "dags",
"readOnly": expected_read_only,
}
assert expected_mount in jmespath.search("spec.template.spec.containers[0].volumeMounts", doc)

def test_annotations(self):
"""
Test Annotations are correctly applied on all pods created Scheduler, Webserver & Worker
deployments.
"""
release_name = "TEST-BASIC"
k8s_objects = render_chart(
name=release_name,
values={"airflowPodAnnotations": {"test-annotation/safe-to-evict": "true"}},
show_only=[
"templates/scheduler/scheduler-deployment.yaml",
"templates/workers/worker-deployment.yaml",
"templates/webserver/webserver-deployment.yaml",
"templates/flower/flower-deployment.yaml",
],
)

assert 4 == len(k8s_objects)

for k8s_object in k8s_objects:
annotations = k8s_object["spec"]["template"]["metadata"]["annotations"]
assert "test-annotation/safe-to-evict" in annotations
assert "true" in annotations["test-annotation/safe-to-evict"]
4 changes: 2 additions & 2 deletions chart/tests/test_git_sync_webserver.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ def test_should_add_git_sync_container_to_webserver_if_persistence_is_not_enable
show_only=["templates/webserver/webserver-deployment.yaml"],
)

assert "git-sync" == jmespath.search("spec.template.spec.containers[0].name", docs[0])
assert "git-sync" == jmespath.search("spec.template.spec.containers[1].name", docs[0])

def test_should_have_service_account_defined(self):
docs = render_chart(
Expand Down Expand Up @@ -146,5 +146,5 @@ def test_should_add_env(self):
)

assert {"name": "FOO", "value": "bar"} in jmespath.search(
"spec.template.spec.containers[0].env", docs[0]
"spec.template.spec.containers[1].env", docs[0]
)
31 changes: 9 additions & 22 deletions chart/tests/test_pod_template_file.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,42 +122,29 @@ def test_should_not_add_init_container_if_dag_persistence_is_true(self):

@parameterized.expand(
[
({"gitSync": {"enabled": True}},),
({"persistence": {"enabled": True}},),
({"gitSync": {"enabled": True}}, True),
({"persistence": {"enabled": True}}, False),
(
{
"gitSync": {"enabled": True},
"persistence": {"enabled": True},
},
True,
),
]
)
def test_dags_mount(self, dag_values):
def test_dags_mount(self, dag_values, expected_read_only):
docs = render_chart(
values={"dags": dag_values},
show_only=["templates/pod-template-file.yaml"],
chart_dir=self.temp_chart_dir,
)

assert {"mountPath": "/opt/airflow/dags", "name": "dags", "readOnly": True} in jmespath.search(
"spec.containers[0].volumeMounts", docs[0]
)

def test_dags_mount_with_gitsync_and_persistence(self):
docs = render_chart(
values={
"dags": {
"gitSync": {"enabled": True},
"persistence": {"enabled": True},
}
},
show_only=["templates/pod-template-file.yaml"],
chart_dir=self.temp_chart_dir,
)

assert {"mountPath": "/opt/airflow/dags", "name": "dags", "readOnly": True} in jmespath.search(
"spec.containers[0].volumeMounts", docs[0]
)
assert {
"mountPath": "/opt/airflow/dags",
"name": "dags",
"readOnly": expected_read_only,
} in jmespath.search("spec.containers[0].volumeMounts", docs[0])

def test_validate_if_ssh_params_are_added(self):
docs = render_chart(
Expand Down

0 comments on commit e01b4e6

Please sign in to comment.