Fix empty dts panic

[vishnukothakapu]

What this PR does:
Adds validation for empty DeployTargets across multiple pipedv1 stage plugins to prevent index out-of-bounds panics when accessing dts[0]. It also fixes a bug in the SDK where StagePluginServiceServer.ExecuteStage passed nil for deploy targets instead of correctly parsing them.

Why we need it:
If a user misconfigures their application with an empty targets list, or if the SDK fails to pass the targets properly, affected plugins (Terraform deployment, ECS plan preview, and Kubernetes plan preview) would experience an unhandled panic and crash the process during the execution or plan preview stages. These checks ensure the plugins return a clean failure status with a descriptive log message instead.

Which issue(s) this PR fixes:

Fixes #7115

Does this PR introduce a user-facing change?:
No. This is purely an internal bug fix and adds protective checks to prevent unhandled panics.

• How are users affected by this change:
  Users with misconfigured deploy targets will now see a descriptive failure log in the PipeCD UI instead of the plugin crashing silently or hanging.
• Is this breaking change:
  No.
• How to migrate (if breaking change):
  N/A

[khanhtc1202]

@vishnukothakapu Nice catch, but I think instead of doing this in each plugin, this should be part of the SDK, wdyt?
https://github.com/pipe-cd/pipecd/blob/master/pkg/plugin/sdk/planpreview.go#L51
cc @mohammedfirdouss @Warashi

[armistcxy]

@vishnukothakapu Nice catch, but I think instead of doing this in each plugin, this should be part of the SDK, wdyt? https://github.com/pipe-cd/pipecd/blob/master/pkg/plugin/sdk/planpreview.go#L51 cc @mohammedfirdouss @Warashi

I also think this kind of validation should be part of the SDK, basically no deployment plugin has zero deployTarget right. If that the case then just check when the plugin start running to avoid check per stage (and let the stage focus on its work also)

[armistcxy]

I belive the first place to parse and store deploy targets from piped configuration file is Plugin.run()

pipecd/pkg/plugin/sdk/plugin.go

