Skip to content

Validate Dag and task IDs in the Go SDK#69965

Open
Andrushika wants to merge 2 commits into
apache:mainfrom
Andrushika:go-sdk-validate-ids
Open

Validate Dag and task IDs in the Go SDK#69965
Andrushika wants to merge 2 commits into
apache:mainfrom
Andrushika:go-sdk-validate-ids

Conversation

@Andrushika

@Andrushika Andrushika commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Validate Dag and task IDs in the Go SDK

Why

Go SDK currently accepts any string as Dag/task ID. Python SDK already validates this with validate_key (max 250 chars, only letters, digits, -, ., _), and ts-sdk added the same check in #69400.

What

Adds the same validation to the Go SDK, so invalid IDs fail early at registration.
related: #69400, #69937

Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

Generated-by: Claude Code Opus 4.8 following the guidelines


  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

Comment thread go-sdk/bundle/bundlev1/registry_test.go Outdated
@viiccwen

Copy link
Copy Markdown
Contributor

PR description says "Adds the same validation to the Java SDK".
It's Go SDK, right?

@Andrushika

Copy link
Copy Markdown
Contributor Author

PR description says "Adds the same validation to the Java SDK". It's Go SDK, right?

You’re right, already fixed it. Thanks!

@Andrushika
Andrushika force-pushed the go-sdk-validate-ids branch from 1ce3197 to d2b2962 Compare July 16, 2026 17:50

@jason810496 jason810496 left a comment

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.

LGTM overall, thanks.


UPDATE:

As mentioned in #69937 (review), we will need to wait for the coordinator side first.

Comment thread go-sdk/bundle/bundlev1/registry.go Outdated
Comment thread go-sdk/bundle/bundlev1/registry.go
@potiuk

potiuk commented Jul 20, 2026

Copy link
Copy Markdown
Member

@Andrushika — There are 2 unresolved review thread(s) on this PR from @jason810496. Could you either push a fix or reply in each thread explaining why the feedback doesn't apply? Once you believe the feedback is addressed, mark the thread as resolved so the reviewer isn't re-pinged needlessly. Thanks!


Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you.

@jason810496 jason810496 left a comment

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.

Hi @Andrushika,
After revisiting this, I'd like to make this best-effort build time validation instead of task runtime validation or dag processing side validation for both Go and Java SDK (You can raise PR for TS SDK as well if you like! or I can clean them up as well).

The reason is that the following snippet will validate the task runtime in server side and be served as source of truth for validation.

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}'"
)

For the a..b case, we can just raise the warning (since we can't trust the env / config at client side).
Please let me know if there's other direction that makes more sense to you.

Thanks.

@Andrushika

Copy link
Copy Markdown
Contributor Author

Got it, thanks for revisiting! I will move the validation into build time as you described. Would be happy to also clean up ts-sdk after current work is done.

@Andrushika

Andrushika commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

@jason810496 A small question to clarify: since we are going to validate the IDs in build time, I am planning to implement it in runPack() (the function behind the go tool airflow-go-pack).

Since you mentioned we should keep the validation best-effort, I want to confirm how strict the pack step should be. The .. case depends on [core] allow_double_dot_in_ids and can be customed by users on the server side, so the lang-sdk can only warn. But the length and charset rules are rejected by the server unconditionally, so failing early seems possible for them.

I have two options and would like to ask your opinion:

  • Reject the pack when the id is invalid, except the .. case, which only gives a warning
  • Still allow the pack even if some id is invalid, give warnings in all the invalid cases

Which one do you prefer? Thanks!
I would prefer to keep all of them as warnings and still allow building, because it can prevent drifting that TP mentioned.

@jason810496 jason810496 left a comment

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.

I would prefer to keep all of them as warnings and still allow building, because it can prevent drifting that TP mentioned.

Sure, let's go with returning warning for all the cases. Thanks! I don't have strong opinion on keeping it strict regarding the validation since it's best-effort anyway.

@Andrushika
Andrushika force-pushed the go-sdk-validate-ids branch from d2b2962 to d177572 Compare July 24, 2026 11:44
@Andrushika

Copy link
Copy Markdown
Contributor Author

All tests passed, ready for review!

The reason is that the following snippet will validate the task runtime in server side and be served as source of truth for validation.

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}'"
)

For the a..b case, we can just raise the warning (since we can't trust the env / config at client side). Please let me know if there's other direction that makes more sense to you.

A little fact I found: On the current main, only run_id checks the a..b case. Core and task-sdk do not validate dag_id and task_id in the case of double dots.

I guess we should also handle this in the core? If it is, I would be happy to open a follow-up PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants