[feat]: [CI-12562]: support for output secrets plugins #282

Sapa96 · 2024-05-15T14:39:13Z

Requirement : https://harness.atlassian.net/wiki/x/rgFbEgU
Changes

introduction of secret env file for output secrets of plugin

Testing

Tested in local setup for the following scenarios

   - with old output yaml runstep 
   -  new output yaml  runstep 
   - plugin step with outputs
   - plugin step with output secrets
   - plugin step with combination of outputs and  output secrets
   - plugin step with no exports

CLAassistant · 2024-05-15T14:39:20Z

All committers have signed the CLA.

raghavharness · 2024-05-20T09:23:17Z

pipeline/runtime/run.go

+ finalErr = err
+ for _, key := range r.OutputVars {
+ if _, ok := outputs[key]; ok {
+ output := &api.OutputV2{
+ Key: key,
+ Value: outputs[key],
+ Type: outputVariableTypeString,
+ }
+ outputsV2 = append(outputsV2, output)
+ }
+ }


Why do we need to add this ?
OutputVars is deprecated so we can skip this

pipelines with older yaml can still use this OutputVars fields right ? added this code for backward compatibility

Yeah but yaml field is same. Internally we parse it to the new format. You can once check with @sahithibanda01 since she implemented it. AFIAK this is not required.

I think you can remove this elseif cond altogether

pipeline/runtime/run.go

pipeline/runtime/runtest.go

raghavharness · 2024-05-20T09:25:40Z

pipeline/runtime/runtestsV2.go

