Skip to content

Conversation

@ffjlabo
Copy link
Member

@ffjlabo ffjlabo commented Dec 14, 2023

What this PR does / why we need it:

Implement SCRIPT_RUN stage to execute any command on the pipeline.
First, this PR is for just executing commands, not considered for rollbacking.

Which issue(s) this PR fixes:

Part of #4643

Does this PR introduce a user-facing change?:
yes

  • How are users affected by this change:
    • User can execute any command on the pipeline.

Signed-off-by: Yoshiki Fujikane <[email protected]>
@codecov
Copy link

codecov bot commented Dec 14, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (a47b2f2) 30.83% compared to head (b198ecc) 30.80%.

Files Patch % Lines
pkg/config/application.go 50.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4720      +/-   ##
==========================================
- Coverage   30.83%   30.80%   -0.04%     
==========================================
  Files         221      221              
  Lines       26005    26015      +10     
==========================================
- Hits         8018     8013       -5     
- Misses      17337    17352      +15     
  Partials      650      650              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Yoshiki Fujikane <[email protected]>
@ffjlabo
Copy link
Member Author

ffjlabo commented Dec 15, 2023

/review

@github-actions
Copy link
Contributor

PR Analysis

Main theme

Enhancement

PR summary

The PR introduces a new stage for running arbitrary scripts. This change enables SCRIPT_RUN as a new stage option in PipeCD's deployment pipeline configurations.

Type of PR

Enhancement

PR Feedback:

General suggestions

This enhancement is useful as it provides users with a flexible mechanism for running custom scripts during deployments. However, it's crucial to ensure the execution of arbitrary scripts does not compromise security or stability. Take care to manage environment variables and consider validation or sanitation of the scripts if necessary.

Code feedback

  • relevant file: pkg/app/piped/executor/scriptrun/scriptrun.go
    suggestion: |-
    Ensure the command execution is error-checked to provide a detailed cause of failure. The error returned from cmd.Run() should be logged to aid in diagnosis of failed script executions. [important]
    relevant line: + if err := cmd.Run(); err != nil {

  • relevant file: pkg/app/piped/executor/scriptrun/scriptrun.go
    suggestion: |-
    The RollbackExecutor is not yet implemented. Consider adding implementation for rollback scenarios or remove this placeholder if the rollback is not applicable for script execution. [medium]
    relevant line: +func (e *RollbackExecutor) Execute(sig executor.StopSignal) model.StageStatus {

Security concerns:

yes

Executing arbitrary scripts can be a security risk, especially if the script content is sourced from an untrusted input or manipulated before execution. Although the scripts seem to be specified in stage configurations, which may be from controlled sources (e.g., git repositories), it is important to ensure they are not susceptible to injection attacks and that proper measures, like sanitation and permissions checks, are in place for both the contents of the scripts and the environment variables used within them.

@ffjlabo
Copy link
Member Author

ffjlabo commented Dec 15, 2023

It works like this :)↓
PipeCD

apiVersion: pipecd.dev/v1beta1
kind: KubernetesApp
spec:
  name: canary
  labels:
    env: example
    team: product
  pipeline:
    stages:
      # Deploy the workloads of CANARY variant. In this case, the number of
      # workload replicas of CANARY variant is 10% of the replicas number of PRIMARY variant.
      - name: K8S_CANARY_ROLLOUT
        with:
          replicas: 10%
      # Wait 10 seconds before going to the next stage.
      - name: WAIT
        with:
          duration: 10s
      # Update the workload of PRIMARY variant to the new version.
      - name: K8S_PRIMARY_ROLLOUT
      # Destroy all workloads of CANARY variant.
      - name: K8S_CANARY_CLEAN
      - name: SCRIPT_RUN
        with:
          env:
            MSG: "Hello from script_run"
          run: |
            echo $MSG
            kubectl get all
  description: |
    This app demonstrates how to deploy a Kubernetes app by Canary strategy without requering any mesh.\
    References: [adding a new app](https://pipecd.dev/docs/user-guide/managing-application/adding-an-application/), [app configuration](https://pipecd.dev/docs/user-guide/configuration-reference/)

@ffjlabo ffjlabo marked this pull request as ready for review December 15, 2023 05:25
@ffjlabo ffjlabo changed the title [WIP] Script run stage Script run stage Dec 15, 2023
}

func (e *RollbackExecutor) Execute(sig executor.StopSignal) model.StageStatus {
return model.StageStatus_STAGE_NOT_STARTED_YET
Copy link
Member

@khanhtc1202 khanhtc1202 Dec 16, 2023

Choose a reason for hiding this comment

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

Use stage failed, and print out unimplemented in stage log instead, or this will block the deployment forever 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed on 2594710 🙏

Signed-off-by: Yoshiki Fujikane <[email protected]>
@ffjlabo ffjlabo requested a review from khanhtc1202 December 18, 2023 01:22
}
}

