Make ownership override allowed apps flag dangerous

Signed-off-by: Soumik Majumder <soumikm@vmware.com>

pkg/kapp/cmd/app/deploy_flags.go

@@ -49,8 +49,8 @@ func (s *DeployFlags) Set(cmd *cobra.Command) {
 		100, "Concurrency to check for existing non-labeled resources")
 	cmd.Flags().BoolVar(&s.OverrideOwnershipOfExistingResources, "dangerous-override-ownership-of-existing-resources",
 		false, "Steal existing resources from another app")
-	cmd.Flags().StringSliceVar(&s.OwnershipOverrideAllowedApps, "ownership-override-allowed-apps", nil,
-		"Specify existing apps in the same namespace that existing resources can be stolen from if --dangerous-override-ownership-of-existing-resources is set")
+	cmd.Flags().StringSliceVar(&s.OwnershipOverrideAllowedApps, "dangerous-ownership-override-allowed-apps", nil,
+		"Specify existing apps in the same namespace that existing resources can be stolen from")
 
 	cmd.Flags().BoolVar(&s.DefaultLabelScopingRules, "default-label-scoping-rules",
 		true, "Use default label scoping rules")

pkg/kapp/resources/labeled_resources.go

@@ -134,8 +134,7 @@ func (a *LabeledResources) AllAndMatching(newResources []Resource, opts AllAndMa
 		}
 	}
 
-	if len(nonLabeledResources) > 0 && (!opts.SkipResourceOwnershipCheck ||
-		(opts.SkipResourceOwnershipCheck && len(opts.SkipOwnershipCheckAllowedApps) > 0)) {
+	if len(nonLabeledResources) > 0 && !opts.SkipResourceOwnershipCheck {
 		resourcesForCheck := a.resourcesForOwnershipCheck(newResources, nonLabeledResources)
 		if len(resourcesForCheck) > 0 {
 			err := a.checkResourceOwnership(resourcesForCheck, opts)
@@ -185,14 +184,16 @@ func (a *LabeledResources) checkResourceOwnership(resources []Resource, opts All
 
 	var errs []error
 	labelValAppMap := map[string]string{}
-	if len(opts.SkipOwnershipCheckAllowedApps) > 0 && opts.SkipResourceOwnershipCheck {
+	isSelectiveOwnershipOverride := len(opts.SkipOwnershipCheckAllowedApps) > 0
+	if isSelectiveOwnershipOverride {
 		labelValAppMap = opts.LabelValAppMapResolverFunc()
 	}
 
 	for _, res := range resources {
 		if val, found := res.Labels()[expectedLabelKey]; found {
 			ownershipOverrideAllowed := false
-			if opts.SkipResourceOwnershipCheck {
+
+			if isSelectiveOwnershipOverride {
 				ownershipOverrideAllowed = a.ownershipOverrideAllowed(labelValAppMap, res,
 					expectedLabelKey, opts.SkipOwnershipCheckAllowedApps)
 			}

test/e2e/selective_ownership_override_test.go

@@ -11,6 +11,48 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func TestOwnershipOverride(t *testing.T) {
+	env := BuildEnv(t)
+	logger := Logger{}
+	kapp := Kapp{t, env.Namespace, env.KappBinaryPath, logger}
+
+	const existingAppName = "existing-app"
+	const newAppName = "new-app"
+
+	resourceYAML := `
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: cm-%s
+data:
+  foo: bar
+`
+
+	cleanUp := func() {
+		kapp.Run([]string{"delete", "-a", existingAppName})
+		kapp.Run([]string{"delete", "-a", newAppName})
+	}
+	cleanUp()
+	defer cleanUp()
+
+	logger.Section("deploy existing app", func() {
+		kapp.RunWithOpts([]string{"deploy", "-a", existingAppName, "-f", "-"}, RunOpts{StdinReader: strings.NewReader(fmt.Sprintf(resourceYAML, "1"))})
+	})
+
+	logger.Section("deploy new app with ownership overrides", func() {
+		resourcesString := fmt.Sprintf("%s", fmt.Sprintf(resourceYAML, "1"))
+		// Check for ownership errors without flag
+		_, err := kapp.RunWithOpts([]string{"deploy", "-a", newAppName, "-f", "-"}, RunOpts{StdinReader: strings.NewReader(resourcesString), AllowError: true})
+		require.Error(t, err)
+		require.Contains(t, err.Error(), existingAppName)
+
+		// Check for successful ownership override
+		kapp.RunWithOpts([]string{"deploy", "-a", newAppName, "-f", "-", "--dangerous-override-ownership-of-existing-resources"},
+			RunOpts{StdinReader: strings.NewReader(resourcesString)})
+	})
+}
+
 func TestSelectiveOwnershipOverride(t *testing.T) {
 	env := BuildEnv(t)
 	logger := Logger{}
@@ -51,22 +93,15 @@ data:
 		require.Contains(t, err.Error(), existingAppName1)
 		require.Contains(t, err.Error(), existingAppName2)
 
-		// Test with override scoped while override is disallowed
-		_, err = kapp.RunWithOpts([]string{"deploy", "-a", newAppName, "-f", "-", "--ownership-override-allowed-apps", existingAppName1},
-			RunOpts{StdinReader: strings.NewReader(resourcesString), AllowError: true})
-		require.Error(t, err)
-		require.Contains(t, err.Error(), existingAppName1)
-		require.Contains(t, err.Error(), existingAppName2)
-
 		// Test with override scoped to single existing app
-		_, err = kapp.RunWithOpts([]string{"deploy", "-a", newAppName, "-f", "-", "--dangerous-override-ownership-of-existing-resources", "--ownership-override-allowed-apps", existingAppName1},
+		_, err = kapp.RunWithOpts([]string{"deploy", "-a", newAppName, "-f", "-", "--dangerous-ownership-override-allowed-apps", existingAppName1},
 			RunOpts{StdinReader: strings.NewReader(resourcesString), AllowError: true})
 		require.Error(t, err)
 		require.NotContains(t, err.Error(), existingAppName1)
 		require.Contains(t, err.Error(), existingAppName2)
 
 		// Test with override scoped to both existing app
-		kapp.RunWithOpts([]string{"deploy", "-a", newAppName, "-f", "-", "--dangerous-override-ownership-of-existing-resources", "--ownership-override-allowed-apps", fmt.Sprintf("%s,%s", existingAppName1, existingAppName2)},
+		kapp.RunWithOpts([]string{"deploy", "-a", newAppName, "-f", "-", "--dangerous-ownership-override-allowed-apps", fmt.Sprintf("%s,%s", existingAppName1, existingAppName2)},
 			RunOpts{StdinReader: strings.NewReader(resourcesString)})
 	})
 }