From ac58a93b6285cd5ee7ec221e43df2cf70cdf1c0c Mon Sep 17 00:00:00 2001 From: m00g3n Date: Fri, 2 Aug 2024 09:13:04 +0200 Subject: [PATCH 1/9] Add kubeconfig access handling --- api/v1/runtime_types.go | 1 + cmd/main.go | 2 + .../runtime_fsm_apply_clusterrolebindings.go | 189 ++++++++++++++++++ .../fsm/runtime_fsm_create_kubeconfig.go | 2 +- .../runtime/fsm/runtime_fsm_patch_shoot.go | 2 +- .../runtime/fsm/runtime_fsm_process_shoot.go | 20 -- ...runtime_fsm_waiting_for_shoot_reconcile.go | 2 +- 7 files changed, 195 insertions(+), 23 deletions(-) create mode 100644 internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go delete mode 100644 internal/controller/runtime/fsm/runtime_fsm_process_shoot.go diff --git a/api/v1/runtime_types.go b/api/v1/runtime_types.go index 1e6af618..4079fcec 100644 --- a/api/v1/runtime_types.go +++ b/api/v1/runtime_types.go @@ -230,6 +230,7 @@ func (k *Runtime) UpdateStateDeletion(c RuntimeConditionType, r RuntimeCondition meta.SetStatusCondition(&k.Status.Conditions, condition) } +// FIXME: create update status for failed func (k *Runtime) UpdateStatePending(c RuntimeConditionType, r RuntimeConditionReason, status, msg string) { if status != "False" { k.Status.State = RuntimeStatePending diff --git a/cmd/main.go b/cmd/main.go index 352b6c93..c89048fc 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -35,6 +35,7 @@ import ( "github.com/kyma-project/infrastructure-manager/internal/gardener/kubeconfig" "github.com/kyma-project/infrastructure-manager/internal/gardener/shoot" "github.com/pkg/errors" + rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" @@ -54,6 +55,7 @@ var ( func init() { utilruntime.Must(clientgoscheme.AddToScheme(scheme)) utilruntime.Must(infrastructuremanagerv1.AddToScheme(scheme)) + utilruntime.Must(rbacv1.AddToScheme(scheme)) //+kubebuilder:scaffold:scheme } diff --git a/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go b/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go new file mode 100644 index 00000000..09f7a9b8 --- /dev/null +++ b/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go @@ -0,0 +1,189 @@ +package fsm + +import ( + "context" + "slices" + + authenticationv1alpha1 "github.com/gardener/gardener/pkg/apis/authentication/v1alpha1" + gardener_api "github.com/gardener/gardener/pkg/apis/core/v1beta1" + imv1 "github.com/kyma-project/infrastructure-manager/api/v1" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + + "k8s.io/client-go/tools/clientcmd" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +var ( + labelsClusterRoleBindings = map[string]string{ + "app": "kyma", + "reconciler.kyma-project.io/managed-by": "kim", + } +) + +func getShootClientWithAdmin(ctx context.Context, + adminKubeconfigClient client.SubResourceClient, + shoot *gardener_api.Shoot) (client.Client, error) { + // request for admin kubeconfig with low expiration timeout + var req authenticationv1alpha1.AdminKubeconfigRequest + if err := adminKubeconfigClient.Create(ctx, shoot, &req); err != nil { + return nil, err + } + + restConfig, err := clientcmd.RESTConfigFromKubeConfig(req.Status.Kubeconfig) + if err != nil { + return nil, err + } + + shootClientWithAdmin, err := client.New(restConfig, client.Options{}) + if err != nil { + return nil, err + } + + return shootClientWithAdmin, nil +} + +func isRBACUserKind(s rbacv1.Subject) bool { + return s.Kind == rbacv1.UserKind && + s.APIGroup == rbacv1.GroupName +} + +func getRemoved(crbs []rbacv1.ClusterRoleBinding, admins []string) (removed []rbacv1.ClusterRoleBinding) { + // iterate over cluster role bindings to find out removed administrators + for _, crb := range crbs { + if !labels.Set(crb.Labels).AsSelector().Matches(labels.Set(labelsClusterRoleBindings)) { + // cluster role binding is not controlled by KIM + continue + } + + index := slices.IndexFunc(crb.Subjects, isRBACUserKind) + if index < 0 { + // cluster role binding does not contain user subject + continue + } + + subjectUserName := crb.Subjects[index].Name + if slices.Contains(admins, subjectUserName) { + continue + } + // administrator was removed + removed = append(removed, crb) + } + + return removed +} + +var newContainsAdmin = func(admin string) func(rbacv1.ClusterRoleBinding) bool { + return func(r rbacv1.ClusterRoleBinding) bool { + for _, subject := range r.Subjects { + if !isRBACUserKind(subject) || subject.Name != admin { + continue + } + // admin found + return true + } + // admin not found in the slice + return false + } +} + +func getMissing(crbs []rbacv1.ClusterRoleBinding, admins []string) (missing []rbacv1.ClusterRoleBinding) { + for _, admin := range admins { + containsAdmin := newContainsAdmin(admin) + if slices.ContainsFunc(crbs, containsAdmin) { + continue + } + crb := toAdminClusterRoleBinding(admin) + missing = append(missing, crb) + } + + return missing +} + +func toAdminClusterRoleBinding(name string) rbacv1.ClusterRoleBinding { + return rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "admin-", + Labels: labelsClusterRoleBindings, + }, + Subjects: []rbacv1.Subject{{ + Kind: rbacv1.UserKind, + Name: name, + APIGroup: rbacv1.GroupName, + }}, + RoleRef: rbacv1.RoleRef{ + APIGroup: rbacv1.GroupName, + Kind: "ClusterRole", + Name: "cluster-admin", + }, + } +} + +var newDelCRBs = func(ctx context.Context, shootClient client.Client, crbs []rbacv1.ClusterRoleBinding) func() error { + return func() error { + for _, crb := range crbs { + if err := shootClient.Delete(ctx, &crb); err != nil { + return err + } + } + return nil + } +} + +var newAddCRBs = func(ctx context.Context, shootClient client.Client, crbs []rbacv1.ClusterRoleBinding) func() error { + return func() error { + for _, crb := range crbs { + if err := shootClient.Create(ctx, &crb); err != nil { + return err + } + } + return nil + } +} + +func updateCRBApplyFailed(rt *imv1.Runtime) { + rt.UpdateStatePending( + imv1.ConditionTypeRuntimeConfigured, + imv1.ConditionReasonConfigurationErr, + string(metav1.ConditionFalse), + "failed to update kubeconfig admin access", + ) +} + +func sFnApplyClusterRoleBindings(ctx context.Context, m *fsm, s *systemState) (stateFn, *ctrl.Result, error) { + // prepare subresource client to request admin kubeconfig + srscClient := m.ShootClient.SubResource("adminkubeconfig") + shootAdminClient, err := getShootClientWithAdmin(ctx, srscClient, s.shoot) + if err != nil { + updateCRBApplyFailed(&s.instance) + return updateStatusAndStopWithError(err) + } + // list existing cluster role bindings + var crbList rbacv1.ClusterRoleBindingList + if err := shootAdminClient.List(ctx, &crbList); err != nil { + updateCRBApplyFailed(&s.instance) + return updateStatusAndStopWithError(err) + } + + removed := getRemoved(crbList.Items, s.instance.Spec.Security.Administrators) + var missing []rbacv1.ClusterRoleBinding + + // FIXME add status check + if len(removed) == 0 && len(missing) == 0 { + stop() + } + + for _, fn := range []func() error{ + newDelCRBs(ctx, shootAdminClient, removed), + newAddCRBs(ctx, shootAdminClient, missing), + } { + if err := fn(); err != nil { + updateCRBApplyFailed(&s.instance) + return updateStatusAndStopWithError(err) + } + } + + return updateStatusAndRequeue() +} diff --git a/internal/controller/runtime/fsm/runtime_fsm_create_kubeconfig.go b/internal/controller/runtime/fsm/runtime_fsm_create_kubeconfig.go index cce4effe..fa11a312 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_create_kubeconfig.go +++ b/internal/controller/runtime/fsm/runtime_fsm_create_kubeconfig.go @@ -56,7 +56,7 @@ func sFnCreateKubeconfig(ctx context.Context, m *fsm, s *systemState) (stateFn, imv1.ConditionTypeRuntimeKubeconfigReady, imv1.ConditionReasonGardenerCRReady, "Gardener Cluster CR is ready.", - sFnProcessShoot) + sFnApplyClusterRoleBindings) } func makeGardenerClusterForRuntime(runtime imv1.Runtime, shoot *gardener.Shoot) *imv1.GardenerCluster { diff --git a/internal/controller/runtime/fsm/runtime_fsm_patch_shoot.go b/internal/controller/runtime/fsm/runtime_fsm_patch_shoot.go index 63fbcbe7..fb8e1e15 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_patch_shoot.go +++ b/internal/controller/runtime/fsm/runtime_fsm_patch_shoot.go @@ -35,7 +35,7 @@ func sFnPatchExistingShoot(ctx context.Context, m *fsm, s *systemState) (stateFn if updatedShoot.Generation == s.shoot.Generation { m.log.Info("Gardener shoot for runtime did not change after patch, moving to processing", "Name", s.shoot.Name, "Namespace", s.shoot.Namespace) - return switchState(sFnProcessShoot) + return switchState(sFnApplyClusterRoleBindings) } m.log.Info("Gardener shoot for runtime patched successfully", "Name", s.shoot.Name, "Namespace", s.shoot.Namespace) diff --git a/internal/controller/runtime/fsm/runtime_fsm_process_shoot.go b/internal/controller/runtime/fsm/runtime_fsm_process_shoot.go deleted file mode 100644 index 47956767..00000000 --- a/internal/controller/runtime/fsm/runtime_fsm_process_shoot.go +++ /dev/null @@ -1,20 +0,0 @@ -package fsm - -import ( - "context" - - imv1 "github.com/kyma-project/infrastructure-manager/api/v1" - ctrl "sigs.k8s.io/controller-runtime" -) - -func sFnProcessShoot(_ context.Context, m *fsm, s *systemState) (stateFn, *ctrl.Result, error) { - m.log.Info("Process cluster state - the last one") - - // process shoot get kubeconfig and create cluster role bindings - s.instance.UpdateStateReady( - imv1.ConditionTypeRuntimeProvisioned, - imv1.ConditionReasonConfigurationCompleted, - "Runtime processing completed successfully") - - return updateStatusAndStop() -} 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 ec79dcd6..f8c65e8a 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 @@ -44,7 +44,7 @@ func sFnWaitForShootReconcile(_ context.Context, m *fsm, s *systemState) (stateF case gardener.LastOperationStateSucceeded: m.log.Info(fmt.Sprintf("Shoot %s successfully updated, moving to processing", s.shoot.Name)) - return ensureStatusConditionIsSetAndContinue(&s.instance, imv1.ConditionTypeRuntimeProvisioned, imv1.ConditionReasonProcessing, "Shoot update is completed", sFnProcessShoot) + return ensureStatusConditionIsSetAndContinue(&s.instance, imv1.ConditionTypeRuntimeProvisioned, imv1.ConditionReasonProcessing, "Shoot update is completed", sFnApplyClusterRoleBindings) } m.log.Info("Update did not processed, exiting with no retry") From a7cc13649896b16853686bb3d028b95f8045053f Mon Sep 17 00:00:00 2001 From: m00g3n Date: Mon, 5 Aug 2024 12:46:52 +0200 Subject: [PATCH 2/9] fix getting cluster role bindings missing admins list --- .../runtime/fsm/runtime_fsm_apply_clusterrolebindings.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go b/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go index 09f7a9b8..9ce84954 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go +++ b/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go @@ -168,7 +168,7 @@ func sFnApplyClusterRoleBindings(ctx context.Context, m *fsm, s *systemState) (s } removed := getRemoved(crbList.Items, s.instance.Spec.Security.Administrators) - var missing []rbacv1.ClusterRoleBinding + missing := getMissing(crbList.Items, s.instance.Spec.Security.Administrators) // FIXME add status check if len(removed) == 0 && len(missing) == 0 { From e21b8abfff94ecc985a097a7962d758bd9067b59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Przemys=C5=82aw=20Golicz?= Date: Mon, 5 Aug 2024 13:00:18 +0200 Subject: [PATCH 3/9] Return fake shoot client for tests with dependency injection and Global package method --- .../runtime_fsm_apply_clusterrolebindings.go | 9 +++--- .../runtime/runtime_controller_test.go | 6 +++- internal/controller/runtime/suite_test.go | 31 +++++++++++++------ 3 files changed, 30 insertions(+), 16 deletions(-) diff --git a/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go b/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go index 9ce84954..6c5923ab 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go +++ b/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go @@ -10,7 +10,6 @@ import ( rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" - "k8s.io/client-go/tools/clientcmd" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -23,9 +22,9 @@ var ( } ) -func getShootClientWithAdmin(ctx context.Context, - adminKubeconfigClient client.SubResourceClient, - shoot *gardener_api.Shoot) (client.Client, error) { +//nolint:gochecknoglobals +var GetShootClient = func(ctx context.Context, + adminKubeconfigClient client.SubResourceClient, shoot *gardener_api.Shoot) (client.Client, error) { // request for admin kubeconfig with low expiration timeout var req authenticationv1alpha1.AdminKubeconfigRequest if err := adminKubeconfigClient.Create(ctx, shoot, &req); err != nil { @@ -155,7 +154,7 @@ func updateCRBApplyFailed(rt *imv1.Runtime) { func sFnApplyClusterRoleBindings(ctx context.Context, m *fsm, s *systemState) (stateFn, *ctrl.Result, error) { // prepare subresource client to request admin kubeconfig srscClient := m.ShootClient.SubResource("adminkubeconfig") - shootAdminClient, err := getShootClientWithAdmin(ctx, srscClient, s.shoot) + shootAdminClient, err := GetShootClient(ctx, srscClient, s.shoot) if err != nil { updateCRBApplyFailed(&s.instance) return updateStatusAndStopWithError(err) diff --git a/internal/controller/runtime/runtime_controller_test.go b/internal/controller/runtime/runtime_controller_test.go index 4015ee45..07997f0d 100644 --- a/internal/controller/runtime/runtime_controller_test.go +++ b/internal/controller/runtime/runtime_controller_test.go @@ -31,6 +31,7 @@ import ( ) var _ = Describe("Runtime Controller", func() { + Context("When reconciling a resource", func() { const ResourceName = "test-resource" @@ -257,7 +258,10 @@ func CreateRuntimeStub(resourceName string) *imv1.Runtime { }, }, Security: imv1.Security{ - Administrators: []string{}, + Administrators: []string{ + "test-admin1@sap.com", + "test-admin2@sap.com", + }, }, }, } diff --git a/internal/controller/runtime/suite_test.go b/internal/controller/runtime/suite_test.go index 9a4d9658..5670828d 100644 --- a/internal/controller/runtime/suite_test.go +++ b/internal/controller/runtime/suite_test.go @@ -18,17 +18,20 @@ package runtime import ( "context" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "path/filepath" "testing" "time" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + 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/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 + rbacv1 "k8s.io/api/rbac/v1" + //nolint:revive "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" @@ -49,14 +52,15 @@ import ( // http://onsi.github.io/ginkgo/ to learn more about Ginkgo. var ( - cfg *rest.Config //nolint:gochecknoglobals - k8sClient client.Client //nolint:gochecknoglobals - gardenerTestClient client.Client //nolint:gochecknoglobals - testEnv *envtest.Environment //nolint:gochecknoglobals - suiteCtx context.Context //nolint:gochecknoglobals - cancelSuiteCtx context.CancelFunc //nolint:gochecknoglobals - runtimeReconciler *RuntimeReconciler //nolint:gochecknoglobals - customTracker *CustomTracker //nolint:gochecknoglobals + cfg *rest.Config //nolint:gochecknoglobals + k8sClient client.Client //nolint:gochecknoglobals + k8sFakeClientRoleBindings client.Client //nolint:gochecknoglobals + gardenerTestClient client.Client //nolint:gochecknoglobals + testEnv *envtest.Environment //nolint:gochecknoglobals + suiteCtx context.Context //nolint:gochecknoglobals + cancelSuiteCtx context.CancelFunc //nolint:gochecknoglobals + runtimeReconciler *RuntimeReconciler //nolint:gochecknoglobals + customTracker *CustomTracker //nolint:gochecknoglobals ) func TestControllers(t *testing.T) { @@ -111,6 +115,14 @@ var _ = BeforeSuite(func() { Expect(err).NotTo(HaveOccurred()) Expect(k8sClient).NotTo(BeNil()) + shootClientScheme := runtime.NewScheme() + _ = rbacv1.AddToScheme(shootClientScheme) + k8sFakeClientRoleBindings = fake.NewClientBuilder().WithScheme(shootClientScheme).Build() + + fsm.GetShootClient = func(_ context.Context, _ client.SubResourceClient, _ *gardener_api.Shoot) (client.Client, error) { + return k8sFakeClientRoleBindings, nil + } + go func() { defer GinkgoRecover() suiteCtx, cancelSuiteCtx = context.WithCancel(context.Background()) @@ -152,7 +164,6 @@ func setupShootClientWithSequence(shoots []*gardener_api.Shoot) { tracker := clienttesting.NewObjectTracker(clientScheme, serializer.NewCodecFactory(clientScheme).UniversalDecoder()) customTracker = NewCustomTracker(tracker, shoots) gardenerTestClient = fake.NewClientBuilder().WithScheme(clientScheme).WithObjectTracker(customTracker).Build() - runtimeReconciler.UpdateShootClient(gardenerTestClient) } From 3f1bae4b71ae0ee61735dabee0c9c074184b973f Mon Sep 17 00:00:00 2001 From: m00g3n Date: Mon, 12 Aug 2024 09:24:31 +0200 Subject: [PATCH 4/9] update fsm and add tests --- api/v1/runtime_types.go | 7 + .../runtime_fsm_apply_clusterrolebindings.go | 72 +++--- .../runtime/fsm/runtime_fsm_apply_crb_test.go | 236 ++++++++++++++++++ .../fsm/runtime_fsm_create_kubeconfig.go | 14 +- .../runtime/fsm/runtime_fsm_create_shoot.go | 13 +- ...runtime_fsm_waiting_for_shoot_reconcile.go | 11 +- .../fsm/runtime_fsm_waiting_shoot_creation.go | 7 +- .../controller/runtime/fsm/utilz_for_test.go | 16 +- .../runtime/runtime_controller_test.go | 2 +- 9 files changed, 329 insertions(+), 49 deletions(-) create mode 100644 internal/controller/runtime/fsm/runtime_fsm_apply_crb_test.go diff --git a/api/v1/runtime_types.go b/api/v1/runtime_types.go index 4079fcec..70cb8781 100644 --- a/api/v1/runtime_types.go +++ b/api/v1/runtime_types.go @@ -280,6 +280,13 @@ func (k *Runtime) IsConditionSetWithStatus(c RuntimeConditionType, r RuntimeCond return false } +func (k *Runtime) IsConditionConfiguredSuccess() bool { + return k.IsConditionSetWithStatus( + ConditionTypeRuntimeConfigured, + ConditionReasonShootCreationCompleted, + metav1.ConditionTrue) +} + func (k *Runtime) ValidateRequiredLabels() error { var requiredLabelKeys = []string{ LabelKymaInstanceID, diff --git a/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go b/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go index 6c5923ab..1eda4e64 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go +++ b/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go @@ -22,6 +22,42 @@ var ( } ) +func sFnApplyClusterRoleBindings(ctx context.Context, m *fsm, s *systemState) (stateFn, *ctrl.Result, error) { + // prepare subresource client to request admin kubeconfig + srscClient := m.ShootClient.SubResource("adminkubeconfig") + shootAdminClient, err := GetShootClient(ctx, srscClient, s.shoot) + if err != nil { + updateCRBApplyFailed(&s.instance) + return updateStatusAndStopWithError(err) + } + // list existing cluster role bindings + var crbList rbacv1.ClusterRoleBindingList + if err := shootAdminClient.List(ctx, &crbList); err != nil { + updateCRBApplyFailed(&s.instance) + return updateStatusAndStopWithError(err) + } + + removed := getRemoved(crbList.Items, s.instance.Spec.Security.Administrators) + missing := getMissing(crbList.Items, s.instance.Spec.Security.Administrators) + + for _, fn := range []func() error{ + newDelCRBs(ctx, shootAdminClient, removed), + newAddCRBs(ctx, shootAdminClient, missing), + } { + if err := fn(); err != nil { + updateCRBApplyFailed(&s.instance) + return updateStatusAndStopWithError(err) + } + } + + s.instance.UpdateStateReady( + imv1.ConditionTypeRuntimeConfigured, + imv1.ConditionReasonConfigurationCompleted, + "kubeconfig admin access updated", + ) + return updateStatusAndStop() +} + //nolint:gochecknoglobals var GetShootClient = func(ctx context.Context, adminKubeconfigClient client.SubResourceClient, shoot *gardener_api.Shoot) (client.Client, error) { @@ -150,39 +186,3 @@ func updateCRBApplyFailed(rt *imv1.Runtime) { "failed to update kubeconfig admin access", ) } - -func sFnApplyClusterRoleBindings(ctx context.Context, m *fsm, s *systemState) (stateFn, *ctrl.Result, error) { - // prepare subresource client to request admin kubeconfig - srscClient := m.ShootClient.SubResource("adminkubeconfig") - shootAdminClient, err := GetShootClient(ctx, srscClient, s.shoot) - if err != nil { - updateCRBApplyFailed(&s.instance) - return updateStatusAndStopWithError(err) - } - // list existing cluster role bindings - var crbList rbacv1.ClusterRoleBindingList - if err := shootAdminClient.List(ctx, &crbList); err != nil { - updateCRBApplyFailed(&s.instance) - return updateStatusAndStopWithError(err) - } - - removed := getRemoved(crbList.Items, s.instance.Spec.Security.Administrators) - missing := getMissing(crbList.Items, s.instance.Spec.Security.Administrators) - - // FIXME add status check - if len(removed) == 0 && len(missing) == 0 { - stop() - } - - for _, fn := range []func() error{ - newDelCRBs(ctx, shootAdminClient, removed), - newAddCRBs(ctx, shootAdminClient, missing), - } { - if err := fn(); err != nil { - updateCRBApplyFailed(&s.instance) - return updateStatusAndStopWithError(err) - } - } - - return updateStatusAndRequeue() -} diff --git a/internal/controller/runtime/fsm/runtime_fsm_apply_crb_test.go b/internal/controller/runtime/fsm/runtime_fsm_apply_crb_test.go new file mode 100644 index 00000000..c42bbdab --- /dev/null +++ b/internal/controller/runtime/fsm/runtime_fsm_apply_crb_test.go @@ -0,0 +1,236 @@ +package fsm + +import ( + "context" + "fmt" + "time" + + gardener_api "github.com/gardener/gardener/pkg/apis/core/v1beta1" + imv1 "github.com/kyma-project/infrastructure-manager/api/v1" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" +) + +var _ = Describe(`runtime_fsm_apply_crb`, Label("applyCRB"), func() { + + var testErr = fmt.Errorf("test error") + + DescribeTable("isRBACUserKind", + func(s rbacv1.Subject, expected bool) { + actual := isRBACUserKind(s) + Expect(actual).To(Equal(expected)) + }, + Entry("shoud detect if subject is not user kind", rbacv1.Subject{}, false), + Entry("shoud detect if subject is from invalid group", + rbacv1.Subject{ + Kind: rbacv1.UserKind, + }, false), + Entry("shoud detect if subject user from valid group", + rbacv1.Subject{ + APIGroup: rbacv1.GroupName, + Kind: rbacv1.UserKind, + }, true), + ) + + DescribeTable("getMissing", + func(tc tcGetCRB) { + actual := getMissing(tc.crbs, tc.admins) + Expect(actual).To(BeComparableTo(tc.expected)) + }, + Entry("should return a list with CRBs to be created", tcGetCRB{ + admins: []string{"test1", "test2"}, + crbs: nil, + expected: []rbacv1.ClusterRoleBinding{ + toAdminClusterRoleBinding("test1"), + toAdminClusterRoleBinding("test2"), + }, + }), + Entry("should return nil list if no admins missing", tcGetCRB{ + admins: []string{"test1"}, + crbs: []rbacv1.ClusterRoleBinding{ + toAdminClusterRoleBinding("test1"), + }, + expected: nil, + }), + ) + + DescribeTable("getRemoved", + func(tc tcGetCRB) { + actual := getRemoved(tc.crbs, tc.admins) + Expect(actual).To(BeComparableTo(tc.expected)) + }, + Entry("should return nil list if CRB list is nil", tcGetCRB{ + admins: []string{"test1"}, + crbs: nil, + expected: nil, + }), + Entry("should return nil list if CRB list is empty", tcGetCRB{ + admins: []string{"test1"}, + crbs: []rbacv1.ClusterRoleBinding{}, + expected: nil, + }), + Entry("should return nil list if no admins to remove", tcGetCRB{ + admins: []string{"test1"}, + crbs: []rbacv1.ClusterRoleBinding{toAdminClusterRoleBinding("test1")}, + expected: nil, + }), + Entry("should return list if with CRBs to remove", tcGetCRB{ + admins: []string{"test2"}, + crbs: []rbacv1.ClusterRoleBinding{ + toAdminClusterRoleBinding("test1"), + toAdminClusterRoleBinding("test2"), + toAdminClusterRoleBinding("test3"), + }, + expected: []rbacv1.ClusterRoleBinding{ + toAdminClusterRoleBinding("test1"), + toAdminClusterRoleBinding("test3"), + }, + }), + ) + + testRuntime := imv1.Runtime{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testme1", + Namespace: "default", + }, + } + + testRuntimeWithAdmin := imv1.Runtime{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testme1", + Namespace: "default", + }, + Spec: imv1.RuntimeSpec{ + Security: imv1.Security{ + Administrators: []string{ + "test-admin1", + }, + }, + }, + } + + testScheme, err := newTestScheme() + Expect(err).ShouldNot(HaveOccurred()) + + defaultSetup := func(f *fsm) error { + GetShootClient = func( + _ context.Context, + _ client.SubResourceClient, + _ *gardener_api.Shoot) (client.Client, error) { + return f.Client, nil + } + return nil + } + + DescribeTable("sFnAppluClusterRoleBindings", + func(tc tcApplySfn) { + // initialize test data if required + Expect(tc.init()).ShouldNot(HaveOccurred()) + + ctx, cancel := context.WithTimeout(context.Background(), time.Second*10000) + defer cancel() + + actualResult, actualErr := tc.fsm.Run(ctx, tc.instance) + Expect(actualResult).Should(BeComparableTo(tc.expected.result)) + + matchErr := BeNil() + if tc.expected.err != nil { + matchErr = MatchError(tc.expected.err) + } + Expect(actualErr).Should(matchErr) + }, + + Entry("add admin", tcApplySfn{ + instance: testRuntimeWithAdmin, + expected: tcSfnExpected{ + err: nil, + }, + fsm: must( + newFakeFSM, + withFakedK8sClient(testScheme, &testRuntimeWithAdmin), + withFn(sFnApplyClusterRoleBindings), + withFakeEventRecorder(1), + ), + setup: defaultSetup, + }), + + Entry("nothing change", tcApplySfn{ + instance: testRuntime, + expected: tcSfnExpected{ + err: nil, + }, + fsm: must( + newFakeFSM, + withFakedK8sClient(testScheme, &testRuntime), + withFn(sFnApplyClusterRoleBindings), + withFakeEventRecorder(1), + ), + setup: defaultSetup, + }), + + Entry("error getting client", tcApplySfn{ + expected: tcSfnExpected{ + err: testErr, + }, + fsm: must( + newFakeFSM, + withFakedK8sClient(testScheme, &testRuntime), + withFn(sFnApplyClusterRoleBindings), + withFakeEventRecorder(1), + ), + setup: defaultSetup, + }), + ) +}) + +type tcGetCRB struct { + crbs []rbacv1.ClusterRoleBinding + admins []string + expected []rbacv1.ClusterRoleBinding +} + +type tcSfnExpected struct { + result ctrl.Result + err error +} + +type tcApplySfn struct { + expected tcSfnExpected + setup func(m *fsm) error + fsm *fsm + instance imv1.Runtime +} + +func (c *tcApplySfn) init() error { + if c.setup != nil { + return c.setup(c.fsm) + } + return nil +} + +func toCRBs(admins []string) (result []rbacv1.ClusterRoleBinding) { + for _, crb := range admins { + result = append(result, toAdminClusterRoleBinding(crb)) + } + return result +} + +func newTestScheme() (*runtime.Scheme, error) { + schema := runtime.NewScheme() + + for _, fn := range []func(*runtime.Scheme) error{ + imv1.AddToScheme, + rbacv1.AddToScheme, + } { + if err := fn(schema); err != nil { + return nil, err + } + } + return schema, nil +} diff --git a/internal/controller/runtime/fsm/runtime_fsm_create_kubeconfig.go b/internal/controller/runtime/fsm/runtime_fsm_create_kubeconfig.go index fa11a312..00c0fde7 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_create_kubeconfig.go +++ b/internal/controller/runtime/fsm/runtime_fsm_create_kubeconfig.go @@ -27,7 +27,12 @@ func sFnCreateKubeconfig(ctx context.Context, m *fsm, s *systemState) (stateFn, if err != nil { if !k8serrors.IsNotFound(err) { m.log.Error(err, "GardenerCluster CR read error", "name", runtimeID) - s.instance.UpdateStatePending(imv1.ConditionTypeRuntimeKubeconfigReady, imv1.ConditionReasonKubernetesAPIErr, "False", err.Error()) + s.instance.UpdateStatePending( + imv1.ConditionTypeRuntimeKubeconfigReady, + imv1.ConditionReasonKubernetesAPIErr, + "False", + err.Error(), + ) return updateStatusAndStop() } @@ -35,7 +40,12 @@ func sFnCreateKubeconfig(ctx context.Context, m *fsm, s *systemState) (stateFn, err = m.Create(ctx, makeGardenerClusterForRuntime(s.instance, s.shoot)) if err != nil { m.log.Error(err, "GardenerCluster CR create error", "name", runtimeID) - s.instance.UpdateStatePending(imv1.ConditionTypeRuntimeKubeconfigReady, imv1.ConditionReasonKubernetesAPIErr, "False", err.Error()) + s.instance.UpdateStatePending( + imv1.ConditionTypeRuntimeKubeconfigReady, + imv1.ConditionReasonKubernetesAPIErr, + "False", + err.Error(), + ) return updateStatusAndStop() } diff --git a/internal/controller/runtime/fsm/runtime_fsm_create_shoot.go b/internal/controller/runtime/fsm/runtime_fsm_create_shoot.go index 8256b893..7f6ea9b6 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_create_shoot.go +++ b/internal/controller/runtime/fsm/runtime_fsm_create_shoot.go @@ -13,7 +13,11 @@ func sFnCreateShoot(ctx context.Context, m *fsm, s *systemState) (stateFn, *ctrl newShoot, err := convertShoot(&s.instance, m.ConverterConfig) if err != nil { m.log.Error(err, "Failed to convert Runtime instance to shoot object") - return updateStatePendingWithErrorAndStop(&s.instance, imv1.ConditionTypeRuntimeProvisioned, imv1.ConditionReasonConversionError, "Runtime conversion error") + return updateStatePendingWithErrorAndStop( + &s.instance, + imv1.ConditionTypeRuntimeProvisioned, + imv1.ConditionReasonConversionError, + "Runtime conversion error") } err = m.ShootClient.Create(ctx, &newShoot) @@ -29,7 +33,12 @@ func sFnCreateShoot(ctx context.Context, m *fsm, s *systemState) (stateFn, *ctrl ) return updateStatusAndRequeueAfter(gardenerRequeueDuration) } - m.log.Info("Gardener shoot for runtime initialised successfully", "Name", newShoot.Name, "Namespace", newShoot.Namespace) + + m.log.Info( + "Gardener shoot for runtime initialised successfully", + "Name", newShoot.Name, + "Namespace", newShoot.Namespace, + ) s.instance.UpdateStatePending( imv1.ConditionTypeRuntimeProvisioned, 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 f8c65e8a..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 @@ -38,13 +38,18 @@ func sFnWaitForShootReconcile(_ context.Context, m *fsm, s *systemState) (stateF imv1.ConditionTypeRuntimeProvisioned, imv1.ConditionReasonProcessingErr, "False", - string(reason)) - + string(reason), + ) return updateStatusAndStop() case gardener.LastOperationStateSucceeded: m.log.Info(fmt.Sprintf("Shoot %s successfully updated, moving to processing", s.shoot.Name)) - return ensureStatusConditionIsSetAndContinue(&s.instance, imv1.ConditionTypeRuntimeProvisioned, imv1.ConditionReasonProcessing, "Shoot update is completed", sFnApplyClusterRoleBindings) + return ensureStatusConditionIsSetAndContinue( + &s.instance, + imv1.ConditionTypeRuntimeProvisioned, + imv1.ConditionReasonConfigurationCompleted, + "Runtime processing completed successfully", + sFnApplyClusterRoleBindings) } m.log.Info("Update did not processed, exiting with no retry") diff --git a/internal/controller/runtime/fsm/runtime_fsm_waiting_shoot_creation.go b/internal/controller/runtime/fsm/runtime_fsm_waiting_shoot_creation.go index 888c3b1e..ef654676 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_waiting_shoot_creation.go +++ b/internal/controller/runtime/fsm/runtime_fsm_waiting_shoot_creation.go @@ -65,7 +65,12 @@ func sFnWaitForShootCreation(_ context.Context, m *fsm, s *systemState) (stateFn case gardener.LastOperationStateSucceeded: m.log.Info(fmt.Sprintf("Shoot %s successfully created", s.shoot.Name)) - return ensureStatusConditionIsSetAndContinue(&s.instance, imv1.ConditionTypeRuntimeProvisioned, imv1.ConditionReasonShootCreationCompleted, "Shoot creation completed", sFnCreateKubeconfig) + return ensureStatusConditionIsSetAndContinue( + &s.instance, + imv1.ConditionTypeRuntimeProvisioned, + imv1.ConditionReasonShootCreationCompleted, + "Shoot creation completed", + sFnCreateKubeconfig) default: m.log.Info("Unknown shoot operation state, exiting with no retry") diff --git a/internal/controller/runtime/fsm/utilz_for_test.go b/internal/controller/runtime/fsm/utilz_for_test.go index 10239bf1..375d2a86 100644 --- a/internal/controller/runtime/fsm/utilz_for_test.go +++ b/internal/controller/runtime/fsm/utilz_for_test.go @@ -6,6 +6,7 @@ import ( . "github.com/onsi/gomega" //nolint:revive "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) @@ -50,22 +51,29 @@ var ( k8sClient := fake.NewClientBuilder(). WithScheme(scheme). WithObjects(objs...). + WithStatusSubresource(objs...). Build() return func(fsm *fsm) error { fsm.Client = k8sClient + fsm.ShootClient = k8sClient return nil } } - /* linter fix for unused code - withMockedShootClient = func(c *gardener_mocks.ShootClient) fakeFSMOpt { + withFn = func(fn stateFn) fakeFSMOpt { return func(fsm *fsm) error { - fsm.ShootClient = c + fsm.fn = fn + return nil + } + } + + withFakeEventRecorder = func(buffer int) fakeFSMOpt { + return func(fsm *fsm) error { + fsm.EventRecorder = record.NewFakeRecorder(buffer) return nil } } - */ ) func newFakeFSM(opts ...fakeFSMOpt) (*fsm, error) { diff --git a/internal/controller/runtime/runtime_controller_test.go b/internal/controller/runtime/runtime_controller_test.go index 07997f0d..b1aac381 100644 --- a/internal/controller/runtime/runtime_controller_test.go +++ b/internal/controller/runtime/runtime_controller_test.go @@ -106,7 +106,7 @@ var _ = Describe("Runtime Controller", func() { return false } - if !runtime.IsConditionSet(imv1.ConditionTypeRuntimeProvisioned, imv1.ConditionReasonConfigurationCompleted) { + if !runtime.IsConditionSet(imv1.ConditionTypeRuntimeConfigured, imv1.ConditionReasonConfigurationCompleted) { return false } From 19d451a929ea6a6235ebac00b10008aebe2a4c1c Mon Sep 17 00:00:00 2001 From: m00g3n Date: Mon, 12 Aug 2024 12:23:42 +0200 Subject: [PATCH 5/9] fix tests --- .../runtime/fsm/runtime_fsm_apply_crb_test.go | 13 ++++++++++++- .../fsm/runtime_fsm_create_kubeconfig_test.go | 5 +++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/internal/controller/runtime/fsm/runtime_fsm_apply_crb_test.go b/internal/controller/runtime/fsm/runtime_fsm_apply_crb_test.go index c42bbdab..23d1080d 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_apply_crb_test.go +++ b/internal/controller/runtime/fsm/runtime_fsm_apply_crb_test.go @@ -133,6 +133,7 @@ var _ = Describe(`runtime_fsm_apply_crb`, Label("applyCRB"), func() { // initialize test data if required Expect(tc.init()).ShouldNot(HaveOccurred()) + //TODO change timeout ctx, cancel := context.WithTimeout(context.Background(), time.Second*10000) defer cancel() @@ -175,6 +176,7 @@ var _ = Describe(`runtime_fsm_apply_crb`, Label("applyCRB"), func() { }), Entry("error getting client", tcApplySfn{ + instance: testRuntime, expected: tcSfnExpected{ err: testErr, }, @@ -184,7 +186,16 @@ var _ = Describe(`runtime_fsm_apply_crb`, Label("applyCRB"), func() { withFn(sFnApplyClusterRoleBindings), withFakeEventRecorder(1), ), - setup: defaultSetup, + setup: func(f *fsm) error { + GetShootClient = func( + _ context.Context, + _ client.SubResourceClient, + _ *gardener_api.Shoot) (client.Client, error) { + return nil, testErr + } + return nil + + }, }), ) }) diff --git a/internal/controller/runtime/fsm/runtime_fsm_create_kubeconfig_test.go b/internal/controller/runtime/fsm/runtime_fsm_create_kubeconfig_test.go index 43459124..e3478ae0 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_create_kubeconfig_test.go +++ b/internal/controller/runtime/fsm/runtime_fsm_create_kubeconfig_test.go @@ -3,9 +3,10 @@ package fsm import ( "context" "fmt" - "k8s.io/utils/ptr" "time" + "k8s.io/utils/ptr" + gardener "github.com/gardener/gardener/pkg/apis/core/v1beta1" imv1 "github.com/kyma-project/infrastructure-manager/api/v1" . "github.com/onsi/ginkgo/v2" //nolint:revive @@ -99,7 +100,7 @@ var _ = Describe("KIM sFnCreateKubeconfig", func() { &systemState{instance: *inputRtWithLabelsAndCondition, shoot: &testShoot}, testOpts{ MatchExpectedErr: BeNil(), - MatchNextFnState: haveName("sFnProcessShoot"), + MatchNextFnState: haveName("sFnApplyClusterRoleBindings"), }, ), Entry( From f0ce08ad66f6bfeacf0bf326a79ba40003c39291 Mon Sep 17 00:00:00 2001 From: m00g3n Date: Mon, 12 Aug 2024 12:35:03 +0200 Subject: [PATCH 6/9] fix linter findings --- api/v1/runtime_types.go | 1 - .../runtime/fsm/runtime_fsm_apply_clusterrolebindings.go | 4 ++++ internal/controller/runtime/fsm/runtime_fsm_apply_crb_test.go | 3 +-- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/api/v1/runtime_types.go b/api/v1/runtime_types.go index 70cb8781..e03cfb7d 100644 --- a/api/v1/runtime_types.go +++ b/api/v1/runtime_types.go @@ -230,7 +230,6 @@ func (k *Runtime) UpdateStateDeletion(c RuntimeConditionType, r RuntimeCondition meta.SetStatusCondition(&k.Status.Conditions, condition) } -// FIXME: create update status for failed func (k *Runtime) UpdateStatePending(c RuntimeConditionType, r RuntimeConditionReason, status, msg string) { if status != "False" { k.Status.State = RuntimeStatePending diff --git a/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go b/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go index 1eda4e64..f3b02970 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go +++ b/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go @@ -16,6 +16,7 @@ import ( ) var ( + //nolint:gochecknoglobals labelsClusterRoleBindings = map[string]string{ "app": "kyma", "reconciler.kyma-project.io/managed-by": "kim", @@ -110,6 +111,7 @@ func getRemoved(crbs []rbacv1.ClusterRoleBinding, admins []string) (removed []rb return removed } +//nolint:gochecknoglobals var newContainsAdmin = func(admin string) func(rbacv1.ClusterRoleBinding) bool { return func(r rbacv1.ClusterRoleBinding) bool { for _, subject := range r.Subjects { @@ -156,6 +158,7 @@ func toAdminClusterRoleBinding(name string) rbacv1.ClusterRoleBinding { } } +//nolint:gochecknoglobals var newDelCRBs = func(ctx context.Context, shootClient client.Client, crbs []rbacv1.ClusterRoleBinding) func() error { return func() error { for _, crb := range crbs { @@ -167,6 +170,7 @@ var newDelCRBs = func(ctx context.Context, shootClient client.Client, crbs []rba } } +//nolint:gochecknoglobals var newAddCRBs = func(ctx context.Context, shootClient client.Client, crbs []rbacv1.ClusterRoleBinding) func() error { return func() error { for _, crb := range crbs { diff --git a/internal/controller/runtime/fsm/runtime_fsm_apply_crb_test.go b/internal/controller/runtime/fsm/runtime_fsm_apply_crb_test.go index 23d1080d..da2f6855 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_apply_crb_test.go +++ b/internal/controller/runtime/fsm/runtime_fsm_apply_crb_test.go @@ -133,8 +133,7 @@ var _ = Describe(`runtime_fsm_apply_crb`, Label("applyCRB"), func() { // initialize test data if required Expect(tc.init()).ShouldNot(HaveOccurred()) - //TODO change timeout - ctx, cancel := context.WithTimeout(context.Background(), time.Second*10000) + ctx, cancel := context.WithTimeout(context.Background(), time.Second) defer cancel() actualResult, actualErr := tc.fsm.Run(ctx, tc.instance) From 6b8df6fa68d31e67fd13132463a552705e0eb154 Mon Sep 17 00:00:00 2001 From: marcin witalis <45931826+m00g3n@users.noreply.github.com> Date: Tue, 13 Aug 2024 09:45:39 +0200 Subject: [PATCH 7/9] Update internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go Co-authored-by: Przemyslaw Golicz --- .../runtime/fsm/runtime_fsm_apply_clusterrolebindings.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go b/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go index f3b02970..66635185 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go +++ b/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go @@ -19,7 +19,7 @@ var ( //nolint:gochecknoglobals labelsClusterRoleBindings = map[string]string{ "app": "kyma", - "reconciler.kyma-project.io/managed-by": "kim", + "reconciler.kyma-project.io/managed-by": "infrastructure-manager", } ) From e0c541d710075485e8e94813d28ad9187bf918f8 Mon Sep 17 00:00:00 2001 From: m00g3n Date: Tue, 13 Aug 2024 16:43:21 +0200 Subject: [PATCH 8/9] code review comments addressed --- .../runtime_fsm_apply_clusterrolebindings.go | 29 ++++++++----------- .../runtime/fsm/runtime_fsm_apply_crb_test.go | 17 ----------- 2 files changed, 12 insertions(+), 34 deletions(-) diff --git a/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go b/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go index 66635185..d10f0d1e 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go +++ b/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go @@ -81,9 +81,11 @@ var GetShootClient = func(ctx context.Context, return shootClientWithAdmin, nil } -func isRBACUserKind(s rbacv1.Subject) bool { - return s.Kind == rbacv1.UserKind && - s.APIGroup == rbacv1.GroupName +func isRBACUserKindOneOf(names []string) func(rbacv1.Subject) bool { + return func(s rbacv1.Subject) bool { + return s.Kind == rbacv1.UserKind && + slices.Contains(names, s.Name) + } } func getRemoved(crbs []rbacv1.ClusterRoleBinding, admins []string) (removed []rbacv1.ClusterRoleBinding) { @@ -94,16 +96,16 @@ func getRemoved(crbs []rbacv1.ClusterRoleBinding, admins []string) (removed []rb continue } - index := slices.IndexFunc(crb.Subjects, isRBACUserKind) - if index < 0 { - // cluster role binding does not contain user subject + if crb.RoleRef.Kind != "ClusterRole" && crb.RoleRef.Name != "cluster-admin" { continue } - subjectUserName := crb.Subjects[index].Name - if slices.Contains(admins, subjectUserName) { + index := slices.IndexFunc(crb.Subjects, isRBACUserKindOneOf(admins)) + if index >= 0 { + // cluster role binding does not contain user subject continue } + // administrator was removed removed = append(removed, crb) } @@ -114,15 +116,8 @@ func getRemoved(crbs []rbacv1.ClusterRoleBinding, admins []string) (removed []rb //nolint:gochecknoglobals var newContainsAdmin = func(admin string) func(rbacv1.ClusterRoleBinding) bool { return func(r rbacv1.ClusterRoleBinding) bool { - for _, subject := range r.Subjects { - if !isRBACUserKind(subject) || subject.Name != admin { - continue - } - // admin found - return true - } - // admin not found in the slice - return false + isAdmin := isRBACUserKindOneOf([]string{admin}) + return slices.ContainsFunc(r.Subjects, isAdmin) } } diff --git a/internal/controller/runtime/fsm/runtime_fsm_apply_crb_test.go b/internal/controller/runtime/fsm/runtime_fsm_apply_crb_test.go index da2f6855..a9be5c40 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_apply_crb_test.go +++ b/internal/controller/runtime/fsm/runtime_fsm_apply_crb_test.go @@ -21,23 +21,6 @@ var _ = Describe(`runtime_fsm_apply_crb`, Label("applyCRB"), func() { var testErr = fmt.Errorf("test error") - DescribeTable("isRBACUserKind", - func(s rbacv1.Subject, expected bool) { - actual := isRBACUserKind(s) - Expect(actual).To(Equal(expected)) - }, - Entry("shoud detect if subject is not user kind", rbacv1.Subject{}, false), - Entry("shoud detect if subject is from invalid group", - rbacv1.Subject{ - Kind: rbacv1.UserKind, - }, false), - Entry("shoud detect if subject user from valid group", - rbacv1.Subject{ - APIGroup: rbacv1.GroupName, - Kind: rbacv1.UserKind, - }, true), - ) - DescribeTable("getMissing", func(tc tcGetCRB) { actual := getMissing(tc.crbs, tc.admins) From 0ef59b3aeaa19e165aa9129653e2103dd8d53c6d Mon Sep 17 00:00:00 2001 From: m00g3n Date: Wed, 14 Aug 2024 07:40:03 +0200 Subject: [PATCH 9/9] add missing label check when looking for missing admin CRBs --- api/v1/runtime_types.go | 7 ------- .../runtime/fsm/runtime_fsm_apply_clusterrolebindings.go | 7 +++++-- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/api/v1/runtime_types.go b/api/v1/runtime_types.go index e03cfb7d..1e6af618 100644 --- a/api/v1/runtime_types.go +++ b/api/v1/runtime_types.go @@ -279,13 +279,6 @@ func (k *Runtime) IsConditionSetWithStatus(c RuntimeConditionType, r RuntimeCond return false } -func (k *Runtime) IsConditionConfiguredSuccess() bool { - return k.IsConditionSetWithStatus( - ConditionTypeRuntimeConfigured, - ConditionReasonShootCreationCompleted, - metav1.ConditionTrue) -} - func (k *Runtime) ValidateRequiredLabels() error { var requiredLabelKeys = []string{ LabelKymaInstanceID, diff --git a/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go b/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go index d10f0d1e..a0d995ff 100644 --- a/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go +++ b/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go @@ -115,9 +115,12 @@ func getRemoved(crbs []rbacv1.ClusterRoleBinding, admins []string) (removed []rb //nolint:gochecknoglobals var newContainsAdmin = func(admin string) func(rbacv1.ClusterRoleBinding) bool { - return func(r rbacv1.ClusterRoleBinding) bool { + return func(crb rbacv1.ClusterRoleBinding) bool { + if !labels.Set(crb.Labels).AsSelector().Matches(labels.Set(labelsClusterRoleBindings)) { + return false + } isAdmin := isRBACUserKindOneOf([]string{admin}) - return slices.ContainsFunc(r.Subjects, isAdmin) + return slices.ContainsFunc(crb.Subjects, isAdmin) } }