Repository: GoogleContainerTools/skaffold
Pull request #7927 (2022-10-07 07:28:05)

fix: reverse order of deployers during cleanup (#7284) (backport v1)
<!-- Thank you for your contribution! -->

<!-- Include if applicable: -->
Fixes: #7284 <!-- tracking issues that this PR will close -->
**Related**: _Relevant tracking issues, for context_
**Merge before/after**: _Dependent or prerequisite PRs_

**Description**
<!-- Describe your changes here. The more detail, the easier the review! -->
Add a new deployer slice during cleanup function of the DeployerMux which is reversed in it's order. This change should be covered by the existing unittests since no new function was added.

This is a backport for v1.

**User facing changes (remove if N/A)**
<!-- Describe any user facing changes this PR introduces. -->
<!-- "Before" and "After" sections work great - bonus points for screenshots! -->
<!-- Be sure all docs have been updated as well! -->
Before:
Modules are undeployed in the same order as they are deployed. Visible in the console as

```shell
^CCleaning up...
release "postgresql" uninstalled
 - deployment.apps "example-django" deleted
INFO[0016] Cleanup completed in 886.10607ms              subtask=-1 task=DevLoop

```

After:
Modules are undeployed in reverse order.

```shell
^CCleaning up...
 - deployment.apps "example-django" deleted
release "postgresql" uninstalled
INFO[0328] Cleanup completed in 1.038 second             subtask=-1 task=DevLoop
```

<!--
Please be sure your PR includes unit tests - we don't merge code that brings down test coverage! 
Integration tests are sometimes an appropriate substitute. 
-->


codecov[bot]: # [Codecov](https://codecov.io/gh/GoogleContainerTools/skaffold/pull/7927?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools) Report
> :exclamation: No coverage uploaded for pull request base (`v1@894fb67`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools#section-missing-base-commit).
> The diff coverage is `n/a`.

```diff
@@          Coverage Diff          @@
##             v1    #7927   +/-   ##
=====================================
  Coverage      ?   68.30%           
=====================================
  Files         ?      563           
  Lines         ?    26695           
  Branches      ?        0           
=====================================
  Hits          ?    18233           
  Misses        ?     7188           
  Partials      ?     1274           
```



:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GoogleContainerTools)

chronicc: I moved the reversal function into it's own method and added tests.
chronicc: I see, that I updated the wrong function in the rush of backporting it. Will fix this now.

Patch:
diff --git a/.github/workflows/integration-linux.yml b/.github/workflows/integration-linux.yml
index b2d73c301..450a6e23a 100644
--- a/.github/workflows/integration-linux.yml
+++ b/.github/workflows/integration-linux.yml
@@ -19,7 +19,7 @@ jobs:
         kustomize_version: [3.5.4]
         ko_version: [0.4.0]
         kompose_version: [1.21.0]
-        gcloud_sdk_version: [335.0.0]
+        gcloud_sdk_version: [410.0.0]
         container_structure_tests_version: [1.8.0]
         integration_test_partitions: [0, 1, 2, 3]
     steps:
@@ -83,7 +83,7 @@ jobs:
       run: |
         wget -O gcloud.tar.gz https://dl.google.com/dl/cloudsdk/channels/rapid/downloads/google-cloud-sdk-${{ matrix.gcloud_sdk_version }}-linux-x86_64.tar.gz
         tar -xvf gcloud.tar.gz -C ${HOME}/
-        CLOUDSDK_PYTHON="python2.7" ${HOME}/google-cloud-sdk/install.sh --usage-reporting=false --bash-completion=false --disable-installation-options
+        ${HOME}/google-cloud-sdk/install.sh --usage-reporting=false --bash-completion=false --disable-installation-options
         echo "${HOME}/google-cloud-sdk/bin" >> $GITHUB_PATH
 
     - name: Configure GCloud with Docker
diff --git a/.github/workflows/performance-comparison.yml b/.github/workflows/performance-comparison.yml
index c0de6ba96..1c65b41e4 100644
--- a/.github/workflows/performance-comparison.yml
+++ b/.github/workflows/performance-comparison.yml
@@ -22,7 +22,7 @@ jobs:
         kustomize_version: [3.5.4]
         ko_version: [0.4.0]
         kompose_version: [1.21.0]
-        gcloud_sdk_version: [335.0.0]
+        gcloud_sdk_version: [410.0.0]
         container_structure_tests_version: [1.8.0]
     steps:
     - name: "Check if user has write access"
@@ -68,7 +68,7 @@ jobs:
       run: |
         wget -O gcloud.tar.gz https://dl.google.com/dl/cloudsdk/channels/rapid/downloads/google-cloud-sdk-${{ matrix.gcloud_sdk_version }}-linux-x86_64.tar.gz
         tar -xvf gcloud.tar.gz -C ${HOME}/
-        CLOUDSDK_PYTHON="python2.7" ${HOME}/google-cloud-sdk/install.sh --usage-reporting=false --bash-completion=false --disable-installation-options
+        ${HOME}/google-cloud-sdk/install.sh --usage-reporting=false --bash-completion=false --disable-installation-options
         echo "${HOME}/google-cloud-sdk/bin" >> $GITHUB_PATH
 
     - name: Configure GCloud with Docker
diff --git a/.github/workflows/scorecards.yml b/.github/workflows/scorecards.yml
new file mode 100644
index 000000000..d741c844f
--- /dev/null
+++ b/.github/workflows/scorecards.yml
@@ -0,0 +1,55 @@
+name: Scorecards supply-chain security
+on:
+  # Only the default branch is supported.
+  branch_protection_rule:
+  schedule:
+    - cron: '26 6 * * 0'
+  push:
+    branches: [ main ]
+
+# Declare default permissions as read only.
+permissions: read-all
+
+jobs:
+  analysis:
+    name: Scorecards analysis
+    runs-on: ubuntu-latest
+    permissions:
+      # Needed to upload the results to code-scanning dashboard.
+      security-events: write
+      actions: read
+      contents: read
+
+    steps:
+      - name: "Checkout code"
+        uses: actions/checkout@a12a3943b4bdde767164f792f33f40b04645d846 # v3.0.0
+        with:
+          persist-credentials: false
+
+      - name: "Run analysis"
+        uses: ossf/scorecard-action@99c53751e09b9529366343771cc321ec74e9bd3d # v2.0.0
+        with:
+          results_file: results.sarif
+          results_format: sarif
+          # Read-only PAT token. To create it,
+          # follow the steps in https://github.com/ossf/scorecard-action#pat-token-creation.
+          repo_token: ${{ secrets.SCORECARD_READ_TOKEN }}
+          # Publish the results to enable scorecard badges. For more details, see
+          # https://github.com/ossf/scorecard-action#publishing-results.
+          # For private repositories, `publish_results` will automatically be set to `false`,
+          # regardless of the value entered here.
+          publish_results: true
+
+      # Upload the results as artifacts (optional).
+      - name: "Upload artifact"
+        uses: actions/upload-artifact@83fd05a356d7e2593de66fc9913b3002723633cb # v3.1.0
+        with:
+          name: SARIF file
+          path: results.sarif
+          retention-days: 5
+
+      # Upload the results to GitHub's code scanning dashboard.
+      - name: "Upload to code-scanning"
+        uses: github/codeql-action/upload-sarif@5f532563584d71fdef14ee64d17bafb34f751ce5 # v1.0.26
+        with:
+          sarif_file: results.sarif
diff --git a/.gitignore b/.gitignore
index 50b828333..b2967e3b4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -7,6 +7,7 @@ integration/testdata/plugin/local/bazel/bazel-*
 *.new
 *.iml
 .idea/
+.tool-versions
 .vscode/
 docs/.firebase
 docs/firebase-debug*
diff --git a/examples/bazel/.bazelrc b/examples/bazel/.bazelrc
deleted file mode 100644
index 680549907..000000000
--- a/examples/bazel/.bazelrc
+++ /dev/null
@@ -1,8 +0,0 @@
-# The following flags are set to test use of new features for python toolchains
-# These flags will only work with Bazel 0.25.0 or above.
-build --incompatible_use_python_toolchains
-build --host_force_python=PY2
-test --incompatible_use_python_toolchains
-test --host_force_python=PY2
-run --incompatible_use_python_toolchains
-run --host_force_python=PY2
diff --git a/examples/bazel/WORKSPACE b/examples/bazel/WORKSPACE
index 5637093ed..3b5a76e11 100644
--- a/examples/bazel/WORKSPACE
+++ b/examples/bazel/WORKSPACE
@@ -4,9 +4,8 @@ load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
 
 http_archive(
     name = "io_bazel_rules_docker",
-    sha256 = "4521794f0fba2e20f3bf15846ab5e01d5332e587e9ce81629c7f96c793bb7036",
-    strip_prefix = "rules_docker-0.14.4",
-    urls = ["https://github.com/bazelbuild/rules_docker/releases/download/v0.14.4/rules_docker-v0.14.4.tar.gz"],
+    sha256 = "b1e80761a8a8243d03ebca8845e9cc1ba6c82ce7c5179ce2b295cd36f7e394bf",
+    urls = ["https://github.com/bazelbuild/rules_docker/releases/download/v0.25.0/rules_docker-v0.25.0.tar.gz"],
 )
 
 http_archive(
@@ -18,9 +17,10 @@ http_archive(
     ],
 )
 
-load("@io_bazel_rules_go//go:deps.bzl", "go_rules_dependencies", "go_register_toolchains")
+load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")
 
 go_rules_dependencies()
+
 go_register_toolchains(
     go_version = "1.14.4",
 )
@@ -39,13 +39,9 @@ load(
 
 container_deps()
 
-load("@io_bazel_rules_docker//repositories:pip_repositories.bzl", "pip_deps")
-
-pip_deps()
-
 load(
     "@io_bazel_rules_docker//go:image.bzl",
     _go_image_repos = "repositories",
 )
 
-_go_image_repos()
\ No newline at end of file
+_go_image_repos()
diff --git a/integration/examples/bazel/.bazelrc b/integration/examples/bazel/.bazelrc
deleted file mode 100644
index 680549907..000000000
--- a/integration/examples/bazel/.bazelrc
+++ /dev/null
@@ -1,8 +0,0 @@
-# The following flags are set to test use of new features for python toolchains
-# These flags will only work with Bazel 0.25.0 or above.
-build --incompatible_use_python_toolchains
-build --host_force_python=PY2
-test --incompatible_use_python_toolchains
-test --host_force_python=PY2
-run --incompatible_use_python_toolchains
-run --host_force_python=PY2
diff --git a/integration/examples/bazel/WORKSPACE b/integration/examples/bazel/WORKSPACE
index 5637093ed..3b5a76e11 100644
--- a/integration/examples/bazel/WORKSPACE
+++ b/integration/examples/bazel/WORKSPACE
@@ -4,9 +4,8 @@ load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
 
 http_archive(
     name = "io_bazel_rules_docker",
-    sha256 = "4521794f0fba2e20f3bf15846ab5e01d5332e587e9ce81629c7f96c793bb7036",
-    strip_prefix = "rules_docker-0.14.4",
-    urls = ["https://github.com/bazelbuild/rules_docker/releases/download/v0.14.4/rules_docker-v0.14.4.tar.gz"],
+    sha256 = "b1e80761a8a8243d03ebca8845e9cc1ba6c82ce7c5179ce2b295cd36f7e394bf",
+    urls = ["https://github.com/bazelbuild/rules_docker/releases/download/v0.25.0/rules_docker-v0.25.0.tar.gz"],
 )
 
 http_archive(
@@ -18,9 +17,10 @@ http_archive(
     ],
 )
 
-load("@io_bazel_rules_go//go:deps.bzl", "go_rules_dependencies", "go_register_toolchains")
+load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")
 
 go_rules_dependencies()
+
 go_register_toolchains(
     go_version = "1.14.4",
 )
@@ -39,13 +39,9 @@ load(
 
 container_deps()
 
-load("@io_bazel_rules_docker//repositories:pip_repositories.bzl", "pip_deps")
-
-pip_deps()
-
 load(
     "@io_bazel_rules_docker//go:image.bzl",
     _go_image_repos = "repositories",
 )
 
-_go_image_repos()
\ No newline at end of file
+_go_image_repos()
diff --git a/pkg/skaffold/deploy/deploy_mux.go b/pkg/skaffold/deploy/deploy_mux.go
index b43bf3e98..c772ce16d 100644
--- a/pkg/skaffold/deploy/deploy_mux.go
+++ b/pkg/skaffold/deploy/deploy_mux.go
@@ -59,6 +59,14 @@ func (m DeployerMux) GetDeployers() []Deployer {
 	return m.deployers
 }
 
+func (m DeployerMux) GetDeployersInverse() []Deployer {
+	inverse := m.deployers
+	for i, j := 0, len(inverse)-1; i < j; i, j = i+1, j-1 {
+		inverse[i], inverse[j] = inverse[j], inverse[i]
+	}
+	return inverse
+}
+
 func (m DeployerMux) GetAccessor() access.Accessor {
 	var accessors access.AccessorMux
 	for _, deployer := range m.deployers {
@@ -159,7 +167,9 @@ func (m DeployerMux) Dependencies() ([]string, error) {
 }
 
 func (m DeployerMux) Cleanup(ctx context.Context, w io.Writer, dryRun bool) error {
-	for _, deployer := range m.deployers {
+	// Reverse order of deployers for cleanup to ensure resources
+	// are removed before their definitions are removed.
+	for _, deployer := range m.GetDeployersInverse() {
 		ctx, endTrace := instrumentation.StartTrace(ctx, "Cleanup")
 		if dryRun {
 			output.Yellow.Fprintln(w, "Following resources would be deleted:")
diff --git a/pkg/skaffold/deploy/helm/deploy.go b/pkg/skaffold/deploy/helm/deploy.go
index 563a323c3..37195ee3f 100644
--- a/pkg/skaffold/deploy/helm/deploy.go
+++ b/pkg/skaffold/deploy/helm/deploy.go
@@ -439,9 +439,19 @@ func (h *Deployer) Render(ctx context.Context, out io.Writer, builds []graph.Art
 		}
 
 		outBuffer := new(bytes.Buffer)
-		if err := h.exec(ctx, outBuffer, false, nil, args...); err != nil {
-			return userErr("std out err", fmt.Errorf(outBuffer.String()))
+		errBuffer := new(bytes.Buffer)
+
+		err = h.execWithStdoutAndStderr(ctx, outBuffer, errBuffer, false, nil, args...)
+		errorMsg := errBuffer.String()
+
+		if len(errorMsg) > 0 {
+			olog.Entry(ctx).Infof(errorMsg)
+		}
+
+		if err != nil {
+			return userErr("std out err", fmt.Errorf(outBuffer.String(), fmt.Errorf(errorMsg)))
 		}
+
 		renderedManifests.Write(outBuffer.Bytes())
 	}
 
diff --git a/pkg/skaffold/deploy/helm/util.go b/pkg/skaffold/deploy/helm/util.go
index a06bba04d..63eca99e3 100644
--- a/pkg/skaffold/deploy/helm/util.go
+++ b/pkg/skaffold/deploy/helm/util.go
@@ -198,8 +198,8 @@ func envVarForImage(imageName string, digest string) map[string]string {
 	return customMap
 }
 
-// exec executes the helm command, writing combined stdout/stderr to the provided writer
-func (h *Deployer) exec(ctx context.Context, out io.Writer, useSecrets bool, env []string, args ...string) error {
+// generates the helm command to run according to the given configuration
+func (h *Deployer) generateHelmCommand(ctx context.Context, useSecrets bool, env []string, args ...string) *exec.Cmd {
 	args = append([]string{"--kube-context", h.kubeContext}, args...)
 	args = append(args, h.Flags.Global...)
 
@@ -215,8 +215,24 @@ func (h *Deployer) exec(ctx context.Context, out io.Writer, useSecrets bool, env
 	if len(env) > 0 {
 		cmd.Env = env
 	}
+
+	return cmd
+}
+
+// executes the helm command, writing combined stdout/stderr to the provided writer
+func (h *Deployer) exec(ctx context.Context, out io.Writer, useSecrets bool, env []string, args ...string) error {
+	cmd := h.generateHelmCommand(ctx, useSecrets, env, args...)
 	cmd.Stdout = out
 	cmd.Stderr = out
 
 	return util.RunCmd(ctx, cmd)
 }
+
+// executes the helm command, writing stdout and stderr to the provided writers
+func (h *Deployer) execWithStdoutAndStderr(ctx context.Context, stdout io.Writer, stderr io.Writer, useSecrets bool, env []string, args ...string) error {
+	cmd := h.generateHelmCommand(ctx, useSecrets, env, args...)
+	cmd.Stdout = stdout
+	cmd.Stderr = stderr
+
+	return util.RunCmd(ctx, cmd)
+}
diff --git a/pkg/skaffold/runner/deployer.go b/pkg/skaffold/runner/deployer.go
index ffae6ac7f..5c49e2afc 100644
--- a/pkg/skaffold/runner/deployer.go
+++ b/pkg/skaffold/runner/deployer.go
@@ -75,7 +75,7 @@ func GetDeployer(ctx context.Context, runCtx *runcontext.RunContext, labeller *l
 		for _, d := range deployerCfg {
 			// runcontext StatusCheck method returns the value set by the cli flag `--status-check`
 			// which overrides the value set in the individual configs.
-			if runCtx.Opts.StatusCheck.String() == "" && !*d.StatusCheck {
+			if runCtx.Opts.StatusCheck.String() == "" && d.StatusCheck != nil && !*d.StatusCheck {
 				runCtx.Opts.StatusCheck = config.NewBoolOrUndefined(d.StatusCheck)
 			}
 			if d.DockerDeploy != nil || d.KptDeploy != nil || d.KubectlDeploy != nil || d.KustomizeDeploy != nil {


Test patch:
diff --git a/integration/rpc_test.go b/integration/rpc_test.go
index 7819d3a9c..5c2da5078 100644
--- a/integration/rpc_test.go
+++ b/integration/rpc_test.go
@@ -43,7 +43,7 @@ var (
 	connectionRetries = 2
 	readRetries       = 20
 	numLogEntries     = 7
-	waitTime          = 1 * time.Second
+	waitTime          = 2 * time.Second
 )
 
 func TestEnableRPCFlagDeprecation(t *testing.T) {
diff --git a/pkg/skaffold/deploy/deploy_mux_test.go b/pkg/skaffold/deploy/deploy_mux_test.go
index b04121549..9e08d51f7 100644
--- a/pkg/skaffold/deploy/deploy_mux_test.go
+++ b/pkg/skaffold/deploy/deploy_mux_test.go
@@ -25,6 +25,8 @@ import (
 	"os"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
+
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/access"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/debug"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/graph"
@@ -295,3 +297,45 @@ func TestDeployerMux_Render(t *testing.T) {
 		testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expectedRender, string(content))
 	})
 }
+
+func TestDeployerMux_GetDeployersInverse(t *testing.T) {
+	d1 := NewMockDeployer()
+	d2 := NewMockDeployer()
+	d3 := NewMockDeployer()
+	d4 := NewMockDeployer()
+	d5 := NewMockDeployer()
+
+	tests := []struct {
+		name     string
+		args     []Deployer
+		expected []Deployer
+	}{
+		{
+			name:     "uneven slice",
+			args:     []Deployer{d1, d2, d3, d4, d5},
+			expected: []Deployer{d5, d4, d3, d2, d1},
+		},
+		{
+			name:     "even slice",
+			args:     []Deployer{d1, d2, d3, d4},
+			expected: []Deployer{d4, d3, d2, d1},
+		},
+		{
+			name:     "slice of one",
+			args:     []Deployer{d1},
+			expected: []Deployer{d1},
+		},
+		{
+			name:     "slice of zero",
+			args:     []Deployer{},
+			expected: []Deployer{},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			deployerMux := DeployerMux{deployers: test.args, iterativeStatusCheck: false}
+			testutil.CheckDeepEqual(t, test.expected, deployerMux.GetDeployersInverse(), cmp.AllowUnexported(MockDeployer{}))
+		})
+	}
+}