From d177572ecf6b114d3e97ee77c09aca593335b641 Mon Sep 17 00:00:00 2001 From: Andrew Chang Date: Fri, 24 Jul 2026 19:43:46 +0800 Subject: [PATCH 1/2] Warn on suspicious Dag and task IDs at Go bundle pack time --- go-sdk/cmd/airflow-go-pack/pack.go | 1 + go-sdk/cmd/airflow-go-pack/validate.go | 74 ++++++++++++++ go-sdk/cmd/airflow-go-pack/validate_test.go | 105 ++++++++++++++++++++ 3 files changed, 180 insertions(+) create mode 100644 go-sdk/cmd/airflow-go-pack/validate.go create mode 100644 go-sdk/cmd/airflow-go-pack/validate_test.go diff --git a/go-sdk/cmd/airflow-go-pack/pack.go b/go-sdk/cmd/airflow-go-pack/pack.go index 8441b23569be3..f4be6fa8c4578 100644 --- a/go-sdk/cmd/airflow-go-pack/pack.go +++ b/go-sdk/cmd/airflow-go-pack/pack.go @@ -171,6 +171,7 @@ func runPack(stdout, stderr io.Writer, opts *packOptions) error { fmt.Fprintf(stderr, "warning: dag %q has no tasks\n", dagID) } } + warnOnSuspiciousIDs(stderr, meta) manifest, err := renderManifest(meta, filepath.Base(sourcePath)) if err != nil { diff --git a/go-sdk/cmd/airflow-go-pack/validate.go b/go-sdk/cmd/airflow-go-pack/validate.go new file mode 100644 index 0000000000000..b63a6b9dcf96a --- /dev/null +++ b/go-sdk/cmd/airflow-go-pack/validate.go @@ -0,0 +1,74 @@ +// 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. + +package main + +import ( + "fmt" + "io" + "regexp" + "sort" + "strings" + "unicode/utf8" + + "github.com/apache/airflow/go-sdk/internal/airflowmetadata" +) + +const maxIDLength = 250 + +var idRegex = regexp.MustCompile(`^[\p{L}\p{N}_.-]+$`) + +// warnOnSuspiciousIDs checks every dag and task id in the manifest against the +// rules the Airflow server enforces (airflow.utils.helpers.validate_key). It is +// best-effort and only warns: the server validates authoritatively, and checks +// like the '..' one depend on server configuration the packer cannot see. +func warnOnSuspiciousIDs(stderr io.Writer, meta airflowmetadata.Manifest) { + dagIDs := make([]string, 0, len(meta.Dags)) + for id := range meta.Dags { + dagIDs = append(dagIDs, id) + } + sort.Strings(dagIDs) + for _, dagID := range dagIDs { + warnOnSuspiciousID(stderr, fmt.Sprintf("dag id %q", dagID), dagID) + for _, taskID := range meta.Dags[dagID].Tasks { + warnOnSuspiciousID(stderr, fmt.Sprintf("task id %q in dag %q", taskID, dagID), taskID) + } + } +} + +func warnOnSuspiciousID(stderr io.Writer, label, id string) { + if length := utf8.RuneCountInString(id); length > maxIDLength { + fmt.Fprintf( + stderr, + "warning: %s is longer than %d characters (%d); the Airflow server will reject it\n", + label, maxIDLength, length, + ) + } + if !idRegex.MatchString(id) { + fmt.Fprintf( + stderr, + "warning: %s must be made of alphanumeric characters, dashes, dots, and underscores; the Airflow server will reject it\n", + label, + ) + } else if strings.Contains(id, "..") { + fmt.Fprintf( + stderr, + "warning: %s contains '..'; the Airflow server will reject it unless [core] allow_double_dot_in_ids is enabled\n", + label, + ) + } +} diff --git a/go-sdk/cmd/airflow-go-pack/validate_test.go b/go-sdk/cmd/airflow-go-pack/validate_test.go new file mode 100644 index 0000000000000..2e4e3840452f3 --- /dev/null +++ b/go-sdk/cmd/airflow-go-pack/validate_test.go @@ -0,0 +1,105 @@ +// 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. + +package main + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/apache/airflow/go-sdk/internal/airflowmetadata" +) + +func warningsFor(dags map[string]airflowmetadata.Dag) string { + var buf bytes.Buffer + warnOnSuspiciousIDs(&buf, airflowmetadata.Manifest{Dags: dags}) + return buf.String() +} + +func TestWarnOnSuspiciousIDs_ValidIdsProduceNoWarnings(t *testing.T) { + for _, id := range []string{ + "simple", "with-dash", "with.dot", "with_underscore", "0numeric", + "café_dag", "任務", strings.Repeat("a", 250), strings.Repeat("任", 250), + } { + out := warningsFor(map[string]airflowmetadata.Dag{id: {Tasks: []string{id}}}) + assert.Empty(t, out, "id %q should not warn", id) + } +} + +func TestWarnOnSuspiciousIDs_TooLongIDWarns(t *testing.T) { + for _, id := range []string{strings.Repeat("a", 251), strings.Repeat("任", 251)} { + out := warningsFor(map[string]airflowmetadata.Dag{id: {}}) + assert.Contains(t, out, "is longer than 250 characters (251)") + } +} + +func TestWarnOnSuspiciousIDs_InvalidCharsWarn(t *testing.T) { + for _, id := range []string{"", "with space", "with/slash", "with:colon", "with\ttab"} { + out := warningsFor(map[string]airflowmetadata.Dag{id: {}}) + assert.Contains( + t, out, + "must be made of alphanumeric characters, dashes, dots, and underscores", + "id %q should warn", id, + ) + } +} + +func TestWarnOnSuspiciousIDs_DoubleDotWarns(t *testing.T) { + out := warningsFor(map[string]airflowmetadata.Dag{"a..b": {}}) + assert.Contains(t, out, `dag id "a..b" contains '..'`) + assert.Contains(t, out, "allow_double_dot_in_ids") + assert.NotContains(t, out, "must be made of alphanumeric characters") +} + +func TestWarnOnSuspiciousIDs_TaskWarningNamesItsDag(t *testing.T) { + out := warningsFor(map[string]airflowmetadata.Dag{"my_dag": {Tasks: []string{"bad task"}}}) + assert.Contains(t, out, `task id "bad task" in dag "my_dag" must be made of`) +} + +// Packing succeeds despite a suspicious dag id: the check is best-effort and +// the server-side validation stays the source of truth. +func TestRunPack_WarnsOnSuspiciousIDsButPacks(t *testing.T) { + dir := t.TempDir() + exe := filepath.Join(dir, "foreign") + require.NoError(t, os.WriteFile(exe, []byte("foreign-arch-binary-bytes"), 0o755)) + source := filepath.Join(dir, "main.go") + require.NoError(t, os.WriteFile(source, []byte("package main\nfunc main() {}\n"), 0o644)) + meta := filepath.Join(dir, "airflow-metadata.json") + require.NoError(t, os.WriteFile(meta, []byte( + `{"airflow_bundle_metadata_version":"1.0",`+ + `"sdk":{"language":"go","version":"0.1.0","supervisor_schema_version":"2026-06-16"},`+ + `"dags":{"bad dag":{"tasks":["t1"]}}}`, + ), 0o644)) + out := filepath.Join(dir, "bundle") + + var stderr bytes.Buffer + err := runPack(&bytes.Buffer{}, &stderr, &packOptions{ + executable: exe, + source: source, + airflowMetadata: meta, + output: out, + }) + require.NoError(t, err) + assert.Contains(t, stderr.String(), `warning: dag id "bad dag" must be made of`) + assert.FileExists(t, out) +} From 3bad528a72737b31fe8ca22a16fda98f663c7710 Mon Sep 17 00:00:00 2001 From: Andrew Chang Date: Fri, 24 Jul 2026 21:31:24 +0800 Subject: [PATCH 2/2] Lock pack-time ID warning output with exact assertions --- go-sdk/cmd/airflow-go-pack/validate.go | 1 + go-sdk/cmd/airflow-go-pack/validate_test.go | 59 +++++++++++++++++---- 2 files changed, 49 insertions(+), 11 deletions(-) diff --git a/go-sdk/cmd/airflow-go-pack/validate.go b/go-sdk/cmd/airflow-go-pack/validate.go index b63a6b9dcf96a..2c9a40a726405 100644 --- a/go-sdk/cmd/airflow-go-pack/validate.go +++ b/go-sdk/cmd/airflow-go-pack/validate.go @@ -41,6 +41,7 @@ func warnOnSuspiciousIDs(stderr io.Writer, meta airflowmetadata.Manifest) { for id := range meta.Dags { dagIDs = append(dagIDs, id) } + // Map iteration order is random; sort so warnings print in a stable order. sort.Strings(dagIDs) for _, dagID := range dagIDs { warnOnSuspiciousID(stderr, fmt.Sprintf("dag id %q", dagID), dagID) diff --git a/go-sdk/cmd/airflow-go-pack/validate_test.go b/go-sdk/cmd/airflow-go-pack/validate_test.go index 2e4e3840452f3..4aa785ec64298 100644 --- a/go-sdk/cmd/airflow-go-pack/validate_test.go +++ b/go-sdk/cmd/airflow-go-pack/validate_test.go @@ -19,6 +19,7 @@ package main import ( "bytes" + "fmt" "os" "path/filepath" "strings" @@ -36,6 +37,13 @@ func warningsFor(dags map[string]airflowmetadata.Dag) string { return buf.String() } +func dagCharsetWarning(id string) string { + return fmt.Sprintf( + "warning: dag id %q must be made of alphanumeric characters, dashes, dots, and underscores; the Airflow server will reject it\n", + id, + ) +} + func TestWarnOnSuspiciousIDs_ValidIdsProduceNoWarnings(t *testing.T) { for _, id := range []string{ "simple", "with-dash", "with.dot", "with_underscore", "0numeric", @@ -49,31 +57,60 @@ func TestWarnOnSuspiciousIDs_ValidIdsProduceNoWarnings(t *testing.T) { func TestWarnOnSuspiciousIDs_TooLongIDWarns(t *testing.T) { for _, id := range []string{strings.Repeat("a", 251), strings.Repeat("任", 251)} { out := warningsFor(map[string]airflowmetadata.Dag{id: {}}) - assert.Contains(t, out, "is longer than 250 characters (251)") + expected := fmt.Sprintf( + "warning: dag id %q is longer than 250 characters (251); the Airflow server will reject it\n", + id, + ) + assert.Equal(t, expected, out, "id %q", id) } } func TestWarnOnSuspiciousIDs_InvalidCharsWarn(t *testing.T) { - for _, id := range []string{"", "with space", "with/slash", "with:colon", "with\ttab"} { + // "a..b c" also locks the else-if: a charset failure suppresses the '..' warning. + for _, id := range []string{"", "with space", "with/slash", "with:colon", "with\ttab", "a..b c"} { out := warningsFor(map[string]airflowmetadata.Dag{id: {}}) - assert.Contains( - t, out, - "must be made of alphanumeric characters, dashes, dots, and underscores", - "id %q should warn", id, - ) + assert.Equal(t, dagCharsetWarning(id), out, "id %q", id) } } func TestWarnOnSuspiciousIDs_DoubleDotWarns(t *testing.T) { out := warningsFor(map[string]airflowmetadata.Dag{"a..b": {}}) - assert.Contains(t, out, `dag id "a..b" contains '..'`) - assert.Contains(t, out, "allow_double_dot_in_ids") - assert.NotContains(t, out, "must be made of alphanumeric characters") + assert.Equal( + t, + "warning: dag id \"a..b\" contains '..'; the Airflow server will reject it unless [core] allow_double_dot_in_ids is enabled\n", + out, + ) +} + +func TestWarnOnSuspiciousIDs_TooLongInvalidIDGetsBothWarnings(t *testing.T) { + id := strings.Repeat("a", 250) + " b" + out := warningsFor(map[string]airflowmetadata.Dag{id: {}}) + expected := fmt.Sprintf( + "warning: dag id %q is longer than 250 characters (252); the Airflow server will reject it\n", + id, + ) + dagCharsetWarning(id) + assert.Equal(t, expected, out) +} + +func TestWarnOnSuspiciousIDs_SortsDagIDsForStableOutput(t *testing.T) { + out := warningsFor(map[string]airflowmetadata.Dag{ + "delta d": {}, + "alpha d": {}, + "charlie d": {}, + "bravo d": {}, + }) + expected := dagCharsetWarning("alpha d") + dagCharsetWarning("bravo d") + + dagCharsetWarning("charlie d") + dagCharsetWarning("delta d") + assert.Equal(t, expected, out) } func TestWarnOnSuspiciousIDs_TaskWarningNamesItsDag(t *testing.T) { out := warningsFor(map[string]airflowmetadata.Dag{"my_dag": {Tasks: []string{"bad task"}}}) - assert.Contains(t, out, `task id "bad task" in dag "my_dag" must be made of`) + assert.Equal( + t, + "warning: task id \"bad task\" in dag \"my_dag\" must be made of alphanumeric characters, dashes, dots, and underscores; the Airflow server will reject it\n", + out, + ) } // Packing succeeds despite a suspicious dag id: the check is best-effort and