@@ -106,6 +106,9 @@ func executeRunTestsV2Step(ctx context.Context, f RunFunc, r *api.StartStepReque
 outputFile := fmt.Sprintf("%s/%s-output.env", pipeline.SharedVolPath, step.ID)
 step.Envs["DRONE_OUTPUT"] = outputFile

+ outputSecretsFile := fmt.Sprintf("%s/%s-output-secrets.env", pipeline.SharedVolPath, step.ID)


Same for this

observed the following support in the file . so added logic for secrets here also . can be removed if not required
step.Envs["DRONE_OUTPUT"] = outputFile

I think let's do for run.go for now only ? If the requirement comes for runtest we can add that later ?

raghavharness · 2024-05-20T09:52:24Z

pipeline/runtime/run.go

+ finalErr = err
+ for _, key := range r.OutputVars {
+ if _, ok := outputs[key]; ok {
+ output := &api.OutputV2{
+ Key: key,
+ Value: outputs[key],
+ Type: outputVariableTypeString,
+ }
+ outputsV2 = append(outputsV2, output)
+ }
+ }


Yeah but yaml field is same. Internally we parse it to the new format. You can once check with @sahithibanda01 since she implemented it. AFIAK this is not required.

raghavharness · 2024-05-20T09:52:52Z

pipeline/runtime/run.go

+ finalErr = err
+ for _, key := range r.OutputVars {
+ if _, ok := outputs[key]; ok {
+ output := &api.OutputV2{
+ Key: key,
+ Value: outputs[key],
+ Type: outputVariableTypeString,
+ }
+ outputsV2 = append(outputsV2, output)
+ }
+ }


Yeah but yaml field is same. Internally we parse it to the new format. You can once check with @sahithibanda01 since she implemented it. AFIAK this is not required.

raghavharness · 2024-05-20T10:08:27Z

pipeline/runtime/run.go

+ finalErr = err
+ for _, key := range r.OutputVars {
+ if _, ok := outputs[key]; ok {
+ output := &api.OutputV2{
+ Key: key,
+ Value: outputs[key],
+ Type: outputVariableTypeString,
+ }
+ outputsV2 = append(outputsV2, output)
+ }
+ }


I think you can remove this elseif cond altogether

raghavharness · 2024-05-20T10:09:54Z

pipeline/runtime/runtestsV2.go

@@ -106,6 +106,9 @@ func executeRunTestsV2Step(ctx context.Context, f RunFunc, r *api.StartStepReque
 outputFile := fmt.Sprintf("%s/%s-output.env", pipeline.SharedVolPath, step.ID)
 step.Envs["DRONE_OUTPUT"] = outputFile

+ outputSecretsFile := fmt.Sprintf("%s/%s-output-secrets.env", pipeline.SharedVolPath, step.ID)


I think let's do for run.go for now only ? If the requirement comes for runtest we can add that later ?

vistaarjuneja · 2024-05-20T11:33:02Z

pipeline/runtime/run.go

@@ -66,6 +67,9 @@ func executeRunStep(ctx context.Context, f RunFunc, r *api.StartStepRequest, out
 }
 }

+ outputSecretsFile := fmt.Sprintf("%s/%s-output-secrets.env", pipeline.SharedVolPath, step.ID)


Can you add the same condition as L52 and add a SecretVarFile variable. If it's set, we will use that directly otherwise generate it similarly to what you have. This will help integrate with runner in the future.

if r.SecretVarFile != "" { // set the env variable to r.SecretVarFile }

vistaarjuneja · 2024-05-20T11:36:37Z

pipeline/runtime/run.go

- return exited, outputs, exportEnvs, artifact, nil, string(optimizationState), nil
+
+ //checking exported secrets from plugins if any
+ _, secretErr := os.Stat(outputSecretsFile)


Let's log if secretErr != nil

@vistaarjuneja
Why do we need to log it ? this will return err for all the existing plugins and run steps also as they dont use secret file as of now . this check is to understand whether a secret is exported or not .
if we log it , the error will be logged for all the existing run steps and plugin steps .

Ack, let's change it to this in a single line:

if _, err := os.Stat(outputSecretsFile); err == nil { }

pipeline/runtime/run.go

pipeline/runtime/runtest.go

pipeline/runtime/common.go

vistaarjuneja · 2024-05-20T13:54:19Z

api/api.go

+type OutputType string
+
+const (
+ STRING OutputType = "STRING"


Let's renamename these so it's more clear what it does

OutputTypeString OutputType = "STRING" OutputTypeSecret OutputType = "SECRET"

so api.OutputTypeString is more clear

api/api.go

vistaarjuneja · 2024-05-20T13:58:54Z

pipeline/runtime/run.go

- return exited, outputs, exportEnvs, artifact, nil, string(optimizationState), nil
+
+ //checking exported secrets from plugins if any
+ _, secretErr := os.Stat(outputSecretsFile)


Ack, let's change it to this in a single line:

if _, err := os.Stat(outputSecretsFile); err == nil { }

This reverts commit 122e501.

pipeline/runtime/run.go

[feat]: [CI-12562]: initial commit support for secrets

8463d5f

Sapa96 requested review from harshjain12 and raghavharness May 20, 2024 05:26

[feat]: [CI-12562]: added logic for deleting env files after parsing

9b22760

Sapa96 force-pushed the feature/output_secrets_plugin branch from 4e18543 to 9b22760 Compare May 20, 2024 05:43

Sapa96 added 2 commits May 20, 2024 11:33

[feat]: [CI-12562]: updated for runtests step

43247f8

reverted delete changes for now

9a94426

Sapa96 changed the title ~~[feat]: [CI-12562]: initial commit support for secrets~~ [feat]: [CI-12562]: support for output secrets plugins May 20, 2024

Sapa96 requested a review from vistaarjuneja May 20, 2024 07:27

raghavharness requested changes May 20, 2024

View reviewed changes

raghavharness reviewed May 20, 2024

View reviewed changes

raghavharness requested changes May 20, 2024

View reviewed changes

vistaarjuneja reviewed May 20, 2024

View reviewed changes

Sapa96 added 3 commits May 20, 2024 18:27

reverting runtests code

0e4928a

review comments addressed

33306fc

review comment

da80128

vistaarjuneja reviewed May 20, 2024

View reviewed changes

minor comments

28f772e

vistaarjuneja approved these changes May 20, 2024

View reviewed changes

Sapa96 added 3 commits May 21, 2024 14:06

better error handling

122e501

Revert "better error handling"

abf1e2f

This reverts commit 122e501.

error handling revisited

a00054c

raghavharness requested changes May 21, 2024

View reviewed changes

pipeline/runtime/run.go Outdated Show resolved Hide resolved

log fix

ca8b60b

raghavharness approved these changes May 21, 2024

View reviewed changes

removed error log

1a6566c

Sapa96 merged commit c6b9be5 into main May 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat]: [CI-12562]: support for output secrets plugins #282

[feat]: [CI-12562]: support for output secrets plugins #282

Sapa96 commented May 15, 2024 •

edited

CLAassistant commented May 15, 2024 •

edited

raghavharness May 20, 2024

Sapa96 May 20, 2024

raghavharness May 20, 2024

raghavharness May 20, 2024

raghavharness May 20, 2024

raghavharness May 20, 2024

Sapa96 May 20, 2024

raghavharness May 20, 2024

raghavharness May 20, 2024

raghavharness May 20, 2024

raghavharness May 20, 2024

raghavharness May 20, 2024

vistaarjuneja May 20, 2024

vistaarjuneja May 20, 2024

vistaarjuneja May 20, 2024

Sapa96 May 20, 2024 •

edited

vistaarjuneja May 20, 2024

vistaarjuneja May 20, 2024

vistaarjuneja May 20, 2024

[feat]: [CI-12562]: support for output secrets plugins #282

[feat]: [CI-12562]: support for output secrets plugins #282

Conversation

Sapa96 commented May 15, 2024 • edited

CLAassistant commented May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sapa96 May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sapa96 commented May 15, 2024 •

edited

CLAassistant commented May 15, 2024 •

edited

Sapa96 May 20, 2024 •

edited