From e57465898a3d562eba7b9760bf1e8a0d13c65203 Mon Sep 17 00:00:00 2001 From: Plamen Kokanov Date: Mon, 6 Dec 2021 16:20:47 +0200 Subject: [PATCH 1/6] Fixes deletion of remedy controller resources during cp migration --- pkg/controller/controlplane/actuator.go | 52 +++- pkg/controller/controlplane/actuator_test.go | 70 +++++- .../gardener/pkg/utils/test/gomock.go | 49 ++++ .../gardener/pkg/utils/test/options.go | 142 +++++++++++ .../gardener/gardener/pkg/utils/test/test.go | 232 ++++++++++++++++++ .../gardener/pkg/utils/test/test_resources.go | 135 ++++++++++ vendor/modules.txt | 1 + 7 files changed, 671 insertions(+), 10 deletions(-) create mode 100644 vendor/github.com/gardener/gardener/pkg/utils/test/gomock.go create mode 100644 vendor/github.com/gardener/gardener/pkg/utils/test/options.go create mode 100644 vendor/github.com/gardener/gardener/pkg/utils/test/test.go create mode 100644 vendor/github.com/gardener/gardener/pkg/utils/test/test_resources.go diff --git a/pkg/controller/controlplane/actuator.go b/pkg/controller/controlplane/actuator.go index ccc2bcc88..2493da36c 100644 --- a/pkg/controller/controlplane/actuator.go +++ b/pkg/controller/controlplane/actuator.go @@ -28,6 +28,7 @@ import ( extensionscontroller "github.com/gardener/gardener/extensions/pkg/controller" "github.com/gardener/gardener/extensions/pkg/controller/controlplane" extensionsv1alpha1 "github.com/gardener/gardener/pkg/apis/extensions/v1alpha1" + "github.com/gardener/gardener/pkg/controllerutils" kutil "github.com/gardener/gardener/pkg/utils/kubernetes" azurev1alpha1 "github.com/gardener/remedy-controller/pkg/apis/azure/v1alpha1" ) @@ -70,6 +71,9 @@ func (a *actuator) Delete( cluster *extensionscontroller.Cluster, ) error { if cp.Spec.Purpose == nil || *cp.Spec.Purpose == extensionsv1alpha1.Normal { + if err := a.annotatePublicIPAddresses(ctx, cp); err != nil { + return err + } // Delete all remaining remedy controller resources if err := a.deleteRemedyControllerResources(ctx, cp); err != nil { return err @@ -88,20 +92,23 @@ func (a *actuator) Migrate( cp *extensionsv1alpha1.ControlPlane, cluster *extensionscontroller.Cluster, ) error { + // Call Migrate on the composed Actuator so that the controlplane chart is deleted and therefore + // the remedy controller is also removed. + if err := a.Actuator.Migrate(ctx, cp, cluster); err != nil { + return err + } if cp.Spec.Purpose == nil || *cp.Spec.Purpose == extensionsv1alpha1.Normal { // Delete all remaining remedy controller resources - if err := a.deleteRemedyControllerResources(ctx, cp); err != nil { + if err := a.removeFinalizersFromRemedyControllerResources(ctx, cp); err != nil { return err } + return a.deleteRemedyControllerResources(ctx, cp) } - - // Call Migrate on the composed Actuator - return a.Actuator.Migrate(ctx, cp, cluster) + return nil } -func (a *actuator) deleteRemedyControllerResources(ctx context.Context, cp *extensionsv1alpha1.ControlPlane) error { - // Forcefully delete all remaining remedy controller resources - a.logger.Info("Deleting all remaining remedy controller resources") +func (a *actuator) annotatePublicIPAddresses(ctx context.Context, cp *extensionsv1alpha1.ControlPlane) error { + a.logger.Info("Adding do-not-clean annotation on publicipaddresses") pubipList := &azurev1alpha1.PublicIPAddressList{} if err := a.client.List(ctx, pubipList, client.InNamespace(cp.Namespace)); err != nil { return fmt.Errorf("could not list publicipaddresses: %w", err) @@ -116,6 +123,37 @@ func (a *actuator) deleteRemedyControllerResources(ctx context.Context, cp *exte return fmt.Errorf("could not add do-not-clean annotation on publicipaddress: %w", err) } } + + return nil +} + +func (a *actuator) removeFinalizersFromRemedyControllerResources(ctx context.Context, cp *extensionsv1alpha1.ControlPlane) error { + a.logger.Info("Removing finalizers from remedy controller resources") + pubipList := &azurev1alpha1.PublicIPAddressList{} + if err := a.client.List(ctx, pubipList, client.InNamespace(cp.Namespace)); err != nil { + return fmt.Errorf("could not list publicipaddresses: %w", err) + } + for _, pubip := range pubipList.Items { + if err := controllerutils.PatchRemoveFinalizers(ctx, a.client, &pubip, "azure.remedy.gardener.cloud/publicipaddress"); err != nil { + return fmt.Errorf("could not remove finalizers from publicipaddress: %w", err) + } + } + + virtualMachineList := &azurev1alpha1.VirtualMachineList{} + if err := a.client.List(ctx, virtualMachineList, client.InNamespace(cp.Namespace)); err != nil { + return fmt.Errorf("could not list virtualmachines: %w", err) + } + for _, virtualMachine := range virtualMachineList.Items { + if err := controllerutils.PatchRemoveFinalizers(ctx, a.client, &virtualMachine, "azure.remedy.gardener.cloud/virtualmachine"); err != nil { + return fmt.Errorf("could not remove finalizers from virtualmachine: %w", err) + } + } + + return nil +} + +func (a *actuator) deleteRemedyControllerResources(ctx context.Context, cp *extensionsv1alpha1.ControlPlane) error { + a.logger.Info("Deleting all remaining remedy controller resources") if err := a.client.DeleteAllOf(ctx, &azurev1alpha1.PublicIPAddress{}, client.InNamespace(cp.Namespace)); err != nil { return fmt.Errorf("could not delete publicipaddress resources: %w", err) } diff --git a/pkg/controller/controlplane/actuator_test.go b/pkg/controller/controlplane/actuator_test.go index 668d8387e..49dde45ed 100644 --- a/pkg/controller/controlplane/actuator_test.go +++ b/pkg/controller/controlplane/actuator_test.go @@ -26,6 +26,7 @@ import ( gardencorev1beta1 "github.com/gardener/gardener/pkg/apis/core/v1beta1" extensionsv1alpha1 "github.com/gardener/gardener/pkg/apis/extensions/v1alpha1" mockclient "github.com/gardener/gardener/pkg/mock/controller-runtime/client" + "github.com/gardener/gardener/pkg/utils/test" "github.com/golang/mock/gomock" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -60,15 +61,26 @@ var _ = Describe("Actuator", func() { newPubip = func(annotations map[string]string) *azurev1alpha1.PublicIPAddress { return &azurev1alpha1.PublicIPAddress{ ObjectMeta: metav1.ObjectMeta{ - Name: "foo-1.2.3.4", - Namespace: namespace, - Annotations: annotations, + Name: "foo-1.2.3.4", + Namespace: namespace, + Annotations: annotations, + ResourceVersion: "1", }, Spec: azurev1alpha1.PublicIPAddressSpec{ IPAddress: "1.2.3.4", }, } } + + newVirtualMachine = func() *azurev1alpha1.VirtualMachine { + return &azurev1alpha1.VirtualMachine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node-name", + Namespace: namespace, + ResourceVersion: "1", + }, + } + } ) BeforeEach(func() { @@ -124,4 +136,56 @@ var _ = Describe("Actuator", func() { Expect(err).NotTo(HaveOccurred()) }) }) + + Describe("#Migrate", func() { + It("should remove finalizers from remedy controller resources and then delete them", func() { + cp := newControlPlane(nil) + a.EXPECT().Migrate(ctx, cp, cluster).Return(nil) + + pubip := newPubip(nil) + pubipWithFinalizers := pubip.DeepCopy() + pubipWithFinalizers.Finalizers = append(pubipWithFinalizers.Finalizers, "azure.remedy.gardener.cloud/publicipaddress") + c.EXPECT().List(ctx, &azurev1alpha1.PublicIPAddressList{}, client.InNamespace(namespace)). + DoAndReturn(func(_ context.Context, list *azurev1alpha1.PublicIPAddressList, _ ...client.ListOption) error { + list.Items = []azurev1alpha1.PublicIPAddress{*pubipWithFinalizers} + return nil + }) + test.EXPECTPatchWithOptimisticLock(ctx, c, pubip, pubipWithFinalizers) + c.EXPECT().DeleteAllOf(ctx, &azurev1alpha1.PublicIPAddress{}, client.InNamespace(namespace)).Return(nil) + + vm := newVirtualMachine() + vmWithFinalizers := vm.DeepCopy() + vmWithFinalizers.Finalizers = append(vmWithFinalizers.Finalizers, "azure.remedy.gardener.cloud/virtualmachine") + c.EXPECT().List(ctx, &azurev1alpha1.VirtualMachineList{}, client.InNamespace(namespace)). + DoAndReturn(func(_ context.Context, list *azurev1alpha1.VirtualMachineList, _ ...client.ListOption) error { + list.Items = []azurev1alpha1.VirtualMachine{*vmWithFinalizers} + return nil + }) + test.EXPECTPatchWithOptimisticLock(ctx, c, vm, vmWithFinalizers) + c.EXPECT().DeleteAllOf(ctx, &azurev1alpha1.VirtualMachine{}, client.InNamespace(namespace)).Return(nil) + + c.EXPECT().List(gomock.Any(), &azurev1alpha1.PublicIPAddressList{}, client.InNamespace(namespace)). + DoAndReturn(func(_ context.Context, list *azurev1alpha1.PublicIPAddressList, _ ...client.ListOption) error { + list.Items = []azurev1alpha1.PublicIPAddress{} + return nil + }) + c.EXPECT().List(gomock.Any(), &azurev1alpha1.VirtualMachineList{}, client.InNamespace(namespace)). + DoAndReturn(func(_ context.Context, list *azurev1alpha1.VirtualMachineList, _ ...client.ListOption) error { + list.Items = []azurev1alpha1.VirtualMachine{} + return nil + }) + + err := actuator.Migrate(ctx, cp, cluster) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should neither remove finalizers from remaining remedy controller resources nor delete them for controlplane with purpose exposure", func() { + exposure := extensionsv1alpha1.Exposure + cp := newControlPlane(&exposure) + a.EXPECT().Migrate(ctx, cp, cluster).Return(nil) + + err := actuator.Migrate(ctx, cp, cluster) + Expect(err).NotTo(HaveOccurred()) + }) + }) }) diff --git a/vendor/github.com/gardener/gardener/pkg/utils/test/gomock.go b/vendor/github.com/gardener/gardener/pkg/utils/test/gomock.go new file mode 100644 index 000000000..9da2a3a05 --- /dev/null +++ b/vendor/github.com/gardener/gardener/pkg/utils/test/gomock.go @@ -0,0 +1,49 @@ +// Copyright (c) 2021 SAP SE or an SAP affiliate company. All rights reserved. This file is licensed under the Apache Software License, v. 2 except as noted otherwise in the LICENSE file +// +// 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 test + +import ( + "fmt" + + "github.com/golang/mock/gomock" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// HasObjectKeyOf returns a gomock.Matcher that matches if actual is a client.Object that has the same +// ObjectKey as expected. +func HasObjectKeyOf(expected client.Object) gomock.Matcher { + return &objectKeyMatcher{key: client.ObjectKeyFromObject(expected)} +} + +type objectKeyMatcher struct { + key client.ObjectKey +} + +func (o *objectKeyMatcher) Matches(actual interface{}) bool { + if actual == nil { + return false + } + + obj, ok := actual.(client.Object) + if !ok { + return false + } + + return o.key == client.ObjectKeyFromObject(obj) +} + +func (o *objectKeyMatcher) String() string { + return fmt.Sprintf("has object key %q", o.key) +} diff --git a/vendor/github.com/gardener/gardener/pkg/utils/test/options.go b/vendor/github.com/gardener/gardener/pkg/utils/test/options.go new file mode 100644 index 000000000..8f7d1a165 --- /dev/null +++ b/vendor/github.com/gardener/gardener/pkg/utils/test/options.go @@ -0,0 +1,142 @@ +// Copyright (c) 2019 SAP SE or an SAP affiliate company. All rights reserved. This file is licensed under the Apache Software License, v. 2 except as noted otherwise in the LICENSE file +// +// 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 test + +import ( + "fmt" + "strings" +) + +// Flag is a flag that can be represented as a slice of strings. +type Flag interface { + // Slice returns a representation of this Flag as a slice of strings. + Slice() []string +} + +func keyToFlag(key string) string { + return fmt.Sprintf("--%s", key) +} + +type intFlag struct { + key string + value int +} + +func (f *intFlag) Slice() []string { + return []string{keyToFlag(f.key), fmt.Sprintf("%d", f.value)} +} + +type stringFlag struct { + key string + value string +} + +func (f *stringFlag) Slice() []string { + return []string{keyToFlag(f.key), f.value} +} + +type boolFlag struct { + key string + value bool +} + +func (f *boolFlag) Slice() []string { + var value string + if f.value { + value = "true" + } else { + value = "false" + } + + return []string{keyToFlag(f.key), value} +} + +type stringSliceFlag struct { + key string + value []string +} + +func (f *stringSliceFlag) Slice() []string { + return []string{keyToFlag(f.key), strings.Join(f.value, ",")} +} + +// IntFlag returns a Flag with the given key and integer value. +func IntFlag(key string, value int) Flag { + return &intFlag{key, value} +} + +// StringFlag returns a Flag with the given key and string value. +func StringFlag(key, value string) Flag { + return &stringFlag{key, value} +} + +// BoolFlag returns a Flag with the given key and boolean value. +func BoolFlag(key string, value bool) Flag { + return &boolFlag{key, value} +} + +// StringSliceFlag returns a flag with the given key and string slice value. +func StringSliceFlag(key string, value ...string) Flag { + return &stringSliceFlag{key, value} +} + +// Command is a command that has a name, a list of flags, and a list of arguments. +type Command struct { + Name string + Flags []Flag + Args []string +} + +// CommandBuilder is a builder for Command objects. +type CommandBuilder struct { + command Command +} + +// NewCommandBuilder creates and returns a new CommandBuilder with the given name. +func NewCommandBuilder(name string) *CommandBuilder { + return &CommandBuilder{Command{Name: name}} +} + +// Flags appends the given flags to this CommandBuilder. +func (c *CommandBuilder) Flags(flags ...Flag) *CommandBuilder { + c.command.Flags = append(c.command.Flags, flags...) + return c +} + +// Args appends the given arguments to this CommandBuilder. +func (c *CommandBuilder) Args(args ...string) *CommandBuilder { + c.command.Args = append(c.command.Args, args...) + return c +} + +// Command returns the Command that has been built by this CommandBuilder. +func (c *CommandBuilder) Command() *Command { + return &c.command +} + +// Slice returns a representation of this Command as a slice of strings. +func (c *Command) Slice() []string { + out := []string{c.Name} + for _, flag := range c.Flags { + out = append(out, flag.Slice()...) + } + out = append(out, c.Args...) + return out +} + +// String returns a representation of this Command as a string. +func (c *Command) String() string { + return strings.Join(c.Slice(), " ") +} diff --git a/vendor/github.com/gardener/gardener/pkg/utils/test/test.go b/vendor/github.com/gardener/gardener/pkg/utils/test/test.go new file mode 100644 index 000000000..c798fbe18 --- /dev/null +++ b/vendor/github.com/gardener/gardener/pkg/utils/test/test.go @@ -0,0 +1,232 @@ +// Copyright (c) 2018 SAP SE or an SAP affiliate company. All rights reserved. This file is licensed under the Apache Software License, v. 2 except as noted otherwise in the LICENSE file +// +// 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 test + +import ( + "context" + "fmt" + "os" + "reflect" + + . "github.com/gardener/gardener/pkg/utils/test/matchers" + "github.com/golang/mock/gomock" + "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/types" + "k8s.io/component-base/featuregate" + "sigs.k8s.io/controller-runtime/pkg/client" + + mockclient "github.com/gardener/gardener/pkg/mock/controller-runtime/client" +) + +// WithVar sets the given var to the src value and returns a function to revert to the original state. +// The type of `dst` has to be a settable pointer. +// The value of `src` has to be assignable to the type of `dst`. +// +// Example usage: +// v := "foo" +// defer WithVar(&v, "bar")() +func WithVar(dst, src interface{}) func() { + dstValue := reflect.ValueOf(dst) + if dstValue.Type().Kind() != reflect.Ptr { + ginkgo.Fail(fmt.Sprintf("destination value %T is not a pointer", dst)) + } + + if dstValue.CanSet() { + ginkgo.Fail(fmt.Sprintf("value %T cannot be set", dst)) + } + + srcValue := reflect.ValueOf(src) + if srcValue.Type().AssignableTo(dstValue.Type()) { + ginkgo.Fail(fmt.Sprintf("cannot write %T into %T", src, dst)) + } + + tmp := dstValue.Elem().Interface() + dstValue.Elem().Set(srcValue) + return func() { + dstValue.Elem().Set(reflect.ValueOf(tmp)) + } +} + +// WithVars sets the given vars to the given values and returns a function to revert back. +// dstsAndSrcs have to appear in pairs of 2, otherwise there will be a runtime panic. +// +// Example usage: +// defer WithVars(&v, "foo", &x, "bar")() +func WithVars(dstsAndSrcs ...interface{}) func() { + if len(dstsAndSrcs)%2 != 0 { + ginkgo.Fail(fmt.Sprintf("dsts and srcs are not of equal length: %v", dstsAndSrcs)) + } + reverts := make([]func(), 0, len(dstsAndSrcs)/2) + + for i := 0; i < len(dstsAndSrcs); i += 2 { + dst := dstsAndSrcs[i] + src := dstsAndSrcs[i+1] + + reverts = append(reverts, WithVar(dst, src)) + } + + return func() { + for _, revert := range reverts { + revert() + } + } +} + +// WithEnvVar sets the env variable to the given environment variable and returns a function to revert. +// If the value is empty, the environment variable will be unset. +func WithEnvVar(key, value string) func() { + tmp := os.Getenv(key) + + var err error + if value == "" { + err = os.Unsetenv(key) + } else { + err = os.Setenv(key, value) + } + if err != nil { + ginkgo.Fail(fmt.Sprintf("Could not set the env variable %q to %q: %v", key, value, err)) + } + + return func() { + var err error + if tmp == "" { + err = os.Unsetenv(key) + } else { + err = os.Setenv(key, tmp) + } + if err != nil { + ginkgo.Fail(fmt.Sprintf("Could not revert the env variable %q to %q: %v", key, value, err)) + } + } +} + +// WithWd sets the working directory and returns a function to revert to the previous one. +func WithWd(path string) func() { + oldPath, err := os.Getwd() + if err != nil { + ginkgo.Fail(fmt.Sprintf("Could not obtain current working diretory: %v", err)) + } + + if err := os.Chdir(path); err != nil { + ginkgo.Fail(fmt.Sprintf("Could not change working diretory: %v", err)) + } + + return func() { + if err := os.Chdir(oldPath); err != nil { + ginkgo.Fail(fmt.Sprintf("Could not revert working diretory: %v", err)) + } + } +} + +// WithFeatureGate sets the specified gate to the specified value, and returns a function that restores the original value. +// Failures to set or restore cause the test to fail. +// Example use: +// defer WithFeatureGate(utilfeature.DefaultFeatureGate, features., true)() +func WithFeatureGate(gate featuregate.FeatureGate, f featuregate.Feature, value bool) func() { + originalValue := gate.Enabled(f) + + if err := gate.(featuregate.MutableFeatureGate).Set(fmt.Sprintf("%s=%v", f, value)); err != nil { + ginkgo.Fail(fmt.Sprintf("could not set feature gate %s=%v: %v", f, value, err)) + } + + return func() { + if err := gate.(featuregate.MutableFeatureGate).Set(fmt.Sprintf("%s=%v", f, originalValue)); err != nil { + ginkgo.Fail(fmt.Sprintf("could not restore feature gate %s=%v: %v", f, originalValue, err)) + } + } +} + +// WithTempFile creates a temporary file with the given dir and pattern, writes the given content to it, +// and returns a function to delete it. Failures to create, open, close, or delete the file case the test to fail. +// +// The filename is generated by taking pattern and adding a random string to the end. If pattern includes a "*", +// the random string replaces the last "*". If dir is the empty string, WriteTempFile uses the default directory for +// temporary files (see ioutil.TempFile). The caller can use the value of fileName to find the pathname of the file. +// +// Example usage: +// var fileName string +// defer WithTempFile("", "test", []byte("test file content"), &fileName)() +func WithTempFile(dir, pattern string, content []byte, fileName *string) func() { + file, err := os.CreateTemp(dir, pattern) + if err != nil { + ginkgo.Fail(fmt.Sprintf("could not create temp file in directory %s: %v", dir, err)) + } + + *fileName = file.Name() + + if _, err := file.Write(content); err != nil { + ginkgo.Fail(fmt.Sprintf("could not write to temp file %s: %v", file.Name(), err)) + } + if err := file.Close(); err != nil { + ginkgo.Fail(fmt.Sprintf("could not close temp file %s: %v", file.Name(), err)) + } + + return func() { + if err := os.Remove(file.Name()); err != nil { + ginkgo.Fail(fmt.Sprintf("could not delete temp file %s: %v", file.Name(), err)) + } + } +} + +// EXPECTPatch is a helper function for a GoMock call expecting a patch with the mock client. +func EXPECTPatch(ctx context.Context, c *mockclient.MockClient, expectedObj, mergeFrom client.Object, patchType types.PatchType, rets ...interface{}) *gomock.Call { + var expectedPatch client.Patch + + switch patchType { + case types.MergePatchType: + expectedPatch = client.MergeFrom(mergeFrom) + case types.StrategicMergePatchType: + expectedPatch = client.StrategicMergeFrom(mergeFrom.DeepCopyObject().(client.Object)) + } + + return expectPatch(ctx, c, expectedObj, expectedPatch, rets...) +} + +// EXPECTPatchWithOptimisticLock is a helper function for a GoMock call with the mock client +// expecting a merge patch with optimistic lock. +func EXPECTPatchWithOptimisticLock(ctx context.Context, c *mockclient.MockClient, expectedObj, mergeFrom client.Object, rets ...interface{}) *gomock.Call { + expectedPatch := client.MergeFromWithOptions(mergeFrom, client.MergeFromWithOptimisticLock{}) + return expectPatch(ctx, c, expectedObj, expectedPatch, rets...) +} + +func expectPatch(ctx context.Context, c *mockclient.MockClient, expectedObj client.Object, expectedPatch client.Patch, rets ...interface{}) *gomock.Call { + expectedData, expectedErr := expectedPatch.Data(expectedObj) + Expect(expectedErr).To(BeNil()) + + if rets == nil { + rets = []interface{}{nil} + } + + // match object key here, but verify contents only inside DoAndReturn. + // This is to tell gomock, for which object we expect the given patch, but to enable rich yaml diff between + // actual and expected via `DeepEqual`. + return c. + EXPECT(). + Patch(ctx, HasObjectKeyOf(expectedObj), gomock.Any()). + DoAndReturn(func(_ context.Context, obj client.Object, patch client.Patch, _ ...client.PatchOption) error { + // if one of these Expects fails and Patch is called in some goroutine (e.g. via flow.Parallel) + // the failures will not be shown, as the ginkgo panic is not recovered, so the test is hard to fix + defer ginkgo.GinkgoRecover() + + Expect(obj).To(DeepEqual(expectedObj)) + data, err := patch.Data(obj) + Expect(err).To(BeNil()) + Expect(patch.Type()).To(Equal(expectedPatch.Type())) + Expect(string(data)).To(Equal(string(expectedData))) + return nil + }). + Return(rets...) +} diff --git a/vendor/github.com/gardener/gardener/pkg/utils/test/test_resources.go b/vendor/github.com/gardener/gardener/pkg/utils/test/test_resources.go new file mode 100644 index 000000000..9ead6fd76 --- /dev/null +++ b/vendor/github.com/gardener/gardener/pkg/utils/test/test_resources.go @@ -0,0 +1,135 @@ +// Copyright (c) 2021 SAP SE or an SAP affiliate company. All rights reserved. This file is licensed under the Apache Software License, v. 2 except as noted otherwise in the LICENSE file +// +// 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 test + +import ( + "bufio" + "bytes" + "context" + "fmt" + "io" + "os" + "path/filepath" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/serializer" + "k8s.io/apimachinery/pkg/util/sets" + k8syaml "k8s.io/apimachinery/pkg/util/yaml" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// EnsureTestResources reads test resources from path, applies them using the given client and returns the created +// objects. +func EnsureTestResources(ctx context.Context, c client.Client, path string) ([]client.Object, error) { + objects, err := ReadTestResources(c.Scheme(), path) + if err != nil { + return nil, fmt.Errorf("error decoding resources: %w", err) + } + + for _, obj := range objects { + current := obj.DeepCopyObject().(client.Object) + if err := c.Get(ctx, client.ObjectKeyFromObject(current), current); err != nil { + if !apierrors.IsNotFound(err) { + return nil, err + } + + // object doesn't exists, create it + if err := c.Create(ctx, obj); err != nil { + return nil, err + } + } else { + // object already exists, update it + if err := c.Patch(ctx, obj, client.MergeFromWithOptions(current, client.MergeFromWithOptimisticLock{})); err != nil { + return nil, err + } + } + } + return objects, nil +} + +// ReadTestResources reads test resources from path, decodes them using the given scheme and returns the parsed objects. +// Objects are values of the proper API types, if registered in the given scheme, and *unstructured.Unstructured +// otherwise. +func ReadTestResources(scheme *runtime.Scheme, path string) ([]client.Object, error) { + decoder := serializer.NewCodecFactory(scheme).UniversalDeserializer() + + files, err := os.ReadDir(path) + if err != nil { + return nil, err + } + + // file extensions that may contain Webhooks + resourceExtensions := sets.NewString(".json", ".yaml", ".yml") + + var objects []client.Object + for _, file := range files { + + if file.IsDir() { + continue + } + // Only parse allowlisted file types + if !resourceExtensions.Has(filepath.Ext(file.Name())) { + continue + } + + // Unmarshal Webhooks from file into structs + docs, err := readDocuments(filepath.Join(path, file.Name())) + if err != nil { + return nil, err + } + + for _, doc := range docs { + obj, err := runtime.Decode(decoder, doc) + if err != nil { + return nil, err + } + clientObj, ok := obj.(client.Object) + if !ok { + return nil, fmt.Errorf("%T does not implement client.Object", obj) + } + + objects = append(objects, clientObj) + } + } + return objects, nil + +} + +// readDocuments reads documents from file +func readDocuments(fp string) ([][]byte, error) { + b, err := os.ReadFile(fp) + if err != nil { + return nil, err + } + + var docs [][]byte + reader := k8syaml.NewYAMLReader(bufio.NewReader(bytes.NewReader(b))) + for { + // Read document + doc, err := reader.Read() + if err != nil { + if err == io.EOF { + break + } + + return nil, err + } + + docs = append(docs, doc) + } + + return docs, nil +} diff --git a/vendor/modules.txt b/vendor/modules.txt index 9225a57ef..964c41547 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -276,6 +276,7 @@ github.com/gardener/gardener/pkg/utils/kubernetes/unstructured github.com/gardener/gardener/pkg/utils/managedresources github.com/gardener/gardener/pkg/utils/retry github.com/gardener/gardener/pkg/utils/secrets +github.com/gardener/gardener/pkg/utils/test github.com/gardener/gardener/pkg/utils/test/matchers github.com/gardener/gardener/pkg/utils/time github.com/gardener/gardener/pkg/utils/validation/cidr From beac634ade3bc737ddd15719b12f8e78dce84d94 Mon Sep 17 00:00:00 2001 From: Plamen Kokanov Date: Tue, 7 Dec 2021 17:53:52 +0200 Subject: [PATCH 2/6] Refactor deletion logic --- pkg/controller/controlplane/actuator.go | 99 +++++++++----------- pkg/controller/controlplane/actuator_test.go | 96 +++++++++++++------ pkg/controller/controlplane/add.go | 2 +- 3 files changed, 111 insertions(+), 86 deletions(-) diff --git a/pkg/controller/controlplane/actuator.go b/pkg/controller/controlplane/actuator.go index 2493da36c..e92cacfd0 100644 --- a/pkg/controller/controlplane/actuator.go +++ b/pkg/controller/controlplane/actuator.go @@ -17,38 +17,54 @@ package controlplane import ( "context" "fmt" - "strconv" "time" "github.com/go-logr/logr" - "k8s.io/client-go/util/retry" + "k8s.io/apimachinery/pkg/api/meta" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/runtime/inject" extensionscontroller "github.com/gardener/gardener/extensions/pkg/controller" "github.com/gardener/gardener/extensions/pkg/controller/controlplane" + controllererror "github.com/gardener/gardener/extensions/pkg/controller/error" extensionsv1alpha1 "github.com/gardener/gardener/pkg/apis/extensions/v1alpha1" "github.com/gardener/gardener/pkg/controllerutils" - kutil "github.com/gardener/gardener/pkg/utils/kubernetes" azurev1alpha1 "github.com/gardener/remedy-controller/pkg/apis/azure/v1alpha1" ) +const ( + // GracefulDeletionWaitInterval is the default interval for retry operations. + GracefulDeletionWaitInterval = 1 * time.Minute + // GracefulDeletionTimeout is the timeout that defines how long the actuator should wait for remedy controller resources to be deleted + // gracefully by the remedy controller itself + GracefulDeletionTimeout = 10 * time.Minute +) + +// TimeNow returns the current time. Exposed for testing. +var TimeNow = time.Now + // NewActuator creates a new Actuator that acts upon and updates the status of ControlPlane resources. func NewActuator( a controlplane.Actuator, logger logr.Logger, + gracefulDeletionTimeout time.Duration, + gracefulDeletionWaitInterval time.Duration, ) controlplane.Actuator { return &actuator{ - Actuator: a, - logger: logger.WithName("azure-controlplane-actuator"), + Actuator: a, + logger: logger.WithName("azure-controlplane-actuator"), + gracefulDeletionTimeout: gracefulDeletionTimeout, + gracefulDeletionWaitInterval: gracefulDeletionWaitInterval, } } // actuator is an Actuator that acts upon and updates the status of ControlPlane resources. type actuator struct { controlplane.Actuator - client client.Client - logger logr.Logger + client client.Client + logger logr.Logger + gracefulDeletionTimeout time.Duration + gracefulDeletionWaitInterval time.Duration } // InjectFunc enables injecting Kubernetes dependencies into actuator's dependencies. @@ -71,17 +87,32 @@ func (a *actuator) Delete( cluster *extensionscontroller.Cluster, ) error { if cp.Spec.Purpose == nil || *cp.Spec.Purpose == extensionsv1alpha1.Normal { - if err := a.annotatePublicIPAddresses(ctx, cp); err != nil { + list := &azurev1alpha1.PublicIPAddressList{} + if err := a.client.List(ctx, list, client.InNamespace(cp.Namespace)); err != nil { return err } - // Delete all remaining remedy controller resources - if err := a.deleteRemedyControllerResources(ctx, cp); err != nil { - return err + if meta.LenList(list) != 0 { + if time.Now().Sub(cp.DeletionTimestamp.Time) <= a.gracefulDeletionTimeout { + a.logger.Info("Some publicipaddresses still exist. Deletion will be retried ...") + return &controllererror.RequeueAfterError{ + RequeueAfter: a.gracefulDeletionWaitInterval, + } + } else { + a.logger.Info("Timeout while waiting for publicipaddresses to be gracefully deleted has expired. They will be forcefully removed.") + } } } // Call Delete on the composed Actuator - return a.Actuator.Delete(ctx, cp, cluster) + if err := a.Actuator.Delete(ctx, cp, cluster); err != nil { + return err + } + + if cp.Spec.Purpose == nil || *cp.Spec.Purpose == extensionsv1alpha1.Normal { + // Delete all remaining remedy controller resources + return a.forceDeleteRemedyControllerResources(ctx, cp) + } + return nil } // Migrate reconciles the given controlplane and cluster, migrating the additional @@ -99,35 +130,12 @@ func (a *actuator) Migrate( } if cp.Spec.Purpose == nil || *cp.Spec.Purpose == extensionsv1alpha1.Normal { // Delete all remaining remedy controller resources - if err := a.removeFinalizersFromRemedyControllerResources(ctx, cp); err != nil { - return err - } - return a.deleteRemedyControllerResources(ctx, cp) + return a.forceDeleteRemedyControllerResources(ctx, cp) } return nil } -func (a *actuator) annotatePublicIPAddresses(ctx context.Context, cp *extensionsv1alpha1.ControlPlane) error { - a.logger.Info("Adding do-not-clean annotation on publicipaddresses") - pubipList := &azurev1alpha1.PublicIPAddressList{} - if err := a.client.List(ctx, pubipList, client.InNamespace(cp.Namespace)); err != nil { - return fmt.Errorf("could not list publicipaddresses: %w", err) - } - for _, pubip := range pubipList.Items { - // Add the do-not-clean annotation to the publicipaddress resource - // This annotation prevents attempts to clean the Azure IP address if it still exists, resulting in much faster deletion - if err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { - pubip.Annotations = add(pubip.Annotations, "azure.remedy.gardener.cloud/do-not-clean", strconv.FormatBool(true)) - return a.client.Update(ctx, &pubip) - }); err != nil { - return fmt.Errorf("could not add do-not-clean annotation on publicipaddress: %w", err) - } - } - - return nil -} - -func (a *actuator) removeFinalizersFromRemedyControllerResources(ctx context.Context, cp *extensionsv1alpha1.ControlPlane) error { +func (a *actuator) forceDeleteRemedyControllerResources(ctx context.Context, cp *extensionsv1alpha1.ControlPlane) error { a.logger.Info("Removing finalizers from remedy controller resources") pubipList := &azurev1alpha1.PublicIPAddressList{} if err := a.client.List(ctx, pubipList, client.InNamespace(cp.Namespace)); err != nil { @@ -149,10 +157,6 @@ func (a *actuator) removeFinalizersFromRemedyControllerResources(ctx context.Con } } - return nil -} - -func (a *actuator) deleteRemedyControllerResources(ctx context.Context, cp *extensionsv1alpha1.ControlPlane) error { a.logger.Info("Deleting all remaining remedy controller resources") if err := a.client.DeleteAllOf(ctx, &azurev1alpha1.PublicIPAddress{}, client.InNamespace(cp.Namespace)); err != nil { return fmt.Errorf("could not delete publicipaddress resources: %w", err) @@ -161,19 +165,6 @@ func (a *actuator) deleteRemedyControllerResources(ctx context.Context, cp *exte return fmt.Errorf("could not delete virtualmachine resources: %w", err) } - // Wait until the remaining remedy controller resources have been deleted - a.logger.Info("Waiting for the remaining remedy controller resources to be deleted") - timeoutCtx1, cancel := context.WithTimeout(ctx, 5*time.Minute) - defer cancel() - if err := kutil.WaitUntilResourcesDeleted(timeoutCtx1, a.client, &azurev1alpha1.PublicIPAddressList{}, 5*time.Second, client.InNamespace(cp.Namespace)); err != nil { - return fmt.Errorf("could not wait for publicipaddress resources to be deleted: %w", err) - } - timeoutCtx2, cancel := context.WithTimeout(ctx, 5*time.Minute) - defer cancel() - if err := kutil.WaitUntilResourcesDeleted(timeoutCtx2, a.client, &azurev1alpha1.VirtualMachineList{}, 5*time.Second, client.InNamespace(cp.Namespace)); err != nil { - return fmt.Errorf("could not wait for virtualmachine resources to be deleted: %w", err) - } - return nil } diff --git a/pkg/controller/controlplane/actuator_test.go b/pkg/controller/controlplane/actuator_test.go index 49dde45ed..b895c7d55 100644 --- a/pkg/controller/controlplane/actuator_test.go +++ b/pkg/controller/controlplane/actuator_test.go @@ -16,9 +16,11 @@ package controlplane import ( "context" + "time" extensionscontroller "github.com/gardener/gardener/extensions/pkg/controller" "github.com/gardener/gardener/extensions/pkg/controller/controlplane" + controllererror "github.com/gardener/gardener/extensions/pkg/controller/error" azurev1alpha1 "github.com/gardener/remedy-controller/pkg/apis/azure/v1alpha1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -45,7 +47,9 @@ var _ = Describe("Actuator", func() { a *mockcontrolplane.MockActuator actuator controlplane.Actuator - newControlPlane = func(purpose *extensionsv1alpha1.Purpose) *extensionsv1alpha1.ControlPlane { + gracefulDeletionTimeout time.Duration + gracefulDeletionWaitInterval time.Duration + newControlPlane = func(purpose *extensionsv1alpha1.Purpose) *extensionsv1alpha1.ControlPlane { return &extensionsv1alpha1.ControlPlane{ ObjectMeta: metav1.ObjectMeta{Name: "control-plane", Namespace: namespace}, Spec: extensionsv1alpha1.ControlPlaneSpec{ @@ -87,8 +91,9 @@ var _ = Describe("Actuator", func() { ctrl = gomock.NewController(GinkgoT()) c = mockclient.NewMockClient(ctrl) a = mockcontrolplane.NewMockActuator(ctrl) - - actuator = NewActuator(a, logger) + gracefulDeletionTimeout = 10 * time.Second + gracefulDeletionWaitInterval = 1 * time.Second + actuator = NewActuator(a, logger, gracefulDeletionTimeout, gracefulDeletionWaitInterval) err := actuator.(inject.Client).InjectClient(c) Expect(err).NotTo(HaveOccurred()) @@ -99,48 +104,53 @@ var _ = Describe("Actuator", func() { }) Describe("#Delete", func() { - It("should delete remaining remedy controller resources", func() { - pubip := newPubip(nil) + It("should successfully delete controlplane if there are no remedy controller resources", func() { cp := newControlPlane(nil) - c.EXPECT().List(ctx, &azurev1alpha1.PublicIPAddressList{}, client.InNamespace(namespace)). - DoAndReturn(func(_ context.Context, list *azurev1alpha1.PublicIPAddressList, _ ...client.ListOption) error { - list.Items = []azurev1alpha1.PublicIPAddress{*pubip} - return nil - }) - annotatedPubip := newPubip(map[string]string{"azure.remedy.gardener.cloud/do-not-clean": "true"}) - c.EXPECT().Update(ctx, annotatedPubip).Return(nil) - c.EXPECT().DeleteAllOf(ctx, &azurev1alpha1.PublicIPAddress{}, client.InNamespace(namespace)).Return(nil) - c.EXPECT().DeleteAllOf(ctx, &azurev1alpha1.VirtualMachine{}, client.InNamespace(namespace)).Return(nil) + time := metav1.Now() + cp.DeletionTimestamp = &time c.EXPECT().List(gomock.Any(), &azurev1alpha1.PublicIPAddressList{}, client.InNamespace(namespace)). DoAndReturn(func(_ context.Context, list *azurev1alpha1.PublicIPAddressList, _ ...client.ListOption) error { list.Items = []azurev1alpha1.PublicIPAddress{} return nil - }) - c.EXPECT().List(gomock.Any(), &azurev1alpha1.VirtualMachineList{}, client.InNamespace(namespace)). + }).Times(2) + + c.EXPECT().List(ctx, &azurev1alpha1.VirtualMachineList{}, client.InNamespace(namespace)). DoAndReturn(func(_ context.Context, list *azurev1alpha1.VirtualMachineList, _ ...client.ListOption) error { list.Items = []azurev1alpha1.VirtualMachine{} return nil }) - a.EXPECT().Delete(ctx, cp, cluster).Return(nil) + c.EXPECT().DeleteAllOf(ctx, &azurev1alpha1.PublicIPAddress{}, client.InNamespace(namespace)).Return(nil) + c.EXPECT().DeleteAllOf(ctx, &azurev1alpha1.VirtualMachine{}, client.InNamespace(namespace)).Return(nil) + + a.EXPECT().Delete(ctx, cp, cluster).Return(nil) err := actuator.Delete(ctx, cp, cluster) Expect(err).NotTo(HaveOccurred()) }) - It("should not delete remaining remedy controller resources for controlplane with purpose exposure", func() { - exposure := extensionsv1alpha1.Exposure - cp := newControlPlane(&exposure) - a.EXPECT().Delete(ctx, cp, cluster).Return(nil) + It("should return RequeueAfterError if there are publicipaddresses remaining and timeout is not yet reached", func() { + cp := newControlPlane(nil) + time := metav1.Now() + cp.DeletionTimestamp = &time + + pubip := newPubip(nil) + pubipWithFinalizers := pubip.DeepCopy() + pubipWithFinalizers.Finalizers = append(pubipWithFinalizers.Finalizers, "azure.remedy.gardener.cloud/publicipaddress") + + c.EXPECT().List(ctx, &azurev1alpha1.PublicIPAddressList{}, client.InNamespace(namespace)). + DoAndReturn(func(_ context.Context, list *azurev1alpha1.PublicIPAddressList, _ ...client.ListOption) error { + list.Items = []azurev1alpha1.PublicIPAddress{*pubipWithFinalizers} + return nil + }) err := actuator.Delete(ctx, cp, cluster) - Expect(err).NotTo(HaveOccurred()) + Expect(err).To(MatchError(&controllererror.RequeueAfterError{RequeueAfter: gracefulDeletionWaitInterval})) }) - }) - Describe("#Migrate", func() { - It("should remove finalizers from remedy controller resources and then delete them", func() { + It("should forcefully remove remedy controller resources after grace period timeout has been reached", func() { cp := newControlPlane(nil) - a.EXPECT().Migrate(ctx, cp, cluster).Return(nil) + time := metav1.NewTime(time.Now().Add(time.Duration(-2 * gracefulDeletionTimeout))) + cp.DeletionTimestamp = &time pubip := newPubip(nil) pubipWithFinalizers := pubip.DeepCopy() @@ -149,7 +159,8 @@ var _ = Describe("Actuator", func() { DoAndReturn(func(_ context.Context, list *azurev1alpha1.PublicIPAddressList, _ ...client.ListOption) error { list.Items = []azurev1alpha1.PublicIPAddress{*pubipWithFinalizers} return nil - }) + }).Times(2) + test.EXPECTPatchWithOptimisticLock(ctx, c, pubip, pubipWithFinalizers) c.EXPECT().DeleteAllOf(ctx, &azurev1alpha1.PublicIPAddress{}, client.InNamespace(namespace)).Return(nil) @@ -164,16 +175,39 @@ var _ = Describe("Actuator", func() { test.EXPECTPatchWithOptimisticLock(ctx, c, vm, vmWithFinalizers) c.EXPECT().DeleteAllOf(ctx, &azurev1alpha1.VirtualMachine{}, client.InNamespace(namespace)).Return(nil) - c.EXPECT().List(gomock.Any(), &azurev1alpha1.PublicIPAddressList{}, client.InNamespace(namespace)). + a.EXPECT().Delete(ctx, cp, cluster).Return(nil) + + err := actuator.Delete(ctx, cp, cluster) + Expect(err).NotTo(HaveOccurred()) + }) + }) + + Describe("#Migrate", func() { + It("should remove finalizers from remedy controller resources and then delete them", func() { + cp := newControlPlane(nil) + a.EXPECT().Migrate(ctx, cp, cluster).Return(nil) + + pubip := newPubip(nil) + pubipWithFinalizers := pubip.DeepCopy() + pubipWithFinalizers.Finalizers = append(pubipWithFinalizers.Finalizers, "azure.remedy.gardener.cloud/publicipaddress") + c.EXPECT().List(ctx, &azurev1alpha1.PublicIPAddressList{}, client.InNamespace(namespace)). DoAndReturn(func(_ context.Context, list *azurev1alpha1.PublicIPAddressList, _ ...client.ListOption) error { - list.Items = []azurev1alpha1.PublicIPAddress{} + list.Items = []azurev1alpha1.PublicIPAddress{*pubipWithFinalizers} return nil }) - c.EXPECT().List(gomock.Any(), &azurev1alpha1.VirtualMachineList{}, client.InNamespace(namespace)). + test.EXPECTPatchWithOptimisticLock(ctx, c, pubip, pubipWithFinalizers) + c.EXPECT().DeleteAllOf(ctx, &azurev1alpha1.PublicIPAddress{}, client.InNamespace(namespace)).Return(nil) + + vm := newVirtualMachine() + vmWithFinalizers := vm.DeepCopy() + vmWithFinalizers.Finalizers = append(vmWithFinalizers.Finalizers, "azure.remedy.gardener.cloud/virtualmachine") + c.EXPECT().List(ctx, &azurev1alpha1.VirtualMachineList{}, client.InNamespace(namespace)). DoAndReturn(func(_ context.Context, list *azurev1alpha1.VirtualMachineList, _ ...client.ListOption) error { - list.Items = []azurev1alpha1.VirtualMachine{} + list.Items = []azurev1alpha1.VirtualMachine{*vmWithFinalizers} return nil }) + test.EXPECTPatchWithOptimisticLock(ctx, c, vm, vmWithFinalizers) + c.EXPECT().DeleteAllOf(ctx, &azurev1alpha1.VirtualMachine{}, client.InNamespace(namespace)).Return(nil) err := actuator.Migrate(ctx, cp, cluster) Expect(err).NotTo(HaveOccurred()) diff --git a/pkg/controller/controlplane/add.go b/pkg/controller/controlplane/add.go index 7332d1ff0..ba3899ea4 100644 --- a/pkg/controller/controlplane/add.go +++ b/pkg/controller/controlplane/add.go @@ -48,7 +48,7 @@ func AddToManagerWithOptions(mgr manager.Manager, opts AddOptions) error { return controlplane.Add(mgr, controlplane.AddArgs{ Actuator: NewActuator(genericactuator.NewActuator(azure.Name, controlPlaneSecrets, nil, configChart, controlPlaneChart, controlPlaneShootChart, controlPlaneShootCRDsChart, storageClassChart, nil, NewValuesProvider(logger), extensionscontroller.ChartRendererFactoryFunc(util.NewChartRendererForShoot), - imagevector.ImageVector(), "", nil, mgr.GetWebhookServer().Port, logger), logger), + imagevector.ImageVector(), "", nil, mgr.GetWebhookServer().Port, logger), logger, GracefulDeletionTimeout, GracefulDeletionWaitInterval), ControllerOptions: opts.Controller, Predicates: controlplane.DefaultPredicates(opts.IgnoreOperationAnnotation), Type: azure.Type, From e64be1120cf5a0a6ee45db6d9d412eb0876de0ad Mon Sep 17 00:00:00 2001 From: Plamen Kokanov Date: Wed, 8 Dec 2021 08:16:15 +0200 Subject: [PATCH 3/6] Apply review comments --- pkg/controller/controlplane/actuator.go | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/pkg/controller/controlplane/actuator.go b/pkg/controller/controlplane/actuator.go index e92cacfd0..3f13c0590 100644 --- a/pkg/controller/controlplane/actuator.go +++ b/pkg/controller/controlplane/actuator.go @@ -40,9 +40,6 @@ const ( GracefulDeletionTimeout = 10 * time.Minute ) -// TimeNow returns the current time. Exposed for testing. -var TimeNow = time.Now - // NewActuator creates a new Actuator that acts upon and updates the status of ControlPlane resources. func NewActuator( a controlplane.Actuator, @@ -80,7 +77,7 @@ func (a *actuator) InjectClient(client client.Client) error { // Delete reconciles the given controlplane and cluster, deleting the additional // control plane components as needed. -// Before delegating to the composed Actuator, it ensures that all remedy controller resources have been deleted. +// Before delegating to the composed Actuator, it ensures that all remedy controller resources have been deleted gracefully. func (a *actuator) Delete( ctx context.Context, cp *extensionsv1alpha1.ControlPlane, @@ -92,13 +89,13 @@ func (a *actuator) Delete( return err } if meta.LenList(list) != 0 { - if time.Now().Sub(cp.DeletionTimestamp.Time) <= a.gracefulDeletionTimeout { + if time.Since(cp.DeletionTimestamp.Time) <= a.gracefulDeletionTimeout { a.logger.Info("Some publicipaddresses still exist. Deletion will be retried ...") return &controllererror.RequeueAfterError{ RequeueAfter: a.gracefulDeletionWaitInterval, } } else { - a.logger.Info("Timeout while waiting for publicipaddresses to be gracefully deleted has expired. They will be forcefully removed.") + a.logger.Info("The timeout for waiting for publicipaddresses to be gracefully deleted has expired. They will be forcefully removed.") } } } @@ -167,11 +164,3 @@ func (a *actuator) forceDeleteRemedyControllerResources(ctx context.Context, cp return nil } - -func add(m map[string]string, key, value string) map[string]string { - if m == nil { - m = make(map[string]string) - } - m[key] = value - return m -} From 88efbd3b65995d9a3983ea3687cd93edbef52427 Mon Sep 17 00:00:00 2001 From: Ismail Alidzhikov Date: Wed, 8 Dec 2021 17:29:25 +0200 Subject: [PATCH 4/6] Fix `make install` (#412) Signed-off-by: ialidzhikov --- pkg/controller/controlplane/actuator.go | 4 ++-- pkg/controller/controlplane/actuator_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/controller/controlplane/actuator.go b/pkg/controller/controlplane/actuator.go index 3f13c0590..515a0105b 100644 --- a/pkg/controller/controlplane/actuator.go +++ b/pkg/controller/controlplane/actuator.go @@ -26,9 +26,9 @@ import ( extensionscontroller "github.com/gardener/gardener/extensions/pkg/controller" "github.com/gardener/gardener/extensions/pkg/controller/controlplane" - controllererror "github.com/gardener/gardener/extensions/pkg/controller/error" extensionsv1alpha1 "github.com/gardener/gardener/pkg/apis/extensions/v1alpha1" "github.com/gardener/gardener/pkg/controllerutils" + reconcilerutils "github.com/gardener/gardener/pkg/controllerutils/reconciler" azurev1alpha1 "github.com/gardener/remedy-controller/pkg/apis/azure/v1alpha1" ) @@ -91,7 +91,7 @@ func (a *actuator) Delete( if meta.LenList(list) != 0 { if time.Since(cp.DeletionTimestamp.Time) <= a.gracefulDeletionTimeout { a.logger.Info("Some publicipaddresses still exist. Deletion will be retried ...") - return &controllererror.RequeueAfterError{ + return &reconcilerutils.RequeueAfterError{ RequeueAfter: a.gracefulDeletionWaitInterval, } } else { diff --git a/pkg/controller/controlplane/actuator_test.go b/pkg/controller/controlplane/actuator_test.go index b895c7d55..4295953a2 100644 --- a/pkg/controller/controlplane/actuator_test.go +++ b/pkg/controller/controlplane/actuator_test.go @@ -20,7 +20,7 @@ import ( extensionscontroller "github.com/gardener/gardener/extensions/pkg/controller" "github.com/gardener/gardener/extensions/pkg/controller/controlplane" - controllererror "github.com/gardener/gardener/extensions/pkg/controller/error" + reconcilerutils "github.com/gardener/gardener/pkg/controllerutils/reconciler" azurev1alpha1 "github.com/gardener/remedy-controller/pkg/apis/azure/v1alpha1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -144,7 +144,7 @@ var _ = Describe("Actuator", func() { }) err := actuator.Delete(ctx, cp, cluster) - Expect(err).To(MatchError(&controllererror.RequeueAfterError{RequeueAfter: gracefulDeletionWaitInterval})) + Expect(err).To(MatchError(&reconcilerutils.RequeueAfterError{RequeueAfter: gracefulDeletionWaitInterval})) }) It("should forcefully remove remedy controller resources after grace period timeout has been reached", func() { From b343a5067083b4d574fa75ce1ac6dfe864dade9e Mon Sep 17 00:00:00 2001 From: Konstantinos Angelopoulos Date: Thu, 9 Dec 2021 15:58:56 +0100 Subject: [PATCH 5/6] user lenient decoder for infra status --- pkg/apis/azure/helper/scheme.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/apis/azure/helper/scheme.go b/pkg/apis/azure/helper/scheme.go index ff6b1c3b6..70d5b60d9 100644 --- a/pkg/apis/azure/helper/scheme.go +++ b/pkg/apis/azure/helper/scheme.go @@ -36,6 +36,9 @@ var ( Scheme *runtime.Scheme decoder runtime.Decoder + + // lenientDecoder is a decoder that does not use strict mode. + lenientDecoder runtime.Decoder ) func init() { @@ -43,6 +46,7 @@ func init() { utilruntime.Must(install.AddToScheme(Scheme)) decoder = serializer.NewCodecFactory(Scheme, serializer.EnableStrict).UniversalDecoder() + lenientDecoder = serializer.NewCodecFactory(Scheme).UniversalDecoder() } // InfrastructureConfigFromInfrastructure extracts the InfrastructureConfig from the @@ -63,7 +67,7 @@ func InfrastructureConfigFromInfrastructure(infra *extensionsv1alpha1.Infrastruc func InfrastructureStatusFromInfrastructure(infra *extensionsv1alpha1.Infrastructure) (*api.InfrastructureStatus, error) { config := &api.InfrastructureStatus{} if infra.Status.ProviderStatus != nil && infra.Status.ProviderStatus.Raw != nil { - if _, _, err := decoder.Decode(infra.Status.ProviderStatus.Raw, nil, config); err != nil { + if _, _, err := lenientDecoder.Decode(infra.Status.ProviderStatus.Raw, nil, config); err != nil { return nil, err } return config, nil From e07ac4a7569ade3af9be8e972ce07a8c14309775 Mon Sep 17 00:00:00 2001 From: Konstantinos Angelopoulos Date: Fri, 10 Dec 2021 17:16:45 +0100 Subject: [PATCH 6/6] fix CP decoder usage (#416) --- pkg/apis/azure/helper/scheme.go | 8 ++--- pkg/controller/controlplane/valuesprovider.go | 35 +++++++++++++------ pkg/controller/worker/actuator.go | 6 +++- pkg/controller/worker/add.go | 6 ++-- pkg/controller/worker/helper.go | 8 ++--- pkg/internal/infrastructure/terraform.go | 28 +++++++-------- 6 files changed, 53 insertions(+), 38 deletions(-) diff --git a/pkg/apis/azure/helper/scheme.go b/pkg/apis/azure/helper/scheme.go index 70d5b60d9..defa8428f 100644 --- a/pkg/apis/azure/helper/scheme.go +++ b/pkg/apis/azure/helper/scheme.go @@ -62,12 +62,12 @@ func InfrastructureConfigFromInfrastructure(infra *extensionsv1alpha1.Infrastruc return nil, fmt.Errorf("provider config is not set on the infrastructure resource") } -// InfrastructureStatusFromInfrastructure extracts the InfrastructureStatus from the +// InfrastructureStatusFromRaw extracts the InfrastructureStatus from the // ProviderStatus section of the given Infrastructure. -func InfrastructureStatusFromInfrastructure(infra *extensionsv1alpha1.Infrastructure) (*api.InfrastructureStatus, error) { +func InfrastructureStatusFromRaw(raw *runtime.RawExtension) (*api.InfrastructureStatus, error) { config := &api.InfrastructureStatus{} - if infra.Status.ProviderStatus != nil && infra.Status.ProviderStatus.Raw != nil { - if _, _, err := lenientDecoder.Decode(infra.Status.ProviderStatus.Raw, nil, config); err != nil { + if raw != nil && raw.Raw != nil { + if _, _, err := lenientDecoder.Decode(raw.Raw, nil, config); err != nil { return nil, err } return config, nil diff --git a/pkg/controller/controlplane/valuesprovider.go b/pkg/controller/controlplane/valuesprovider.go index 42e9817fa..6e10e8b37 100644 --- a/pkg/controller/controlplane/valuesprovider.go +++ b/pkg/controller/controlplane/valuesprovider.go @@ -393,9 +393,14 @@ func (vp *valuesProvider) GetConfigChartValues(ctx context.Context, cp *extensio } // Decode infrastructureProviderStatus - infraStatus := &apisazure.InfrastructureStatus{} - if _, _, err := vp.Decoder().Decode(cp.Spec.InfrastructureProviderStatus.Raw, nil, infraStatus); err != nil { - return nil, fmt.Errorf("could not decode infrastructureProviderStatus of controlplane '%s': %w", kutil.ObjectName(cp), err) + var ( + infraStatus = &apisazure.InfrastructureStatus{} + err error + ) + if cp.Spec.InfrastructureProviderStatus != nil { + if infraStatus, err = azureapihelper.InfrastructureStatusFromRaw(cp.Spec.InfrastructureProviderStatus); err != nil { + return nil, fmt.Errorf("could not decode infrastructureProviderStatus of controlplane '%s': %w", kutil.ObjectName(cp), err) + } } // Get client auth @@ -437,11 +442,16 @@ func (vp *valuesProvider) GetControlPlaneChartValues( } checksums[azure.CloudProviderConfigName] = utils.ComputeChecksum(cpConfigSecret.Data) - infraStatus := &apisazure.InfrastructureStatus{} - if _, _, err := vp.Decoder().Decode(cp.Spec.InfrastructureProviderStatus.Raw, nil, infraStatus); err != nil { - return nil, fmt.Errorf("could not decode infrastructureProviderStatus of controlplane '%s': %w", kutil.ObjectName(cp), err) + // Decode infrastructureProviderStatus + var ( + infraStatus = &apisazure.InfrastructureStatus{} + err error + ) + if cp.Spec.InfrastructureProviderStatus != nil { + if infraStatus, err = azureapihelper.InfrastructureStatusFromRaw(cp.Spec.InfrastructureProviderStatus); err != nil { + return nil, fmt.Errorf("could not decode infrastructureProviderStatus of controlplane '%s': %w", kutil.ObjectName(cp), err) + } } - return getControlPlaneChartValues(cpConfig, cp, cluster, checksums, scaledDown, infraStatus) } @@ -453,9 +463,14 @@ func (vp *valuesProvider) GetControlPlaneShootChartValues( checksums map[string]string, ) (map[string]interface{}, error) { // Decode infrastructureProviderStatus - infraStatus := &apisazure.InfrastructureStatus{} - if _, _, err := vp.Decoder().Decode(cp.Spec.InfrastructureProviderStatus.Raw, nil, infraStatus); err != nil { - return nil, fmt.Errorf("could not decode infrastructureProviderStatus of controlplane '%s': %w", kutil.ObjectName(cp), err) + var ( + infraStatus = &apisazure.InfrastructureStatus{} + err error + ) + if cp.Spec.InfrastructureProviderStatus != nil { + if infraStatus, err = azureapihelper.InfrastructureStatusFromRaw(cp.Spec.InfrastructureProviderStatus); err != nil { + return nil, fmt.Errorf("could not decode infrastructureProviderStatus of controlplane '%s': %w", kutil.ObjectName(cp), err) + } } k8sVersionLessThan121, err := version.CompareVersions(cluster.Shoot.Spec.Kubernetes.Version, "<", "1.21") diff --git a/pkg/controller/worker/actuator.go b/pkg/controller/worker/actuator.go index b0cdd505b..f870eb4a9 100644 --- a/pkg/controller/worker/actuator.go +++ b/pkg/controller/worker/actuator.go @@ -31,6 +31,8 @@ import ( extensionsv1alpha1 "github.com/gardener/gardener/pkg/apis/extensions/v1alpha1" gardener "github.com/gardener/gardener/pkg/client/kubernetes" "github.com/go-logr/logr" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/serializer" "k8s.io/client-go/kubernetes" "sigs.k8s.io/controller-runtime/pkg/log" ) @@ -79,6 +81,7 @@ func (d *delegateFactory) WorkerDelegate(ctx context.Context, worker *extensions type workerDelegate struct { common.ClientContext + lenientDecoder runtime.Decoder seedChartApplier gardener.ChartApplier serverVersion string @@ -102,7 +105,8 @@ func NewWorkerDelegate(clientContext common.ClientContext, seedChartApplier gard } return &workerDelegate{ - ClientContext: clientContext, + ClientContext: clientContext, + lenientDecoder: serializer.NewCodecFactory(clientContext.Scheme()).UniversalDecoder(), seedChartApplier: seedChartApplier, serverVersion: serverVersion, diff --git a/pkg/controller/worker/add.go b/pkg/controller/worker/add.go index 2fc1b2cea..a4890ab1a 100644 --- a/pkg/controller/worker/add.go +++ b/pkg/controller/worker/add.go @@ -24,10 +24,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/manager" ) -var ( - // DefaultAddOptions are the default AddOptions for AddToManager. - DefaultAddOptions = AddOptions{} -) +// DefaultAddOptions are the default AddOptions for AddToManager. +var DefaultAddOptions = AddOptions{} // AddOptions are options to apply when adding the Azure worker controller to the manager. type AddOptions struct { diff --git a/pkg/controller/worker/helper.go b/pkg/controller/worker/helper.go index 33da33cf4..32b43aba3 100644 --- a/pkg/controller/worker/helper.go +++ b/pkg/controller/worker/helper.go @@ -29,15 +29,15 @@ import ( ) func (w *workerDelegate) decodeAzureInfrastructureStatus() (*azureapi.InfrastructureStatus, error) { - var infrastructureStatus = &azureapi.InfrastructureStatus{} - if _, _, err := w.Decoder().Decode(w.worker.Spec.InfrastructureProviderStatus.Raw, nil, infrastructureStatus); err != nil { + infrastructureStatus := &azureapi.InfrastructureStatus{} + if _, _, err := w.lenientDecoder.Decode(w.worker.Spec.InfrastructureProviderStatus.Raw, nil, infrastructureStatus); err != nil { return nil, err } return infrastructureStatus, nil } func (w *workerDelegate) decodeWorkerProviderStatus() (*azureapi.WorkerStatus, error) { - var workerStatus = &azureapi.WorkerStatus{} + workerStatus := &azureapi.WorkerStatus{} if w.worker.Status.ProviderStatus == nil { return workerStatus, nil } @@ -49,7 +49,7 @@ func (w *workerDelegate) decodeWorkerProviderStatus() (*azureapi.WorkerStatus, e } func (w *workerDelegate) updateWorkerProviderStatus(ctx context.Context, workerStatus *azureapi.WorkerStatus) error { - var workerStatusV1alpha1 = &v1alpha1.WorkerStatus{ + workerStatusV1alpha1 := &v1alpha1.WorkerStatus{ TypeMeta: metav1.TypeMeta{ APIVersion: v1alpha1.SchemeGroupVersion.String(), Kind: "WorkerStatus", diff --git a/pkg/internal/infrastructure/terraform.go b/pkg/internal/infrastructure/terraform.go index 564313eb8..975da86f1 100644 --- a/pkg/internal/infrastructure/terraform.go +++ b/pkg/internal/infrastructure/terraform.go @@ -204,7 +204,7 @@ func ComputeTerraformerTemplateValues( } func generateNatGatewayValues(config *api.InfrastructureConfig) (map[string]interface{}, bool) { - var natGatewayConfig = make(map[string]interface{}) + natGatewayConfig := make(map[string]interface{}) if config.Networks.NatGateway == nil || !config.Networks.NatGateway.Enabled { return natGatewayConfig, false } @@ -218,7 +218,7 @@ func generateNatGatewayValues(config *api.InfrastructureConfig) (map[string]inte } if len(config.Networks.NatGateway.IPAddresses) > 0 { - var ipAddresses = make([]map[string]interface{}, len(config.Networks.NatGateway.IPAddresses)) + ipAddresses := make([]map[string]interface{}, len(config.Networks.NatGateway.IPAddresses)) for i, ip := range config.Networks.NatGateway.IPAddresses { ipAddresses[i] = map[string]interface{}{ "name": ip.Name, @@ -270,15 +270,13 @@ type TerraformState struct { // ExtractTerraformState extracts the TerraformState from the given Terraformer. func ExtractTerraformState(ctx context.Context, tf terraformer.Terraformer, infra *extensionsv1alpha1.Infrastructure, config *api.InfrastructureConfig, cluster *controller.Cluster) (*TerraformState, error) { - var ( - outputKeys = []string{ - TerraformerOutputKeyResourceGroupName, - TerraformerOutputKeyRouteTableName, - TerraformerOutputKeySecurityGroupName, - TerraformerOutputKeySubnetName, - TerraformerOutputKeyVNetName, - } - ) + outputKeys := []string{ + TerraformerOutputKeyResourceGroupName, + TerraformerOutputKeyRouteTableName, + TerraformerOutputKeySecurityGroupName, + TerraformerOutputKeySubnetName, + TerraformerOutputKeyVNetName, + } primaryAvSetRequired, err := isPrimaryAvailabilitySetRequired(infra, config, cluster) if err != nil { @@ -302,7 +300,7 @@ func ExtractTerraformState(ctx context.Context, tf terraformer.Terraformer, infr return nil, err } - var tfState = TerraformState{ + tfState := TerraformState{ VNetName: vars[TerraformerOutputKeyVNetName], ResourceGroupName: vars[TerraformerOutputKeyResourceGroupName], RouteTableName: vars[TerraformerOutputKeyRouteTableName], @@ -345,7 +343,7 @@ func ExtractTerraformState(ctx context.Context, tf terraformer.Terraformer, infr // StatusFromTerraformState computes an InfrastructureStatus from the given // Terraform variables. func StatusFromTerraformState(tfState *TerraformState) *apiv1alpha1.InfrastructureStatus { - var infraState = apiv1alpha1.InfrastructureStatus{ + infraState := apiv1alpha1.InfrastructureStatus{ TypeMeta: StatusTypeMeta, ResourceGroup: apiv1alpha1.ResourceGroup{ Name: tfState.ResourceGroupName, @@ -428,7 +426,7 @@ func findDomainCounts(cluster *controller.Cluster, infra *extensionsv1alpha1.Inf ) if infra.Status.ProviderStatus != nil { - infrastructureStatus, err := helper.InfrastructureStatusFromInfrastructure(infra) + infrastructureStatus, err := helper.InfrastructureStatusFromRaw(infra.Status.ProviderStatus) if err != nil { return nil, fmt.Errorf("error obtaining update and fault domain counts from infrastructure status: %v", err) } @@ -492,7 +490,7 @@ func isPrimaryAvailabilitySetRequired(infra *extensionsv1alpha1.Infrastructure, } // If the infrastructureStatus already exists that mean the Infrastucture is already created. - infrastructureStatus, err := helper.InfrastructureStatusFromInfrastructure(infra) + infrastructureStatus, err := helper.InfrastructureStatusFromRaw(infra.Status.ProviderStatus) if err != nil { return false, err }