From cdd127f2e916dad83461ae9a477050521e894848 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Drzewiecki?= Date: Tue, 10 Sep 2024 12:00:50 +0200 Subject: [PATCH 01/17] oidc extension added only if controlled-by-provisioner value is disabled --- internal/gardener/shoot/extender/dns_test.go | 3 +++ internal/gardener/shoot/extender/oidc.go | 15 ++++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/internal/gardener/shoot/extender/dns_test.go b/internal/gardener/shoot/extender/dns_test.go index 802c52be..7d90ce05 100644 --- a/internal/gardener/shoot/extender/dns_test.go +++ b/internal/gardener/shoot/extender/dns_test.go @@ -58,6 +58,9 @@ func fixEmptyGardenerShoot(name, namespace string) gardener.Shoot { ObjectMeta: v1.ObjectMeta{ Name: name, Namespace: namespace, + Labels: map[string]string{ + "kyma-project.io/controlled-by-provisioner": "false", + }, }, Spec: gardener.ShootSpec{}, } diff --git a/internal/gardener/shoot/extender/oidc.go b/internal/gardener/shoot/extender/oidc.go index 8e8da762..90e7e5e8 100644 --- a/internal/gardener/shoot/extender/oidc.go +++ b/internal/gardener/shoot/extender/oidc.go @@ -13,12 +13,25 @@ const ( func ExtendWithOIDC(runtime imv1.Runtime, shoot *gardener.Shoot) error { oidcConfig := runtime.Spec.Shoot.Kubernetes.KubeAPIServer.OidcConfig - setOIDCExtension(shoot) + if CanEnableExtension(shoot) { + setOIDCExtension(shoot) + } setKubeAPIServerOIDCConfig(shoot, oidcConfig) return nil } +func CanEnableExtension(shoot *gardener.Shoot) bool { + controlledByProvisionerLabelValue := shoot.Labels[imv1.LabelControlledByProvisioner] + canEnable := false + + if controlledByProvisionerLabelValue == "false" { + canEnable = true + } + + return canEnable +} + func setOIDCExtension(shoot *gardener.Shoot) { oidcService := gardener.Extension{ Type: OidcExtensionType, From 4908db32739be02f1ac5303166de36517219dd3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Drzewiecki?= Date: Wed, 11 Sep 2024 09:10:53 +0200 Subject: [PATCH 02/17] oidc is not being configured for migrated clusters --- internal/gardener/shoot/extender/oidc.go | 8 +- internal/gardener/shoot/extender/oidc_test.go | 84 ++++++++++++------- 2 files changed, 59 insertions(+), 33 deletions(-) diff --git a/internal/gardener/shoot/extender/oidc.go b/internal/gardener/shoot/extender/oidc.go index 90e7e5e8..ae93b6d8 100644 --- a/internal/gardener/shoot/extender/oidc.go +++ b/internal/gardener/shoot/extender/oidc.go @@ -22,11 +22,11 @@ func ExtendWithOIDC(runtime imv1.Runtime, shoot *gardener.Shoot) error { } func CanEnableExtension(shoot *gardener.Shoot) bool { - controlledByProvisionerLabelValue := shoot.Labels[imv1.LabelControlledByProvisioner] - canEnable := false + canEnable := true + createdByMigrator := shoot.Labels["operator.kyma-project.io/created-by-migrator"] - if controlledByProvisionerLabelValue == "false" { - canEnable = true + if createdByMigrator == "true" { + canEnable = false } return canEnable diff --git a/internal/gardener/shoot/extender/oidc_test.go b/internal/gardener/shoot/extender/oidc_test.go index 2fda2db5..75e8cbe6 100644 --- a/internal/gardener/shoot/extender/oidc_test.go +++ b/internal/gardener/shoot/extender/oidc_test.go @@ -10,42 +10,68 @@ import ( ) func TestOidcExtender(t *testing.T) { - t.Run("Create kubernetes config", func(t *testing.T) { - // given - clientID := "client-id" - groupsClaim := "groups" - issuerURL := "https://my.cool.tokens.com" - usernameClaim := "sub" + const MigratorLabel = "operator.kyma-project.io/created-by-migrator" + for _, testCase := range []struct { + name string + migratorLabel map[string]string + expectedOidcExtensionEnabled bool + }{ + { + name: "label created-by-migrator=true should not configure OIDC", + migratorLabel: map[string]string{MigratorLabel: "true"}, + expectedOidcExtensionEnabled: false, + }, + { + name: "label created-by-migrator=false should configure OIDC", + migratorLabel: map[string]string{MigratorLabel: "false"}, + expectedOidcExtensionEnabled: true, + }, + { + name: "label created-by-migrator unset should configure OIDC", + migratorLabel: nil, + expectedOidcExtensionEnabled: true, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + // given + clientID := "client-id" + groupsClaim := "groups" + issuerURL := "https://my.cool.tokens.com" + usernameClaim := "sub" - shoot := fixEmptyGardenerShoot("test", "kcp-system") - runtimeShoot := imv1.Runtime{ - Spec: imv1.RuntimeSpec{ - Shoot: imv1.RuntimeShoot{ - Kubernetes: imv1.Kubernetes{ - KubeAPIServer: imv1.APIServer{ - OidcConfig: gardener.OIDCConfig{ - ClientID: &clientID, - GroupsClaim: &groupsClaim, - IssuerURL: &issuerURL, - SigningAlgs: []string{ - "RS256", + shoot := fixEmptyGardenerShoot("test", "kcp-system") + shoot.Labels[MigratorLabel] = testCase.migratorLabel[MigratorLabel] + runtimeShoot := imv1.Runtime{ + Spec: imv1.RuntimeSpec{ + Shoot: imv1.RuntimeShoot{ + Kubernetes: imv1.Kubernetes{ + KubeAPIServer: imv1.APIServer{ + OidcConfig: gardener.OIDCConfig{ + ClientID: &clientID, + GroupsClaim: &groupsClaim, + IssuerURL: &issuerURL, + SigningAlgs: []string{ + "RS256", + }, + UsernameClaim: &usernameClaim, }, - UsernameClaim: &usernameClaim, }, }, }, }, - }, - } + } - // when - err := ExtendWithOIDC(runtimeShoot, &shoot) + // when + err := ExtendWithOIDC(runtimeShoot, &shoot) - // then - require.NoError(t, err) + // then + require.NoError(t, err) - assert.Equal(t, runtimeShoot.Spec.Shoot.Kubernetes.KubeAPIServer.OidcConfig, *shoot.Spec.Kubernetes.KubeAPIServer.OIDCConfig) - assert.Equal(t, false, *shoot.Spec.Extensions[0].Disabled) - assert.Equal(t, "shoot-oidc-service", shoot.Spec.Extensions[0].Type) - }) + assert.Equal(t, runtimeShoot.Spec.Shoot.Kubernetes.KubeAPIServer.OidcConfig, *shoot.Spec.Kubernetes.KubeAPIServer.OIDCConfig) + if testCase.expectedOidcExtensionEnabled { + assert.Equal(t, testCase.expectedOidcExtensionEnabled, !*shoot.Spec.Extensions[0].Disabled) + assert.Equal(t, "shoot-oidc-service", shoot.Spec.Extensions[0].Type) + } + }) + } } From cdf28343f813f947cee4c4d345ce6289d3336def Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Drzewiecki?= Date: Wed, 11 Sep 2024 09:15:05 +0200 Subject: [PATCH 03/17] removes bootstraping of controlled-by-provisioner label in tests --- internal/gardener/shoot/extender/dns_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/internal/gardener/shoot/extender/dns_test.go b/internal/gardener/shoot/extender/dns_test.go index 7d90ce05..f0e4c032 100644 --- a/internal/gardener/shoot/extender/dns_test.go +++ b/internal/gardener/shoot/extender/dns_test.go @@ -58,9 +58,7 @@ func fixEmptyGardenerShoot(name, namespace string) gardener.Shoot { ObjectMeta: v1.ObjectMeta{ Name: name, Namespace: namespace, - Labels: map[string]string{ - "kyma-project.io/controlled-by-provisioner": "false", - }, + Labels: map[string]string{}, }, Spec: gardener.ShootSpec{}, } From 80c41a87b0f9f46ca45fbf09c3d46bbf4bb99ee9 Mon Sep 17 00:00:00 2001 From: Arkadiusz Galwas Date: Wed, 11 Sep 2024 11:37:27 +0200 Subject: [PATCH 04/17] Fixes to migrator that will remove unimportant differences --- hack/runtime-migrator/main.go | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/hack/runtime-migrator/main.go b/hack/runtime-migrator/main.go index ca5adcef..76f8aaa2 100644 --- a/hack/runtime-migrator/main.go +++ b/hack/runtime-migrator/main.go @@ -206,7 +206,7 @@ func createRuntime(ctx context.Context, shoot v1beta1.Shoot, cfg migrator.Config }, Provider: v1.Provider{ Type: shoot.Spec.Provider.Type, - Workers: shoot.Spec.Provider.Workers, + Workers: getWorkersConfig(shoot.Spec.Provider.Workers), }, Networking: v1.Networking{ Type: shoot.Spec.Networking.Type, @@ -214,7 +214,6 @@ func createRuntime(ctx context.Context, shoot v1beta1.Shoot, cfg migrator.Config Nodes: *shoot.Spec.Networking.Nodes, Services: *shoot.Spec.Networking.Services, }, - ControlPlane: getControlPlane(shoot), }, Security: v1.Security{ Administrators: subjects, @@ -235,9 +234,28 @@ func createRuntime(ctx context.Context, shoot v1beta1.Shoot, cfg migrator.Config Conditions: nil, // deliberately left nil by our migrator to show that controller has not picked it yet }, } + + controlPlane := getControlPlane(shoot) + if controlPlane != nil { + runtime.Spec.Shoot.ControlPlane = controlPlane + } + return runtime, nil } +// The goal of this function is to make the migrator output equal to the shoot created by the converter +// As a result we can automatically verify the correctness of the migrator output +func getWorkersConfig(workers []v1beta1.Worker) []v1beta1.Worker { + // We need to set the following fields to nil, as they are not set by the KIM converter + for i := 0; i < len(workers); i++ { + workers[i].Machine.Architecture = nil + workers[i].SystemComponents = nil + workers[i].CRI = nil + } + + return workers +} + func getOidcConfig(shoot v1beta1.Shoot) v1beta1.OIDCConfig { var oidcConfig = v1beta1.OIDCConfig{ CABundle: nil, // deliberately left empty From 815595afe7788ec5d4873d3a877fe8a539c313be Mon Sep 17 00:00:00 2001 From: Arkadiusz Galwas Date: Wed, 11 Sep 2024 11:46:58 +0200 Subject: [PATCH 05/17] Fixes to migrator that will remove unimportant differences --- hack/runtime-migrator/main.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hack/runtime-migrator/main.go b/hack/runtime-migrator/main.go index 76f8aaa2..da38ce41 100644 --- a/hack/runtime-migrator/main.go +++ b/hack/runtime-migrator/main.go @@ -206,7 +206,7 @@ func createRuntime(ctx context.Context, shoot v1beta1.Shoot, cfg migrator.Config }, Provider: v1.Provider{ Type: shoot.Spec.Provider.Type, - Workers: getWorkersConfig(shoot.Spec.Provider.Workers), + Workers: adjustWorkersConfig(shoot.Spec.Provider.Workers), }, Networking: v1.Networking{ Type: shoot.Spec.Networking.Type, @@ -245,7 +245,7 @@ func createRuntime(ctx context.Context, shoot v1beta1.Shoot, cfg migrator.Config // The goal of this function is to make the migrator output equal to the shoot created by the converter // As a result we can automatically verify the correctness of the migrator output -func getWorkersConfig(workers []v1beta1.Worker) []v1beta1.Worker { +func adjustWorkersConfig(workers []v1beta1.Worker) []v1beta1.Worker { // We need to set the following fields to nil, as they are not set by the KIM converter for i := 0; i < len(workers); i++ { workers[i].Machine.Architecture = nil From aca7e6fd664ca8df8b2102f2bc92220a5ee46586 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Drzewiecki?= Date: Wed, 11 Sep 2024 11:51:34 +0200 Subject: [PATCH 06/17] adds oidc disabled extender test case --- internal/gardener/shoot/extender/oidc_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/gardener/shoot/extender/oidc_test.go b/internal/gardener/shoot/extender/oidc_test.go index 75e8cbe6..941443b1 100644 --- a/internal/gardener/shoot/extender/oidc_test.go +++ b/internal/gardener/shoot/extender/oidc_test.go @@ -71,6 +71,8 @@ func TestOidcExtender(t *testing.T) { if testCase.expectedOidcExtensionEnabled { assert.Equal(t, testCase.expectedOidcExtensionEnabled, !*shoot.Spec.Extensions[0].Disabled) assert.Equal(t, "shoot-oidc-service", shoot.Spec.Extensions[0].Type) + } else { + assert.Equal(t, 0, len(shoot.Spec.Extensions)) } }) } From d46d22f49f77c208f4a2246d8b143ff630d47f7e Mon Sep 17 00:00:00 2001 From: Rafal Foks Date: Wed, 11 Sep 2024 13:03:31 +0200 Subject: [PATCH 07/17] Handle the Kubernetes conflict error in KIM Audit Log --- internal/auditlogging/auditlogging.go | 7 +++- .../fsm/runtime_fsm_configure_auditlog.go | 13 ++++++- .../runtime_fsm_configure_auditlog_test.go | 38 +++++++++++++++++++ ...runtime_fsm_waiting_for_shoot_reconcile.go | 2 +- 4 files changed, 57 insertions(+), 3 deletions(-) diff --git a/internal/auditlogging/auditlogging.go b/internal/auditlogging/auditlogging.go index c4b47aec..feac599d 100644 --- a/internal/auditlogging/auditlogging.go +++ b/internal/auditlogging/auditlogging.go @@ -147,9 +147,14 @@ func ApplyAuditLogConfig(shoot *gardener.Shoot, auditConfigFromFile map[string]m } changedExt, err := configureExtension(shoot, tenant) + + if err != nil { + return false, err + } + changedSec := configureSecret(shoot, tenant) - return changedExt || changedSec, err + return changedExt || changedSec, nil } func configureExtension(shoot *gardener.Shoot, config AuditLogData) (changed bool, err error) { diff --git a/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go b/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go index d03cb5d6..04f3a43c 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go +++ b/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go @@ -2,6 +2,7 @@ package fsm import ( "context" + k8serrors "k8s.io/apimachinery/pkg/api/errors" imv1 "github.com/kyma-project/infrastructure-manager/api/v1" "github.com/kyma-project/infrastructure-manager/internal/auditlogging" @@ -14,7 +15,7 @@ func sFnConfigureAuditLog(ctx context.Context, m *fsm, s *systemState) (stateFn, wasAuditLogEnabled, err := m.AuditLogging.Enable(ctx, s.shoot) - if wasAuditLogEnabled { + if wasAuditLogEnabled && err == nil { m.log.Info("Audit Log configured for shoot: " + s.shoot.Name) s.instance.UpdateStatePending( imv1.ConditionTypeAuditLogConfigured, @@ -27,6 +28,16 @@ func sFnConfigureAuditLog(ctx context.Context, m *fsm, s *systemState) (stateFn, } 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 { diff --git a/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog_test.go b/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog_test.go index 925a93f2..b8bad745 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog_test.go +++ b/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog_test.go @@ -3,6 +3,7 @@ package fsm import ( "context" "github.com/kyma-project/infrastructure-manager/internal/auditlogging" + k8serrors "k8s.io/apimachinery/pkg/api/errors" "testing" gardener "github.com/gardener/gardener/pkg/apis/core/v1beta1" @@ -236,6 +237,43 @@ func TestAuditLogState(t *testing.T) { assert.Equal(t, v1.RuntimeStateReady, string(systemState.instance.Status.State)) assert.Equal(t, expectedRuntimeConditions, systemState.instance.Status.Conditions) }) + + t.Run("Should requeue in case of Kubernetes object conflict error during update of Shoot object and set status on Runtime CR", func(t *testing.T) { + // given + ctx := context.Background() + auditLog := &mocks.AuditLogging{} + shoot := shootForTest() + instance := runtimeForTest() + systemState := &systemState{ + instance: instance, + shoot: shoot, + } + expectedRuntimeConditions := []metav1.Condition{ + { + Type: string(v1.ConditionTypeAuditLogConfigured), + Status: "True", + Reason: string(v1.ConditionReasonAuditLogError), + Message: k8serrors.NewConflict(gardener.Resource("shoots"), shoot.Name, errors.New("k8s conflict on update error")).Error(), + }, + } + + fsm := &fsm{AuditLogging: auditLog} + fsm.RCCfg.AuditLogMandatory = true + + auditLog.On("Enable", ctx, shoot).Return(false, k8serrors.NewConflict(gardener.Resource("shoots"), shoot.Name, errors.New("k8s conflict on update error"))).Once() + + // when + stateFn, _, _ := sFnConfigureAuditLog(ctx, fsm, systemState) + + // set the time to its zero value for comparison purposes + systemState.instance.Status.Conditions[0].LastTransitionTime = metav1.Time{} + + // then + auditLog.AssertExpectations(t) + require.Contains(t, stateFn.name(), "sFnUpdateStatus") + assert.Equal(t, v1.RuntimeStatePending, string(systemState.instance.Status.State)) + assert.Equal(t, expectedRuntimeConditions, systemState.instance.Status.Conditions) + }) } func shootForTest() *gardener.Shoot { diff --git a/internal/controller/runtime/fsm/runtime_fsm_waiting_for_shoot_reconcile.go b/internal/controller/runtime/fsm/runtime_fsm_waiting_for_shoot_reconcile.go index ce700502..a4a8fb5f 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_waiting_for_shoot_reconcile.go +++ b/internal/controller/runtime/fsm/runtime_fsm_waiting_for_shoot_reconcile.go @@ -49,7 +49,7 @@ func sFnWaitForShootReconcile(_ context.Context, m *fsm, s *systemState) (stateF imv1.ConditionTypeRuntimeProvisioned, imv1.ConditionReasonAuditLogConfigured, "Runtime processing completed successfully", - sFnConfigureAuditLog) + sFnApplyClusterRoleBindings) } m.log.Info("Update did not processed, exiting with no retry") From bbd4ddbb4d69112d6e676680518331434ba67522 Mon Sep 17 00:00:00 2001 From: Rafal Foks Date: Wed, 11 Sep 2024 13:10:12 +0200 Subject: [PATCH 08/17] Fix linter --- .../controller/runtime/fsm/runtime_fsm_configure_auditlog.go | 1 + .../runtime/fsm/runtime_fsm_configure_auditlog_test.go | 3 ++- 2 files changed, 3 insertions(+), 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 04f3a43c..368a9dd0 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go +++ b/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go @@ -2,6 +2,7 @@ package fsm import ( "context" + k8serrors "k8s.io/apimachinery/pkg/api/errors" imv1 "github.com/kyma-project/infrastructure-manager/api/v1" diff --git a/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog_test.go b/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog_test.go index b8bad745..7fb57e56 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog_test.go +++ b/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog_test.go @@ -2,9 +2,10 @@ package fsm import ( "context" + "testing" + "github.com/kyma-project/infrastructure-manager/internal/auditlogging" k8serrors "k8s.io/apimachinery/pkg/api/errors" - "testing" gardener "github.com/gardener/gardener/pkg/apis/core/v1beta1" v1 "github.com/kyma-project/infrastructure-manager/api/v1" From 1229e7d5b05034cfcca40c171c4131bfd7f2fc66 Mon Sep 17 00:00:00 2001 From: Rafal Foks Date: Wed, 11 Sep 2024 13:20:19 +0200 Subject: [PATCH 09/17] Fix gci --- .../controller/runtime/fsm/runtime_fsm_configure_auditlog.go | 3 +-- 1 file changed, 1 insertion(+), 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 368a9dd0..1491f549 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go +++ b/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go @@ -3,11 +3,10 @@ package fsm import ( "context" - k8serrors "k8s.io/apimachinery/pkg/api/errors" - imv1 "github.com/kyma-project/infrastructure-manager/api/v1" "github.com/kyma-project/infrastructure-manager/internal/auditlogging" "github.com/pkg/errors" + k8serrors "k8s.io/apimachinery/pkg/api/errors" ctrl "sigs.k8s.io/controller-runtime" ) From fa8284dc633c590950d8857527817d6c98f5a5cb Mon Sep 17 00:00:00 2001 From: Rafal Foks Date: Wed, 11 Sep 2024 13:20:38 +0200 Subject: [PATCH 10/17] Fix gci in tests --- .../runtime/fsm/runtime_fsm_configure_auditlog_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog_test.go b/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog_test.go index 7fb57e56..cf48b935 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog_test.go +++ b/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog_test.go @@ -4,15 +4,14 @@ import ( "context" "testing" - "github.com/kyma-project/infrastructure-manager/internal/auditlogging" - k8serrors "k8s.io/apimachinery/pkg/api/errors" - gardener "github.com/gardener/gardener/pkg/apis/core/v1beta1" v1 "github.com/kyma-project/infrastructure-manager/api/v1" + "github.com/kyma-project/infrastructure-manager/internal/auditlogging" "github.com/kyma-project/infrastructure-manager/internal/auditlogging/mocks" "github.com/pkg/errors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) From 2a618e77c04cffbdebb1e59c1d0372de105a7b65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Drzewiecki?= Date: Thu, 12 Sep 2024 16:03:58 +0200 Subject: [PATCH 11/17] fixes logger panics --- .../runtime/fsm/runtime_fsm_configure_auditlog.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 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..c06681f9 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go +++ b/internal/controller/runtime/fsm/runtime_fsm_configure_auditlog.go @@ -2,6 +2,7 @@ package fsm import ( "context" + "strconv" imv1 "github.com/kyma-project/infrastructure-manager/api/v1" "github.com/kyma-project/infrastructure-manager/internal/auditlogging" @@ -26,11 +27,13 @@ func sFnConfigureAuditLog(ctx context.Context, m *fsm, s *systemState) (stateFn, return updateStatusAndRequeueAfter(gardenerRequeueDuration) } + auditLogMandatoryString := strconv.FormatBool(m.RCCfg.AuditLogMandatory) + 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) + m.log.Error(err, "AuditLogMandatory", auditLogMandatoryString, "providerType", s.shoot.Spec.Provider.Type, "region", s.shoot.Spec.Region) s.instance.UpdateStatePending( imv1.ConditionTypeAuditLogConfigured, imv1.ConditionReasonAuditLogMissingRegionMapping, @@ -38,7 +41,7 @@ func sFnConfigureAuditLog(ctx context.Context, m *fsm, s *systemState) (stateFn, 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) + m.log.Info(errorMessage, "AuditLogMandatory", auditLogMandatoryString, "providerType", s.shoot.Spec.Provider.Type, "region", s.shoot.Spec.Region) s.instance.UpdateStateReady( imv1.ConditionTypeAuditLogConfigured, imv1.ConditionReasonAuditLogMissingRegionMapping, @@ -46,14 +49,14 @@ func sFnConfigureAuditLog(ctx context.Context, m *fsm, s *systemState) (stateFn, } } else { if m.RCCfg.AuditLogMandatory { - m.log.Error(err, "Failed to configure Audit Log", "AuditLogMandatory", m.RCCfg.AuditLogMandatory) + m.log.Error(err, "AuditLogMandatory", auditLogMandatoryString) s.instance.UpdateStatePending( imv1.ConditionTypeAuditLogConfigured, imv1.ConditionReasonAuditLogError, "False", errorMessage) } else { - m.log.Info(errorMessage, "AuditLogMandatory", m.RCCfg.AuditLogMandatory) + m.log.Info(errorMessage, "AuditLogMandatory", auditLogMandatoryString) s.instance.UpdateStateReady( imv1.ConditionTypeAuditLogConfigured, imv1.ConditionReasonAuditLogError, From 696edfb8b7e0c69d4755cf71ec5c15451a8cd08f Mon Sep 17 00:00:00 2001 From: VOID404 Date: Fri, 13 Sep 2024 09:48:26 +0200 Subject: [PATCH 12/17] Support `Security.Networking.Filter.Egress.Enabled` --- internal/gardener/shoot/extender/network_filter.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/gardener/shoot/extender/network_filter.go b/internal/gardener/shoot/extender/network_filter.go index 1f07da72..40989f64 100644 --- a/internal/gardener/shoot/extender/network_filter.go +++ b/internal/gardener/shoot/extender/network_filter.go @@ -3,15 +3,16 @@ package extender import ( gardener "github.com/gardener/gardener/pkg/apis/core/v1beta1" imv1 "github.com/kyma-project/infrastructure-manager/api/v1" - "k8s.io/utils/ptr" ) const NetworkFilterType = "shoot-networking-filter" func ExtendWithNetworkFilter(runtime imv1.Runtime, shoot *gardener.Shoot) error { //nolint:revive + networkingFilter := gardener.Extension{ - Type: NetworkFilterType, - Disabled: ptr.To(false), + Type: NetworkFilterType, + // this pointer is safe, because runtime is fully pass-by-value + Disabled: &runtime.Spec.Security.Networking.Filter.Egress.Enabled, } shoot.Spec.Extensions = append(shoot.Spec.Extensions, networkingFilter) From 8161e84adfb7906eeffd4288be907387f2c983a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Drzewiecki?= Date: Fri, 13 Sep 2024 15:19:13 +0200 Subject: [PATCH 13/17] relies on Runtime CR label instead of Shoot CR --- internal/gardener/shoot/extender/oidc.go | 6 +++--- internal/gardener/shoot/extender/oidc_test.go | 7 ++++++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/internal/gardener/shoot/extender/oidc.go b/internal/gardener/shoot/extender/oidc.go index ae93b6d8..b776cf71 100644 --- a/internal/gardener/shoot/extender/oidc.go +++ b/internal/gardener/shoot/extender/oidc.go @@ -13,7 +13,7 @@ const ( func ExtendWithOIDC(runtime imv1.Runtime, shoot *gardener.Shoot) error { oidcConfig := runtime.Spec.Shoot.Kubernetes.KubeAPIServer.OidcConfig - if CanEnableExtension(shoot) { + if CanEnableExtension(runtime) { setOIDCExtension(shoot) } setKubeAPIServerOIDCConfig(shoot, oidcConfig) @@ -21,9 +21,9 @@ func ExtendWithOIDC(runtime imv1.Runtime, shoot *gardener.Shoot) error { return nil } -func CanEnableExtension(shoot *gardener.Shoot) bool { +func CanEnableExtension(runtime imv1.Runtime) bool { canEnable := true - createdByMigrator := shoot.Labels["operator.kyma-project.io/created-by-migrator"] + createdByMigrator := runtime.Labels["operator.kyma-project.io/created-by-migrator"] if createdByMigrator == "true" { canEnable = false diff --git a/internal/gardener/shoot/extender/oidc_test.go b/internal/gardener/shoot/extender/oidc_test.go index 941443b1..3fc0e683 100644 --- a/internal/gardener/shoot/extender/oidc_test.go +++ b/internal/gardener/shoot/extender/oidc_test.go @@ -1,6 +1,7 @@ package extender import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "testing" gardener "github.com/gardener/gardener/pkg/apis/core/v1beta1" @@ -40,8 +41,12 @@ func TestOidcExtender(t *testing.T) { usernameClaim := "sub" shoot := fixEmptyGardenerShoot("test", "kcp-system") - shoot.Labels[MigratorLabel] = testCase.migratorLabel[MigratorLabel] runtimeShoot := imv1.Runtime{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + MigratorLabel: testCase.migratorLabel[MigratorLabel], + }, + }, Spec: imv1.RuntimeSpec{ Shoot: imv1.RuntimeShoot{ Kubernetes: imv1.Kubernetes{ From e0e245be591feaf2e9aadfee40bb2f3229d7801c Mon Sep 17 00:00:00 2001 From: Rafal Foks Date: Fri, 13 Sep 2024 15:27:37 +0200 Subject: [PATCH 14/17] Apply review sugestions --- .../runtime/fsm/runtime_fsm_waiting_for_shoot_reconcile.go | 2 +- internal/controller/runtime/runtime_controller_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/controller/runtime/fsm/runtime_fsm_waiting_for_shoot_reconcile.go b/internal/controller/runtime/fsm/runtime_fsm_waiting_for_shoot_reconcile.go index a4a8fb5f..b70e09a2 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_waiting_for_shoot_reconcile.go +++ b/internal/controller/runtime/fsm/runtime_fsm_waiting_for_shoot_reconcile.go @@ -47,7 +47,7 @@ func sFnWaitForShootReconcile(_ context.Context, m *fsm, s *systemState) (stateF return ensureStatusConditionIsSetAndContinue( &s.instance, imv1.ConditionTypeRuntimeProvisioned, - imv1.ConditionReasonAuditLogConfigured, + imv1.ConditionReasonConfigurationCompleted, "Runtime processing completed successfully", sFnApplyClusterRoleBindings) } diff --git a/internal/controller/runtime/runtime_controller_test.go b/internal/controller/runtime/runtime_controller_test.go index ed6fe337..8811ef74 100644 --- a/internal/controller/runtime/runtime_controller_test.go +++ b/internal/controller/runtime/runtime_controller_test.go @@ -157,7 +157,7 @@ var _ = Describe("Runtime Controller", func() { return false } - if !runtime.IsConditionSet(imv1.ConditionTypeRuntimeProvisioned, imv1.ConditionReasonAuditLogConfigured) { + if !runtime.IsConditionSet(imv1.ConditionTypeRuntimeProvisioned, imv1.ConditionReasonConfigurationCompleted) { return false } From 60735db135209f9717132fcfbb184cd039e859f0 Mon Sep 17 00:00:00 2001 From: VOID404 Date: Mon, 16 Sep 2024 08:18:16 +0200 Subject: [PATCH 15/17] Disable static kubetoken --- internal/gardener/shoot/converter.go | 2 +- .../shoot/extender/kubernetes_version.go | 8 +++++++- .../shoot/extender/kubernetes_version_test.go | 18 ++++++++++++++++-- 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/internal/gardener/shoot/converter.go b/internal/gardener/shoot/converter.go index aa819a47..4e6a891b 100644 --- a/internal/gardener/shoot/converter.go +++ b/internal/gardener/shoot/converter.go @@ -74,7 +74,7 @@ func NewConverter(config ConverterConfig) Converter { extenders := []Extend{ extender.ExtendWithAnnotations, extender.ExtendWithLabels, - extender.NewKubernetesVersionExtender(config.Kubernetes.DefaultVersion), + extender.NewKubernetesExtender(config.Kubernetes.DefaultVersion), extender.NewProviderExtender(config.Provider.AWS.EnableIMDSv2, config.MachineImage.DefaultVersion), extender.NewDNSExtender(config.DNS.SecretName, config.DNS.DomainPrefix, config.DNS.ProviderType), extender.ExtendWithOIDC, diff --git a/internal/gardener/shoot/extender/kubernetes_version.go b/internal/gardener/shoot/extender/kubernetes_version.go index fd2c9ca4..91693f26 100644 --- a/internal/gardener/shoot/extender/kubernetes_version.go +++ b/internal/gardener/shoot/extender/kubernetes_version.go @@ -3,9 +3,14 @@ package extender import ( gardener "github.com/gardener/gardener/pkg/apis/core/v1beta1" imv1 "github.com/kyma-project/infrastructure-manager/api/v1" + "k8s.io/utils/ptr" ) -func NewKubernetesVersionExtender(defaultKubernetesVersion string) func(runtime imv1.Runtime, shoot *gardener.Shoot) error { +// NewKubernetesExtender creates a new Kubernetes extender function. +// It sets the Kubernetes version of the Shoot to the version specified in the Runtime. +// If the version is not specified in the Runtime, it sets the version to the `defaultKubernetesVersion`, set in `converter_config.json`. +// It sets the EnableStaticTokenKubeconfig field of the Shoot to false. +func NewKubernetesExtender(defaultKubernetesVersion string) func(runtime imv1.Runtime, shoot *gardener.Shoot) error { return func(runtime imv1.Runtime, shoot *gardener.Shoot) error { kubernetesVersion := runtime.Spec.Shoot.Kubernetes.Version if kubernetesVersion == nil || *kubernetesVersion == "" { @@ -13,6 +18,7 @@ func NewKubernetesVersionExtender(defaultKubernetesVersion string) func(runtime } shoot.Spec.Kubernetes.Version = *kubernetesVersion + shoot.Spec.Kubernetes.EnableStaticTokenKubeconfig = ptr.To(false) return nil } diff --git a/internal/gardener/shoot/extender/kubernetes_version_test.go b/internal/gardener/shoot/extender/kubernetes_version_test.go index 9de870e3..edbf99aa 100644 --- a/internal/gardener/shoot/extender/kubernetes_version_test.go +++ b/internal/gardener/shoot/extender/kubernetes_version_test.go @@ -16,7 +16,7 @@ func TestKubernetesVersionExtender(t *testing.T) { runtime := imv1.Runtime{} // when - kubernetesVersionExtender := NewKubernetesVersionExtender("1.99") + kubernetesVersionExtender := NewKubernetesExtender("1.99") err := kubernetesVersionExtender(runtime, &shoot) // then @@ -24,6 +24,20 @@ func TestKubernetesVersionExtender(t *testing.T) { assert.Equal(t, "1.99", shoot.Spec.Kubernetes.Version) }) + t.Run("Disable static token kubeconfig", func(t *testing.T) { + // given + shoot := fixEmptyGardenerShoot("test", "kcp-system") + runtime := imv1.Runtime{} + + // when + kubernetesVersionExtender := NewKubernetesExtender("1.99") + err := kubernetesVersionExtender(runtime, &shoot) + + // then + require.NoError(t, err) + assert.Equal(t, false, *shoot.Spec.Kubernetes.EnableStaticTokenKubeconfig) + }) + t.Run("Use version provided in the Runtime CR", func(t *testing.T) { // given shoot := fixEmptyGardenerShoot("test", "kcp-system") @@ -38,7 +52,7 @@ func TestKubernetesVersionExtender(t *testing.T) { } // when - kubernetesVersionExtender := NewKubernetesVersionExtender("1.99") + kubernetesVersionExtender := NewKubernetesExtender("1.99") err := kubernetesVersionExtender(runtime, &shoot) // then From 2d3b77aaa5be759474c4c4048b81b651a34263e4 Mon Sep 17 00:00:00 2001 From: VOID404 Date: Mon, 16 Sep 2024 08:19:11 +0200 Subject: [PATCH 16/17] Change kubernetes extender file names --- .../shoot/extender/{kubernetes_version.go => kubernetes.go} | 0 .../extender/{kubernetes_version_test.go => kubernetes_test.go} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename internal/gardener/shoot/extender/{kubernetes_version.go => kubernetes.go} (100%) rename internal/gardener/shoot/extender/{kubernetes_version_test.go => kubernetes_test.go} (100%) diff --git a/internal/gardener/shoot/extender/kubernetes_version.go b/internal/gardener/shoot/extender/kubernetes.go similarity index 100% rename from internal/gardener/shoot/extender/kubernetes_version.go rename to internal/gardener/shoot/extender/kubernetes.go diff --git a/internal/gardener/shoot/extender/kubernetes_version_test.go b/internal/gardener/shoot/extender/kubernetes_test.go similarity index 100% rename from internal/gardener/shoot/extender/kubernetes_version_test.go rename to internal/gardener/shoot/extender/kubernetes_test.go From 75d63d3a0b0f2f86f25489d4b14dc674221116eb Mon Sep 17 00:00:00 2001 From: VOID404 Date: Tue, 17 Sep 2024 09:07:51 +0200 Subject: [PATCH 17/17] Fix lint --- internal/gardener/shoot/extender/network_filter.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/gardener/shoot/extender/network_filter.go b/internal/gardener/shoot/extender/network_filter.go index 40989f64..d562342b 100644 --- a/internal/gardener/shoot/extender/network_filter.go +++ b/internal/gardener/shoot/extender/network_filter.go @@ -8,7 +8,6 @@ import ( const NetworkFilterType = "shoot-networking-filter" func ExtendWithNetworkFilter(runtime imv1.Runtime, shoot *gardener.Shoot) error { //nolint:revive - networkingFilter := gardener.Extension{ Type: NetworkFilterType, // this pointer is safe, because runtime is fully pass-by-value