Lines 330 to 340 in 3ecbefa

	for _, dt := range cfg.DeployTargets {
	var sdkDt DeployTargetConfig
	if err := json.Unmarshal(dt.Config, &sdkDt); err != nil {
	logger.Fatal("failed to unmarshal deploy target config", zap.Error(err))
	return err
	}
	commonFields.deployTargets[dt.Name] = &DeployTarget[DeployTargetConfig]{
	Name: dt.Name,
	Labels: dt.Labels,
	Config: sdkDt,
	}

so just add validation after parsing deploy targets like this is ok IMO

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

Update: Livestate plugin and plan preview plugin should also check this (not only deployment plugin)

[vishnukothakapu]

@vishnukothakapu Nice catch, but I think instead of doing this in each plugin, this should be part of the SDK, wdyt? https://github.com/pipe-cd/pipecd/blob/master/pkg/plugin/sdk/planpreview.go#L51 cc @mohammedfirdouss @Warashi

Yes, agreed! I’ve updated the PR to handle this centrally. Added startup validation in pkg/plugin/sdk/plugin.go for missing deploy targets, plus per-request checks in wrappers like GetPlanPreview and ExecuteStage to return a clean InvalidArgument error for empty targets. Also reverted the duplicate checks from individual plugins.

[vishnukothakapu]

@vishnukothakapu Nice catch, but I think instead of doing this in each plugin, this should be part of the SDK, wdyt? https://github.com/pipe-cd/pipecd/blob/master/pkg/plugin/sdk/planpreview.go#L51 cc @mohammedfirdouss @Warashi

I also think this kind of validation should be part of the SDK, basically no deployment plugin has zero deployTarget right. If that the case then just check when the plugin start running to avoid check per stage (and let the stage focus on its work also)

Yes, exactly! I've updated the PR to add this validation during plugin startup in pkg/plugin/sdk/plugin.go. This ensures the plugin won't even start if it's missing deploy targets, which keeps the individual stage logic clean and focused on its work.

[vishnukothakapu]

I belive the first place to parse and store deploy targets from piped configuration file is Plugin.run()

pipecd/pkg/plugin/sdk/plugin.go

Lines 330 to 340 in 3ecbefa

	for _, dt := range cfg.DeployTargets {
	var sdkDt DeployTargetConfig
	if err := json.Unmarshal(dt.Config, &sdkDt); err != nil {
	logger.Fatal("failed to unmarshal deploy target config", zap.Error(err))
	return err
	}
	commonFields.deployTargets[dt.Name] = &DeployTarget[DeployTargetConfig]{
	Name: dt.Name,
	Labels: dt.Labels,
	Config: sdkDt,
	}

so just add validation after parsing deploy targets like this is ok IMO

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

Update: Livestate plugin and plan preview plugin should also check this (not only deployment plugin)

Exactly this! I have added this exact validation check in Plugin.run right after parsing the deploy targets, including the checks for Livestate and PlanPreview plugins as you suggested. Thanks for the snippet, it made the fix very straightforward!

I belive the first place to parse and store deploy targets from piped configuration file is Plugin.run()

pipecd/pkg/plugin/sdk/plugin.go

Lines 330 to 340 in 3ecbefa

	for _, dt := range cfg.DeployTargets {
	var sdkDt DeployTargetConfig
	if err := json.Unmarshal(dt.Config, &sdkDt); err != nil {
	logger.Fatal("failed to unmarshal deploy target config", zap.Error(err))
	return err
	}
	commonFields.deployTargets[dt.Name] = &DeployTarget[DeployTargetConfig]{
	Name: dt.Name,
	Labels: dt.Labels,
	Config: sdkDt,
	}

so just add validation after parsing deploy targets like this is ok IMO

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

Update: Livestate plugin and plan preview plugin should also check this (not only deployment plugin)

Exactly this! I have added this exact validation check in Plugin.run right after parsing the deploy targets, including the checks for Livestate and PlanPreview plugins as you suggested. Thanks for the snippet, it made the fix very straightforward!

[netlify]

✅ Deploy Preview for pipecd-site ready!

Name	Link
🔨 Latest commit	d16d063
🔍 Latest deploy log	https://app.netlify.com/projects/pipecd-site/deploys/6a890eab34218c0008befc78
😎 Deploy Preview	https://deploy-preview-7116--pipecd-site.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[armistcxy]

✅ Deploy Preview for pipecd-site ready!

Name Link
🔨 Latest commit 2c51b06
🔍 Latest deploy log https://app.netlify.com/projects/pipecd-site/deploys/6a7edab09a8f580008350e11
😎 Deploy Preview https://deploy-preview-7116--pipecd-site.netlify.app
📱 Preview on mobile
Toggle QR Code...
🤖 Make changes Run an agent on this branch
To edit notification comments on pull requests, go to your Netlify project configuration.

Hi @rahulshendre, out of scope but why this change still trigger netlify ?

[armistcxy]

LGTM overall, only need to modify the test, you can have a look here: https://github.com/pipe-cd/pipecd/actions/runs/31786725487/job/94729559336?pr=7116 @vishnukothakapu

[armistcxy]

A little bit issue with the labeler check, still safe to merge nhe anh @khanhtc1202

[khanhtc1202]

A little bit issue with the labeler check, still safe to merge nhe anh @khanhtc1202

I just merged the labeler patch, could you please help me update branch for this PR, so we can test the labeler workflow 😁

[khanhtc1202]

NOTE: Release sdk after this being merged 📝

[khanhtc1202]

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

[vishnukothakapu]

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]

@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

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

[armistcxy]

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

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

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]

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]

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

[rahulshendre]

Hi @rahulshendre, out of scope but why this change still trigger netlify ?

great catch @armistcxy - our netlify.toml has no ignore build rule, so Netlify runs a preview on every PR regardless of what changed, even a Go-only one.
I think we should add an ignore command to skip builds that don't touch docs, what do you think?

[armistcxy]

@rahulshendre Yeah, I think we should ignore non-docs changes

[rahulshendre]

@rahulshendre Yeah, I think we should ignore non-docs changes

@harshitghagre, could you please open a PR for this?
Since you own the Netlify migration (#6930), you'd be the best fit for this one 😄
Fix = add an ignore build command to docs/netlify.toml so PRs that don't touch docs/ skip the preview build.

[rahulshendre]

LGTM
Great work @vishnukothakapu, thanks : )

[harshitghagre]

@rahulshendre thanks for flagging this opened the fix in #7179 netlify now skips the build if there are no changes under /docs.

[rahulshendre]

@rahulshendre thanks for flagging this opened the fix in #7179 netlify now skips the build if there are no changes under /docs.

that was fast @harshitghagre, thanks a lot : )