From 0b9555c5d0b8a9fbff437284df8dcdbc42d72562 Mon Sep 17 00:00:00 2001 From: Milos Tomic Date: Tue, 3 Sep 2024 09:17:52 +0200 Subject: [PATCH 1/4] chore: feature flags logger --- cmd/main.go | 2 +- ...GcpNfsVolumeAutomaticLocationAllocation.go | 2 +- .../ffIpRangeAutomaticCidrAllocation.go | 2 +- pkg/feature/init.go | 27 +++++++------------ 4 files changed, 12 insertions(+), 21 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 1f9ef4c05..6dfde3160 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -308,7 +308,7 @@ func main() { setupLog.Info("starting manager") ctx := ctrl.SetupSignalHandler() - if err := feature.Initialize(ctx, feature.WithLogger(setupLog.WithName("ff"))); err != nil { + if err := feature.Initialize(ctx, rootLogger.WithName("ff")); err != nil { setupLog.Error(err, "problem initializing feature flags") } diff --git a/pkg/feature/ffGcpNfsVolumeAutomaticLocationAllocation.go b/pkg/feature/ffGcpNfsVolumeAutomaticLocationAllocation.go index d5f562a35..d85ec4531 100644 --- a/pkg/feature/ffGcpNfsVolumeAutomaticLocationAllocation.go +++ b/pkg/feature/ffGcpNfsVolumeAutomaticLocationAllocation.go @@ -9,5 +9,5 @@ var GcpNfsVolumeAutomaticLocationAllocation = &gcpNfsVolumeAutomaticLocationAllo type gcpNfsVolumeAutomaticLocationAllocationInfo struct{} func (k *gcpNfsVolumeAutomaticLocationAllocationInfo) Value(ctx context.Context) bool { - return provider.BoolVariation(ctx, gcpNfsVolumeAutomaticLocationAllocationFlagName, false) + return provider.BoolVariation(ctx, gcpNfsVolumeAutomaticLocationAllocationFlagName, true) } diff --git a/pkg/feature/ffIpRangeAutomaticCidrAllocation.go b/pkg/feature/ffIpRangeAutomaticCidrAllocation.go index 8d7c9c319..22fdadc6f 100644 --- a/pkg/feature/ffIpRangeAutomaticCidrAllocation.go +++ b/pkg/feature/ffIpRangeAutomaticCidrAllocation.go @@ -9,5 +9,5 @@ var IpRangeAutomaticCidrAllocation = &ipRangeAutomaticCidrAllocationInfo{} type ipRangeAutomaticCidrAllocationInfo struct{} func (k *ipRangeAutomaticCidrAllocationInfo) Value(ctx context.Context) bool { - return provider.BoolVariation(ctx, ipRangeAutomaticCidrAllocationFlagName, false) + return provider.BoolVariation(ctx, ipRangeAutomaticCidrAllocationFlagName, true) } diff --git a/pkg/feature/init.go b/pkg/feature/init.go index 4c3df90d8..56e419355 100644 --- a/pkg/feature/init.go +++ b/pkg/feature/init.go @@ -10,6 +10,7 @@ import ( "github.com/thomaspoignant/go-feature-flag/retriever" "github.com/thomaspoignant/go-feature-flag/retriever/fileretriever" "github.com/thomaspoignant/go-feature-flag/retriever/httpretriever" + "log/slog" "os" "time" ) @@ -21,10 +22,8 @@ func init() { } type ProviderOptions struct { - logger logr.Logger - loggerSet bool - filename string - url string + filename string + url string } type ProviderOption func(o *ProviderOptions) @@ -41,21 +40,12 @@ func WithUrl(url string) ProviderOption { } } -func WithLogger(logger logr.Logger) ProviderOption { - return func(o *ProviderOptions) { - o.logger = logger - o.loggerSet = true - } -} - -func Initialize(ctx context.Context, opts ...ProviderOption) (err error) { +func Initialize(ctx context.Context, logger logr.Logger, opts ...ProviderOption) (err error) { o := &ProviderOptions{} for _, x := range opts { x(o) } - if !o.loggerSet { - o.logger = logr.Discard() - } + if len(o.filename) == 0 && len(o.url) == 0 { o.url = os.Getenv("FEATURE_FLAG_CONFIG_URL") if len(o.url) == 0 { @@ -67,19 +57,20 @@ func Initialize(ctx context.Context, opts ...ProviderOption) (err error) { } var rtvr retriever.Retriever if o.url != "" { - o.logger.WithValues("url", o.url).Info("Using http retriever") + logger.WithValues("ffRetrieverUrl", o.url).Info("Using http retriever") rtvr = &httpretriever.Retriever{URL: o.url} } else { - o.logger.WithValues("file", o.filename).Info("Using file retriever") + logger.WithValues("ffRetrieverFile", o.filename).Info("Using file retriever") rtvr = &fileretriever.Retriever{Path: o.filename} } ff, errLoading := ffclient.New(ffclient.Config{ - PollingInterval: 10 * time.Second, + PollingInterval: 30 * time.Second, Context: ctx, FileFormat: "yaml", EnablePollingJitter: true, StartWithRetrieverError: true, Retriever: rtvr, + LeveledLogger: slog.New(logr.ToSlogHandler(logger)), }) if errLoading != nil { err = fmt.Errorf("loading error: %w", errLoading) From c472d699ba24db63be461087907c4abbbdf4e137 Mon Sep 17 00:00:00 2001 From: Milos Tomic Date: Tue, 3 Sep 2024 09:26:15 +0200 Subject: [PATCH 2/4] chore: feature flags logger --- pkg/feature/ffApiDisabled_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/feature/ffApiDisabled_test.go b/pkg/feature/ffApiDisabled_test.go index b21a58d88..9f187b22c 100644 --- a/pkg/feature/ffApiDisabled_test.go +++ b/pkg/feature/ffApiDisabled_test.go @@ -2,6 +2,7 @@ package feature import ( "context" + "github.com/go-logr/logr" "testing" cloudresourcesv1beta1 "github.com/kyma-project/cloud-manager/api/cloud-resources/v1beta1" @@ -17,7 +18,7 @@ func TestApiDisabled(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - err := Initialize(ctx, WithFile("testdata/apiDisabled.yaml")) + err := Initialize(ctx, logr.Discard(), WithFile("testdata/apiDisabled.yaml")) assert.NoError(t, err) sch := runtime.NewScheme() From 8a0763cc1f0333392be1d9c5e35e12d711e2c1b6 Mon Sep 17 00:00:00 2001 From: Milos Tomic Date: Tue, 3 Sep 2024 11:32:25 +0200 Subject: [PATCH 3/4] chore: cleanup GCP NfsVolume for unhappy path resources --- .../cloud-control/nfsinstance_gcp_test.go | 21 ++++++++++++++++++ pkg/composed/state.go | 22 +++++++++++++------ pkg/testinfra/dsl/gcpNfsVolume.go | 2 -- 3 files changed, 36 insertions(+), 9 deletions(-) diff --git a/internal/controller/cloud-control/nfsinstance_gcp_test.go b/internal/controller/cloud-control/nfsinstance_gcp_test.go index 8ca12de2b..f08704c57 100644 --- a/internal/controller/cloud-control/nfsinstance_gcp_test.go +++ b/internal/controller/cloud-control/nfsinstance_gcp_test.go @@ -1,6 +1,8 @@ package cloudcontrol import ( + "context" + "github.com/kyma-project/cloud-manager/pkg/composed" "time" cloudcontrolv1beta1 "github.com/kyma-project/cloud-manager/api/cloud-control/v1beta1" @@ -436,6 +438,25 @@ var _ = Describe("Feature: KCP NFSVolume for GCP", func() { infra.GcpMock().SetOperationError(nil) }) + It("// cleanup: delete GCP NfsVolume for unhappy path", func() { + infra.GcpMock().SetCreateError(nil) + infra.GcpMock().SetPatchError(nil) + infra.GcpMock().SetDeleteError(nil) + infra.GcpMock().SetGetError(nil) + infra.GcpMock().SetOperationError(nil) + Eventually(Delete). + WithArguments(infra.Ctx(), infra.KCP().Client(), gcpNfsInstance). + Should(Succeed()) + Eventually(func(ctx context.Context) error { + _, err := composed.PatchObjRemoveFinalizer(ctx, cloudcontrolv1beta1.FinalizerName, gcpNfsInstance, infra.KCP().Client()) + return err + }). + WithContext(infra.Ctx()). + Should(Succeed()) + Eventually(IsDeleted). + WithArguments(infra.Ctx(), infra.KCP().Client(), gcpNfsInstance). + Should(Succeed()) + }) }) Context("Scenario: GCP NFSVolume With Restore Happy Path", Ordered, func() { diff --git a/pkg/composed/state.go b/pkg/composed/state.go index 0a330dd15..d5e2bc160 100644 --- a/pkg/composed/state.go +++ b/pkg/composed/state.go @@ -179,18 +179,26 @@ func (s *baseState) PatchObjStatus(ctx context.Context) error { // characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName', // or 'my.name', or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')"} func (s *baseState) PatchObjAddFinalizer(ctx context.Context, f string) (bool, error) { - added := controllerutil.AddFinalizer(s.Obj(), f) + return PatchObjAddFinalizer(ctx, f, s.Obj(), s.Cluster().K8sClient()) +} + +// PatchObjRemoveFinalizer patches obj with JSONPatchType removing the specified finalizer name +func (s *baseState) PatchObjRemoveFinalizer(ctx context.Context, f string) (bool, error) { + return PatchObjRemoveFinalizer(ctx, f, s.Obj(), s.Cluster().K8sClient()) +} + +func PatchObjAddFinalizer(ctx context.Context, f string, obj client.Object, clnt client.Client) (bool, error) { + added := controllerutil.AddFinalizer(obj, f) if !added { return false, nil } p := []byte(fmt.Sprintf(`{"metadata": {"finalizers":["%s"]}}`, f)) - return true, s.Cluster().K8sClient().Patch(ctx, s.Obj(), client.RawPatch(types.MergePatchType, p)) + return true, clnt.Patch(ctx, obj, client.RawPatch(types.MergePatchType, p)) } -// PatchObjRemoveFinalizer patches obj with JSONPatchType removing the specified finalizer name -func (s *baseState) PatchObjRemoveFinalizer(ctx context.Context, f string) (bool, error) { +func PatchObjRemoveFinalizer(ctx context.Context, f string, obj client.Object, clnt client.Client) (bool, error) { idx := -1 - for i, s := range s.Obj().GetFinalizers() { + for i, s := range obj.GetFinalizers() { if s == f { idx = i break @@ -199,7 +207,7 @@ func (s *baseState) PatchObjRemoveFinalizer(ctx context.Context, f string) (bool if idx == -1 { return false, nil } - controllerutil.RemoveFinalizer(s.Obj(), f) + controllerutil.RemoveFinalizer(obj, f) p := []byte(fmt.Sprintf(`[{"op": "remove", "path": "/metadata/finalizers/%d"}]`, idx)) - return true, s.Cluster().K8sClient().Patch(ctx, s.Obj(), client.RawPatch(types.JSONPatchType, p)) + return true, clnt.Patch(ctx, obj, client.RawPatch(types.JSONPatchType, p)) } diff --git a/pkg/testinfra/dsl/gcpNfsVolume.go b/pkg/testinfra/dsl/gcpNfsVolume.go index a58b57129..9f24098d5 100644 --- a/pkg/testinfra/dsl/gcpNfsVolume.go +++ b/pkg/testinfra/dsl/gcpNfsVolume.go @@ -47,8 +47,6 @@ func CreateGcpNfsVolume(ctx context.Context, clnt client.Client, obj *cloudresou // some error return err } - //write entire object on the console - fmt.Println(obj) err = clnt.Create(ctx, obj) return err } From 7fde0eb626f413ec7a30d88726b7ca98bff7ad1e Mon Sep 17 00:00:00 2001 From: Milos Tomic Date: Tue, 3 Sep 2024 11:41:11 +0200 Subject: [PATCH 4/4] chore: make ignorant concurrent safe --- pkg/common/ignorant/store.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pkg/common/ignorant/store.go b/pkg/common/ignorant/store.go index 5245aed83..5539a4f17 100644 --- a/pkg/common/ignorant/store.go +++ b/pkg/common/ignorant/store.go @@ -2,6 +2,7 @@ package ignorant import ( ctrl "sigs.k8s.io/controller-runtime" + "sync" ) type Ignorant interface { @@ -19,18 +20,25 @@ func New() Ignorant { var _ Ignorant = &ignorant{} type ignorant struct { + m sync.Mutex nameMap map[string]struct{} } func (i *ignorant) AddName(name string) { + i.m.Lock() + defer i.m.Unlock() i.nameMap[name] = struct{}{} } func (i *ignorant) RemoveName(name string) { + i.m.Lock() + defer i.m.Unlock() delete(i.nameMap, name) } func (i *ignorant) ShouldIgnoreKey(req ctrl.Request) bool { + i.m.Lock() + defer i.m.Unlock() _, ok := i.nameMap[req.Name] return ok }