Skip signing a download URL when no artifacts can exist yet

[Ralthos]

Thank you for contributing to Velero!

Please add a summary of your change

Follows #10232, where the answer was "let's do free first". This is the free half: the controller already has the backup, and the restore for restore targets, in hand before it signs, so checking the phase adds no call to the object store.

A DownloadRequest whose target never ran now stops at New with no URL, so nothing hands a caller a link that 404s.

Does your change fix a particular issue?

Refs #10232.

Please indicate you've done the following:

Three things worth flagging. Two of them are decisions, not mechanics.

The guard is narrower than the CLI's allowlist, on purpose. pkg/cmd/cli/backup/logs.go permits five phases and refuses the rest, and reusing that list in the controller was my first instinct. It would have been wrong. velero backup download has no client side phase check at all, so a controller side guard built on the log allowlist would have changed what backup download can fetch, and the phases in which a tarball is available are not the phases in which a log is.

So this refuses only the pre-execution phases, where nothing exists for any target kind:

	refused
Backup	New, Queued, ReadyToStart, FailedValidation
Restore	New, FailedValidation

InProgress onwards may hold a partial log or other artifacts, and Deleting may still hold all of them, so those sign exactly as they do today. The only requests that change behaviour are ones that could never have succeeded.

An empty phase is left alone. A backup that has not been reconciled yet has phase: "", and that state is transient, so blocking it would trade a 404 for a spurious refusal during a race.

Where the check lives. In the controller, next to where the backup and restore are already loaded. GetDownloadURL in pkg/persistence has the target kind but not the object, so putting it there would mean fetching the backup a second time and would stop being free.

On testing: backupPhaseHasNoArtifacts and restorePhaseHasNoArtifacts are covered exhaustively, one case per phase the API defines, plus a guard test that fails if a phase is added later without updating either table.

I could not run the TestAPIs envtest suite locally, since it needs control plane binaries I do not have on this machine. It fails the same way on a clean checkout of main here, so I do not believe this change affects it, but CI is the real check.

• Added a changelog
• go test ./pkg/controller/ -run 'TestBackupPhaseHasNoArtifacts|TestRestorePhaseHasNoArtifacts|TestPhaseTablesCoverAllPhases' passes, 26 subtests
• gofmt -s clean
• Signed off the commit

[netlify]

👷 Deploy request for velero pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	b79df65

[Ralthos]

Both good catches. Thank you.

The coverage test. You are right, and it was worse than you describe: the assertion compares a slice I wrote by hand against its own length, so it could never fail for any reason. It has been replaced with one that reads the status.phase enum out of the generated CRDs, through the exported v1crds.CRDs that pkg/install already uses. The enum comes from the same kubebuilder markers as the Go constants, so a phase added to the API now fails here until someone classifies it. The old version could not do that whatever the phase count was.

The abandoned request. The symptom is real and I had understated it. The CLI polls until its own timeout and then reports:

download request download url timeout, check velero server logs for errors.
backup storage location may not be available

For this case that message points at the wrong thing entirely. The BSL is fine; the backup never ran. So the change swaps a fast wrong answer for a slow wrong answer, which is not obviously an improvement for anyone driving it from the CLI.

Setting a terminal phase is the right shape. The obstacle is that there is no phase to set:

// +kubebuilder:validation:Enum=New;Processed
type DownloadRequestPhase string

Adding Failed changes the CRD schema, and on #10232 @kaovilai scoped this work to a non-breaking change of the API. An additive enum value may well qualify, but that is a maintainer's call.

@kaovilai, three options as I see them:

1. Add DownloadRequestPhaseFailed, with a message field or an event carrying the reason. Best behaviour, and it needs a decision on whether an added enum value counts as non-breaking.
2. Set Status.Expiration to now when the guard fires, so the controller deletes the request on the next pass and the CLI's Get fails immediately. No schema change, though the caller then sees a not-found error carrying no reason.
3. Hold the guard until 1 is settled, and land only the documentation in Document what the DownloadRequest Processed phase means #10245.

I did not want to pick between those on your behalf. Happy to write whichever you prefer.

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 11 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/controller/download_request_controller.go	75.86%	6 Missing and 1 partial ⚠️
pkg/cmd/util/downloadrequest/downloadrequest.go	0.00%	3 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[kaovilai]

added enum value counts as non-breaking.

Adding enum imo is non breaking. removing enum is breaking.

WTAL at the approaches later and give more thoughts later.

[Ralthos]

@kaovilai thanks, that settles it. I have taken option 1.

DownloadRequestPhase gains Failed, and DownloadRequestStatus gains Message. Both are additive: no existing value changes meaning, and a client that only knows New and Processed sees a request that never becomes Processed, which is what it saw before this change anyway.

The reason option 2 is not enough is worth recording. Setting Expiration to now would make the CLI's Get fail immediately, but the error it surfaces would be a not-found on a request the caller just created, which explains less than the current timeout does. The message is the part that matters here:

backup "a-backup" is in phase "FailedValidation" and has not written any artifacts

@Ayush4958's point about the CLI is handled in the same commit rather than deferred. getDownloadURL in pkg/cmd/util/downloadrequest polls on Status.DownloadURL != "" and ignores the phase, so the guard alone traded a fast wrong answer for a slow one. It now returns as soon as it sees Failed, carrying the message. ErrDownloadRequestDownloadURLTimeout still exists and still means what it says, but it no longer fires for a backup that never ran.

Checking the phase in the CLI before creating the request would work too, and I did consider it. I did not do that because it puts the rule in one caller: velero backup download, restore describe and anything else driving the API would each need their own copy, and a UI talking to the API directly would get none of it. The controller is the one place all of them go through.

Two behaviour tests cover this, asserting on the phase and the exact message a client would read. I checked they fail when the message assignment is removed, since the last version of this test could not fail at all.

CRD regenerated, gofmt clean, rebased on main.

[kaovilai]

This is cool!

[kaovilai]

rerunning tests.. github was kinda down today

[Ayush4958]

CLI polling now aborts correctly on the Failed phase
Hardcoded test array replaced with dynamic CRD OpenAPI schema parsing

LGTM