Repository: google/kf
Pull request #951 (2022-11-02 15:55:32)

Add command to fix binding owners.
Due to a bug in earlier versions of Kf, bindings created in app manifests weren't getting owner references.

This change creates a new command: fix-orphaned-bindings to clean them up in two cases:

1. Where a binding has no owning app.
2. Where a binding's owning app was deleted.

It allows both a dry-run mode (default) which prints out equivalent kubectl commands and an actuation mode which uses the K8s API to perform the updates.


Patch:
diff --git a/pkg/kf/commands/root.go b/pkg/kf/commands/root.go
index 8384c76..32e4cef 100644
--- a/pkg/kf/commands/root.go
+++ b/pkg/kf/commands/root.go
@@ -196,6 +196,7 @@ func NewRawKfCommand() *cobra.Command {
 				InjectGetService(p),
 				InjectListServices(p),
 				InjectMarketplace(p),
+				InjectFixOrphanedBindingsCommand(p),
 			},
 		},
 		{
diff --git a/pkg/kf/commands/service-bindings/fix-orphaned-bindings.go b/pkg/kf/commands/service-bindings/fix-orphaned-bindings.go
new file mode 100644
index 0000000..16ab110
--- /dev/null
+++ b/pkg/kf/commands/service-bindings/fix-orphaned-bindings.go
@@ -0,0 +1,167 @@
+// Copyright 2022 Google LLC
+//
+// 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
+//
+//     https://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 servicebindings
+
+import (
+	"encoding/json"
+	"fmt"
+
+	"github.com/fatih/color"
+	"github.com/google/kf/v2/pkg/apis/kf/v1alpha1"
+	"github.com/google/kf/v2/pkg/kf/apps"
+	"github.com/google/kf/v2/pkg/kf/commands/config"
+	"github.com/google/kf/v2/pkg/kf/serviceinstancebindings"
+	"github.com/spf13/cobra"
+	"knative.dev/pkg/kmeta"
+)
+
+// NewDeleteOrphanedRoutesCommand creates a command to delete orphaned routes.
+func NewFixOrphanedBindingsCommand(
+	p *config.KfParams,
+	bindingsClient serviceinstancebindings.Client,
+	appsClient apps.Client,
+) *cobra.Command {
+	var (
+		dryRun = false
+	)
+
+	cmd := &cobra.Command{
+		Use:   "fix-orphaned-bindings",
+		Short: "Fix bindings without an app owner in a space.",
+		Long:  `Fix bindings with a missing owner reference to an App.`,
+		Example: `
+		# Identify broken bindings in the targeted space.
+		kf fix-orphaned-bindings
+
+		# Fix bindings in the targeted space.
+		kf fix-orphaned-bindings --dry-run=false
+		`,
+		Args:         cobra.ExactArgs(0),
+		SilenceUsage: true,
+		RunE: func(cmd *cobra.Command, args []string) error {
+			ctx := cmd.Context()
+			out := cmd.OutOrStdout()
+
+			if err := p.ValidateSpaceTargeted(); err != nil {
+				return err
+			}
+
+			fmt.Fprintf(out, "Finding orphaned bindings in Space: %s\n", p.Space)
+
+			bindings, err := bindingsClient.List(ctx, p.Space)
+			if err != nil {
+				return fmt.Errorf("failed to list Bindings: %v", err)
+			}
+
+			apps, err := appsClient.List(ctx, p.Space)
+			if err != nil {
+				return fmt.Errorf("failed to list Apps: %v", err)
+			}
+			appLookupTable := make(map[string]v1alpha1.App)
+			for _, a := range apps {
+				appLookupTable[a.Name] = a
+			}
+
+			cmdColor := color.New(color.FgCyan)
+
+			deletedBindingsCount := 0
+			ownersFixedCount := 0
+			for _, b := range bindings {
+				appRef := b.Spec.App
+				if appRef == nil {
+					// Case: binding refers to something other than an app.
+					continue
+				}
+
+				app, ok := appLookupTable[appRef.Name]
+				if !ok {
+					// Case: App has been deleted, remove binding.
+					fmt.Fprintf(out, "Deleting binding %q which refers to a missing app %q\n", b.Name, appRef.Name)
+					deletedBindingsCount++
+					fmt.Fprintf(out, cmdColor.Sprintf("  kubectl delete serviceinstancebindings -n %q %q", p.Space, b.Name))
+					fmt.Fprintln(out)
+
+					if !dryRun {
+						if err := bindingsClient.Delete(ctx, p.Space, b.Name); err != nil {
+							return fmt.Errorf("failed to delete binding: %v", err)
+						}
+					}
+
+					continue
+				}
+
+				if ownedByApp(b, app) {
+					// Case: Binding is valid
+					continue
+				}
+
+				// Case: Missing owner
+				fmt.Fprintf(out, "Binding %q is missing an owner reference to app %q\n", b.Name, app.Name)
+				ownersFixedCount++
+				ownerRef := *kmeta.NewControllerRef(&app)
+				jsonOwnerRef, err := json.Marshal(ownerRef)
+				if err != nil {
+					return err
+				}
+
+				fmt.Fprintf(out, cmdColor.Sprintf(
+					`  kubectl patch serviceinstancebinding -n %q %q --type=json -p='[{"op":"add", "path":"/metadata/ownerReferences", "value":[%s] }]'`,
+					p.Space,
+					b.Name,
+					jsonOwnerRef,
+				))
+				fmt.Fprintln(out)
+				if !dryRun {
+					if _, err := bindingsClient.Transform(ctx, p.Space, b.Name, func(binding *v1alpha1.ServiceInstanceBinding) error {
+						binding.OwnerReferences = append(binding.OwnerReferences, ownerRef)
+						return nil
+					}); err != nil {
+						return fmt.Errorf("failed to update binding: %v", err)
+					}
+				}
+			}
+
+			fmt.Fprintf(
+				out,
+				"Processed %d binding(s), %d owners fixed, %d deleted\n",
+				len(bindings),
+				ownersFixedCount,
+				deletedBindingsCount,
+			)
+			if dryRun {
+				fmt.Fprintf(
+					out, color.New(color.FgHiYellow).Sprint("Run with --dry-run=false to apply."))
+				fmt.Fprintln(out)
+			}
+
+			return nil
+		},
+	}
+
+	cmd.Flags().BoolVar(&dryRun, "dry-run", true, "Run the command without applying changes.")
+
+	return cmd
+}
+
+func ownedByApp(binding v1alpha1.ServiceInstanceBinding, app v1alpha1.App) bool {
+	for _, owner := range binding.OwnerReferences {
+		// Use UIDs for matching because versions might be mismatched.
+		if owner.UID == app.UID {
+			return true
+		}
+	}
+
+	return false
+}
diff --git a/pkg/kf/commands/wire_gen.go b/pkg/kf/commands/wire_gen.go
index 1996c1e..f245680 100644
--- a/pkg/kf/commands/wire_gen.go
+++ b/pkg/kf/commands/wire_gen.go
@@ -398,6 +398,21 @@ func InjectVcapServices(p *config.KfParams) *cobra.Command {
 	return command
 }
 
+func InjectFixOrphanedBindingsCommand(p *config.KfParams) *cobra.Command {
+	kfV1alpha1Interface := config.GetKfClient(p)
+	serviceInstanceBindingsGetter := provideServiceInstanceBindingsGetter(kfV1alpha1Interface)
+	client := serviceinstancebindings.NewClient(serviceInstanceBindingsGetter)
+	appsGetter := provideAppsGetter(kfV1alpha1Interface)
+	buildsGetter := provideKfBuilds(kfV1alpha1Interface)
+	kubernetesInterface := config.GetKubernetes(p)
+	buildTailer := builds.TektonLoggingShim(kubernetesInterface)
+	buildsClient := builds.NewClient(p, buildsGetter, buildTailer)
+	tailer := logs.NewTailer(kubernetesInterface)
+	appsClient := apps.NewClient(appsGetter, buildsClient, tailer)
+	command := servicebindings.NewFixOrphanedBindingsCommand(p, client, appsClient)
+	return command
+}
+
 func InjectCreateServiceBroker(p *config.KfParams) *cobra.Command {
 	kfV1alpha1Interface := config.GetKfClient(p)
 	client := cluster.NewClient(kfV1alpha1Interface)
diff --git a/pkg/kf/commands/wire_injector.go b/pkg/kf/commands/wire_injector.go
index 75ef39b..7f05f27 100644
--- a/pkg/kf/commands/wire_injector.go
+++ b/pkg/kf/commands/wire_injector.go
@@ -350,6 +350,14 @@ func InjectVcapServices(p *config.KfParams) *cobra.Command {
 	return nil
 }
 
+func InjectFixOrphanedBindingsCommand(p *config.KfParams) *cobra.Command {
+	wire.Build(
+		servicebindingscmd.NewFixOrphanedBindingsCommand,
+		ServiceBindingsSet,
+	)
+	return nil
+}
+
 ///////////////////////
 // Service Brokers  //
 /////////////////////


Test patch:
diff --git a/pkg/kf/commands/service-bindings/fix-orphaned-bindings_test.go b/pkg/kf/commands/service-bindings/fix-orphaned-bindings_test.go
new file mode 100644
index 0000000..a3423d8
--- /dev/null
+++ b/pkg/kf/commands/service-bindings/fix-orphaned-bindings_test.go
@@ -0,0 +1,216 @@
+// Copyright 2022 Google LLC
+//
+// 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
+//
+//     https://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 servicebindings
+
+import (
+	"bytes"
+	"errors"
+	"testing"
+
+	"github.com/golang/mock/gomock"
+	"github.com/google/kf/v2/pkg/apis/kf/v1alpha1"
+	appsfake "github.com/google/kf/v2/pkg/kf/apps/fake"
+	"github.com/google/kf/v2/pkg/kf/commands/config"
+	serviceinstancebindingsfake "github.com/google/kf/v2/pkg/kf/serviceinstancebindings/fake"
+	"github.com/google/kf/v2/pkg/kf/testutil"
+	"knative.dev/pkg/kmeta"
+)
+
+func TestFixOrphanedBindings(t *testing.T) {
+	t.Parallel()
+
+	appA := v1alpha1.App{}
+	appA.UID = "aaaa-aaaa-aaaa-aaaa"
+	appA.Name = "app-a"
+
+	bindingA := v1alpha1.ServiceInstanceBinding{}
+	bindingA.Name = "binding-a"
+	bindingA.ObjectMeta.OwnerReferences = append(bindingA.ObjectMeta.OwnerReferences, *kmeta.NewControllerRef(&appA))
+	bindingA.Spec.App = &v1alpha1.AppRef{Name: "app-a"}
+
+	type fakes struct {
+		servicebindings *serviceinstancebindingsfake.FakeClient
+		apps            *appsfake.FakeClient
+	}
+	cases := map[string]struct {
+		space       string
+		args        []string
+		setup       func(*testing.T, fakes)
+		wantErr     error
+		wantOutputs []string
+	}{
+		"wrong number of args": {
+			args:    []string{"example.com"},
+			wantErr: errors.New("accepts 0 arg(s), received 1"),
+		},
+		"listing bindings fails": {
+			space: "some-namespace",
+			setup: func(t *testing.T, fakes fakes) {
+				fakes.servicebindings.EXPECT().List(gomock.Any(), gomock.Any()).Return(nil, errors.New("some-error"))
+			},
+			wantErr: errors.New("failed to list Bindings: some-error"),
+		},
+		"listing apps fails": {
+			space: "some-namespace",
+			setup: func(t *testing.T, fakes fakes) {
+				fakes.servicebindings.EXPECT().List(gomock.Any(), gomock.Any())
+				fakes.apps.EXPECT().List(gomock.Any(), gomock.Any()).Return(nil, errors.New("some-error"))
+			},
+			wantErr: errors.New("failed to list Apps: some-error"),
+		},
+		"empty space": {
+			space: "some-namespace",
+			setup: func(t *testing.T, fakes fakes) {
+				fakes.servicebindings.EXPECT().List(gomock.Any(), gomock.Any()).Return([]v1alpha1.ServiceInstanceBinding{}, nil)
+				fakes.apps.EXPECT().List(gomock.Any(), gomock.Any()).Return([]v1alpha1.App{}, nil)
+			},
+			wantOutputs: []string{"Processed 0 binding(s), 0 owners fixed, 0 deleted"},
+		},
+		"valid ref": {
+			space: "some-namespace",
+			setup: func(t *testing.T, fakes fakes) {
+				fakes.servicebindings.EXPECT().List(gomock.Any(), gomock.Any()).Return([]v1alpha1.ServiceInstanceBinding{
+					bindingA,
+				}, nil)
+				fakes.apps.EXPECT().List(gomock.Any(), gomock.Any()).Return([]v1alpha1.App{
+					appA,
+				}, nil)
+			},
+			wantOutputs: []string{"Processed 1 binding(s), 0 owners fixed, 0 deleted"},
+		},
+		"deleted app": {
+			space: "some-namespace",
+			setup: func(t *testing.T, fakes fakes) {
+				missingApp := v1alpha1.ServiceInstanceBinding{}
+				missingApp.Spec.App = &v1alpha1.AppRef{Name: "missing"}
+				fakes.servicebindings.EXPECT().List(gomock.Any(), gomock.Any()).Return([]v1alpha1.ServiceInstanceBinding{
+					missingApp,
+				}, nil)
+
+				fakes.apps.EXPECT().List(gomock.Any(), gomock.Any()).Return([]v1alpha1.App{}, nil)
+			},
+		},
+		"dry run by default": {
+			space: "some-namespace",
+			setup: func(t *testing.T, fakes fakes) {
+				fakes.servicebindings.EXPECT().List(gomock.Any(), gomock.Any()).Return([]v1alpha1.ServiceInstanceBinding{}, nil)
+				fakes.apps.EXPECT().List(gomock.Any(), gomock.Any()).Return([]v1alpha1.App{}, nil)
+			},
+			wantOutputs: []string{"Run with --dry-run=false to apply"},
+		},
+		"dry-run delete": {
+			space: "some-namespace",
+			args:  []string{"--dry-run=true"},
+			setup: func(t *testing.T, fakes fakes) {
+				fakes.servicebindings.EXPECT().List(gomock.Any(), gomock.Any()).Return([]v1alpha1.ServiceInstanceBinding{
+					bindingA,
+				}, nil)
+				fakes.apps.EXPECT().List(gomock.Any(), gomock.Any()).Return([]v1alpha1.App{}, nil)
+			},
+			wantOutputs: []string{
+				"Processed 1 binding(s), 0 owners fixed, 1 deleted",
+				"Run with --dry-run=false to apply",
+			},
+		},
+		"dry-run false actuates deletes": {
+			space: "some-namespace",
+			args:  []string{"--dry-run=false"},
+			setup: func(t *testing.T, fakes fakes) {
+				fakes.servicebindings.EXPECT().List(gomock.Any(), gomock.Any()).Return([]v1alpha1.ServiceInstanceBinding{
+					bindingA,
+				}, nil)
+				fakes.apps.EXPECT().List(gomock.Any(), gomock.Any()).Return([]v1alpha1.App{}, nil)
+
+				fakes.servicebindings.EXPECT().Delete(gomock.Any(), gomock.Eq("some-namespace"), gomock.Eq("binding-a"))
+			},
+			wantOutputs: []string{"Processed 1 binding(s), 0 owners fixed, 1 deleted"},
+		},
+		"dry-run transforms": {
+			space: "some-namespace",
+			args:  []string{"--dry-run=true"},
+			setup: func(t *testing.T, fakes fakes) {
+				bindingANoOwner := v1alpha1.ServiceInstanceBinding{}
+				bindingANoOwner.Spec.App = &v1alpha1.AppRef{Name: appA.Name}
+
+				fakes.servicebindings.EXPECT().List(gomock.Any(), gomock.Any()).Return([]v1alpha1.ServiceInstanceBinding{
+					bindingANoOwner,
+				}, nil)
+				fakes.apps.EXPECT().List(gomock.Any(), gomock.Any()).Return([]v1alpha1.App{
+					appA,
+				}, nil)
+			},
+			wantOutputs: []string{
+				"Processed 1 binding(s), 1 owners fixed, 0 deleted",
+				"Run with --dry-run=false to apply",
+			},
+		},
+		"dry-run false actuates transforms": {
+			space: "some-namespace",
+			args:  []string{"--dry-run=false"},
+			setup: func(t *testing.T, fakes fakes) {
+				bindingANoOwner := v1alpha1.ServiceInstanceBinding{}
+				bindingANoOwner.Spec.App = &v1alpha1.AppRef{Name: appA.Name}
+
+				fakes.servicebindings.EXPECT().List(gomock.Any(), gomock.Any()).Return([]v1alpha1.ServiceInstanceBinding{
+					bindingANoOwner,
+				}, nil)
+				fakes.apps.EXPECT().List(gomock.Any(), gomock.Any()).Return([]v1alpha1.App{
+					appA,
+				}, nil)
+
+				fakes.servicebindings.EXPECT().Transform(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
+			},
+			wantOutputs: []string{"Processed 1 binding(s), 1 owners fixed, 0 deleted"},
+		},
+	}
+
+	for tn, tc := range cases {
+		t.Run(tn, func(t *testing.T) {
+			ctrl := gomock.NewController(t)
+			sbClient := serviceinstancebindingsfake.NewFakeClient(ctrl)
+			appsClient := appsfake.NewFakeClient(ctrl)
+
+			if tc.setup != nil {
+				tc.setup(t, fakes{
+					servicebindings: sbClient,
+					apps:            appsClient,
+				})
+			}
+
+			var buffer bytes.Buffer
+			cmd := NewFixOrphanedBindingsCommand(
+				&config.KfParams{
+					Space: tc.space,
+				},
+				sbClient,
+				appsClient,
+			)
+
+			if tc.args == nil {
+				// We have to set this to something that is non-nil so that
+				// cobra doesn't go use os.Args (which might have test flags
+				// set).
+				tc.args = make([]string, 0)
+			}
+
+			cmd.SetArgs(tc.args)
+			cmd.SetOutput(&buffer)
+
+			gotErr := cmd.Execute()
+			testutil.AssertErrorsEqual(t, tc.wantErr, gotErr)
+			testutil.AssertContainsAll(t, buffer.String(), tc.wantOutputs)
+		})
+	}
+}