// TODO: Add testcases for other kinds of applications.
Copy link
Member

Choose a reason for hiding this comment

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

What do we want here? I don't see any kinds here 👀

Copy link
Member Author

Choose a reason for hiding this comment

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

b1e6555
Sorry this comment is meaningless 🙏 so deleted

Signed-off-by: Yoshiki Fujikane <[email protected]>
Signed-off-by: Yoshiki Fujikane <[email protected]>
@ffjlabo
Copy link
Member Author

ffjlabo commented Dec 21, 2023

I fixed the implementation like custom sync :)
https://github.com/pipe-cd/pipecd/blob/master/pkg/app/piped/executor/customsync/customsync.go#L55
92c6dd9

I will implement the rollback on another PR 🙏

@ffjlabo ffjlabo requested a review from khanhtc1202 December 21, 2023 02:34
Copy link
Member

@khanhtc1202 khanhtc1202 left a comment

Choose a reason for hiding this comment

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

Nice, thank you 🚀

@khanhtc1202
Copy link
Member

@ffjlabo Please sign-off the commit 👀

Signed-off-by: Yoshiki Fujikane <[email protected]>
@ffjlabo
Copy link
Member Author

ffjlabo commented Dec 21, 2023

@khanhtc1202 oops, sorry 🙏 signed!

Copy link
Member

@khanhtc1202 khanhtc1202 left a comment

Choose a reason for hiding this comment

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

🚀

@ffjlabo
Copy link
Member Author

ffjlabo commented Dec 21, 2023

Also, I will create docs for it on another PR:)

@ffjlabo ffjlabo enabled auto-merge (squash) December 21, 2023 09:38
Copy link
Member

@t-kikuc t-kikuc left a comment

Choose a reason for hiding this comment

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

🚀

@ffjlabo ffjlabo merged commit 02343e0 into master Dec 21, 2023
@ffjlabo ffjlabo deleted the script-run-stage branch December 21, 2023 09:44
@ffjlabo
Copy link
Member Author

ffjlabo commented Dec 22, 2023

The stage works with multiple script run stage 👍

PipeCD_🔊 PipeCD_🔊

t-kikuc pushed a commit that referenced this pull request Dec 26, 2023
* Add option script run stage

Signed-off-by: Yoshiki Fujikane <[email protected]>

* Implement Executor for script run

Signed-off-by: Yoshiki Fujikane <[email protected]>

* Use StageStatus_STAGE_FAILURE

Signed-off-by: Yoshiki Fujikane <[email protected]>

* Add error log

Signed-off-by: Yoshiki Fujikane <[email protected]>

* Add Copyright

Signed-off-by: Yoshiki Fujikane <[email protected]>

* Re-implement script run stage like custom sync

Signed-off-by: Yoshiki Fujikane <[email protected]>

* Delete comment

Signed-off-by: Yoshiki Fujikane <[email protected]>

---------

Signed-off-by: Yoshiki Fujikane <[email protected]>
Signed-off-by: t-kikuc <[email protected]>
nnnkkk7 pushed a commit to nnnkkk7/pipecd that referenced this pull request Feb 1, 2024
* Add option script run stage

Signed-off-by: Yoshiki Fujikane <[email protected]>

* Implement Executor for script run

Signed-off-by: Yoshiki Fujikane <[email protected]>

* Use StageStatus_STAGE_FAILURE

Signed-off-by: Yoshiki Fujikane <[email protected]>

* Add error log

Signed-off-by: Yoshiki Fujikane <[email protected]>

* Add Copyright

Signed-off-by: Yoshiki Fujikane <[email protected]>

* Re-implement script run stage like custom sync

Signed-off-by: Yoshiki Fujikane <[email protected]>

* Delete comment

Signed-off-by: Yoshiki Fujikane <[email protected]>

---------

Signed-off-by: Yoshiki Fujikane <[email protected]>
@github-actions github-actions bot mentioned this pull request Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants