From 9e0f2ab06a38364032f2812c7dc5bfd033dca3bc Mon Sep 17 00:00:00 2001 From: Arkadiusz Galwas Date: Wed, 11 Sep 2024 09:29:06 +0200 Subject: [PATCH 1/8] Refactor in audit log state --- .../fsm/runtime_fsm_configure_auditlog.go | 84 +++++++++++-------- 1 file changed, 47 insertions(+), 37 deletions(-) diff --git a/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go b/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go index d03cb5d6..12d8979d 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go +++ b/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go @@ -12,9 +12,9 @@ import ( func sFnConfigureAuditLog(ctx context.Context, m *fsm, s *systemState) (stateFn, *ctrl.Result, error) { m.log.Info("Configure Audit Log state") - wasAuditLogEnabled, err := m.AuditLogging.Enable(ctx, s.shoot) + shootNeedsToBeReconciled, err := m.AuditLogging.Enable(ctx, s.shoot) - if wasAuditLogEnabled { + if err == nil && shootNeedsToBeReconciled { m.log.Info("Audit Log configured for shoot: " + s.shoot.Name) s.instance.UpdateStatePending( imv1.ConditionTypeAuditLogConfigured, @@ -26,46 +26,56 @@ func sFnConfigureAuditLog(ctx context.Context, m *fsm, s *systemState) (stateFn, return updateStatusAndRequeueAfter(gardenerRequeueDuration) } - if err != nil { //nolint:nestif - errorMessage := err.Error() - if errors.Is(err, auditlogging.ErrMissingMapping) { - if m.RCCfg.AuditLogMandatory { - m.log.Error(err, "Failed to configure Audit Log, missing region mapping for this shoot", "AuditLogMandatory", m.RCCfg.AuditLogMandatory, "providerType", s.shoot.Spec.Provider.Type, "region", s.shoot.Spec.Region) - s.instance.UpdateStatePending( - imv1.ConditionTypeAuditLogConfigured, - imv1.ConditionReasonAuditLogMissingRegionMapping, - "False", - errorMessage, - ) - } else { - m.log.Info(errorMessage, "Audit Log was not configured, missing region mapping for this shoot.", "AuditLogMandatory", m.RCCfg.AuditLogMandatory, "providerType", s.shoot.Spec.Provider.Type, "region", s.shoot.Spec.Region) - s.instance.UpdateStateReady( - imv1.ConditionTypeAuditLogConfigured, - imv1.ConditionReasonAuditLogMissingRegionMapping, - "Missing region mapping for this shoot. Audit Log is not mandatory. Skipping configuration") - } - } else { - if m.RCCfg.AuditLogMandatory { - m.log.Error(err, "Failed to configure Audit Log", "AuditLogMandatory", m.RCCfg.AuditLogMandatory) - s.instance.UpdateStatePending( - imv1.ConditionTypeAuditLogConfigured, - imv1.ConditionReasonAuditLogError, - "False", - errorMessage) - } else { - m.log.Info(errorMessage, "AuditLogMandatory", m.RCCfg.AuditLogMandatory) - s.instance.UpdateStateReady( - imv1.ConditionTypeAuditLogConfigured, - imv1.ConditionReasonAuditLogError, - "Configuration of Audit Log is not mandatory, error for context: "+errorMessage) - } - } - } else { + if err == nil { s.instance.UpdateStateReady( imv1.ConditionTypeAuditLogConfigured, imv1.ConditionReasonAuditLogConfigured, "Audit Log state completed successfully", ) + + return updateStatusAndStop() + } + + setStateForAuditLogError := func(reason imv1.RuntimeConditionReason, pendingMsg string, readyMsg string) { + if m.RCCfg.AuditLogMandatory { + s.instance.UpdateStatePending( + imv1.ConditionTypeAuditLogConfigured, + reason, + "False", + pendingMsg, + ) + } else { + s.instance.UpdateStateReady( + imv1.ConditionTypeAuditLogConfigured, + reason, + readyMsg) + } + } + + logError := func(err error, errorMsg, infoMsg string) { + if m.RCCfg.AuditLogMandatory { + m.log.Error(err, errorMsg) + } else { + m.log.Info(err.Error(), "Failed to configure Audit Log, but is not mandatory to be configured") + } + } + + if errors.Is(err, auditlogging.ErrMissingMapping) { + pendingStatusMsg := err.Error() + readyStatusMsg := "Missing region mapping for this shoot. Audit Log is not mandatory. Skipping configuration" + setStateForAuditLogError(imv1.ConditionReasonAuditLogMissingRegionMapping, pendingStatusMsg, readyStatusMsg) + + errorMsg := "Failed to configure Audit Log, missing region mapping for this shoot" + infoMsg := "Failed to configure Audit Log, missing region mapping for this shoot, but is not mandatory to be configured" + logError(err, errorMsg, infoMsg) + } else { + pendingStatusMsg := err.Error() + readyStatusMsg := "Configuration of Audit Log is not mandatory, error for context: " + err.Error() + setStateForAuditLogError(imv1.ConditionReasonAuditLogError, pendingStatusMsg, readyStatusMsg) + + errorMsg := "Failed to configure Audit Log" + infoMsg := "Failed to configure Audit Log, but is not mandatory to be configured" + logError(err, errorMsg, infoMsg) } return updateStatusAndStop() From 8825b0718aa58766666366fda6a54081f908340f Mon Sep 17 00:00:00 2001 From: Arkadiusz Galwas Date: Wed, 11 Sep 2024 10:02:28 +0200 Subject: [PATCH 2/8] Refactor in audit log state --- .../controller/runtime/fsm/runtime_fsm_configure_auditlog.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go b/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go index 12d8979d..4637e738 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go +++ b/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go @@ -56,7 +56,7 @@ func sFnConfigureAuditLog(ctx context.Context, m *fsm, s *systemState) (stateFn, if m.RCCfg.AuditLogMandatory { m.log.Error(err, errorMsg) } else { - m.log.Info(err.Error(), "Failed to configure Audit Log, but is not mandatory to be configured") + m.log.Info(err.Error(), infoMsg) } } From 44877bfa56359b21c311e1ebb2354a94aa2c3536 Mon Sep 17 00:00:00 2001 From: Arkadiusz Galwas Date: Wed, 18 Sep 2024 16:44:01 +0200 Subject: [PATCH 3/8] Fixed problem with requeue logic --- .../fsm/runtime_fsm_configure_auditlog.go | 68 +++---------------- 1 file changed, 9 insertions(+), 59 deletions(-) diff --git a/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go b/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go index 4bfbbb0a..13ab05e6 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go +++ b/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go @@ -14,9 +14,9 @@ import ( func sFnConfigureAuditLog(ctx context.Context, m *fsm, s *systemState) (stateFn, *ctrl.Result, error) { m.log.Info("Configure Audit Log state") - wasAuditLogEnabled, err := m.AuditLogging.Enable(ctx, s.shoot) + shootNeedsToBeReconciled, err := m.AuditLogging.Enable(ctx, s.shoot) - if wasAuditLogEnabled && err == nil { + if shootNeedsToBeReconciled && err == nil { m.log.Info("Audit Log configured for shoot: " + s.shoot.Name) s.instance.UpdateStatePending( imv1.ConditionTypeAuditLogConfigured, @@ -39,52 +39,6 @@ func sFnConfigureAuditLog(ctx context.Context, m *fsm, s *systemState) (stateFn, } return handleError(err, m, s) - //if err != nil { //nolint:nestif - // if k8serrors.IsConflict(err) { - // m.log.Error(err, "Conflict while updating Shoot object after applying Audit Log configuration, retrying") - // s.instance.UpdateStatePending( - // imv1.ConditionTypeAuditLogConfigured, - // imv1.ConditionReasonAuditLogError, - // "True", - // err.Error(), - // ) - // return updateStatusAndRequeue() - // } - // errorMessage := err.Error() - // - // if errors.Is(err, auditlogging.ErrMissingMapping) { - // if m.RCCfg.AuditLogMandatory { - // m.log.Error(err, "AuditLogMandatory", auditLogMandatoryString, "providerType", s.shoot.Spec.Provider.Type, "region", s.shoot.Spec.Region) - // s.instance.UpdateStatePending( - // imv1.ConditionTypeAuditLogConfigured, - // imv1.ConditionReasonAuditLogMissingRegionMapping, - // "False", - // errorMessage, - // ) - // } else { - // m.log.Info(errorMessage, "AuditLogMandatory", auditLogMandatoryString, "providerType", s.shoot.Spec.Provider.Type, "region", s.shoot.Spec.Region) - // s.instance.UpdateStateReady( - // imv1.ConditionTypeAuditLogConfigured, - // imv1.ConditionReasonAuditLogMissingRegionMapping, - // "Missing region mapping for this shoot. Audit Log is not mandatory. Skipping configuration") - // } - // } else { - // if m.RCCfg.AuditLogMandatory { - // m.log.Error(err, "AuditLogMandatory", auditLogMandatoryString) - // s.instance.UpdateStatePending( - // imv1.ConditionTypeAuditLogConfigured, - // imv1.ConditionReasonAuditLogError, - // "False", - // errorMessage) - // } else { - // m.log.Info(errorMessage, "AuditLogMandatory", auditLogMandatoryString) - // s.instance.UpdateStateReady( - // imv1.ConditionTypeAuditLogConfigured, - // imv1.ConditionReasonAuditLogError, - // "Configuration of Audit Log is not mandatory, error for context: "+errorMessage) - // } - // } - //} } func handleError(err error, m *fsm, s *systemState) (stateFn, *ctrl.Result, error) { @@ -104,11 +58,11 @@ func handleError(err error, m *fsm, s *systemState) (stateFn, *ctrl.Result, erro } } - logError := func(err error, errorMsg, infoMsg string, keysAndValues ...any) { + logError := func(err error, keysAndValues ...any) { if m.RCCfg.AuditLogMandatory { - m.log.Error(err, errorMsg, keysAndValues) + m.log.Error(nil, err.Error(), keysAndValues) } else { - m.log.Info(err.Error(), infoMsg) + m.log.Info(err.Error(), keysAndValues) } } @@ -131,20 +85,16 @@ func handleError(err error, m *fsm, s *systemState) (stateFn, *ctrl.Result, erro readyStatusMsg := "Missing region mapping for this shoot. Audit Log is not mandatory. Skipping configuration" setStateForAuditLogError(imv1.ConditionReasonAuditLogMissingRegionMapping, pendingStatusMsg, readyStatusMsg) - errorMsg := "Failed to configure Audit Log" - infoMsg := "Failed to configure Audit Log" - logError(err, errorMsg, infoMsg, "AuditLogMandatory", auditLogMandatoryString, "providerType", s.shoot.Spec.Provider.Type, "region", s.shoot.Spec.Region) + logError(err, "AuditLogMandatory", auditLogMandatoryString, "providerType", s.shoot.Spec.Provider.Type, "region", s.shoot.Spec.Region) - return updateStatusAndRequeue() + return updateStatusAndStop() } pendingStatusMsg := err.Error() readyStatusMsg := "Configuration of Audit Log is not mandatory, error for context: " + err.Error() setStateForAuditLogError(imv1.ConditionReasonAuditLogError, pendingStatusMsg, readyStatusMsg) - errorMsg := "Failed to configure Audit Log" - infoMsg := "Failed to configure Audit Log" - logError(err, errorMsg, infoMsg, "AuditLogMandatory", auditLogMandatoryString) + logError(err, "AuditLogMandatory", auditLogMandatoryString) - return updateStatusAndRequeue() + return updateStatusAndStop() } From b05851ebf2c5e60b5c9ce445158994aef5aeacc5 Mon Sep 17 00:00:00 2001 From: Arkadiusz Galwas Date: Wed, 18 Sep 2024 16:56:45 +0200 Subject: [PATCH 4/8] Linter --- .../controller/runtime/fsm/runtime_fsm_configure_auditlog.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go b/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go index 13ab05e6..81ccd276 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go +++ b/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go @@ -60,9 +60,9 @@ func handleError(err error, m *fsm, s *systemState) (stateFn, *ctrl.Result, erro logError := func(err error, keysAndValues ...any) { if m.RCCfg.AuditLogMandatory { - m.log.Error(nil, err.Error(), keysAndValues) + m.log.Error(nil, err.Error(), keysAndValues...) } else { - m.log.Info(err.Error(), keysAndValues) + m.log.Info(err.Error(), keysAndValues...) } } From 26f8316230c47eba699c6eed9d1b593bb1176aa8 Mon Sep 17 00:00:00 2001 From: Wojciech Nawa Date: Thu, 19 Sep 2024 13:40:41 +0200 Subject: [PATCH 5/8] Add default machine image name (#382) * Add default image name * Run gci --------- Co-authored-by: Arkadiusz Galwas --- hack/shoot-comparator/cmd/comparator/directories.go | 5 +++-- hack/shoot-comparator/cmd/comparator/files.go | 1 + .../internal/directories/comparator.go | 3 ++- .../internal/directories/comparator_test.go | 6 +++--- hack/shoot-comparator/internal/files/comparator.go | 3 ++- hack/shoot-comparator/pkg/shoot/extensionmatcher.go | 7 ++++--- .../pkg/shoot/extensionmatcher_test.go | 1 + internal/auditlogging/mocks/AuditLogConfigurator.go | 5 +---- internal/controller/runtime/suite_test.go | 6 +++--- .../runtime/test_client_obj_tracker_test.go | 3 ++- internal/gardener/mocks/ShootClient.go | 7 ++----- internal/gardener/shoot/converter.go | 3 ++- internal/gardener/shoot/converter_test.go | 2 ++ internal/gardener/shoot/extender/oidc_test.go | 2 +- internal/gardener/shoot/extender/provider.go | 11 ++++++++--- internal/gardener/shoot/extender/provider_test.go | 11 +++++++---- 16 files changed, 44 insertions(+), 32 deletions(-) diff --git a/hack/shoot-comparator/cmd/comparator/directories.go b/hack/shoot-comparator/cmd/comparator/directories.go index ec4136de..41e9a138 100644 --- a/hack/shoot-comparator/cmd/comparator/directories.go +++ b/hack/shoot-comparator/cmd/comparator/directories.go @@ -2,10 +2,11 @@ package comparator import ( "fmt" - "github.com/kyma-project/infrastructure-manager/tools/shoot-comparator/internal/directories" - "github.com/spf13/cobra" "log/slog" "time" + + "github.com/kyma-project/infrastructure-manager/tools/shoot-comparator/internal/directories" + "github.com/spf13/cobra" ) func init() { diff --git a/hack/shoot-comparator/cmd/comparator/files.go b/hack/shoot-comparator/cmd/comparator/files.go index 4936af9b..4884dfee 100644 --- a/hack/shoot-comparator/cmd/comparator/files.go +++ b/hack/shoot-comparator/cmd/comparator/files.go @@ -2,6 +2,7 @@ package comparator import ( "fmt" + "github.com/kyma-project/infrastructure-manager/tools/shoot-comparator/internal/files" "github.com/spf13/cobra" ) diff --git a/hack/shoot-comparator/internal/directories/comparator.go b/hack/shoot-comparator/internal/directories/comparator.go index 1ef0a5f3..8af19105 100644 --- a/hack/shoot-comparator/internal/directories/comparator.go +++ b/hack/shoot-comparator/internal/directories/comparator.go @@ -1,11 +1,12 @@ package directories import ( - "github.com/kyma-project/infrastructure-manager/tools/shoot-comparator/internal/files" "os" "path" "slices" "time" + + "github.com/kyma-project/infrastructure-manager/tools/shoot-comparator/internal/files" ) type Result struct { diff --git a/hack/shoot-comparator/internal/directories/comparator_test.go b/hack/shoot-comparator/internal/directories/comparator_test.go index fcdf181b..b44828bf 100644 --- a/hack/shoot-comparator/internal/directories/comparator_test.go +++ b/hack/shoot-comparator/internal/directories/comparator_test.go @@ -1,16 +1,16 @@ package directories import ( - "github.com/gardener/gardener/pkg/apis/core/v1beta1" - "gopkg.in/yaml.v3" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "os" "path" "testing" "time" + "github.com/gardener/gardener/pkg/apis/core/v1beta1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) const onlyLeftFilename = "onlyLeftFile.yaml" diff --git a/hack/shoot-comparator/internal/files/comparator.go b/hack/shoot-comparator/internal/files/comparator.go index 0c1acafe..7cb3ba51 100644 --- a/hack/shoot-comparator/internal/files/comparator.go +++ b/hack/shoot-comparator/internal/files/comparator.go @@ -1,9 +1,10 @@ package files import ( + "os" + "github.com/gardener/gardener/pkg/apis/core/v1beta1" "github.com/kyma-project/infrastructure-manager/tools/shoot-comparator/pkg/shoot" - "os" "sigs.k8s.io/yaml" ) diff --git a/hack/shoot-comparator/pkg/shoot/extensionmatcher.go b/hack/shoot-comparator/pkg/shoot/extensionmatcher.go index fa7a1179..c71f995d 100644 --- a/hack/shoot-comparator/pkg/shoot/extensionmatcher.go +++ b/hack/shoot-comparator/pkg/shoot/extensionmatcher.go @@ -2,12 +2,13 @@ package shoot import ( "fmt" - "github.com/gardener/gardener/pkg/apis/core/v1beta1" - "github.com/onsi/gomega" - "github.com/onsi/gomega/types" "reflect" "sort" "strings" + + "github.com/gardener/gardener/pkg/apis/core/v1beta1" + "github.com/onsi/gomega" + "github.com/onsi/gomega/types" ) type ExtensionMatcher struct { diff --git a/hack/shoot-comparator/pkg/shoot/extensionmatcher_test.go b/hack/shoot-comparator/pkg/shoot/extensionmatcher_test.go index ed8ffb36..652ec483 100644 --- a/hack/shoot-comparator/pkg/shoot/extensionmatcher_test.go +++ b/hack/shoot-comparator/pkg/shoot/extensionmatcher_test.go @@ -2,6 +2,7 @@ package shoot import ( "fmt" + "github.com/gardener/gardener/pkg/apis/core/v1beta1" . "github.com/onsi/ginkgo/v2" //nolint:revive . "github.com/onsi/gomega" //nolint:revive diff --git a/internal/auditlogging/mocks/AuditLogConfigurator.go b/internal/auditlogging/mocks/AuditLogConfigurator.go index 92016d0a..3977f019 100644 --- a/internal/auditlogging/mocks/AuditLogConfigurator.go +++ b/internal/auditlogging/mocks/AuditLogConfigurator.go @@ -5,13 +5,10 @@ package mocks import ( context "context" + v1beta1 "github.com/gardener/gardener/pkg/apis/core/v1beta1" auditlogging "github.com/kyma-project/infrastructure-manager/internal/auditlogging" - mock "github.com/stretchr/testify/mock" - types "k8s.io/apimachinery/pkg/types" - - v1beta1 "github.com/gardener/gardener/pkg/apis/core/v1beta1" ) // AuditLogConfigurator is an autogenerated mock type for the AuditLogConfigurator type diff --git a/internal/controller/runtime/suite_test.go b/internal/controller/runtime/suite_test.go index 22c538b2..f29815dd 100644 --- a/internal/controller/runtime/suite_test.go +++ b/internal/controller/runtime/suite_test.go @@ -19,19 +19,19 @@ package runtime import ( "context" "encoding/json" - "github.com/kyma-project/infrastructure-manager/internal/auditlogging" - v1 "k8s.io/api/autoscaling/v1" - v12 "k8s.io/api/core/v1" "path/filepath" "testing" "time" gardener_api "github.com/gardener/gardener/pkg/apis/core/v1beta1" infrastructuremanagerv1 "github.com/kyma-project/infrastructure-manager/api/v1" + "github.com/kyma-project/infrastructure-manager/internal/auditlogging" "github.com/kyma-project/infrastructure-manager/internal/controller/runtime/fsm" gardener_shoot "github.com/kyma-project/infrastructure-manager/internal/gardener/shoot" . "github.com/onsi/ginkgo/v2" //nolint:revive . "github.com/onsi/gomega" //nolint:revive + v1 "k8s.io/api/autoscaling/v1" + v12 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" //nolint:revive diff --git a/internal/controller/runtime/test_client_obj_tracker_test.go b/internal/controller/runtime/test_client_obj_tracker_test.go index 2604945a..5fe1388f 100644 --- a/internal/controller/runtime/test_client_obj_tracker_test.go +++ b/internal/controller/runtime/test_client_obj_tracker_test.go @@ -1,11 +1,12 @@ package runtime import ( + "testing" + gardener_api "github.com/gardener/gardener/pkg/apis/core/v1beta1" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" - "testing" ) func TestCustomTracker_Get(t *testing.T) { diff --git a/internal/gardener/mocks/ShootClient.go b/internal/gardener/mocks/ShootClient.go index ac582f90..4c85276e 100644 --- a/internal/gardener/mocks/ShootClient.go +++ b/internal/gardener/mocks/ShootClient.go @@ -5,13 +5,10 @@ package mocks import ( context "context" + v1beta1 "github.com/gardener/gardener/pkg/apis/core/v1beta1" mock "github.com/stretchr/testify/mock" - - types "k8s.io/apimachinery/pkg/types" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - v1beta1 "github.com/gardener/gardener/pkg/apis/core/v1beta1" + types "k8s.io/apimachinery/pkg/types" ) // ShootClient is an autogenerated mock type for the ShootClient type diff --git a/internal/gardener/shoot/converter.go b/internal/gardener/shoot/converter.go index 4e6a891b..dcb81089 100644 --- a/internal/gardener/shoot/converter.go +++ b/internal/gardener/shoot/converter.go @@ -67,6 +67,7 @@ type GardenerConfig struct { } type MachineImageConfig struct { + DefaultName string `json:"defaultName" validate:"required"` DefaultVersion string `json:"defaultVersion" validate:"required"` } @@ -75,7 +76,7 @@ func NewConverter(config ConverterConfig) Converter { extender.ExtendWithAnnotations, extender.ExtendWithLabels, extender.NewKubernetesExtender(config.Kubernetes.DefaultVersion), - extender.NewProviderExtender(config.Provider.AWS.EnableIMDSv2, config.MachineImage.DefaultVersion), + extender.NewProviderExtender(config.Provider.AWS.EnableIMDSv2, config.MachineImage.DefaultName, config.MachineImage.DefaultVersion), extender.NewDNSExtender(config.DNS.SecretName, config.DNS.DomainPrefix, config.DNS.ProviderType), extender.ExtendWithOIDC, extender.ExtendWithCloudProfile, diff --git a/internal/gardener/shoot/converter_test.go b/internal/gardener/shoot/converter_test.go index 51c1d4d3..67389c12 100644 --- a/internal/gardener/shoot/converter_test.go +++ b/internal/gardener/shoot/converter_test.go @@ -151,6 +151,7 @@ var testReader io.Reader = strings.NewReader(`{ } }, "machineImage": { + "defaultName": "test-image-name", "defaultVersion": "0.1.2.3.4" }, "gardener": { @@ -188,6 +189,7 @@ func Test_ConverterConfig_Load_OK(t *testing.T) { }, }, MachineImage: MachineImageConfig{ + DefaultName: "test-image-name", DefaultVersion: "0.1.2.3.4", }, Gardener: GardenerConfig{ diff --git a/internal/gardener/shoot/extender/oidc_test.go b/internal/gardener/shoot/extender/oidc_test.go index 3fc0e683..88082cdf 100644 --- a/internal/gardener/shoot/extender/oidc_test.go +++ b/internal/gardener/shoot/extender/oidc_test.go @@ -1,13 +1,13 @@ package extender import ( - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "testing" gardener "github.com/gardener/gardener/pkg/apis/core/v1beta1" imv1 "github.com/kyma-project/infrastructure-manager/api/v1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func TestOidcExtender(t *testing.T) { diff --git a/internal/gardener/shoot/extender/provider.go b/internal/gardener/shoot/extender/provider.go index 5796b355..3f23a489 100644 --- a/internal/gardener/shoot/extender/provider.go +++ b/internal/gardener/shoot/extender/provider.go @@ -14,7 +14,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" ) -func NewProviderExtender(enableIMDSv2 bool, defaultMachineImageVersion string) func(runtime imv1.Runtime, shoot *gardener.Shoot) error { +func NewProviderExtender(enableIMDSv2 bool, defaultMachineImageName, defaultMachineImageVersion string) func(runtime imv1.Runtime, shoot *gardener.Shoot) error { return func(runtime imv1.Runtime, shoot *gardener.Shoot) error { provider := &shoot.Spec.Provider provider.Type = runtime.Spec.Shoot.Provider.Type @@ -26,7 +26,7 @@ func NewProviderExtender(enableIMDSv2 bool, defaultMachineImageVersion string) f return err } - setDefaultMachineImageVersion(provider, defaultMachineImageVersion) + setDefaultMachineImage(provider, defaultMachineImageName, defaultMachineImageVersion) err = setWorkerConfig(provider, provider.Type, enableIMDSv2) setWorkerSettings(provider) @@ -122,12 +122,13 @@ func setWorkerSettings(provider *gardener.Provider) { } } -func setDefaultMachineImageVersion(provider *gardener.Provider, defaultMachineImageVersion string) { +func setDefaultMachineImage(provider *gardener.Provider, defaultMachineImageName, defaultMachineImageVersion string) { for i := 0; i < len(provider.Workers); i++ { worker := &provider.Workers[i] if worker.Machine.Image == nil { worker.Machine.Image = &gardener.ShootMachineImage{ + Name: defaultMachineImageName, Version: &defaultMachineImageVersion, } @@ -138,6 +139,10 @@ func setDefaultMachineImageVersion(provider *gardener.Provider, defaultMachineIm machineImageVersion = &defaultMachineImageVersion } + if worker.Machine.Image.Name == "" { + worker.Machine.Image.Name = defaultMachineImageName + } + worker.Machine.Image.Version = machineImageVersion } } diff --git a/internal/gardener/shoot/extender/provider_test.go b/internal/gardener/shoot/extender/provider_test.go index 999790f8..1d24ff3f 100644 --- a/internal/gardener/shoot/extender/provider_test.go +++ b/internal/gardener/shoot/extender/provider_test.go @@ -18,6 +18,8 @@ func TestProviderExtender(t *testing.T) { EnableIMDSv2 bool DefaultMachineImageVersion string ExpectedMachineImageVersion string + DefaultMachineImageName string + ExpectedMachineImageName string ExpectedZonesCount int }{ "Create provider specific config for AWS without worker config": { @@ -65,13 +67,13 @@ func TestProviderExtender(t *testing.T) { shoot := fixEmptyGardenerShoot("cluster", "kcp-system") // when - extender := NewProviderExtender(testCase.EnableIMDSv2, testCase.DefaultMachineImageVersion) + extender := NewProviderExtender(testCase.EnableIMDSv2, testCase.DefaultMachineImageName, testCase.DefaultMachineImageVersion) err := extender(testCase.Runtime, &shoot) // then require.NoError(t, err) - assertProvider(t, testCase.Runtime.Spec.Shoot, shoot, testCase.EnableIMDSv2, testCase.ExpectedMachineImageVersion) + assertProvider(t, testCase.Runtime.Spec.Shoot, shoot, testCase.EnableIMDSv2, testCase.ExpectedMachineImageName, testCase.ExpectedMachineImageVersion) assertProviderSpecificConfig(t, shoot, testCase.ExpectedZonesCount) }) } @@ -90,7 +92,7 @@ func TestProviderExtender(t *testing.T) { } // when - extender := NewProviderExtender(false, "") + extender := NewProviderExtender(false, "", "") err := extender(runtime, &shoot) // then @@ -175,7 +177,7 @@ func fixAWSProviderWithMultipleWorkers() imv1.Provider { } } -func assertProvider(t *testing.T, runtimeShoot imv1.RuntimeShoot, shoot gardener.Shoot, expectWorkerConfig bool, expectedMachineImageVersion string) { +func assertProvider(t *testing.T, runtimeShoot imv1.RuntimeShoot, shoot gardener.Shoot, expectWorkerConfig bool, expectedMachineImageName, expectedMachineImageVersion string) { assert.Equal(t, runtimeShoot.Provider.Type, shoot.Spec.Provider.Type) assert.Equal(t, runtimeShoot.Provider.Workers, shoot.Spec.Provider.Workers) assert.Equal(t, false, shoot.Spec.Provider.WorkersSettings.SSHAccess.Enabled) @@ -192,6 +194,7 @@ func assertProvider(t *testing.T, runtimeShoot imv1.RuntimeShoot, shoot gardener assert.Empty(t, worker.ProviderConfig) } assert.Equal(t, expectedMachineImageVersion, *worker.Machine.Image.Version) + assert.Equal(t, expectedMachineImageName, worker.Machine.Image.Name) } } From 2c772ea1fe5d0b4d760b7b405fb6fce98050a42b Mon Sep 17 00:00:00 2001 From: Wojciech Nawa Date: Fri, 20 Sep 2024 10:38:59 +0200 Subject: [PATCH 6/8] Add worker field to GCP (#392) --- internal/gardener/shoot/hyperscaler/gcp/config.go | 1 + internal/gardener/shoot/hyperscaler/gcp/config_test.go | 1 + 2 files changed, 2 insertions(+) diff --git a/internal/gardener/shoot/hyperscaler/gcp/config.go b/internal/gardener/shoot/hyperscaler/gcp/config.go index 12facfc8..5abf1061 100644 --- a/internal/gardener/shoot/hyperscaler/gcp/config.go +++ b/internal/gardener/shoot/hyperscaler/gcp/config.go @@ -36,6 +36,7 @@ func NewInfrastructureConfig(workerCIDR string) v1alpha1.InfrastructureConfig { // Provisioner sets also deprecated Worker field. // Logic for comparing shoots in integration tests must be adjusted accordingly Workers: workerCIDR, + Worker: workerCIDR, }, } } diff --git a/internal/gardener/shoot/hyperscaler/gcp/config_test.go b/internal/gardener/shoot/hyperscaler/gcp/config_test.go index 9721c8c9..9e69fb7b 100644 --- a/internal/gardener/shoot/hyperscaler/gcp/config_test.go +++ b/internal/gardener/shoot/hyperscaler/gcp/config_test.go @@ -54,5 +54,6 @@ func TestInfrastructureConfig(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "10.250.0.0/22", infrastructureConfig.Networks.Workers) + assert.Equal(t, "10.250.0.0/22", infrastructureConfig.Networks.Worker) }) } From 2f985204ca43eb833acba0bf5f3c0b04d59e31e1 Mon Sep 17 00:00:00 2001 From: Arkadiusz Galwas Date: Fri, 20 Sep 2024 10:39:46 +0200 Subject: [PATCH 7/8] Fix dump shoot name (#385) * Fixed dumping shoot specs in dry run mode * Adjusted sFnInitialize to make dumping scpe in dry run mode working * Linter * Unit tests * Fix in unit test --------- Co-authored-by: marcin witalis <45931826+m00g3n@users.noreply.github.com> --- .../fsm/runtime_fsm_create_shoot_dry_run.go | 11 +--- .../runtime/fsm/runtime_fsm_initialise.go | 29 +++++++-- .../fsm/runtime_fsm_initialise_test.go | 60 +++++++++++++++++++ 3 files changed, 87 insertions(+), 13 deletions(-) diff --git a/internal/controller/runtime/fsm/runtime_fsm_create_shoot_dry_run.go b/internal/controller/runtime/fsm/runtime_fsm_create_shoot_dry_run.go index 31900056..61a021f7 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_create_shoot_dry_run.go +++ b/internal/controller/runtime/fsm/runtime_fsm_create_shoot_dry_run.go @@ -2,7 +2,6 @@ package fsm import ( "context" - "fmt" imv1 "github.com/kyma-project/infrastructure-manager/api/v1" ctrl "sigs.k8s.io/controller-runtime" @@ -16,7 +15,7 @@ func sFnCreateShootDryRun(_ context.Context, m *fsm, s *systemState) (stateFn, * m.log.Error(err, "Failed to convert Runtime instance to shoot object [dry-run]") return updateStatePendingWithErrorAndStop( &s.instance, - imv1.ConditionTypeRuntimeProvisioned, + imv1.ConditionTypeRuntimeProvisionedDryRun, imv1.ConditionReasonConversionError, "Runtime conversion error") } @@ -28,13 +27,9 @@ func sFnCreateShootDryRun(_ context.Context, m *fsm, s *systemState) (stateFn, * "Runtime processing completed successfully [dry-run]") // stop machine if persistence not enabled - if m.PVCPath == "" { - return updateStatusAndStop() + if m.PVCPath != "" { + return switchState(sFnDumpShootSpec) } - path := fmt.Sprintf("%s/%s-%s.yaml", m.PVCPath, s.shoot.Namespace, s.shoot.Name) - if err := persist(path, s.shoot, m.writerProvider); err != nil { - return updateStatusAndStopWithError(err) - } return updateStatusAndStop() } diff --git a/internal/controller/runtime/fsm/runtime_fsm_initialise.go b/internal/controller/runtime/fsm/runtime_fsm_initialise.go index bc4bf39a..7c2ebc3b 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_initialise.go +++ b/internal/controller/runtime/fsm/runtime_fsm_initialise.go @@ -16,16 +16,25 @@ func sFnInitialize(ctx context.Context, m *fsm, s *systemState) (stateFn, *ctrl. instanceIsNotBeingDeleted := s.instance.GetDeletionTimestamp().IsZero() instanceHasFinalizer := controllerutil.ContainsFinalizer(&s.instance, m.Finalizer) provisioningCondition := meta.FindStatusCondition(s.instance.Status.Conditions, string(imv1.ConditionTypeRuntimeProvisioned)) + dryRunProvisioningCondition := meta.FindStatusCondition(s.instance.Status.Conditions, string(imv1.ConditionTypeRuntimeProvisionedDryRun)) dryRunMode := s.instance.IsControlledByProvisioner() if instanceIsNotBeingDeleted && !instanceHasFinalizer { return addFinalizerAndRequeue(ctx, m, s) } - if instanceIsNotBeingDeleted && s.shoot == nil && provisioningCondition == nil { + if instanceIsNotBeingDeleted && s.shoot == nil && provisioningCondition == nil && dryRunProvisioningCondition == nil { m.log.Info("Update Runtime state to Pending - initialised") + + getConditionType := func() imv1.RuntimeConditionType { + if dryRunMode { + return imv1.ConditionTypeRuntimeProvisionedDryRun + } + return imv1.ConditionTypeRuntimeProvisioned + } + s.instance.UpdateStatePending( - imv1.ConditionTypeRuntimeProvisioned, + getConditionType(), imv1.ConditionReasonInitialized, "Unknown", "Runtime initialized", @@ -33,11 +42,21 @@ func sFnInitialize(ctx context.Context, m *fsm, s *systemState) (stateFn, *ctrl. return updateStatusAndRequeue() } - if instanceIsNotBeingDeleted && s.shoot == nil { - m.log.Info("Gardener shoot does not exist, creating new one") + shootNeedsToBeCreated := func() bool { + if dryRunMode { + return instanceIsNotBeingDeleted && dryRunProvisioningCondition != nil && + dryRunProvisioningCondition.Status != "True" + } + + return instanceIsNotBeingDeleted && s.shoot == nil + } + + if shootNeedsToBeCreated() { if !dryRunMode { return switchState(sFnCreateShoot) } + + m.log.Info("Gardener shoot does not exist, creating new one") return switchState(sFnCreateShootDryRun) } @@ -55,7 +74,7 @@ func sFnInitialize(ctx context.Context, m *fsm, s *systemState) (stateFn, *ctrl. return removeFinalizerAndStop(ctx, m, s) // resource cleanup completed } - m.log.Info("noting to reconcile, stopping sfm") + m.log.Info("noting to reconcile, stopping fsm") return stop() } diff --git a/internal/controller/runtime/fsm/runtime_fsm_initialise_test.go b/internal/controller/runtime/fsm/runtime_fsm_initialise_test.go index 563d9b57..79648dc1 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_initialise_test.go +++ b/internal/controller/runtime/fsm/runtime_fsm_initialise_test.go @@ -64,6 +64,28 @@ var _ = Describe("KIM sFnInitialise", func() { }, } + testDryRunRtWithFinalizerAndProvisioningCondition := imv1.Runtime{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-instance", + Namespace: "default", + Finalizers: []string{"test-me-plz"}, + Labels: map[string]string{ + imv1.LabelControlledByProvisioner: "true", + }, + }, + } + + testDryRunRtWithFinalizerAndProvisioningReadyCondition := imv1.Runtime{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-instance", + Namespace: "default", + Finalizers: []string{"test-me-plz"}, + Labels: map[string]string{ + imv1.LabelControlledByProvisioner: "true", + }, + }, + } + provisioningCondition := metav1.Condition{ Type: string(imv1.ConditionTypeRuntimeProvisioned), Status: metav1.ConditionUnknown, @@ -73,6 +95,24 @@ var _ = Describe("KIM sFnInitialise", func() { } meta.SetStatusCondition(&testRtWithFinalizerAndProvisioningCondition.Status.Conditions, provisioningCondition) + provisioningDryRunCondition := metav1.Condition{ + Type: string(imv1.ConditionTypeRuntimeProvisionedDryRun), + Status: metav1.ConditionUnknown, + LastTransitionTime: now, + Reason: "Test reason", + Message: "Test message", + } + meta.SetStatusCondition(&testDryRunRtWithFinalizerAndProvisioningCondition.Status.Conditions, provisioningDryRunCondition) + + provisioningDryRunConditionReady := metav1.Condition{ + Type: string(imv1.ConditionTypeRuntimeProvisionedDryRun), + Status: metav1.ConditionTrue, + LastTransitionTime: now, + Reason: "Test reason", + Message: "Test message", + } + meta.SetStatusCondition(&testDryRunRtWithFinalizerAndProvisioningReadyCondition.Status.Conditions, provisioningDryRunConditionReady) + testRtWithDeletionTimestamp := imv1.Runtime{ ObjectMeta: metav1.ObjectMeta{ DeletionTimestamp: &now, @@ -167,6 +207,26 @@ var _ = Describe("KIM sFnInitialise", func() { MatchNextFnState: haveName("sFnCreateShoot"), }, ), + Entry( + "should return sFnCreateShootDryRun and no error when exists Provisioning Condition and shoot is missing", + testCtx, + must(newFakeFSM, withTestFinalizer), + &systemState{instance: testDryRunRtWithFinalizerAndProvisioningCondition}, + testOpts{ + MatchExpectedErr: BeNil(), + MatchNextFnState: haveName("sFnCreateShootDryRun"), + }, + ), + Entry( + "should stop when sFnCreateShootDryRun was already executed", + testCtx, + must(newFakeFSM, withTestFinalizer), + &systemState{instance: testDryRunRtWithFinalizerAndProvisioningReadyCondition}, + testOpts{ + MatchExpectedErr: BeNil(), + MatchNextFnState: BeNil(), + }, + ), Entry( "should return sFnSelectShootProcessing and no error when exists Provisioning Condition and shoot exists", testCtx, From 6c15d44ce0b26763517ec68c8cba3b7295f81089 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Drzewiecki?= Date: Fri, 20 Sep 2024 13:33:10 +0200 Subject: [PATCH 8/8] OIDCConfig is now optional --- api/v1/runtime_types.go | 2 +- .../bases/infrastructuremanager.kyma-project.io_runtimes.yaml | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/api/v1/runtime_types.go b/api/v1/runtime_types.go index dbbc7c83..b1fb7619 100644 --- a/api/v1/runtime_types.go +++ b/api/v1/runtime_types.go @@ -164,7 +164,7 @@ type Kubernetes struct { } type APIServer struct { - OidcConfig gardener.OIDCConfig `json:"oidcConfig"` + OidcConfig gardener.OIDCConfig `json:"oidcConfig,omitempty"` AdditionalOidcConfig *[]gardener.OIDCConfig `json:"additionalOidcConfig,omitempty"` } diff --git a/config/crd/bases/infrastructuremanager.kyma-project.io_runtimes.yaml b/config/crd/bases/infrastructuremanager.kyma-project.io_runtimes.yaml index 034516ab..f3743ca0 100644 --- a/config/crd/bases/infrastructuremanager.kyma-project.io_runtimes.yaml +++ b/config/crd/bases/infrastructuremanager.kyma-project.io_runtimes.yaml @@ -279,8 +279,6 @@ spec: the value '-'. type: string type: object - required: - - oidcConfig type: object version: type: string