Validate empty host in route set and route show commands#591
Merged
Conversation
📝 WalkthroughWalkthroughTwo command handlers have been updated with early input validation: RouteSet and RouteShow both now validate that the Host option is provided before proceeding with their respective operations. In RouteSet, the validation occurs before app name lookup and RPC calls. In RouteShow, it occurs before RPC client initialization. Both validations return an error if the Host option is empty, ensuring this required parameter is present at the start of command execution. No actionable comments were generated in the recent review. 🎉 Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@servers/deployment/server.go`:
- Around line 667-857: DeployVersion creates a deployment entity as "active"
before calling AppClient.SetActiveVersion, and if SetActiveVersion fails the
deployment remains recorded as active; also the sourceDeployment lookup uses
appVersionId (which may be a derived version) losing provenance. Fix by
preserving originalAppVersionId := args.AppVersionId() and use
originalAppVersionId when searching for sourceDeployment so SourceDeploymentId
is populated from the original version; after calling
d.AppClient.SetActiveVersion, if it returns an error update the created
deployment entity (deployment.ID / newDeploymentId) to set Status="failed",
Phase="failed" (or appropriate), set ErrorMessage to the activation error,
update CompletedAt, and persist the update via d.EAC.Get to obtain revision then
d.EAC.Put (similar to the expired-lock update flow), then set results.SetError
and return.
Comment on lines
667
to
857
| func (d *DeploymentServer) DeployVersion(ctx context.Context, req *deployment_v1alpha.DeploymentDeployVersion) error { | ||
| args := req.Args() | ||
| results := req.Results() | ||
|
|
||
| // Validate required fields | ||
| if !args.HasAppName() || args.AppName() == "" { | ||
| return cond.ValidationFailure("missing-field", "app_name is required") | ||
| } | ||
| if !args.HasClusterId() || args.ClusterId() == "" { | ||
| return cond.ValidationFailure("missing-field", "cluster_id is required") | ||
| } | ||
| if !args.HasAppVersionId() || args.AppVersionId() == "" { | ||
| return cond.ValidationFailure("missing-field", "app_version_id is required") | ||
| } | ||
|
|
||
| appName := args.AppName() | ||
| clusterId := args.ClusterId() | ||
| appVersionId := args.AppVersionId() | ||
| isRollback := args.HasIsRollback() && args.IsRollback() | ||
|
|
||
| // Verify the AppVersion entity exists | ||
| var appVersion core_v1alpha.AppVersion | ||
| if err := d.EC.Get(ctx, appVersionId, &appVersion); err != nil { | ||
| if errors.Is(err, cond.ErrNotFound{}) { | ||
| results.SetError(fmt.Sprintf("app version %q not found", appVersionId)) | ||
| return nil | ||
| } | ||
| d.Log.Error("Failed to look up app version", "app_version_id", appVersionId, "error", err) | ||
| results.SetError("failed to look up app version") | ||
| return nil | ||
| } | ||
|
|
||
| // If env vars are provided, create a derived version with merged variables | ||
| if args.HasEnvVars() && len(args.EnvVars()) > 0 { | ||
| derivedVersion, err := d.createDerivedVersion(ctx, &appVersion, args.EnvVars()) | ||
| if err != nil { | ||
| d.Log.Error("Failed to create derived version with env vars", "error", err) | ||
| results.SetError(fmt.Sprintf("failed to apply env vars: %v", err)) | ||
| return nil | ||
| } | ||
| appVersion = *derivedVersion | ||
| appVersionId = derivedVersion.Version | ||
| d.Log.Info("Created derived version with env vars", | ||
| "original", args.AppVersionId(), "derived", appVersionId, | ||
| "env_var_count", len(args.EnvVars())) | ||
| } | ||
|
|
||
| // Check for existing in_progress deployments (deployment lock) | ||
| existingDeployments, err := d.listDeploymentsInternal(ctx, appName, clusterId, "in_progress", 1) | ||
| if err != nil { | ||
| d.Log.Error("Failed to check for existing deployments", "error", err) | ||
| results.SetError("failed to check deployment lock") | ||
| return nil | ||
| } | ||
|
|
||
| if len(existingDeployments) > 0 { | ||
| existing := existingDeployments[0] | ||
|
|
||
| deploymentTime, parseErr := time.Parse(time.RFC3339, existing.DeployedBy.Timestamp) | ||
| if parseErr != nil { | ||
| deploymentTime = time.Time{} | ||
| } | ||
|
|
||
| isExpired := deploymentTime.IsZero() || time.Since(deploymentTime) >= deploymentLockTimeout | ||
| if !isExpired { | ||
| lockExpiresAt := deploymentTime.Add(deploymentLockTimeout) | ||
|
|
||
| displayEmail := existing.DeployedBy.UserEmail | ||
| if displayEmail == "" || displayEmail == "unknown@example.com" || displayEmail == "user@example.com" { | ||
| displayEmail = "-" | ||
| } | ||
|
|
||
| lockInfo := &deployment_v1alpha.DeploymentLockInfo{} | ||
| lockInfo.SetAppName(appName) | ||
| lockInfo.SetClusterId(clusterId) | ||
| lockInfo.SetBlockingDeploymentId(string(existing.ID)) | ||
| lockInfo.SetStartedBy(displayEmail) | ||
| lockInfo.SetStartedAt(standard.ToTimestamp(deploymentTime)) | ||
| lockInfo.SetCurrentPhase(existing.Phase) | ||
| lockInfo.SetLockExpiresAt(standard.ToTimestamp(lockExpiresAt)) | ||
|
|
||
| results.SetLockInfo(lockInfo) | ||
| results.SetError("deployment blocked by existing in-progress deployment") | ||
| return nil | ||
| } | ||
|
|
||
| // Expired lock — mark as failed and continue | ||
| d.Log.Warn("Found expired in_progress deployment, marking as failed", | ||
| "deployment_id", string(existing.ID), | ||
| "age", time.Since(deploymentTime)) | ||
|
|
||
| existing.Status = "failed" | ||
| existing.ErrorMessage = fmt.Sprintf("Deployment timed out after %v", deploymentLockTimeout) | ||
| existing.CompletedAt = time.Now().Format(time.RFC3339) | ||
|
|
||
| updateAttrs := existing.Encode() | ||
| updateEntity := &entityserver_v1alpha.Entity{} | ||
| updateEntity.SetId(string(existing.ID)) | ||
| updateEntity.SetAttrs(updateAttrs) | ||
|
|
||
| if existingEntity, getErr := d.EAC.Get(ctx, string(existing.ID)); getErr == nil { | ||
| updateEntity.SetRevision(existingEntity.Entity().Revision()) | ||
| if _, putErr := d.EAC.Put(ctx, updateEntity); putErr != nil { | ||
| d.Log.Error("Failed to mark expired deployment as failed", "error", putErr) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Find the source deployment — the most recent deployment with this app_version_id | ||
| allDeployments, err := d.listDeploymentsInternal(ctx, appName, clusterId, "", 100) | ||
| if err != nil { | ||
| d.Log.Error("Failed to list deployments for source lookup", "error", err) | ||
| // Continue without source info | ||
| } | ||
|
|
||
| var sourceDeployment *core_v1alpha.Deployment | ||
| for _, dep := range allDeployments { | ||
| if dep.AppVersion == appVersionId { | ||
| sourceDeployment = dep | ||
| break // listDeploymentsInternal returns newest first | ||
| } | ||
| } | ||
|
|
||
| // Create new deployment entity | ||
| now := time.Now() | ||
|
|
||
| deployment := &core_v1alpha.Deployment{ | ||
| AppName: appName, | ||
| AppVersion: appVersionId, | ||
| ClusterId: clusterId, | ||
| Status: "active", | ||
| Phase: "activating", | ||
| DeployedBy: core_v1alpha.DeployedBy{ | ||
| Timestamp: now.Format(time.RFC3339), | ||
| }, | ||
| CompletedAt: now.Format(time.RFC3339), | ||
| } | ||
|
|
||
| // Copy git info and source ID from the source deployment | ||
| if sourceDeployment != nil { | ||
| deployment.GitInfo = sourceDeployment.GitInfo | ||
| deployment.SourceDeploymentId = string(sourceDeployment.ID) | ||
| } | ||
|
|
||
| // Create entity | ||
| attrs := deployment.Encode() | ||
| rpcEntity := &entityserver_v1alpha.Entity{} | ||
| rpcEntity.SetAttrs(attrs) | ||
|
|
||
| putResp, err := d.EAC.Put(ctx, rpcEntity) | ||
| if err != nil { | ||
| d.Log.Error("Failed to create deployment entity", "error", err) | ||
| results.SetError("failed to create deployment") | ||
| return nil | ||
| } | ||
|
|
||
| deployment.ID = entity.Id(putResp.Id()) | ||
| newDeploymentId := putResp.Id() | ||
|
|
||
| // Activate the version via AppClient | ||
| if err := d.AppClient.SetActiveVersion(ctx, appName, string(appVersion.ID)); err != nil { | ||
| d.Log.Error("Failed to set active version", "error", err, "app", appName, "version_id", string(appVersion.ID)) | ||
| results.SetError(fmt.Sprintf("failed to activate version: %v", err)) | ||
| return nil | ||
| } | ||
|
|
||
| // Mark previous active deployments | ||
| targetStatus := "succeeded" | ||
| if isRollback { | ||
| targetStatus = "rolled_back" | ||
| } | ||
| if err := d.markPreviousActiveAs(ctx, appName, clusterId, newDeploymentId, targetStatus); err != nil { | ||
| d.Log.Error("Failed to mark previous active deployments", "error", err) | ||
| // Don't fail — the new deployment is already created and active | ||
| } | ||
|
|
||
| deploymentInfo := d.toDeploymentInfo(deployment) | ||
| results.SetDeployment(deploymentInfo) | ||
|
|
||
| accessInfo := d.getAccessInfo(ctx, appName) | ||
| results.SetAccessInfo(&accessInfo) | ||
|
|
||
| d.Log.Info("Deployed version", | ||
| "deployment_id", newDeploymentId, | ||
| "app", appName, | ||
| "cluster", clusterId, | ||
| "version", appVersionId, | ||
| "is_rollback", isRollback) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Prevent inconsistent “active” deployments if activation fails.
SetActiveVersion errors currently leave the deployment recorded as active/completed. Also, when env vars create a derived version, the source-deployment lookup uses the derived version string, which can drop provenance.
💡 Suggested fix: mark the deployment failed on activation errors
- // Activate the version via AppClient
- if err := d.AppClient.SetActiveVersion(ctx, appName, string(appVersion.ID)); err != nil {
- d.Log.Error("Failed to set active version", "error", err, "app", appName, "version_id", string(appVersion.ID))
- results.SetError(fmt.Sprintf("failed to activate version: %v", err))
- return nil
- }
+ // Activate the version via AppClient
+ if err := d.AppClient.SetActiveVersion(ctx, appName, string(appVersion.ID)); err != nil {
+ d.Log.Error("Failed to set active version", "error", err, "app", appName, "version_id", string(appVersion.ID))
+
+ deployment.Status = "failed"
+ deployment.ErrorMessage = fmt.Sprintf("failed to activate version: %v", err)
+ deployment.CompletedAt = time.Now().Format(time.RFC3339)
+
+ updateEntity := &entityserver_v1alpha.Entity{}
+ updateEntity.SetId(newDeploymentId)
+ updateEntity.SetAttrs(deployment.Encode())
+ if current, getErr := d.EAC.Get(ctx, newDeploymentId); getErr == nil {
+ updateEntity.SetRevision(current.Entity().Revision())
+ _, _ = d.EAC.Put(ctx, updateEntity)
+ }
+
+ results.SetError(deployment.ErrorMessage)
+ return nil
+ }For provenance with derived versions, consider using the original args.AppVersionId() when finding sourceDeployment so SourceDeploymentId stays populated even when env vars are applied.
🤖 Prompt for AI Agents
In `@servers/deployment/server.go` around lines 667 - 857, DeployVersion creates a
deployment entity as "active" before calling AppClient.SetActiveVersion, and if
SetActiveVersion fails the deployment remains recorded as active; also the
sourceDeployment lookup uses appVersionId (which may be a derived version)
losing provenance. Fix by preserving originalAppVersionId := args.AppVersionId()
and use originalAppVersionId when searching for sourceDeployment so
SourceDeploymentId is populated from the original version; after calling
d.AppClient.SetActiveVersion, if it returns an error update the created
deployment entity (deployment.ID / newDeploymentId) to set Status="failed",
Phase="failed" (or appropriate), set ErrorMessage to the activation error,
update CompletedAt, and persist the update via d.EAC.Get to obtain revision then
d.EAC.Put (similar to the expired-lock update flow), then set results.SetError
and return.
evanphx
approved these changes
Feb 10, 2026
Contributor
evanphx
left a comment
evanphx
left a comment
There was a problem hiding this comment.
You pushed the other PR commits. Checked just the last one, looks good.
The `required:"true"` struct tag on the Host positional arg isn't actually enforced by mflags, so `m route set` with no host silently creates a route with an empty hostname. Same deal with `route show` where you'd get a confusing "route not found: " error. Added explicit empty checks matching the existing appName validation pattern.
phinze
force-pushed
the
phinze/mir-684-route-set-accepts-empty-host-argument
branch
from
576bc66 to
031c768
Compare
Contributor
Author
I know a little helper script that's gonna get a sittin' down and a talkin' to! 🤠 |
phinze
deleted the
phinze/mir-684-route-set-accepts-empty-host-argument
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
required:"true"struct tag on theHostpositional arg isn'tactually enforced by
mflags, som route setwith no host wouldsilently succeed and create a route with an empty hostname.
route showhad the same issue — you'd get a confusing "route not found: " error
instead of something helpful.
Added explicit empty-string checks for
Hostin both handlers, samepattern already used for
appNamevalidation inroute_set.