Skip to content

Fix empty dts panic #7116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
khanhtc1202 merged 2 commits into from
Aug 22, 2026
Merged

Fix empty dts panic #7116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4cd65cf
Fix out-of-bounds panic on empty DeployTargets in plugins
vishnukothakapu Aug 3, 2026
d16d063
Merge branch 'master' into fix-empty-dts-panic
armistcxy Aug 22, 2026
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
4 changes: 4 additions & 0 deletions pkg/plugin/sdk/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,10 @@ func (s *DeploymentPluginServiceServer[Config, DeployTargetConfig, ApplicationCo

// Get the deploy targets set on the deployment from the piped plugin config.
dtNames := request.GetInput().GetDeployment().GetDeployTargets(s.config.Name)
if len(dtNames) == 0 {

@khanhtc1202 khanhtc1202 Aug 14, 2026

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.

Still, do we need to validate it here, after validating deployTargets in the plugin?go (run function)? cc @armistcxy

@vishnukothakapu vishnukothakapu Aug 14, 2026

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.

Yes, we still need this check here. The validation in plugin.go only verifies that the plugin itself has deploy targets configured.

dtNames comes from the individual app’s deployment request, so an app can still send an empty targets list. Without this check, we’d pass an empty deployTargets slice to stage execution and hit the same dts[0] panic this PR is meant to prevent.

So this check is needed to return a clean InvalidArgument error for that case.

@armistcxy armistcxy Aug 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@khanhtc1202 yes anh, I have checked and as what @vishnukothakapu said, we need to check in both places

The first one in the plugin's Run function is for when starting the plugin

The next ones are for each application deployment (I've just checked that you can create an application without deploy target through Web UI)

Evidence: it can cause panic when you trigger new deployment for the application with no deploy target
image

TL,DR: Yes, we still need because validate in plugin running != validate in executing stage in application's deployment

@armistcxy armistcxy Aug 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But there's one part I still concern @vishnukothakapu

Your fix check the dtNames, but the one we actually pass to the function executeStage is deployTargets

	dtNames := request.GetInput().GetDeployment().GetDeployTargets(s.config.Name)
	if len(dtNames) == 0 {
		return nil, status.Errorf(codes.InvalidArgument, "no deploy targets provided")
	}

	deployTargets := make([]*DeployTarget[DeployTargetConfig], 0, len(dtNames))
	for _, name := range dtNames {
		dt, ok := s.deployTargets[name]
		if !ok {
			return nil, status.Errorf(codes.Internal, "the deploy target %s is not found in the piped plugin config", name)
		}

		deployTargets = append(deployTargets, dt)
	}

   return executeStage(ctx, s.name, s.base, s.pluginConfig, deployTargets, client, request, s

So there will be a bug like this, in application configuration we specify deploy target ecs-dev

Image

But in piped.yaml we specify deploy target as ecs-target

Image

There will be mismatch => len(deployTargets) = 0

Maybe you should do something like if len(deployTargets) ==0 then log error like "Mismatch deploy target between plugin and application, or no specified deploy target in application configuration"

CC anh @khanhtc1202

@vishnukothakapu vishnukothakapu Aug 15, 2026

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 for checking this! @armistcxy .

Actually, if there is a mismatch (e.g., the application asks for ecs-dev but the plugin only has ecs-target), the code inside the for loop will hit the !ok condition and immediately return an error:

return nil, status.Errorf(codes.Internal, "the deploy target %s is not found in the piped plugin config", name)

Because it returns this error immediately, it never proceeds to executeStage with an empty deployTargets slice. If len(dtNames) > 0, len(deployTargets) is guaranteed to be greater than 0, otherwise it fails safely before execution. So we are completely covered for the mismatch scenario!

@armistcxy armistcxy Aug 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vishnukothakapu Oh I see, sorry for false alert, then I think it's all good now

return nil, status.Errorf(codes.InvalidArgument, "no deploy targets provided")
}

deployTargets := make([]*DeployTarget[DeployTargetConfig], 0, len(dtNames))
for _, name := range dtNames {
dt, ok := s.deployTargets[name]
Expand Down
4 changes: 4 additions & 0 deletions pkg/plugin/sdk/livestate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ func (s *LivestatePluginServer[Config, DeployTargetConfig, ApplicationConfigSpec
// GetLivestate returns the live state of the resources in the given application.
func (s *LivestatePluginServer[Config, DeployTargetConfig, ApplicationConfigSpec]) GetLivestate(ctx context.Context, request *livestate.GetLivestateRequest) (*livestate.GetLivestateResponse, error) {
// Get the deploy targets set on the deployment from the piped plugin config.
if len(request.GetDeployTargets()) == 0 {
Comment thread
armistcxy marked this conversation as resolved.
return nil, status.Errorf(codes.InvalidArgument, "no deploy targets provided")
}

deployTargets := make([]*DeployTarget[DeployTargetConfig], 0, len(request.GetDeployTargets()))
for _, name := range request.GetDeployTargets() {
dt, ok := s.deployTargets[name]
Expand Down
2 changes: 1 addition & 1 deletion pkg/plugin/sdk/livestate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ spec: {}
result: &GetLivestateResponse{},
err: errors.New("some error"),
expectErr: true,
expectedStatus: codes.Internal,
expectedStatus: codes.InvalidArgument,
},
}

Expand Down
4 changes: 4 additions & 0 deletions pkg/plugin/sdk/planpreview.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ func (s *PlanPreviewPluginServer[Config, DeployTargetConfig, ApplicationConfigSp
// GetPlanPreview returns the plan preview of the resources in the given application.
func (s *PlanPreviewPluginServer[Config, DeployTargetConfig, ApplicationConfigSpec]) GetPlanPreview(ctx context.Context, request *planpreview.GetPlanPreviewRequest) (*planpreview.GetPlanPreviewResponse, error) {
// Get the deploy targets set on the deployment from the piped plugin config.
if len(request.GetDeployTargets()) == 0 {
return nil, status.Errorf(codes.InvalidArgument, "no deploy targets provided")
Comment thread
armistcxy marked this conversation as resolved.
}

deployTargets := make([]*DeployTarget[DeployTargetConfig], 0, len(request.GetDeployTargets()))
for _, name := range request.GetDeployTargets() {
dt, ok := s.deployTargets[name]
Expand Down
2 changes: 1 addition & 1 deletion pkg/plugin/sdk/planpreview_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ spec: {}
mockResp: &GetPlanPreviewResponse{},
err: errors.New("some error"),
expectErr: true,
expectedStatus: codes.Internal,
expectedStatus: codes.InvalidArgument,
},
}

Expand Down
7 changes: 7 additions & 0 deletions pkg/plugin/sdk/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,13 @@ func (p *Plugin[Config, DeployTargetConfig, ApplicationConfigSpec]) run(ctx cont
}
}

if len(commonFields.deployTargets) == 0 {
if p.deploymentPlugin != nil || p.livestatePlugin != nil || p.planPreviewPlugin != nil {
logger.Error("plugin requires at least one deploy target to be configured", zap.String("name", cfg.Name))
return fmt.Errorf("plugin requires at least one deploy target to be configured")
}
}

client := &Client{
base: commonFields.client,
pluginName: commonFields.name,
Expand Down
Loading