From 3cebefb54d47d62ae54a7fb3748d7a8df11e975a Mon Sep 17 00:00:00 2001 From: Mattia Buccarella Date: Thu, 16 Nov 2023 18:53:00 +0100 Subject: [PATCH 01/12] feat(runner): allow for optional environment variables Signed-off-by: Mattia Buccarella --- internal/attestation/crafter/crafter.go | 57 +++++++++---------- internal/attestation/crafter/crafter_test.go | 38 +++++++------ internal/attestation/crafter/runner.go | 8 ++- .../crafter/runners/azurepipeline.go | 32 +++++------ .../crafter/runners/azurepipeline_test.go | 5 +- .../crafter/runners/circleci_build.go | 22 +++---- .../crafter/runners/circleci_build_test.go | 4 +- .../attestation/crafter/runners/generic.go | 21 ++----- .../crafter/runners/generic_test.go | 14 +---- .../crafter/runners/githubaction.go | 28 ++++----- .../crafter/runners/githubaction_test.go | 4 +- .../crafter/runners/gitlabpipeline.go | 30 +++++----- .../crafter/runners/gitlabpipeline_test.go | 4 +- .../attestation/crafter/runners/jenkinsjob.go | 26 ++++----- .../crafter/runners/jenkinsjob_test.go | 6 +- .../attestation/crafter/runners/runner.go | 43 ++++++++++++++ 16 files changed, 189 insertions(+), 153 deletions(-) create mode 100644 internal/attestation/crafter/runners/runner.go diff --git a/internal/attestation/crafter/crafter.go b/internal/attestation/crafter/crafter.go index 0c2b2d3e4..884093d4f 100644 --- a/internal/attestation/crafter/crafter.go +++ b/internal/attestation/crafter/crafter.go @@ -372,49 +372,44 @@ func persistCraftingState(craftState *api.CraftingState, stateFilePath string) e // ResolveEnvVars will iterate on the env vars in the allow list and resolve them from the system context // strict indicates if it should fail if any env variable can not be found -func (c *Crafter) ResolveEnvVars(strict bool) error { +func (c *Crafter) ResolveEnvVars() error { if err := c.requireStateLoaded(); err != nil { return err } - // Runner specific env variables + // Runner specific environment variables + c.logger.Debug().Str("runnerType", c.Runner.String()).Msg("loading runner specific env variables") var outputEnvVars = make(map[string]string) if !c.Runner.CheckEnv() { errorStr := fmt.Sprintf("couldn't detect the environment %q. Is the crafting process happening in the target env?", c.Runner.String()) - if strict { - return fmt.Errorf("%s - %w", errorStr, ErrRunnerContextNotFound) - } - c.logger.Warn().Msg(errorStr) - } else { - c.logger.Debug().Str("runnerType", c.Runner.String()).Strs("variables", c.Runner.ListEnvVars()).Msg("list of env variables to automatically extract") - outputEnvVars = c.Runner.ResolveEnvVars() - if notFound := notResolvedVars(outputEnvVars, c.Runner.ListEnvVars()); len(notFound) > 0 { - if strict { - return fmt.Errorf("required env variables not present %q", notFound) - } - c.logger.Warn().Strs("key", notFound).Msg("required env variables not present") - } + return fmt.Errorf("%s - %w", errorStr, ErrRunnerContextNotFound) } - // User-defined env vars - varsAllowList := c.CraftingState.InputSchema.EnvAllowList - if len(varsAllowList) > 0 { - c.logger.Debug().Strs("allowList", varsAllowList).Msg("loading env variables") - for _, want := range varsAllowList { - val := os.Getenv(want) - if val == "" { - continue - } + // Workflow run environment variables - outputEnvVars[want] = val - } + var varNames []string + for _, envVarDef := range c.Runner.ListEnvVars() { + varNames = append(varNames, envVarDef.Name) + } + c.logger.Debug().Str("runnerType", c.Runner.String()).Strs("variables", varNames).Msg("list of env variables to automatically extract") - if notFound := notResolvedVars(outputEnvVars, varsAllowList); len(notFound) > 0 { - if strict { - return fmt.Errorf("required env variables not present %q", notFound) - } - c.logger.Warn().Strs("key", notFound).Msg("required env variables not present") + outputEnvVars, err := c.Runner.ResolveEnvVars() + if err != nil { + return err + } + + // User-defined environment vars + + if len(c.CraftingState.InputSchema.EnvAllowList) > 0 { + c.logger.Debug().Strs("allowList", c.CraftingState.InputSchema.EnvAllowList).Msg("loading env variables") + } + for _, want := range c.CraftingState.InputSchema.EnvAllowList { + val := os.Getenv(want) + if val != "" { + outputEnvVars[want] = val + } else if !c.CraftingState.DryRun { + return fmt.Errorf("required env variables not present %q", want) } } diff --git a/internal/attestation/crafter/crafter_test.go b/internal/attestation/crafter/crafter_test.go index aa5dcb369..da3130c93 100644 --- a/internal/attestation/crafter/crafter_test.go +++ b/internal/attestation/crafter/crafter_test.go @@ -222,24 +222,27 @@ func (s *crafterSuite) TestLoadSchema() { func (s *crafterSuite) TestResolveEnvVars() { testCases := []struct { name string - strict bool + dryrun bool + // Custom env variables to expose envVars map[string]string + // Simulate that the crafting process is hapenning in a github action runner inGithubEnv bool wantErr bool + // Total list of resolved env vars want map[string]string }{ { - name: "strict missing custom vars", - strict: true, + name: "dryrun missing custom vars", + dryrun: true, inGithubEnv: true, wantErr: true, }, { - name: "strict, not running in github env", - strict: true, + name: "dryrun, not running in github env", + dryrun: true, inGithubEnv: false, wantErr: true, envVars: map[string]string{ @@ -248,8 +251,8 @@ func (s *crafterSuite) TestResolveEnvVars() { }, }, { - name: "strict, missing some github env", - strict: true, + name: "dryrun, missing some github env", + dryrun: true, inGithubEnv: false, wantErr: true, envVars: map[string]string{ @@ -260,8 +263,8 @@ func (s *crafterSuite) TestResolveEnvVars() { }, }, { - name: "strict and all envs available", - strict: true, + name: "dryrun and all envs available", + dryrun: true, inGithubEnv: true, envVars: map[string]string{ "CUSTOM_VAR_1": "custom_value_1", @@ -281,15 +284,15 @@ func (s *crafterSuite) TestResolveEnvVars() { }, }, { - name: "non strict missing custom vars", - strict: false, + name: "non dryrun missing custom vars", + dryrun: false, inGithubEnv: true, wantErr: false, want: gitHubTestingEnvVars, }, { - name: "non strict, missing some github env", - strict: false, + name: "non dryrun, missing some github env", + dryrun: false, inGithubEnv: false, wantErr: false, envVars: map[string]string{ @@ -313,8 +316,8 @@ func (s *crafterSuite) TestResolveEnvVars() { }, }, { - name: "non strict, wrong runner context", - strict: false, + name: "non dryrun, wrong runner context", + dryrun: false, inGithubEnv: false, wantErr: false, envVars: map[string]string{ @@ -334,6 +337,7 @@ func (s *crafterSuite) TestResolveEnvVars() { for k, v := range tc.envVars { s.T().Setenv(k, v) } + // Runner env variables if tc.inGithubEnv { s.T().Setenv("CI", "true") @@ -342,10 +346,10 @@ func (s *crafterSuite) TestResolveEnvVars() { } } - c, err := newInitializedCrafter(s.T(), "testdata/contracts/with_env_vars.yaml", &v1.WorkflowMetadata{}, true, "") + c, err := newInitializedCrafter(s.T(), "testdata/contracts/with_env_vars.yaml", &v1.WorkflowMetadata{}, tc.dryrun, "") require.NoError(s.T(), err) - err = c.ResolveEnvVars(tc.strict) + err = c.ResolveEnvVars() if tc.wantErr { s.Error(err) return diff --git a/internal/attestation/crafter/runner.go b/internal/attestation/crafter/runner.go index d499e248d..5c2b32ffe 100644 --- a/internal/attestation/crafter/runner.go +++ b/internal/attestation/crafter/runner.go @@ -27,11 +27,15 @@ var ErrRunnerContextNotFound = errors.New("the runner environment doesn't match type supportedRunner interface { // Whether the attestation is happening in this environment CheckEnv() bool + // List the env variables registered - ListEnvVars() []string + ListEnvVars() []*runners.EnvVarDefinition + // Return the list of env vars associated with this runner already resolved - ResolveEnvVars() map[string]string + ResolveEnvVars() (map[string]string, error) + String() string + // uri to the running job/workload RunURI() string } diff --git a/internal/attestation/crafter/runners/azurepipeline.go b/internal/attestation/crafter/runners/azurepipeline.go index 9f6aa77c5..b06fe563c 100644 --- a/internal/attestation/crafter/runners/azurepipeline.go +++ b/internal/attestation/crafter/runners/azurepipeline.go @@ -40,25 +40,21 @@ func (r *AzurePipeline) CheckEnv() bool { return true } -func (r *AzurePipeline) ListEnvVars() []string { - return []string{ - "BUILD_REQUESTEDFOREMAIL", - "BUILD_REQUESTEDFOR", - "BUILD_REPOSITORY_URI", - "BUILD_REPOSITORY_NAME", - "BUILD_BUILDID", - "BUILD_BUILDNUMBER", - "BUILD_BUILDURI", - "BUILD_REASON", - "AGENT_VERSION", - "TF_BUILD", +func (r *AzurePipeline) ListEnvVars() []*EnvVarDefinition { + return []*EnvVarDefinition{ + {"BUILD_REQUESTEDFOREMAIL", false}, + {"BUILD_REQUESTEDFOR", false}, + {"BUILD_REPOSITORY_URI", false}, + {"BUILD_REPOSITORY_NAME", false}, + {"BUILD_BUILDID", false}, + {"BUILD_BUILDNUMBER", false}, + {"BUILD_BUILDURI", false}, + {"BUILD_REASON", false}, + {"AGENT_VERSION", false}, + {"TF_BUILD", false}, } } -func (r *AzurePipeline) ResolveEnvVars() map[string]string { - return resolveEnvVars(r.ListEnvVars()) -} - func (r *AzurePipeline) String() string { return AzurePipelineID } @@ -84,3 +80,7 @@ func (r *AzurePipeline) RunURI() (url string) { return uri.String() } + +func (r *AzurePipeline) ResolveEnvVars() (map[string]string, error) { + return resolveEnvVars(r.ListEnvVars()) +} diff --git a/internal/attestation/crafter/runners/azurepipeline_test.go b/internal/attestation/crafter/runners/azurepipeline_test.go index cd821737e..7d47b1d41 100644 --- a/internal/attestation/crafter/runners/azurepipeline_test.go +++ b/internal/attestation/crafter/runners/azurepipeline_test.go @@ -93,6 +93,9 @@ func (s *azurePipelineSuite) TestListEnvVars() { } func (s *azurePipelineSuite) TestResolveEnvVars() { + resolvedEnvVars, err := s.runner.ResolveEnvVars() + s.NoError(err) + s.Equal(map[string]string{ "AGENT_VERSION": "3.220.5", "BUILD_BUILDID": "6", @@ -104,7 +107,7 @@ func (s *azurePipelineSuite) TestResolveEnvVars() { "BUILD_REQUESTEDFOR": "Jan Kowalsky", "BUILD_REQUESTEDFOREMAIL": "jan@kowalscy.onmicrosoft.com", "TF_BUILD": "True", - }, s.runner.ResolveEnvVars()) + }, resolvedEnvVars) } func (s *azurePipelineSuite) TestRunURI() { diff --git a/internal/attestation/crafter/runners/circleci_build.go b/internal/attestation/crafter/runners/circleci_build.go index 32bbb1b0c..7295d449a 100644 --- a/internal/attestation/crafter/runners/circleci_build.go +++ b/internal/attestation/crafter/runners/circleci_build.go @@ -35,25 +35,21 @@ func (r *CircleCIBuild) CheckEnv() bool { return true } -func (r *CircleCIBuild) ListEnvVars() []string { - return []string{ +func (r *CircleCIBuild) ListEnvVars() []*EnvVarDefinition { + return []*EnvVarDefinition{ // Some info about the job - "CIRCLE_BUILD_URL", - "CIRCLE_JOB", + {"CIRCLE_BUILD_URL", false}, + {"CIRCLE_JOB", false}, // Some info about the commit - "CIRCLE_BRANCH", + {"CIRCLE_BRANCH", false}, // Some info about the agent - "CIRCLE_NODE_TOTAL", - "CIRCLE_NODE_INDEX", + {"CIRCLE_NODE_TOTAL", false}, + {"CIRCLE_NODE_INDEX", false}, } } -func (r *CircleCIBuild) ResolveEnvVars() map[string]string { - return resolveEnvVars(r.ListEnvVars()) -} - func (r *CircleCIBuild) String() string { return JenkinsJobID } @@ -61,3 +57,7 @@ func (r *CircleCIBuild) String() string { func (r *CircleCIBuild) RunURI() string { return os.Getenv("CIRCLE_BUILD_URL") } + +func (r *CircleCIBuild) ResolveEnvVars() (map[string]string, error) { + return resolveEnvVars(r.ListEnvVars()) +} diff --git a/internal/attestation/crafter/runners/circleci_build_test.go b/internal/attestation/crafter/runners/circleci_build_test.go index ced160b1c..142830384 100644 --- a/internal/attestation/crafter/runners/circleci_build_test.go +++ b/internal/attestation/crafter/runners/circleci_build_test.go @@ -98,7 +98,9 @@ func (s *circleCIBuildSuite) TestListEnvVars() { } func (s *circleCIBuildSuite) TestResolveEnvVars() { - s.Equal(circleCIBuildTestingEnvVars, s.runner.ResolveEnvVars()) + resolvedEnvVars, err := s.runner.ResolveEnvVars() + s.NoError(err) + s.Equal(circleCIBuildTestingEnvVars, resolvedEnvVars) } func (s *circleCIBuildSuite) TestRunURI() { diff --git a/internal/attestation/crafter/runners/generic.go b/internal/attestation/crafter/runners/generic.go index c60e90af5..85119e178 100644 --- a/internal/attestation/crafter/runners/generic.go +++ b/internal/attestation/crafter/runners/generic.go @@ -15,10 +15,6 @@ package runners -import ( - "os" -) - type Generic struct{} const GenericID = "generic" @@ -33,12 +29,8 @@ func (r *Generic) CheckEnv() bool { // Returns a list of environment variables names. This list is used to // automatically inject environment variables into the attestation. -func (r *Generic) ListEnvVars() []string { - return []string{} -} - -func (r *Generic) ResolveEnvVars() map[string]string { - return make(map[string]string) +func (r *Generic) ListEnvVars() []*EnvVarDefinition { + return []*EnvVarDefinition{} } func (r *Generic) String() string { @@ -49,11 +41,6 @@ func (r *Generic) RunURI() string { return "" } -func resolveEnvVars(varsL []string) map[string]string { - res := make(map[string]string) - for _, name := range varsL { - e := os.Getenv(name) - res[name] = e - } - return res +func (r *Generic) ResolveEnvVars() (map[string]string, error) { + return resolveEnvVars(r.ListEnvVars()) } diff --git a/internal/attestation/crafter/runners/generic_test.go b/internal/attestation/crafter/runners/generic_test.go index 55ec99d5d..121d88164 100644 --- a/internal/attestation/crafter/runners/generic_test.go +++ b/internal/attestation/crafter/runners/generic_test.go @@ -18,7 +18,6 @@ package runners import ( "testing" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/suite" ) @@ -36,7 +35,9 @@ func (s *genericSuite) TestListEnvVars() { } func (s *genericSuite) TestResolveEnvVars() { - s.Equal(map[string]string{}, s.runner.ResolveEnvVars()) + result, err := s.runner.ResolveEnvVars() + s.NoError(err) + s.Equal(map[string]string{}, result) } func (s *genericSuite) TestRunURI() { @@ -56,12 +57,3 @@ func (s *genericSuite) SetupTest() { func TestGenericRunner(t *testing.T) { suite.Run(t, new(genericSuite)) } - -func TestResolveEnvVars(t *testing.T) { - t.Setenv("A", "a") - t.Setenv("C", "c") - t.Setenv("D", "d") - varsL := []string{"A", "B", "C"} - res := resolveEnvVars(varsL) - assert.Equal(t, map[string]string{"A": "a", "B": "", "C": "c"}, res) -} diff --git a/internal/attestation/crafter/runners/githubaction.go b/internal/attestation/crafter/runners/githubaction.go index b5fc4a9f3..e4b911795 100644 --- a/internal/attestation/crafter/runners/githubaction.go +++ b/internal/attestation/crafter/runners/githubaction.go @@ -39,23 +39,19 @@ func (r *GitHubAction) CheckEnv() bool { return true } -func (r *GitHubAction) ListEnvVars() []string { - return []string{ - "GITHUB_ACTOR", - "GITHUB_REF", - "GITHUB_REPOSITORY", - "GITHUB_REPOSITORY_OWNER", - "GITHUB_RUN_ID", - "GITHUB_SHA", - "RUNNER_NAME", - "RUNNER_OS", +func (r *GitHubAction) ListEnvVars() []*EnvVarDefinition { + return []*EnvVarDefinition{ + {"GITHUB_ACTOR", false}, + {"GITHUB_REF", false}, + {"GITHUB_REPOSITORY", false}, + {"GITHUB_REPOSITORY_OWNER", false}, + {"GITHUB_RUN_ID", false}, + {"GITHUB_SHA", false}, + {"RUNNER_NAME", false}, + {"RUNNER_OS", false}, } } -func (r *GitHubAction) ResolveEnvVars() map[string]string { - return resolveEnvVars(r.ListEnvVars()) -} - func (r *GitHubAction) String() string { return GitHubActionID } @@ -68,3 +64,7 @@ func (r *GitHubAction) RunURI() (url string) { return url } + +func (r *GitHubAction) ResolveEnvVars() (map[string]string, error) { + return resolveEnvVars(r.ListEnvVars()) +} diff --git a/internal/attestation/crafter/runners/githubaction_test.go b/internal/attestation/crafter/runners/githubaction_test.go index 8a19711f0..05f802239 100644 --- a/internal/attestation/crafter/runners/githubaction_test.go +++ b/internal/attestation/crafter/runners/githubaction_test.go @@ -102,7 +102,9 @@ func (s *githubActionSuite) TestListEnvVars() { } func (s *githubActionSuite) TestResolveEnvVars() { - s.Equal(gitHubTestingEnvVars, s.runner.ResolveEnvVars()) + resolvedEnvVars, err := s.runner.ResolveEnvVars() + s.NoError(err) + s.Equal(gitHubTestingEnvVars, resolvedEnvVars) } func (s *githubActionSuite) TestRunURI() { diff --git a/internal/attestation/crafter/runners/gitlabpipeline.go b/internal/attestation/crafter/runners/gitlabpipeline.go index 4373783f1..16673922a 100644 --- a/internal/attestation/crafter/runners/gitlabpipeline.go +++ b/internal/attestation/crafter/runners/gitlabpipeline.go @@ -38,24 +38,20 @@ func (r *GitlabPipeline) CheckEnv() bool { return true } -func (r *GitlabPipeline) ListEnvVars() []string { - return []string{ - "GITLAB_USER_EMAIL", - "GITLAB_USER_LOGIN", - "CI_PROJECT_URL", - "CI_COMMIT_SHA", - "CI_JOB_URL", - "CI_PIPELINE_URL", - "CI_RUNNER_VERSION", - "CI_RUNNER_DESCRIPTION", - "CI_COMMIT_REF_NAME", +func (r *GitlabPipeline) ListEnvVars() []*EnvVarDefinition { + return []*EnvVarDefinition{ + {"GITLAB_USER_EMAIL", false}, + {"GITLAB_USER_LOGIN", false}, + {"CI_PROJECT_URL", false}, + {"CI_COMMIT_SHA", false}, + {"CI_JOB_URL", false}, + {"CI_PIPELINE_URL", false}, + {"CI_RUNNER_VERSION", false}, + {"CI_RUNNER_DESCRIPTION", false}, + {"CI_COMMIT_REF_NAME", false}, } } -func (r *GitlabPipeline) ResolveEnvVars() map[string]string { - return resolveEnvVars(r.ListEnvVars()) -} - func (r *GitlabPipeline) String() string { return GitlabPipelineID } @@ -63,3 +59,7 @@ func (r *GitlabPipeline) String() string { func (r *GitlabPipeline) RunURI() (url string) { return os.Getenv("CI_JOB_URL") } + +func (r *GitlabPipeline) ResolveEnvVars() (map[string]string, error) { + return resolveEnvVars(r.ListEnvVars()) +} diff --git a/internal/attestation/crafter/runners/gitlabpipeline_test.go b/internal/attestation/crafter/runners/gitlabpipeline_test.go index 2480b49ad..d48c29b98 100644 --- a/internal/attestation/crafter/runners/gitlabpipeline_test.go +++ b/internal/attestation/crafter/runners/gitlabpipeline_test.go @@ -92,6 +92,8 @@ func (s *gitlabPipelineSuite) TestListEnvVars() { } func (s *gitlabPipelineSuite) TestResolveEnvVars() { + resolvedEnvVars, err := s.runner.ResolveEnvVars() + s.NoError(err) s.Equal(map[string]string{ "GITLAB_USER_EMAIL": "foo@foo.com", "GITLAB_USER_LOGIN": "foo", @@ -102,7 +104,7 @@ func (s *gitlabPipelineSuite) TestResolveEnvVars() { "CI_RUNNER_VERSION": "13.10.0", "CI_RUNNER_DESCRIPTION": "chainloop-runner", "CI_COMMIT_REF_NAME": "main", - }, s.runner.ResolveEnvVars()) + }, resolvedEnvVars) } func (s *gitlabPipelineSuite) TestRunURI() { diff --git a/internal/attestation/crafter/runners/jenkinsjob.go b/internal/attestation/crafter/runners/jenkinsjob.go index 85c80410a..62e10bc9e 100644 --- a/internal/attestation/crafter/runners/jenkinsjob.go +++ b/internal/attestation/crafter/runners/jenkinsjob.go @@ -36,28 +36,22 @@ func (r *JenkinsJob) CheckEnv() bool { return true } -func (r *JenkinsJob) ListEnvVars() []string { - return []string{ +func (r *JenkinsJob) ListEnvVars() []*EnvVarDefinition { + return []*EnvVarDefinition{ // Some info about the job - "JOB_NAME", - "BUILD_URL", + {"JOB_NAME", false}, + {"BUILD_URL", false}, // Some info about the commit (Jenkins Git Plugin) - // NOTE: Commenting these vars out because they are not always present - // and current chainloop behavior requires these to be set. - // "GIT_BRANCH", - // "GIT_COMMIT", + {"GIT_BRANCH", true}, + {"GIT_COMMIT", true}, // Some info about the agent - "AGENT_WORKDIR", - "NODE_NAME", + {"AGENT_WORKDIR", false}, + {"NODE_NAME", false}, } } -func (r *JenkinsJob) ResolveEnvVars() map[string]string { - return resolveEnvVars(r.ListEnvVars()) -} - func (r *JenkinsJob) String() string { return JenkinsJobID } @@ -65,3 +59,7 @@ func (r *JenkinsJob) String() string { func (r *JenkinsJob) RunURI() string { return os.Getenv("BUILD_URL") } + +func (r *JenkinsJob) ResolveEnvVars() (map[string]string, error) { + return resolveEnvVars(r.ListEnvVars()) +} diff --git a/internal/attestation/crafter/runners/jenkinsjob_test.go b/internal/attestation/crafter/runners/jenkinsjob_test.go index 37dbf7d67..9ed767050 100644 --- a/internal/attestation/crafter/runners/jenkinsjob_test.go +++ b/internal/attestation/crafter/runners/jenkinsjob_test.go @@ -86,7 +86,9 @@ func (s *jenkinsJobSuite) TestListEnvVars() { } func (s *jenkinsJobSuite) TestResolveEnvVars() { - s.Equal(jenkinsJobTestingEnvVars, s.runner.ResolveEnvVars()) + resolvedEnvVars, err := s.runner.ResolveEnvVars() + s.NoError(err) + s.Equal(jenkinsJobTestingEnvVars, resolvedEnvVars) } func (s *jenkinsJobSuite) TestRunURI() { @@ -111,6 +113,8 @@ var jenkinsJobTestingEnvVars = map[string]string{ "BUILD_URL": "http://some-build-url/", "AGENT_WORKDIR": "/home/sample/agent", "NODE_NAME": "some-node", + "GIT_BRANCH": "somebranch", + "GIT_COMMIT": "somecommit", } // Run the tests diff --git a/internal/attestation/crafter/runners/runner.go b/internal/attestation/crafter/runners/runner.go new file mode 100644 index 000000000..b5c25c879 --- /dev/null +++ b/internal/attestation/crafter/runners/runner.go @@ -0,0 +1,43 @@ +// +// Copyright 2023 The Chainloop Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package runners + +import ( + "fmt" + "os" +) + +type EnvVarDefinition struct { + Name string + Optional bool +} + +func resolveEnvVars(envVarsDefinitions []*EnvVarDefinition) (map[string]string, error) { + result := make(map[string]string) + + for _, envVarDef := range envVarsDefinitions { + value := os.Getenv(envVarDef.Name) + if value != "" { + result[envVarDef.Name] = value + } else { + if !envVarDef.Optional { + return nil, fmt.Errorf("environment variable %s cannot be resolved", envVarDef.Name) + } + } + } + + return result, nil +} From 7b1c245392a2849c02f5bb7128e6a308a01413cb Mon Sep 17 00:00:00 2001 From: Mattia Buccarella Date: Fri, 17 Nov 2023 11:26:41 +0100 Subject: [PATCH 02/12] fix params Signed-off-by: Mattia Buccarella --- app/cli/internal/action/attestation_init.go | 2 +- app/cli/internal/action/attestation_status.go | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/app/cli/internal/action/attestation_init.go b/app/cli/internal/action/attestation_init.go index 56dd9b075..311f17f4f 100644 --- a/app/cli/internal/action/attestation_init.go +++ b/app/cli/internal/action/attestation_init.go @@ -123,7 +123,7 @@ func (action *AttestationInit) Run(contractRevision int) error { } // Load the env variables both the system populated and the user predefined ones - if err := action.c.ResolveEnvVars(!action.dryRun); err != nil { + if err := action.c.ResolveEnvVars(); err != nil { _ = action.c.Reset() return err } diff --git a/app/cli/internal/action/attestation_status.go b/app/cli/internal/action/attestation_status.go index 130680bd7..cfab66ac5 100644 --- a/app/cli/internal/action/attestation_status.go +++ b/app/cli/internal/action/attestation_status.go @@ -124,8 +124,14 @@ func (action *AttestationStatus) Run() (*AttestationStatusResult, error) { } res.EnvVars = envVars + + runnerEnvVars, err := c.Runner.ResolveEnvVars() + if err != nil { + return nil, err + } + res.RunnerContext = &AttestationResultRunnerContext{ - EnvVars: c.Runner.ResolveEnvVars(), + EnvVars: runnerEnvVars, RunnerType: att.RunnerType.String(), JobURL: att.RunnerUrl, } From c16973e902a99f7b2df40c5abc4f7e23d6470394 Mon Sep 17 00:00:00 2001 From: Mattia Buccarella Date: Fri, 17 Nov 2023 16:37:18 +0100 Subject: [PATCH 03/12] finishing up Signed-off-by: Mattia Buccarella --- app/cli/internal/action/attestation_init.go | 4 + internal/attestation/crafter/crafter.go | 15 +- internal/attestation/crafter/crafter_test.go | 156 +++++++----------- .../crafter/runners/azurepipeline_test.go | 22 +-- .../crafter/runners/circleci_build_test.go | 12 +- .../crafter/runners/githubaction_test.go | 18 +- .../crafter/runners/gitlabpipeline_test.go | 20 +-- .../crafter/runners/jenkinsjob_test.go | 12 +- .../contracts/jenkins_with_env_vars.yaml | 8 + 9 files changed, 118 insertions(+), 149 deletions(-) create mode 100644 internal/attestation/crafter/testdata/contracts/jenkins_with_env_vars.yaml diff --git a/app/cli/internal/action/attestation_init.go b/app/cli/internal/action/attestation_init.go index 311f17f4f..fe289e7bc 100644 --- a/app/cli/internal/action/attestation_init.go +++ b/app/cli/internal/action/attestation_init.go @@ -124,6 +124,10 @@ func (action *AttestationInit) Run(contractRevision int) error { // Load the env variables both the system populated and the user predefined ones if err := action.c.ResolveEnvVars(); err != nil { + if action.dryRun { + return nil + } + _ = action.c.Reset() return err } diff --git a/internal/attestation/crafter/crafter.go b/internal/attestation/crafter/crafter.go index 884093d4f..a2321830b 100644 --- a/internal/attestation/crafter/crafter.go +++ b/internal/attestation/crafter/crafter.go @@ -109,7 +109,7 @@ func (c *Crafter) Init(opts *InitOpts) error { // Check that the initialization is happening in the right environment runnerType := opts.SchemaV1.Runner.GetType() runnerContext := NewRunner(runnerType) - if !opts.DryRun && !runnerContext.CheckEnv() { + if !runnerContext.CheckEnv() { return fmt.Errorf("%w, expected %s", ErrRunnerContextNotFound, runnerType) } @@ -408,7 +408,7 @@ func (c *Crafter) ResolveEnvVars() error { val := os.Getenv(want) if val != "" { outputEnvVars[want] = val - } else if !c.CraftingState.DryRun { + } else { return fmt.Errorf("required env variables not present %q", want) } } @@ -422,17 +422,6 @@ func (c *Crafter) ResolveEnvVars() error { return nil } -func notResolvedVars(resolved map[string]string, wantList []string) []string { - var notFound []string - for _, want := range wantList { - if val, ok := resolved[want]; !ok || val == "" { - notFound = append(notFound, want) - } - } - - return notFound -} - // Inject material to attestation state func (c *Crafter) AddMaterial(key, value string, casBackend *casclient.CASBackend, runtimeAnnotations map[string]string) error { if err := c.requireStateLoaded(); err != nil { diff --git a/internal/attestation/crafter/crafter_test.go b/internal/attestation/crafter/crafter_test.go index da3130c93..fc88d4576 100644 --- a/internal/attestation/crafter/crafter_test.go +++ b/internal/attestation/crafter/crafter_test.go @@ -221,142 +221,108 @@ func (s *crafterSuite) TestLoadSchema() { func (s *crafterSuite) TestResolveEnvVars() { testCases := []struct { - name string - dryrun bool + name string // Custom env variables to expose envVars map[string]string - // Simulate that the crafting process is hapenning in a github action runner - inGithubEnv bool - wantErr bool + // Simulate that the crafting process is hapenning in a specific runner + inGithubEnv bool + inJenkinsEnv bool - // Total list of resolved env vars - want map[string]string + expectedError string + expectedEnvVars map[string]string }{ { - name: "dryrun missing custom vars", - dryrun: true, - inGithubEnv: true, - wantErr: true, - }, - { - name: "dryrun, not running in github env", - dryrun: true, - inGithubEnv: false, - wantErr: true, + name: "missing custom vars", + inGithubEnv: true, + expectedError: "required env variables not present \"CUSTOM_VAR_1\"", + }, { + name: "missing some github runner env", + inGithubEnv: true, + expectedError: "environment variable GITHUB_ACTOR cannot be resolved", envVars: map[string]string{ "CUSTOM_VAR_1": "custom_value_1", "CUSTOM_VAR_2": "custom_value_2", + "GITHUB_ACTOR": "", // This is removing one necessary variable }, - }, - { - name: "dryrun, missing some github env", - dryrun: true, - inGithubEnv: false, - wantErr: true, - envVars: map[string]string{ - "CUSTOM_VAR_1": "custom_value_1", - "CUSTOM_VAR_2": "custom_value_2", - "CI": "true", - "GITHUB_RUN_ID": "123", - }, - }, - { - name: "dryrun and all envs available", - dryrun: true, - inGithubEnv: true, + }, { + name: "missing optional jenkins variable with no error", + inJenkinsEnv: true, + expectedError: "", envVars: map[string]string{ + // Missing var: GIT_BRANCH "CUSTOM_VAR_1": "custom_value_1", "CUSTOM_VAR_2": "custom_value_2", }, - want: map[string]string{ - "CUSTOM_VAR_1": "custom_value_1", - "CUSTOM_VAR_2": "custom_value_2", - "GITHUB_REPOSITORY": "chainloop/chainloop", - "GITHUB_RUN_ID": "123", - "GITHUB_ACTOR": "chainloop", - "GITHUB_REF": "refs/heads/main", - "GITHUB_REPOSITORY_OWNER": "chainloop", - "GITHUB_SHA": "1234567890", - "RUNNER_NAME": "chainloop-runner", - "RUNNER_OS": "linux", + expectedEnvVars: map[string]string{ + // Missing var: GIT_BRANCH + "CUSTOM_VAR_1": "custom_value_1", + "CUSTOM_VAR_2": "custom_value_2", + "JOB_NAME": "some-job", + "BUILD_URL": "http://some-url", + "AGENT_WORKDIR": "/some/home/dir", + "NODE_NAME": "some-node", }, - }, - { - name: "non dryrun missing custom vars", - dryrun: false, - inGithubEnv: true, - wantErr: false, - want: gitHubTestingEnvVars, - }, - { - name: "non dryrun, missing some github env", - dryrun: false, - inGithubEnv: false, - wantErr: false, - envVars: map[string]string{ - "CUSTOM_VAR_1": "custom_value_1", - "CUSTOM_VAR_2": "custom_value_2", - "CI": "true", - "GITHUB_RUN_ID": "123", - "GITHUB_REPOSITORY": "chainloop/chainloop", - }, - want: map[string]string{ - "CUSTOM_VAR_1": "custom_value_1", - "CUSTOM_VAR_2": "custom_value_2", - "GITHUB_REPOSITORY": "chainloop/chainloop", - "GITHUB_RUN_ID": "123", - "GITHUB_ACTOR": "", - "GITHUB_REF": "", - "GITHUB_REPOSITORY_OWNER": "", - "GITHUB_SHA": "", - "RUNNER_NAME": "", - "RUNNER_OS": "", - }, - }, - { - name: "non dryrun, wrong runner context", - dryrun: false, - inGithubEnv: false, - wantErr: false, + }, { + name: "all optional jenkins variable with no error", + inJenkinsEnv: true, + expectedError: "", envVars: map[string]string{ + "GIT_BRANCH": "some-branch", // optional var 1 + "GIT_COMMIT": "some-commit", // optional var 2 "CUSTOM_VAR_1": "custom_value_1", "CUSTOM_VAR_2": "custom_value_2", }, - want: map[string]string{ - "CUSTOM_VAR_1": "custom_value_1", - "CUSTOM_VAR_2": "custom_value_2", + expectedEnvVars: map[string]string{ + "GIT_BRANCH": "some-branch", // optional var 1 + "GIT_COMMIT": "some-commit", // optional var 2 + "CUSTOM_VAR_1": "custom_value_1", + "CUSTOM_VAR_2": "custom_value_2", + "JOB_NAME": "some-job", + "BUILD_URL": "http://some-url", + "AGENT_WORKDIR": "/some/home/dir", + "NODE_NAME": "some-node", }, }, } for _, tc := range testCases { s.Run(tc.name, func() { - // Customs env vars - for k, v := range tc.envVars { - s.T().Setenv(k, v) - } - - // Runner env variables + contract := "testdata/contracts/with_env_vars.yaml" if tc.inGithubEnv { s.T().Setenv("CI", "true") for k, v := range gitHubTestingEnvVars { s.T().Setenv(k, v) } + } else if tc.inJenkinsEnv { + contract = "testdata/contracts/jenkins_with_env_vars.yaml" + s.T().Setenv("JOB_NAME", "some-job") + s.T().Setenv("BUILD_URL", "http://some-url") + s.T().Setenv("AGENT_WORKDIR", "/some/home/dir") + s.T().Setenv("NODE_NAME", "some-node") + s.T().Setenv("JENKINS_HOME", "/some/home/dir") } - c, err := newInitializedCrafter(s.T(), "testdata/contracts/with_env_vars.yaml", &v1.WorkflowMetadata{}, tc.dryrun, "") + // Customs env vars + for k, v := range tc.envVars { + s.T().Setenv(k, v) + } + + c, err := newInitializedCrafter(s.T(), contract, &v1.WorkflowMetadata{}, false, "") require.NoError(s.T(), err) err = c.ResolveEnvVars() - if tc.wantErr { + + if tc.expectedError != "" { s.Error(err) + actualError := err.Error() + s.Equal(tc.expectedError, actualError) return } s.NoError(err) - s.Equal(tc.want, c.CraftingState.Attestation.EnvVars) + s.Equal(tc.expectedEnvVars, c.CraftingState.Attestation.EnvVars) }) } } diff --git a/internal/attestation/crafter/runners/azurepipeline_test.go b/internal/attestation/crafter/runners/azurepipeline_test.go index 7d47b1d41..9b74fe6ca 100644 --- a/internal/attestation/crafter/runners/azurepipeline_test.go +++ b/internal/attestation/crafter/runners/azurepipeline_test.go @@ -78,17 +78,17 @@ func (s *azurePipelineSuite) TestCheckEnv() { } func (s *azurePipelineSuite) TestListEnvVars() { - assert.Equal(s.T(), []string{ - "BUILD_REQUESTEDFOREMAIL", - "BUILD_REQUESTEDFOR", - "BUILD_REPOSITORY_URI", - "BUILD_REPOSITORY_NAME", - "BUILD_BUILDID", - "BUILD_BUILDNUMBER", - "BUILD_BUILDURI", - "BUILD_REASON", - "AGENT_VERSION", - "TF_BUILD", + assert.Equal(s.T(), []*EnvVarDefinition{ + {"BUILD_REQUESTEDFOREMAIL", false}, + {"BUILD_REQUESTEDFOR", false}, + {"BUILD_REPOSITORY_URI", false}, + {"BUILD_REPOSITORY_NAME", false}, + {"BUILD_BUILDID", false}, + {"BUILD_BUILDNUMBER", false}, + {"BUILD_BUILDURI", false}, + {"BUILD_REASON", false}, + {"AGENT_VERSION", false}, + {"TF_BUILD", false}, }, s.runner.ListEnvVars()) } diff --git a/internal/attestation/crafter/runners/circleci_build_test.go b/internal/attestation/crafter/runners/circleci_build_test.go index 142830384..14cb64cda 100644 --- a/internal/attestation/crafter/runners/circleci_build_test.go +++ b/internal/attestation/crafter/runners/circleci_build_test.go @@ -88,12 +88,12 @@ func (s *circleCIBuildSuite) TestCheckEnv() { } func (s *circleCIBuildSuite) TestListEnvVars() { - s.Equal([]string{ - "CIRCLE_BUILD_URL", - "CIRCLE_JOB", - "CIRCLE_BRANCH", - "CIRCLE_NODE_TOTAL", - "CIRCLE_NODE_INDEX", + s.Equal([]*EnvVarDefinition{ + {"CIRCLE_BUILD_URL", false}, + {"CIRCLE_JOB", false}, + {"CIRCLE_BRANCH", false}, + {"CIRCLE_NODE_TOTAL", false}, + {"CIRCLE_NODE_INDEX", false}, }, s.runner.ListEnvVars()) } diff --git a/internal/attestation/crafter/runners/githubaction_test.go b/internal/attestation/crafter/runners/githubaction_test.go index 05f802239..1a8b5ada2 100644 --- a/internal/attestation/crafter/runners/githubaction_test.go +++ b/internal/attestation/crafter/runners/githubaction_test.go @@ -89,15 +89,15 @@ func (s *githubActionSuite) TestCheckEnv() { } func (s *githubActionSuite) TestListEnvVars() { - s.Equal([]string{ - "GITHUB_ACTOR", - "GITHUB_REF", - "GITHUB_REPOSITORY", - "GITHUB_REPOSITORY_OWNER", - "GITHUB_RUN_ID", - "GITHUB_SHA", - "RUNNER_NAME", - "RUNNER_OS", + s.Equal([]*EnvVarDefinition{ + {"GITHUB_ACTOR", false}, + {"GITHUB_REF", false}, + {"GITHUB_REPOSITORY", false}, + {"GITHUB_REPOSITORY_OWNER", false}, + {"GITHUB_RUN_ID", false}, + {"GITHUB_SHA", false}, + {"RUNNER_NAME", false}, + {"RUNNER_OS", false}, }, s.runner.ListEnvVars()) } diff --git a/internal/attestation/crafter/runners/gitlabpipeline_test.go b/internal/attestation/crafter/runners/gitlabpipeline_test.go index d48c29b98..f6c7c12c0 100644 --- a/internal/attestation/crafter/runners/gitlabpipeline_test.go +++ b/internal/attestation/crafter/runners/gitlabpipeline_test.go @@ -78,16 +78,16 @@ func (s *gitlabPipelineSuite) TestCheckEnv() { } func (s *gitlabPipelineSuite) TestListEnvVars() { - assert.Equal(s.T(), []string{ - "GITLAB_USER_EMAIL", - "GITLAB_USER_LOGIN", - "CI_PROJECT_URL", - "CI_COMMIT_SHA", - "CI_JOB_URL", - "CI_PIPELINE_URL", - "CI_RUNNER_VERSION", - "CI_RUNNER_DESCRIPTION", - "CI_COMMIT_REF_NAME", + assert.Equal(s.T(), []*EnvVarDefinition{ + {"GITLAB_USER_EMAIL", false}, + {"GITLAB_USER_LOGIN", false}, + {"CI_PROJECT_URL", false}, + {"CI_COMMIT_SHA", false}, + {"CI_JOB_URL", false}, + {"CI_PIPELINE_URL", false}, + {"CI_RUNNER_VERSION", false}, + {"CI_RUNNER_DESCRIPTION", false}, + {"CI_COMMIT_REF_NAME", false}, }, s.runner.ListEnvVars()) } diff --git a/internal/attestation/crafter/runners/jenkinsjob_test.go b/internal/attestation/crafter/runners/jenkinsjob_test.go index 9ed767050..b3df8791d 100644 --- a/internal/attestation/crafter/runners/jenkinsjob_test.go +++ b/internal/attestation/crafter/runners/jenkinsjob_test.go @@ -77,11 +77,13 @@ func (s *jenkinsJobSuite) TestCheckEnv() { } func (s *jenkinsJobSuite) TestListEnvVars() { - s.Equal([]string{ - "JOB_NAME", - "BUILD_URL", - "AGENT_WORKDIR", - "NODE_NAME", + s.Equal([]*EnvVarDefinition{ + {"JOB_NAME", false}, + {"BUILD_URL", false}, + {"GIT_BRANCH", true}, + {"GIT_COMMIT", true}, + {"AGENT_WORKDIR", false}, + {"NODE_NAME", false}, }, s.runner.ListEnvVars()) } diff --git a/internal/attestation/crafter/testdata/contracts/jenkins_with_env_vars.yaml b/internal/attestation/crafter/testdata/contracts/jenkins_with_env_vars.yaml new file mode 100644 index 000000000..a4047180c --- /dev/null +++ b/internal/attestation/crafter/testdata/contracts/jenkins_with_env_vars.yaml @@ -0,0 +1,8 @@ +schemaVersion: "v1" + +runner: + type: "JENKINS_JOB" + +envAllowList: + - CUSTOM_VAR_1 + - CUSTOM_VAR_2 From 29ae09bce753b4cd1473746e6396ce133d8a1d5b Mon Sep 17 00:00:00 2001 From: Mattia Buccarella Date: Mon, 27 Nov 2023 19:46:17 +0100 Subject: [PATCH 04/12] addressed some feedback Signed-off-by: Mattia Buccarella --- app/cli/internal/action/attestation_status.go | 2 +- internal/attestation/crafter/crafter.go | 3 +-- .../crafter/runners/{runner.go => runners.go} | 9 +++++---- 3 files changed, 7 insertions(+), 7 deletions(-) rename internal/attestation/crafter/runners/{runner.go => runners.go} (88%) diff --git a/app/cli/internal/action/attestation_status.go b/app/cli/internal/action/attestation_status.go index cfab66ac5..79639ef2c 100644 --- a/app/cli/internal/action/attestation_status.go +++ b/app/cli/internal/action/attestation_status.go @@ -127,7 +127,7 @@ func (action *AttestationStatus) Run() (*AttestationStatusResult, error) { runnerEnvVars, err := c.Runner.ResolveEnvVars() if err != nil { - return nil, err + return nil, fmt.Errorf("error resolving env vars: %w", err) } res.RunnerContext = &AttestationResultRunnerContext{ diff --git a/internal/attestation/crafter/crafter.go b/internal/attestation/crafter/crafter.go index a2321830b..7db24b655 100644 --- a/internal/attestation/crafter/crafter.go +++ b/internal/attestation/crafter/crafter.go @@ -109,7 +109,7 @@ func (c *Crafter) Init(opts *InitOpts) error { // Check that the initialization is happening in the right environment runnerType := opts.SchemaV1.Runner.GetType() runnerContext := NewRunner(runnerType) - if !runnerContext.CheckEnv() { + if !opts.DryRun && !runnerContext.CheckEnv() { return fmt.Errorf("%w, expected %s", ErrRunnerContextNotFound, runnerType) } @@ -380,7 +380,6 @@ func (c *Crafter) ResolveEnvVars() error { // Runner specific environment variables c.logger.Debug().Str("runnerType", c.Runner.String()).Msg("loading runner specific env variables") - var outputEnvVars = make(map[string]string) if !c.Runner.CheckEnv() { errorStr := fmt.Sprintf("couldn't detect the environment %q. Is the crafting process happening in the target env?", c.Runner.String()) return fmt.Errorf("%s - %w", errorStr, ErrRunnerContextNotFound) diff --git a/internal/attestation/crafter/runners/runner.go b/internal/attestation/crafter/runners/runners.go similarity index 88% rename from internal/attestation/crafter/runners/runner.go rename to internal/attestation/crafter/runners/runners.go index b5c25c879..ccb3dc108 100644 --- a/internal/attestation/crafter/runners/runner.go +++ b/internal/attestation/crafter/runners/runners.go @@ -32,10 +32,11 @@ func resolveEnvVars(envVarsDefinitions []*EnvVarDefinition) (map[string]string, value := os.Getenv(envVarDef.Name) if value != "" { result[envVarDef.Name] = value - } else { - if !envVarDef.Optional { - return nil, fmt.Errorf("environment variable %s cannot be resolved", envVarDef.Name) - } + continue + } + + if !envVarDef.Optional { + return nil, fmt.Errorf("environment variable %s cannot be resolved", envVarDef.Name) } } From 840423e93f946f0e984bd489b4993e1979e7a885 Mon Sep 17 00:00:00 2001 From: Mattia Buccarella Date: Tue, 28 Nov 2023 00:53:35 +0100 Subject: [PATCH 05/12] wrapping error Signed-off-by: Mattia Buccarella --- internal/attestation/crafter/crafter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/attestation/crafter/crafter.go b/internal/attestation/crafter/crafter.go index 7db24b655..6a51e5e49 100644 --- a/internal/attestation/crafter/crafter.go +++ b/internal/attestation/crafter/crafter.go @@ -395,7 +395,7 @@ func (c *Crafter) ResolveEnvVars() error { outputEnvVars, err := c.Runner.ResolveEnvVars() if err != nil { - return err + return fmt.Errorf("error while resolving runner environment variables: %w", err) } // User-defined environment vars From 7d45dd28a7eca2d8a08b71f14be968d9cf07cb89 Mon Sep 17 00:00:00 2001 From: Mattia Buccarella Date: Tue, 28 Nov 2023 12:44:23 +0100 Subject: [PATCH 06/12] test without error when optional vars are missing Signed-off-by: Mattia Buccarella --- .../crafter/runners/jenkinsjob_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/internal/attestation/crafter/runners/jenkinsjob_test.go b/internal/attestation/crafter/runners/jenkinsjob_test.go index b3df8791d..7d0a44031 100644 --- a/internal/attestation/crafter/runners/jenkinsjob_test.go +++ b/internal/attestation/crafter/runners/jenkinsjob_test.go @@ -88,9 +88,17 @@ func (s *jenkinsJobSuite) TestListEnvVars() { } func (s *jenkinsJobSuite) TestResolveEnvVars() { + // Test with all of the environment variables present resolvedEnvVars, err := s.runner.ResolveEnvVars() s.NoError(err) s.Equal(jenkinsJobTestingEnvVars, resolvedEnvVars) + + // Test with the optional environment variables unset + os.Unsetenv("GIT_BRANCH") + os.Unsetenv("GIT_COMMIT") + resolvedEnvVars, err = s.runner.ResolveEnvVars() + s.NoError(err) + s.Equal(reuiredJenkinsJobTestingEnvVars, resolvedEnvVars) } func (s *jenkinsJobSuite) TestRunURI() { @@ -119,6 +127,13 @@ var jenkinsJobTestingEnvVars = map[string]string{ "GIT_COMMIT": "somecommit", } +var reuiredJenkinsJobTestingEnvVars = map[string]string{ + "JOB_NAME": "some-jenkins-job", + "BUILD_URL": "http://some-build-url/", + "AGENT_WORKDIR": "/home/sample/agent", + "NODE_NAME": "some-node", +} + // Run the tests func TestJenkinsJobRunner(t *testing.T) { suite.Run(t, new(jenkinsJobSuite)) From 7f2b0dacc7357d5e987c7dbd06ca26868dfe018c Mon Sep 17 00:00:00 2001 From: Mattia Buccarella Date: Tue, 28 Nov 2023 13:02:12 +0100 Subject: [PATCH 07/12] adjusting test Signed-off-by: Mattia Buccarella --- internal/attestation/crafter/crafter_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/attestation/crafter/crafter_test.go b/internal/attestation/crafter/crafter_test.go index fc88d4576..e74568d68 100644 --- a/internal/attestation/crafter/crafter_test.go +++ b/internal/attestation/crafter/crafter_test.go @@ -240,7 +240,7 @@ func (s *crafterSuite) TestResolveEnvVars() { }, { name: "missing some github runner env", inGithubEnv: true, - expectedError: "environment variable GITHUB_ACTOR cannot be resolved", + expectedError: "error while resolving runner environment variables: environment variable GITHUB_ACTOR cannot be resolved", envVars: map[string]string{ "CUSTOM_VAR_1": "custom_value_1", "CUSTOM_VAR_2": "custom_value_2", From ae20a62c52778bfc8504eff12d3656fbca53aa60 Mon Sep 17 00:00:00 2001 From: Mattia Buccarella Date: Tue, 28 Nov 2023 13:27:25 +0100 Subject: [PATCH 08/12] performance/lint Signed-off-by: Mattia Buccarella --- internal/attestation/crafter/crafter.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/attestation/crafter/crafter.go b/internal/attestation/crafter/crafter.go index 6a51e5e49..cf9294448 100644 --- a/internal/attestation/crafter/crafter.go +++ b/internal/attestation/crafter/crafter.go @@ -387,9 +387,9 @@ func (c *Crafter) ResolveEnvVars() error { // Workflow run environment variables - var varNames []string - for _, envVarDef := range c.Runner.ListEnvVars() { - varNames = append(varNames, envVarDef.Name) + varNames := make([]string, len(c.Runner.ListEnvVars())) + for index, envVarDef := range c.Runner.ListEnvVars() { + varNames[index] = envVarDef.Name } c.logger.Debug().Str("runnerType", c.Runner.String()).Strs("variables", varNames).Msg("list of env variables to automatically extract") From 433aba48e1333cf6a927891c5275cd2c181e332f Mon Sep 17 00:00:00 2001 From: Mattia Buccarella Date: Thu, 30 Nov 2023 15:03:56 +0100 Subject: [PATCH 09/12] ResolveEnvVars can return multiple errors Signed-off-by: Mattia Buccarella --- app/cli/internal/action/attestation_status.go | 2 +- internal/attestation/crafter/crafter.go | 3 --- internal/attestation/crafter/runner.go | 2 +- .../attestation/crafter/runners/azurepipeline.go | 2 +- .../attestation/crafter/runners/circleci_build.go | 2 +- internal/attestation/crafter/runners/generic.go | 2 +- internal/attestation/crafter/runners/githubaction.go | 2 +- .../attestation/crafter/runners/gitlabpipeline.go | 2 +- internal/attestation/crafter/runners/jenkinsjob.go | 2 +- internal/attestation/crafter/runners/runners.go | 12 +++++++++--- 10 files changed, 17 insertions(+), 14 deletions(-) diff --git a/app/cli/internal/action/attestation_status.go b/app/cli/internal/action/attestation_status.go index 79639ef2c..7adc2419b 100644 --- a/app/cli/internal/action/attestation_status.go +++ b/app/cli/internal/action/attestation_status.go @@ -126,7 +126,7 @@ func (action *AttestationStatus) Run() (*AttestationStatusResult, error) { res.EnvVars = envVars runnerEnvVars, err := c.Runner.ResolveEnvVars() - if err != nil { + if err != nil && !c.CraftingState.DryRun { return nil, fmt.Errorf("error resolving env vars: %w", err) } diff --git a/internal/attestation/crafter/crafter.go b/internal/attestation/crafter/crafter.go index cf9294448..a41180ac9 100644 --- a/internal/attestation/crafter/crafter.go +++ b/internal/attestation/crafter/crafter.go @@ -378,7 +378,6 @@ func (c *Crafter) ResolveEnvVars() error { } // Runner specific environment variables - c.logger.Debug().Str("runnerType", c.Runner.String()).Msg("loading runner specific env variables") if !c.Runner.CheckEnv() { errorStr := fmt.Sprintf("couldn't detect the environment %q. Is the crafting process happening in the target env?", c.Runner.String()) @@ -386,7 +385,6 @@ func (c *Crafter) ResolveEnvVars() error { } // Workflow run environment variables - varNames := make([]string, len(c.Runner.ListEnvVars())) for index, envVarDef := range c.Runner.ListEnvVars() { varNames[index] = envVarDef.Name @@ -399,7 +397,6 @@ func (c *Crafter) ResolveEnvVars() error { } // User-defined environment vars - if len(c.CraftingState.InputSchema.EnvAllowList) > 0 { c.logger.Debug().Strs("allowList", c.CraftingState.InputSchema.EnvAllowList).Msg("loading env variables") } diff --git a/internal/attestation/crafter/runner.go b/internal/attestation/crafter/runner.go index 5c2b32ffe..fe90c320a 100644 --- a/internal/attestation/crafter/runner.go +++ b/internal/attestation/crafter/runner.go @@ -32,7 +32,7 @@ type supportedRunner interface { ListEnvVars() []*runners.EnvVarDefinition // Return the list of env vars associated with this runner already resolved - ResolveEnvVars() (map[string]string, error) + ResolveEnvVars() (map[string]string, []*error) String() string diff --git a/internal/attestation/crafter/runners/azurepipeline.go b/internal/attestation/crafter/runners/azurepipeline.go index b06fe563c..67e679ecb 100644 --- a/internal/attestation/crafter/runners/azurepipeline.go +++ b/internal/attestation/crafter/runners/azurepipeline.go @@ -81,6 +81,6 @@ func (r *AzurePipeline) RunURI() (url string) { return uri.String() } -func (r *AzurePipeline) ResolveEnvVars() (map[string]string, error) { +func (r *AzurePipeline) ResolveEnvVars() (map[string]string, []*error) { return resolveEnvVars(r.ListEnvVars()) } diff --git a/internal/attestation/crafter/runners/circleci_build.go b/internal/attestation/crafter/runners/circleci_build.go index 7295d449a..5ae47fb23 100644 --- a/internal/attestation/crafter/runners/circleci_build.go +++ b/internal/attestation/crafter/runners/circleci_build.go @@ -58,6 +58,6 @@ func (r *CircleCIBuild) RunURI() string { return os.Getenv("CIRCLE_BUILD_URL") } -func (r *CircleCIBuild) ResolveEnvVars() (map[string]string, error) { +func (r *CircleCIBuild) ResolveEnvVars() (map[string]string, []*error) { return resolveEnvVars(r.ListEnvVars()) } diff --git a/internal/attestation/crafter/runners/generic.go b/internal/attestation/crafter/runners/generic.go index 85119e178..03c704c48 100644 --- a/internal/attestation/crafter/runners/generic.go +++ b/internal/attestation/crafter/runners/generic.go @@ -41,6 +41,6 @@ func (r *Generic) RunURI() string { return "" } -func (r *Generic) ResolveEnvVars() (map[string]string, error) { +func (r *Generic) ResolveEnvVars() (map[string]string, []*error) { return resolveEnvVars(r.ListEnvVars()) } diff --git a/internal/attestation/crafter/runners/githubaction.go b/internal/attestation/crafter/runners/githubaction.go index e4b911795..6ce8b05db 100644 --- a/internal/attestation/crafter/runners/githubaction.go +++ b/internal/attestation/crafter/runners/githubaction.go @@ -65,6 +65,6 @@ func (r *GitHubAction) RunURI() (url string) { return url } -func (r *GitHubAction) ResolveEnvVars() (map[string]string, error) { +func (r *GitHubAction) ResolveEnvVars() (map[string]string, []*error) { return resolveEnvVars(r.ListEnvVars()) } diff --git a/internal/attestation/crafter/runners/gitlabpipeline.go b/internal/attestation/crafter/runners/gitlabpipeline.go index 16673922a..70b71312e 100644 --- a/internal/attestation/crafter/runners/gitlabpipeline.go +++ b/internal/attestation/crafter/runners/gitlabpipeline.go @@ -60,6 +60,6 @@ func (r *GitlabPipeline) RunURI() (url string) { return os.Getenv("CI_JOB_URL") } -func (r *GitlabPipeline) ResolveEnvVars() (map[string]string, error) { +func (r *GitlabPipeline) ResolveEnvVars() (map[string]string, []*error) { return resolveEnvVars(r.ListEnvVars()) } diff --git a/internal/attestation/crafter/runners/jenkinsjob.go b/internal/attestation/crafter/runners/jenkinsjob.go index 62e10bc9e..8d3e509b4 100644 --- a/internal/attestation/crafter/runners/jenkinsjob.go +++ b/internal/attestation/crafter/runners/jenkinsjob.go @@ -60,6 +60,6 @@ func (r *JenkinsJob) RunURI() string { return os.Getenv("BUILD_URL") } -func (r *JenkinsJob) ResolveEnvVars() (map[string]string, error) { +func (r *JenkinsJob) ResolveEnvVars() (map[string]string, []*error) { return resolveEnvVars(r.ListEnvVars()) } diff --git a/internal/attestation/crafter/runners/runners.go b/internal/attestation/crafter/runners/runners.go index ccb3dc108..ffdfc18b0 100644 --- a/internal/attestation/crafter/runners/runners.go +++ b/internal/attestation/crafter/runners/runners.go @@ -25,8 +25,9 @@ type EnvVarDefinition struct { Optional bool } -func resolveEnvVars(envVarsDefinitions []*EnvVarDefinition) (map[string]string, error) { +func resolveEnvVars(envVarsDefinitions []*EnvVarDefinition) (map[string]string, []*error) { result := make(map[string]string) + var errors []*error for _, envVarDef := range envVarsDefinitions { value := os.Getenv(envVarDef.Name) @@ -36,9 +37,14 @@ func resolveEnvVars(envVarsDefinitions []*EnvVarDefinition) (map[string]string, } if !envVarDef.Optional { - return nil, fmt.Errorf("environment variable %s cannot be resolved", envVarDef.Name) + err := fmt.Errorf("environment variable %s cannot be resolved", envVarDef.Name) + errors = append(errors, &err) } } - return result, nil + if len(errors) > 0 { + result = nil + } + + return result, errors } From 6f68cce7551b68d26074e7e3ba4deb5be3ad6ea6 Mon Sep 17 00:00:00 2001 From: Mattia Buccarella Date: Thu, 30 Nov 2023 15:33:39 +0100 Subject: [PATCH 10/12] adjusting test Signed-off-by: Mattia Buccarella --- .../attestation/crafter/runners/azurepipeline_test.go | 4 ++-- .../attestation/crafter/runners/circleci_build_test.go | 4 ++-- internal/attestation/crafter/runners/generic_test.go | 4 ++-- internal/attestation/crafter/runners/githubaction_test.go | 4 ++-- .../attestation/crafter/runners/gitlabpipeline_test.go | 4 ++-- internal/attestation/crafter/runners/jenkinsjob_test.go | 8 ++++---- 6 files changed, 14 insertions(+), 14 deletions(-) diff --git a/internal/attestation/crafter/runners/azurepipeline_test.go b/internal/attestation/crafter/runners/azurepipeline_test.go index 9b74fe6ca..2a9a4d4d8 100644 --- a/internal/attestation/crafter/runners/azurepipeline_test.go +++ b/internal/attestation/crafter/runners/azurepipeline_test.go @@ -93,8 +93,8 @@ func (s *azurePipelineSuite) TestListEnvVars() { } func (s *azurePipelineSuite) TestResolveEnvVars() { - resolvedEnvVars, err := s.runner.ResolveEnvVars() - s.NoError(err) + resolvedEnvVars, errors := s.runner.ResolveEnvVars() + s.Empty(errors) s.Equal(map[string]string{ "AGENT_VERSION": "3.220.5", diff --git a/internal/attestation/crafter/runners/circleci_build_test.go b/internal/attestation/crafter/runners/circleci_build_test.go index 14cb64cda..8cd6b55fd 100644 --- a/internal/attestation/crafter/runners/circleci_build_test.go +++ b/internal/attestation/crafter/runners/circleci_build_test.go @@ -98,8 +98,8 @@ func (s *circleCIBuildSuite) TestListEnvVars() { } func (s *circleCIBuildSuite) TestResolveEnvVars() { - resolvedEnvVars, err := s.runner.ResolveEnvVars() - s.NoError(err) + resolvedEnvVars, errors := s.runner.ResolveEnvVars() + s.Empty(errors) s.Equal(circleCIBuildTestingEnvVars, resolvedEnvVars) } diff --git a/internal/attestation/crafter/runners/generic_test.go b/internal/attestation/crafter/runners/generic_test.go index 121d88164..0e3789cfb 100644 --- a/internal/attestation/crafter/runners/generic_test.go +++ b/internal/attestation/crafter/runners/generic_test.go @@ -35,8 +35,8 @@ func (s *genericSuite) TestListEnvVars() { } func (s *genericSuite) TestResolveEnvVars() { - result, err := s.runner.ResolveEnvVars() - s.NoError(err) + result, errors := s.runner.ResolveEnvVars() + s.Empty(errors) s.Equal(map[string]string{}, result) } diff --git a/internal/attestation/crafter/runners/githubaction_test.go b/internal/attestation/crafter/runners/githubaction_test.go index 1a8b5ada2..6deef4c26 100644 --- a/internal/attestation/crafter/runners/githubaction_test.go +++ b/internal/attestation/crafter/runners/githubaction_test.go @@ -102,8 +102,8 @@ func (s *githubActionSuite) TestListEnvVars() { } func (s *githubActionSuite) TestResolveEnvVars() { - resolvedEnvVars, err := s.runner.ResolveEnvVars() - s.NoError(err) + resolvedEnvVars, errors := s.runner.ResolveEnvVars() + s.Empty(errors) s.Equal(gitHubTestingEnvVars, resolvedEnvVars) } diff --git a/internal/attestation/crafter/runners/gitlabpipeline_test.go b/internal/attestation/crafter/runners/gitlabpipeline_test.go index f6c7c12c0..4fa5b63e5 100644 --- a/internal/attestation/crafter/runners/gitlabpipeline_test.go +++ b/internal/attestation/crafter/runners/gitlabpipeline_test.go @@ -92,8 +92,8 @@ func (s *gitlabPipelineSuite) TestListEnvVars() { } func (s *gitlabPipelineSuite) TestResolveEnvVars() { - resolvedEnvVars, err := s.runner.ResolveEnvVars() - s.NoError(err) + resolvedEnvVars, errors := s.runner.ResolveEnvVars() + s.Empty(errors) s.Equal(map[string]string{ "GITLAB_USER_EMAIL": "foo@foo.com", "GITLAB_USER_LOGIN": "foo", diff --git a/internal/attestation/crafter/runners/jenkinsjob_test.go b/internal/attestation/crafter/runners/jenkinsjob_test.go index 7d0a44031..7bc8d5440 100644 --- a/internal/attestation/crafter/runners/jenkinsjob_test.go +++ b/internal/attestation/crafter/runners/jenkinsjob_test.go @@ -89,15 +89,15 @@ func (s *jenkinsJobSuite) TestListEnvVars() { func (s *jenkinsJobSuite) TestResolveEnvVars() { // Test with all of the environment variables present - resolvedEnvVars, err := s.runner.ResolveEnvVars() - s.NoError(err) + resolvedEnvVars, errors := s.runner.ResolveEnvVars() + s.Empty(errors) s.Equal(jenkinsJobTestingEnvVars, resolvedEnvVars) // Test with the optional environment variables unset os.Unsetenv("GIT_BRANCH") os.Unsetenv("GIT_COMMIT") - resolvedEnvVars, err = s.runner.ResolveEnvVars() - s.NoError(err) + resolvedEnvVars, errors = s.runner.ResolveEnvVars() + s.Empty(errors) s.Equal(reuiredJenkinsJobTestingEnvVars, resolvedEnvVars) } From 26b9ca26649934a0f4724a57749e1f74809cdb10 Mon Sep 17 00:00:00 2001 From: Mattia Buccarella Date: Thu, 30 Nov 2023 15:48:20 +0100 Subject: [PATCH 11/12] adjusting tests and add note Signed-off-by: Mattia Buccarella --- app/cli/internal/action/attestation_status.go | 10 +++++++--- internal/attestation/crafter/crafter.go | 10 +++++++--- internal/attestation/crafter/runners/jenkinsjob.go | 2 ++ 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/app/cli/internal/action/attestation_status.go b/app/cli/internal/action/attestation_status.go index 7adc2419b..401bfad11 100644 --- a/app/cli/internal/action/attestation_status.go +++ b/app/cli/internal/action/attestation_status.go @@ -125,9 +125,13 @@ func (action *AttestationStatus) Run() (*AttestationStatusResult, error) { res.EnvVars = envVars - runnerEnvVars, err := c.Runner.ResolveEnvVars() - if err != nil && !c.CraftingState.DryRun { - return nil, fmt.Errorf("error resolving env vars: %w", err) + runnerEnvVars, errors := c.Runner.ResolveEnvVars() + var combinedErrs string + for _, err := range errors { + combinedErrs += (*err).Error() + "\n" + } + if len(errors) > 0 && !c.CraftingState.DryRun { + return nil, fmt.Errorf("error resolving env vars: %s", combinedErrs) } res.RunnerContext = &AttestationResultRunnerContext{ diff --git a/internal/attestation/crafter/crafter.go b/internal/attestation/crafter/crafter.go index a41180ac9..117180e91 100644 --- a/internal/attestation/crafter/crafter.go +++ b/internal/attestation/crafter/crafter.go @@ -391,9 +391,13 @@ func (c *Crafter) ResolveEnvVars() error { } c.logger.Debug().Str("runnerType", c.Runner.String()).Strs("variables", varNames).Msg("list of env variables to automatically extract") - outputEnvVars, err := c.Runner.ResolveEnvVars() - if err != nil { - return fmt.Errorf("error while resolving runner environment variables: %w", err) + outputEnvVars, errors := c.Runner.ResolveEnvVars() + if len(errors) > 0 { + var combinedErrs string + for _, err := range errors { + combinedErrs += (*err).Error() + "\n" + } + return fmt.Errorf("error while resolving runner environment variables: %s", combinedErrs) } // User-defined environment vars diff --git a/internal/attestation/crafter/runners/jenkinsjob.go b/internal/attestation/crafter/runners/jenkinsjob.go index 8d3e509b4..c3a90960c 100644 --- a/internal/attestation/crafter/runners/jenkinsjob.go +++ b/internal/attestation/crafter/runners/jenkinsjob.go @@ -43,6 +43,8 @@ func (r *JenkinsJob) ListEnvVars() []*EnvVarDefinition { {"BUILD_URL", false}, // Some info about the commit (Jenkins Git Plugin) + // NOTE: These variables are marked as optional because their presence + // depends the jenkins' configuration (e.g., multibranch pipelines). {"GIT_BRANCH", true}, {"GIT_COMMIT", true}, From 152d8509552fc3e561c7a0a3a0a6b9a7b66fbb1c Mon Sep 17 00:00:00 2001 From: Mattia Buccarella Date: Thu, 30 Nov 2023 15:54:17 +0100 Subject: [PATCH 12/12] adjusting expected error message in a test Signed-off-by: Mattia Buccarella --- internal/attestation/crafter/crafter_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/attestation/crafter/crafter_test.go b/internal/attestation/crafter/crafter_test.go index e74568d68..348fb94b2 100644 --- a/internal/attestation/crafter/crafter_test.go +++ b/internal/attestation/crafter/crafter_test.go @@ -240,7 +240,7 @@ func (s *crafterSuite) TestResolveEnvVars() { }, { name: "missing some github runner env", inGithubEnv: true, - expectedError: "error while resolving runner environment variables: environment variable GITHUB_ACTOR cannot be resolved", + expectedError: "error while resolving runner environment variables: environment variable GITHUB_ACTOR cannot be resolved\n", envVars: map[string]string{ "CUSTOM_VAR_1": "custom_value_1", "CUSTOM_VAR_2": "custom_value_2",