fix(backend): validate pipeline names in K8s store before API call

[Priyanshu-u07]

Description of your changes:

Adds early pipeline name validation in pipeline_store_kubernetes.go before k.client.Create() is called. This catches invalid names (uppercase, spaces, etc.) with a clear error message before any round-trip to the Kubernetes API server.

Changes:

• Added ValidatePipelineName() in CreatePipeline() and createPipelineVersionWithPipeline() before the K8s API call
• Error message guides users to use display_name for human-readable labels
• Updated K8s store test names to use DNS-compliant format
• Added tests for invalid pipeline and pipeline version names

This fix only affects the Kubernetes backend. The SQL backend is unchanged

Fixes #13111 .

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign mprahl for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• backend/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[google-oss-prow]

Hi @Priyanshu-u07. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Copilot]

Pull request overview

This PR adds early pipeline and pipeline-version name validation in the Kubernetes pipeline store to fail fast (before calling the Kubernetes API server) when user-provided names are invalid, and updates/extends unit tests accordingly.

Changes:

• Validate pipeline names in CreatePipeline() before k.client.Create().
• Validate pipeline version names in createPipelineVersionWithPipeline() before k.client.Create().
• Update Kubernetes store tests to use DNS-compliant names and add negative tests for invalid names.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
backend/src/apiserver/storage/pipeline_store_kubernetes.go	Adds pre-flight name validation for pipelines and pipeline versions before Kubernetes API calls.
backend/src/apiserver/storage/pipeline_store_kubernetes_test.go	Updates existing tests to use lowercase/dash names and adds new tests for invalid names.

[jeffspahr]

I reviewed the current PR diff against origin/master and found two remaining gaps.

1. HTTP upload still returns generic 500s for invalid names.
   backend/src/apiserver/server/pipeline_upload_server.go:181-189 and 322-339 still collapse every non-conflict error into a generic "Failed to create ..." response. The existing upload validation test at backend/src/apiserver/server/pipeline_upload_server_test.go:235-250 still expects that generic body. That means the upload endpoint called out in [backend] user-provided names bypass ValidatePipelineName() in Kubernetes backend #13111 still does not return the clear validation error this PR is trying to add.

2. Pipeline-version creation still lacks the new preflight Kubernetes-name check.
   backend/src/apiserver/storage/pipeline_store_kubernetes.go:238-245 now validates pipeline names before Create, but backend/src/apiserver/storage/pipeline_store_kubernetes.go:659-668 still sends pipeline versions straight to k.client.Create(...). So CreatePipelineVersion requests can still fail only after the Kubernetes API round-trip for invalid version names. The updated tests only add a negative case for pipelines; backend/src/apiserver/storage/pipeline_store_kubernetes_test.go:274-290 is still happy-path only for versions.

Verification I ran locally:

• go test ./backend/src/apiserver/storage -run 'TestCreateK8sPipeline|TestCreateK8sPipelineVersion|TestCreatePipelineAndPipelineVersion'
• go test ./backend/src/apiserver/server -run TestUploadPipelineV2_NameValidation

Both passed.

[hbelmiro]

/ok-to-test

[hbelmiro]

That's weird. Tests were not triggered.
@Priyanshu-u07 can you please rebase or send an empty commit?

[hbelmiro]

/hold

[Priyanshu-u07]

Thanks @jeffspahr for review! I've addressed both gaps as upload server now surfaces validation errors instead of generic responses and pipeline version creation has preflight DNS check. Both verification commands pass locally.

[Priyanshu-u07]

Thanks @hbelmiro! Could you please /unhold?

[hbelmiro]

/unhold

[jeffspahr]

Two issues remain on the current head.

1. The new upload BadRequest path now passes raw *util.UserError values into writeErrorToResponse, so clients receive the internal wrapped message/details instead of the clean external validation message this PR is trying to surface.
2. CreatePipelineAndPipelineVersion is still not atomic when the pipeline-version name is invalid. The combined create path persists the Pipeline before the new version-name validation runs, so a failed combined create can still leave an orphaned pipeline behind.

Targeted verification on 138da32c0e000bf932708476c496b96312537a6d:

