Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 32 additions & 3 deletions go-sdk/bundle/bundlev1/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,34 @@ package bundlev1
import (
"fmt"
"reflect"
"regexp"
"runtime"
"strings"
"sync"
"unicode/utf8"

"github.com/apache/airflow/go-sdk/pkg/worker"
)

const maxKeyLength = 250

var keyRegex = regexp.MustCompile(`^[\p{L}\p{N}_.-]+$`)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, the a..b case can pass the validation. Perhaps it's worthwhile to add the unit test to exercise. Thanks.

if ".." in run_id and not airflow_conf.getboolean("core", "allow_double_dot_in_ids", fallback=False):
raise ValueError(f"The run_id '{run_id}' must not contain '..' to prevent path traversal")
# This is also done on the DagRun model class, but SQLAlchemy column
# validator does not work well for some reason.
if not re.match(RUN_ID_REGEX, run_id):
regex = airflow_conf.get("scheduler", "allowed_run_id_pattern").strip()
if not regex or not re.match(regex, run_id):
raise ValueError(
f"The run_id provided '{run_id}' does not match regex pattern "
f"'{regex}' or '{RUN_ID_REGEX}'"
)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I may be wrong, but the linked code checks run_id, while this PR checks Dag and task IDs. Should the validation of run_id and Dags, task IDs align? If so, I will open a follow-up PR to fix this in task SDK.

Since the PR is on hold, should I wait for the coordinator's design before making this change?


func validateKey(name, value string) {
if length := utf8.RuneCountInString(value); length > maxKeyLength {
panic(fmt.Errorf("%s must be less than %d characters, not %d", name, maxKeyLength, length))
}
if !keyRegex.MatchString(value) {
panic(
fmt.Errorf(
"%s %q must be made of alphanumeric characters, dashes, dots, and underscores",
name,
value,
),
)
}
}

type (
// Task is one registered task: something the runtime can Execute. Bundle
// authors do not implement this directly; Dag.AddTask wraps a plain Go
Expand Down Expand Up @@ -60,7 +81,9 @@ type (
// AddTaskWithName is like AddTask but sets task_id explicitly instead of
// deriving it from the function name. Use it when the Go function name
// cannot match the Python @task.stub id, for example for an anonymous
// function or a differing name.
// function or a differing name. The task_id must be non-empty, at most
// 250 characters, and made of alphanumeric characters, dashes, dots,
// and underscores; an invalid id panics.
AddTaskWithName(taskId string, fn any)
}

Expand All @@ -70,8 +93,10 @@ type (
Registry interface {
Bundle
// AddDag registers a dag by its dag_id (matching the Python stub dag)
// and returns a Dag handle for attaching tasks. Registering the same
// dag_id twice panics.
// and returns a Dag handle for attaching tasks. The dag_id must be
// non-empty, at most 250 characters, and made of alphanumeric
// characters, dashes, dots, and underscores. An invalid or duplicate
// dag_id panics.
AddDag(dagId string) Dag
}

Expand Down Expand Up @@ -135,6 +160,8 @@ func getFnName(fn reflect.Value) string {
}

func (r *registry) AddDag(dagId string) Dag {
validateKey("Dag ID", dagId)

r.Lock()
defer r.Unlock()

Expand All @@ -159,6 +186,8 @@ func (r *registry) registerTask(dagId string, fn any) {
}

func (r *registry) registerTaskWithName(dagId, taskId string, fn any) {
validateKey("Task ID", taskId)

task, err := NewTaskFunction(fn)
if err != nil {
panic(fmt.Errorf("error registering task %q for DAG %q: %w", taskId, dagId, err))
Expand Down
51 changes: 51 additions & 0 deletions go-sdk/bundle/bundlev1/registry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ package bundlev1
import (
"context"
"errors"
"fmt"
"log/slog"
"strings"
"testing"

"github.com/stretchr/testify/suite"
Expand Down Expand Up @@ -66,6 +68,55 @@ func (s *RegistrySuite) TestAddDag_DuplicatePanics() {
})
}

func (s *RegistrySuite) TestAddDag_AcceptsValidIds() {
reg := New()
for _, id := range []string{"simple", "with-dash", "with.dot", "with_underscore", "0numeric", "café_dag", "任務", strings.Repeat("a", 250), strings.Repeat("任", 250)} {
s.NotPanics(func() { reg.AddDag(id) })
}
}

func (s *RegistrySuite) TestAddDag_InvalidCharsPanic() {
for _, id := range []string{"", "with space", "with/slash", "with:colon", "with\ttab"} {
s.PanicsWithError(
fmt.Sprintf(
"Dag ID %q must be made of alphanumeric characters, dashes, dots, and underscores",
id,
),
func() { New().AddDag(id) },
)
}
}

func (s *RegistrySuite) TestAddDag_TooLongPanics() {
s.PanicsWithError("Dag ID must be less than 250 characters, not 251", func() {
New().AddDag(strings.Repeat("a", 251))
})
}
Comment thread
Andrushika marked this conversation as resolved.

func (s *RegistrySuite) TestAddTask_InvalidCharsPanic() {
for _, id := range []string{"", "with space", "with/slash", "with:colon", "with\ttab"} {
s.PanicsWithError(
fmt.Sprintf(
"Task ID %q must be made of alphanumeric characters, dashes, dots, and underscores",
id,
),
func() { s.dag.AddTaskWithName(id, myTask) },
)
}
}

func (s *RegistrySuite) TestAddTask_AcceptsValidIds() {
for _, id := range []string{"simple", "with-dash", "with.dot", "with_underscore", "0numeric", "café_dag", "任務", strings.Repeat("a", 250), strings.Repeat("任", 250)} {
s.NotPanics(func() { s.dag.AddTaskWithName(id, myTask) })
}
}

func (s *RegistrySuite) TestAddTask_TooLongPanics() {
s.PanicsWithError("Task ID must be less than 250 characters, not 251", func() {
s.dag.AddTaskWithName(strings.Repeat("a", 251), myTask)
})
}

func (s *RegistrySuite) TestAddTask_RegistersAndFindsTask() {
s.dag.AddTask(myTask)
task, exists := s.reg.LookupTask("dag1", "myTask")
Expand Down