jscheffl commented on code in PR #67012: URL: https://github.com/apache/airflow/pull/67012#discussion_r3391240173
########## chart/kustomize-overlays/kerberos/README.rst: ########## @@ -0,0 +1,155 @@ +.. 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. + +Kerberos Test KDC Overlay +========================= + +This overlay stands up a throwaway in-cluster MIT Kerberos KDC, creates +the ``airflow/airflow.<namespace>.svc.cluster.local`` service principal, +and stores its keytab in a Secret named ``<release>-kerberos-keytab``. +It is a standalone addition; no resource produced by the Helm chart is +modified. + +It is intended as a proof-of-concept of how a non-Airflow component +(in this case Kerberos infrastructure) can be expressed as a Kustomize +overlay alongside the chart, rather than baked into the chart itself. +The keytab Secret it produces is consumable as-is by the chart's +existing kerberos sidecar (``kerberos.enabled=true``, +``kerberos.keytab=/etc/airflow.keytab``, +``extraSecrets.<release>-kerberos-keytab: {}``). + +.. warning:: + + The KDC pod uses a fixed admin password and stores its database in + an ``emptyDir``. Do not connect production workloads to it. Treat it + as a test fixture only. + +Prerequisites +------------- + +* The Airflow chart installed in the same namespace (any executor). +* ``kubectl`` access sufficient to apply Deployments, Services, + ConfigMaps, Secrets, ServiceAccounts/Roles/RoleBindings, and Jobs in + that namespace. + +Usage +----- + +Reference this overlay from your own kustomization and substitute the +release name and namespace. A minimal example: + +.. code-block:: yaml + + # my-overlay/kustomization.yaml + apiVersion: kustomize.config.k8s.io/v1beta1 + kind: Kustomization + namespace: airflow + + resources: + - github.com/apache/airflow/chart/kustomize-overlays/kerberos?ref=helm-chart/1.22.0 + +Apply with: + +.. code-block:: bash + + kubectl apply -k my-overlay/ + +For a quick test, you can also just substitute the placeholders inline: + +.. code-block:: bash + + kubectl kustomize chart/kustomize-overlays/kerberos | \ + sed -e 's/RELEASE-NAME/airflow/g' -e 's/NAMESPACE/airflow/g' | \ + kubectl apply -n airflow -f - + +This is exactly what ``breeze k8s smoke-test-overlay kerberos`` does +during the local and CI smoke test. + +Wiring the keytab into the chart's sidecar +------------------------------------------ + +The chart's kerberos sidecar (``workers.celery.kerberosInitContainer`, +``workers.celery.kerberosSidecar``, ``workers.kubernetes.kerberosInitContainer`, ``workers.kubernetes.kerberosSidecar``) mounts a Secret named in +``kerberos.keytab``. Point that at the Secret produced by this overlay: + +.. code-block:: yaml + + # values.yaml fragment + kerberos: + enabled: true + ccacheMountPath: /var/kerberos-ccache + keytabPath: /etc/airflow.keytab + principal: airflow/[email protected] + + workers: + celery: + kerberosSidecar: + enabled: true + + extraSecrets: + airflow-kerberos-keytab: {} # exists from this overlay + +Migration guide from the chart +------------------------------ + +What the chart currently does Review Comment: We need to remember to change this to past tense (or shall we now?) once the Krb feature is removed from chart. ########## chart/kustomize-overlays/kerberos/STATUS.yaml: ########## @@ -0,0 +1,47 @@ +# 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. + +# The verify: block below is the smoke-test contract consumed by +# `breeze k8s smoke-test-overlay kerberos`. Run that command locally +# with --promote-status to refresh chart-version + last-verified +# automatically after a successful run; CI re-runs the smoke test on +# every PR but never mutates this file (CI=true refuses +# --promote-status). +--- +status: tested +chart-version: 1.22.0 +last-verified: '2026-05-15' Review Comment: Is this not a "moving target"? When is the date updated? As alternative would it be maybe possible to change this (or in a README) to a Github status like we have in central readme: `https://github.com/apache/airflow/actions/workflows/ci-amd.yml/badge.svg`: [ ########## chart/kustomize-overlays/CONTRIBUTING.rst: ########## @@ -95,6 +95,90 @@ For an overlay scheduled for removal: status: deprecated message: "Replaced by <overlay-name>. Will be removed in chart 3.0.0." +The optional ``verify:`` block is the smoke-test contract and is also +**the discovery key for CI**: + +.. code-block:: yaml + + verify: + timeout_seconds: 300 # optional; default 300, max 3600 + # `name` is the SUFFIX only - the runner auto-prepends + # `<release-name>-` so the same overlay works under any release. + # Write `foo`, not `RELEASE-NAME-foo`. + resources: + - kind: Deployment + name: foo # -> matches <release-name>-foo + ready: true # waits for rollout to complete + - kind: Job + name: bootstrap + complete: true # waits for condition=complete + - kind: Secret + name: foo # neither flag = waits for create + +How discovery works: + +* ``SelectiveChecks.kustomize_overlay_names`` scans + ``chart/kustomize-overlays/*/STATUS.yaml`` at CI time and emits the + list of overlay directory names whose ``STATUS.yaml`` contains a + ``verify:`` block. An overlay **without** a ``verify:`` block is + invisible to CI - the smoke-test workflow's matrix never sees it, + and the workflow is skipped entirely when the list is empty. +* The same workflow is gated by + ``SelectiveChecks.run_kustomize_overlays_tests``, which only trips + on changes under ``chart/kustomize-overlays/`` and the narrow set + of files that drive the runner (the prek hook, the breeze command, + the workflow file, the chart templates files). Unrelated chart edits do + not pull in a 30-40 minute kind cluster spin-up. + +Practical rule: as soon as an overlay has a ``verify:`` block, CI +starts running its smoke test on every relevant change. Until then, +the prek hook's structural check is the only automation that touches +it. + +Where things live (quick reference) +----------------------------------- + +A declarative map of the moving parts in an overlay and its smoke test, +so authors can answer "where does X go?" without grepping. Everything +in this table is auto-wired by the framework once it sits in the right +place - there is no central registry to also update. + ++--------------------------+-----------------------------------------------------------------+ +| Thing | Where it lives | ++==========================+=================================================================+ +| Kubernetes resources the | ``chart/kustomize-overlays/<name>/*.yaml`` referenced from | +| overlay produces | the overlay's ``kustomization.yaml``. | ++--------------------------+-----------------------------------------------------------------+ +| Container images the | Inline ``image:`` fields on containers / initContainers / | +| overlay uses | sidecars in the overlay YAMLs above. **No second list.** | +| | ``breeze k8s smoke-test-overlay`` discovers them by walking | +| | the rendered manifest and ``docker pull`` + ``kind load``\ s | +| | each one before applying. Always pair with | +| | ``imagePullPolicy: IfNotPresent``. | ++--------------------------+-----------------------------------------------------------------+ +| Resources to wait for | ``verify:`` block in | +| after apply | ``chart/kustomize-overlays/<name>/STATUS.yaml`` (see "STATUS | +| | file format" above). The smoke-test runner walks this list. | ++--------------------------+-----------------------------------------------------------------+ +| Behavioural assertions | ``chart/tests/overlay_tests/test_<name>.py``| +| beyond "resource exists" | Auto-discovered by the smoke-test runner if present.| +| | Use the fixtures in the sibling ``conftest.py`` | Review Comment: nit: spacing ```suggestion +--------------------------+-----------------------------------------------------------------+ | Behavioural assertions | ``chart/tests/overlay_tests/test_<name>.py`` | | beyond "resource exists" | Auto-discovered by the smoke-test runner if present. | | | Use the fixtures in the sibling ``conftest.py`` | ``` ########## .github/workflows/ci-arm.yml: ########## Review Comment: Just thinking... K8s should be CPU neutral... shall we really test on ARM as well as AMD? Or would it not be sufficient testing x86 only? ########## chart/kustomize-overlays/CONTRIBUTING.rst: ########## @@ -95,6 +95,90 @@ For an overlay scheduled for removal: status: deprecated message: "Replaced by <overlay-name>. Will be removed in chart 3.0.0." +The optional ``verify:`` block is the smoke-test contract and is also +**the discovery key for CI**: + +.. code-block:: yaml + + verify: + timeout_seconds: 300 # optional; default 300, max 3600 + # `name` is the SUFFIX only - the runner auto-prepends + # `<release-name>-` so the same overlay works under any release. + # Write `foo`, not `RELEASE-NAME-foo`. + resources: + - kind: Deployment + name: foo # -> matches <release-name>-foo + ready: true # waits for rollout to complete + - kind: Job + name: bootstrap + complete: true # waits for condition=complete + - kind: Secret + name: foo # neither flag = waits for create + +How discovery works: + +* ``SelectiveChecks.kustomize_overlay_names`` scans + ``chart/kustomize-overlays/*/STATUS.yaml`` at CI time and emits the + list of overlay directory names whose ``STATUS.yaml`` contains a + ``verify:`` block. An overlay **without** a ``verify:`` block is + invisible to CI - the smoke-test workflow's matrix never sees it, + and the workflow is skipped entirely when the list is empty. +* The same workflow is gated by + ``SelectiveChecks.run_kustomize_overlays_tests``, which only trips + on changes under ``chart/kustomize-overlays/`` and the narrow set + of files that drive the runner (the prek hook, the breeze command, + the workflow file, the chart templates files). Unrelated chart edits do + not pull in a 30-40 minute kind cluster spin-up. + +Practical rule: as soon as an overlay has a ``verify:`` block, CI Review Comment: Do I see it right that in the GH Workflow above _all_ Kustomize layers in future will be applied in a single test stage? Would we extend this in future to be a matrix action (not this PR but later...) to run each Kustomize in a separate test stage? Else they all need to work together, which would be impossible to test HPA and KEDA in parallel. ########## dev/breeze/src/airflow_breeze/commands/kubernetes_commands.py: ########## Review Comment: Nit: This file is already very large, would it make sense to split utilities out in a separate module? ########## chart/kustomize-overlays/README.rst: ########## @@ -35,11 +35,14 @@ The motivation, criteria, and lifecycle for these overlays are defined in Available overlays ------------------ -+----------+----------------------+----------------------------------------------+ -| Overlay | STATUS | Purpose | -+==========+======================+==============================================+ -| ``keda`` | not-tested (PoC) | Autoscaling for Celery workers via KEDA. | -+----------+----------------------+----------------------------------------------+ ++--------------+----------------------+----------------------------------------------+ +| Overlay | STATUS | Purpose | ++==============+======================+==============================================+ +| ``keda`` | not-tested (PoC) | Autoscaling for Celery workers via KEDA. | ++--------------+----------------------+----------------------------------------------+ +| ``kerberos`` | tested | In-cluster test KDC + keytab Secret bootstrap| Review Comment: Ah, adding a status badge from CI would be fitting here! -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