• go test ./backend/src/apiserver/storage -run 'TestCreateK8sPipeline|TestCreateK8sPipelineVersion|TestCreatePipelineAndPipelineVersion' -count=1
• go test ./backend/src/apiserver/server -run 'TestUploadPipelineV2_NameValidation|TestUploadPipeline_NameAndNamespaceTooLong' -count=1

[jeffspahr]

Passing the raw *util.UserError into writeErrorToResponse leaks the internal wrapped message/details here. That helper serializes err.Error() and %+v, so clients now see strings like Failed to create ...: Invalid input error: ... instead of just the external validation message. Please unwrap UserError to ExternalMessage() before writing the HTTP response, or make the helper do that centrally.

[Priyanshu-u07]

Done, now unwrapping with ExternalMessage() before writing the HTTP response.

[jeffspahr]

This validation still runs too late for the combined create path. CreatePipelineAndPipelineVersion() creates the Pipeline first, then calls createPipelineVersionWithPipeline(), so a valid pipeline name plus invalid version name still returns an error after the Pipeline has already been persisted. Please validate both names before creating either resource, or delete the Pipeline on version-create failure.

[Priyanshu-u07]

Done, now validate both names before creating either resource.

[hbelmiro]

/retest

[jeffspahr]

Re-reviewed the current head and the two previously reported correctness issues are fixed:

• the upload handlers now unwrap ExternalMessage() before returning HTTP 400s
• CreatePipelineAndPipelineVersion() now validates the pipeline-version name before persisting the pipeline, so the combined create path is atomic again for invalid version names

I did not find any new blocking behavior issues on the current diff.

The remaining gap is test coverage. The new invalid-name handling for /pipelines/upload_version still does not have a regression test. TestUploadPipelineV2_NameValidation covers the pipeline upload path, and the existing version-upload tests cover file/length failures, but there is not yet a focused test that proves an invalid version name comes back as the intended 400 with the surfaced validation message.

Please add that upload-version invalid-name test before merge so this HTTP path is covered as well.

Verification I ran locally:

• go test ./backend/src/apiserver/storage -run 'TestCreateK8sPipeline|TestCreateK8sPipelineVersion|TestCreatePipelineAndPipelineVersion' -count=1
• go test ./backend/src/apiserver/server -run 'TestUploadPipelineV2_NameValidation|TestUploadPipeline_NameAndNamespaceTooLong' -count=1
• go test ./backend/src/apiserver/server -run 'TestUploadPipelineVersion' -count=1

[Priyanshu-u07]

@jeffspahr, Added the requested regression test TestUploadPipelineVersion_InvalidName and verified that it correctly handles version uploads with invalid names by returning a clean 400 response.

All verification are passing:

• go test ./backend/src/apiserver/storage -run 'TestCreateK8sPipeline|TestCreateK8sPipelineVersion|TestCreatePipelineAndPipelineVersion' -count=1
• go test ./backend/src/apiserver/server -run 'TestUploadPipelineV2_NameValidation|TestUploadPipeline_NameAndNamespaceTooLong' -count=1
• go test ./backend/src/apiserver/server -run 'TestUploadPipelineVersion' -count=1
• go test ./backend/src/apiserver/server/ -run "TestUploadPipelineVersion_InvalidName" -count=1 -v

[jeffspahr]

Re-reviewed the current head and I did not find any remaining blocking correctness issues in this diff.

The earlier gaps are fixed on 3747302b931283f76f4f70465a2fb66344a70795:

• the upload handlers now unwrap ExternalMessage() before returning HTTP 400 validation responses
• CreatePipelineAndPipelineVersion() now validates the pipeline-version name before persisting the pipeline, so the combined create path is atomic again for invalid version names
• the /pipelines/upload_version invalid-name path now has focused regression coverage via TestUploadPipelineVersion_InvalidName

Verification I ran locally:

• go test ./backend/src/apiserver/storage -run 'TestCreateK8sPipeline|TestCreateK8sPipelineVersion|TestCreatePipelineAndPipelineVersion' -count=1
• go test ./backend/src/apiserver/server -run 'TestUploadPipelineV2_NameValidation|TestUploadPipeline_NameAndNamespaceTooLong|TestUploadPipelineVersion|TestUploadPipelineVersion_InvalidName' -count=1

Residual risk is low and limited to the broader unrun backend/integration suites.

[google-oss-prow]

New changes are detected. LGTM label has been removed.

[Priyanshu-u07]

/retest