diff --git a/incubator/hnc/pkg/reconcilers/hnc_config.go b/incubator/hnc/pkg/reconcilers/hnc_config.go index 157320ef2..2e3a75dd3 100644 --- a/incubator/hnc/pkg/reconcilers/hnc_config.go +++ b/incubator/hnc/pkg/reconcilers/hnc_config.go @@ -5,6 +5,8 @@ import ( "fmt" "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/source" "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/api/errors" @@ -29,6 +31,11 @@ type ConfigReconciler struct { // Forest is the in-memory data structure that is shared with all other reconcilers. Forest *forest.Forest + // Affected is a channel of event.GenericEvent (see "Watching Channels" in + // https://book-v1.book.kubebuilder.io/beyond_basics/controller_watches.html) + // that is used to enqueue the singleton that needs reconciling. + Affected chan event.GenericEvent + // HierarchyConfigUpdates is a channel of events used to update hierarchy configuration changes performed by // ObjectReconcilers. It is passed on to ObjectReconcilers for the updates. The ConfigReconciler itself does // not use it. @@ -173,7 +180,23 @@ func (r *ConfigReconciler) createObjectReconciler(gvk schema.GroupVersionKind) e // SetupWithManager builds a controller with the reconciler. func (r *ConfigReconciler) SetupWithManager(mgr ctrl.Manager) error { + r.enqueueAffected(r.Log, "Enforce reconciliation to create a default"+ + "HNCConfiguration singleton if it does not exist.") return ctrl.NewControllerManagedBy(mgr). For(&api.HNCConfiguration{}). + Watches(&source.Channel{Source: r.Affected}, &handler.EnqueueRequestForObject{}). Complete(r) } + +// enqueueAffected enqueues the HNCConfiguration singleton for later reconciliation. +// This occurs in a goroutine so the caller doesn't block; since the reconciler +// is never garbage-collected, this is safe. +func (r *ConfigReconciler) enqueueAffected(log logr.Logger, reason string) { + go func() { + log.Info("Enqueuing for reconciliation", "reason", reason) + // The watch handler doesn't care about anything except the metadata. + inst := &api.HNCConfiguration{} + inst.ObjectMeta.Name = api.HNCConfigSingleton + r.Affected <- event.GenericEvent{Meta: inst} + }() +} diff --git a/incubator/hnc/pkg/reconcilers/hnc_config_test.go b/incubator/hnc/pkg/reconcilers/hnc_config_test.go index d0978bc5e..05eebcafb 100644 --- a/incubator/hnc/pkg/reconcilers/hnc_config_test.go +++ b/incubator/hnc/pkg/reconcilers/hnc_config_test.go @@ -8,6 +8,8 @@ import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + v1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ) @@ -22,6 +24,11 @@ var _ = Describe("HNCConfiguration", func() { BeforeEach(func() { fooName = createNS(ctx, "foo") barName = createNS(ctx, "bar") + + // Give foo a role. + makeRole(ctx, fooName, "foo-role") + // Give foo a role binding. + makeRoleBinding(ctx, fooName, "foo-role", "foo-admin", "foo-role-binding") }) AfterEach(func() { @@ -31,7 +38,28 @@ var _ = Describe("HNCConfiguration", func() { }).Should(Succeed()) }) - It("should propagate objects whose types have been added to HNCConfiguration", func() { + It("should set mode of Roles and RoleBindings as propagate by default", func() { + config := getHNCConfig(ctx) + + Eventually(hasTypeWithMode("rbac.authorization.k8s.io/v1", "Role", api.Propagate, config)).Should(BeTrue()) + Eventually(hasTypeWithMode("rbac.authorization.k8s.io/v1", "RoleBinding", api.Propagate, config)).Should(BeTrue()) + }) + + It("should propagate Roles by default", func() { + setParent(ctx, barName, fooName) + + Eventually(hasRole(ctx, barName, "foo-role")).Should(BeTrue()) + Expect(roleInheritedFrom(ctx, barName, "foo-role")).Should(Equal(fooName)) + }) + + It("should propagate RoleBindings by default", func() { + setParent(ctx, barName, fooName) + + Eventually(hasRoleBinding(ctx, barName, "foo-role-binding")).Should(BeTrue()) + Expect(roleBindingInheritedFrom(ctx, barName, "foo-role-binding")).Should(Equal(fooName)) + }) + + It("should only propagate objects whose types have been added to HNCConfiguration", func() { setParent(ctx, barName, fooName) makeSecret(ctx, fooName, "foo-sec") @@ -41,22 +69,19 @@ var _ = Describe("HNCConfiguration", func() { Eventually(hasSecret(ctx, fooName, "foo-sec")).Should(BeTrue()) // "foo-sec" is not propagated to bar because Secret hasn't been configured in HNCConfiguration. Eventually(hasSecret(ctx, barName, "foo-sec")).Should(BeFalse()) - - Eventually(func() error { - c := getHNCConfig(ctx) - return addSecretToHNCConfig(ctx, c) - }).Should(Succeed()) - - // "foo-sec" should now be propagated from foo to bar. - Eventually(hasSecret(ctx, barName, "foo-sec")).Should(BeTrue()) - Expect(secretInheritedFrom(ctx, barName, "foo-sec")).Should(Equal(fooName)) }) }) -func addSecretToHNCConfig(ctx context.Context, c *api.HNCConfiguration) error { - secSpec := api.TypeSynchronizationSpec{APIVersion: "v1", Kind: "Secret", Mode: api.Propagate} - c.Spec.Types = append(c.Spec.Types, secSpec) - return updateHNCConfig(ctx, c) +func hasTypeWithMode(apiVersion, kind string, mode api.SynchronizationMode, config *api.HNCConfiguration) func() bool { + // `Eventually` only works with a fn that doesn't take any args + return func() bool { + for _, t := range config.Spec.Types { + if t.APIVersion == apiVersion && t.Kind == kind && t.Mode == mode { + return true + } + } + return false + } } func makeSecret(ctx context.Context, nsName, secretName string) { @@ -76,16 +101,47 @@ func hasSecret(ctx context.Context, nsName, secretName string) func() bool { } } -func secretInheritedFrom(ctx context.Context, nsName, secretName string) string { - nnm := types.NamespacedName{Namespace: nsName, Name: secretName} - sec := &corev1.Secret{} - if err := k8sClient.Get(ctx, nnm, sec); err != nil { +func makeRoleBinding(ctx context.Context, nsName, roleName, userName, roleBindingName string) { + roleBinding := &v1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: roleBindingName, + Namespace: nsName, + }, + Subjects: []v1.Subject{ + { + Kind: "User", + Name: userName, + }, + }, + RoleRef: v1.RoleRef{ + APIGroup: "rbac.authorization.k8s.io", + Kind: "Role", + Name: roleName, + }, + } + ExpectWithOffset(1, k8sClient.Create(ctx, roleBinding)).Should(Succeed()) +} + +func hasRoleBinding(ctx context.Context, nsName, roleBindingName string) func() bool { + // `Eventually` only works with a fn that doesn't take any args + return func() bool { + nnm := types.NamespacedName{Namespace: nsName, Name: roleBindingName} + roleBinding := &v1.RoleBinding{} + err := k8sClient.Get(ctx, nnm, roleBinding) + return err == nil + } +} + +func roleBindingInheritedFrom(ctx context.Context, nsName, roleBindingName string) string { + nnm := types.NamespacedName{Namespace: nsName, Name: roleBindingName} + roleBinding := &v1.RoleBinding{} + if err := k8sClient.Get(ctx, nnm, roleBinding); err != nil { // should have been caught above return err.Error() } - if sec.ObjectMeta.Labels == nil { + if roleBinding.ObjectMeta.Labels == nil { return "" } - lif, _ := sec.ObjectMeta.Labels["hnc.x-k8s.io/inheritedFrom"] + lif, _ := roleBinding.ObjectMeta.Labels["hnc.x-k8s.io/inheritedFrom"] return lif } diff --git a/incubator/hnc/pkg/reconcilers/object_test.go b/incubator/hnc/pkg/reconcilers/object_test.go index 2ca1fbe89..194c9ab32 100644 --- a/incubator/hnc/pkg/reconcilers/object_test.go +++ b/incubator/hnc/pkg/reconcilers/object_test.go @@ -10,7 +10,6 @@ import ( corev1 "k8s.io/api/core/v1" v1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ) @@ -28,7 +27,7 @@ var _ = Describe("Secret", func() { barName = createNS(ctx, "bar") bazName = createNS(ctx, "baz") - // Give them each a secret + // Give them each a role. makeRole(ctx, fooName, "foo-role") makeRole(ctx, barName, "bar-role") makeRole(ctx, bazName, "baz-role") @@ -60,7 +59,7 @@ var _ = Describe("Secret", func() { // Creates an empty ConfigMap. We use ConfigMap for this test because the apiserver will not // add additional fields to an empty ConfigMap object to make it non-empty. makeConfigMap(ctx, fooName, "foo-config") - addConfigMapToHNCConfig(ctx) + addToHNCConfig(ctx, "v1", "ConfigMap", api.Propagate) // "foo-config" should now be propagated from foo to bar. Eventually(hasConfigMap(ctx, barName, "foo-config")).Should(BeTrue()) @@ -243,52 +242,6 @@ func newOrGetHierarchy(ctx context.Context, nm string) *api.HierarchyConfigurati return hier } -func makeRole(ctx context.Context, nsName, roleName string) { - role := &v1.Role{ - ObjectMeta: metav1.ObjectMeta{ - Name: roleName, - Namespace: nsName, - }, - TypeMeta: metav1.TypeMeta{ - Kind: "Role", - APIVersion: "rbac.authorization.k8s.io/v1", - }, - Rules: []v1.PolicyRule{ - // Allow the users to read all secrets, namespaces and configmaps. - { - APIGroups: []string{""}, - Resources: []string{"secrets", "namespaces", "configmaps"}, - Verbs: []string{"get", "watch", "list"}, - }, - }, - } - ExpectWithOffset(1, k8sClient.Create(ctx, role)).Should(Succeed()) -} - -func hasRole(ctx context.Context, nsName, roleName string) func() bool { - // `Eventually` only works with a fn that doesn't take any args - return func() bool { - nnm := types.NamespacedName{Namespace: nsName, Name: roleName} - role := &v1.Role{} - err := k8sClient.Get(ctx, nnm, role) - return err == nil - } -} - -func roleInheritedFrom(ctx context.Context, nsName, roleName string) string { - nnm := types.NamespacedName{Namespace: nsName, Name: roleName} - role := &v1.Role{} - if err := k8sClient.Get(ctx, nnm, role); err != nil { - // should have been caught above - return err.Error() - } - if role.ObjectMeta.Labels == nil { - return "" - } - lif, _ := role.ObjectMeta.Labels["hnc.x-k8s.io/inheritedFrom"] - return lif -} - func modifyRole(ctx context.Context, nsName, roleName string) { nnm := types.NamespacedName{Namespace: nsName, Name: roleName} role := &v1.Role{} @@ -332,15 +285,6 @@ func removeRole(ctx context.Context, nsName, roleName string) { ExpectWithOffset(1, k8sClient.Delete(ctx, role)).Should(Succeed()) } -func addConfigMapToHNCConfig(ctx context.Context) { - Eventually(func() error { - c := getHNCConfig(ctx) - configMap := api.TypeSynchronizationSpec{APIVersion: "v1", Kind: "ConfigMap", Mode: api.Propagate} - c.Spec.Types = append(c.Spec.Types, configMap) - return updateHNCConfig(ctx, c) - }).Should(Succeed()) -} - // Makes an empty ConfigMap object. func makeConfigMap(ctx context.Context, nsName, configMapName string) { configMap := &corev1.ConfigMap{} diff --git a/incubator/hnc/pkg/reconcilers/setup.go b/incubator/hnc/pkg/reconcilers/setup.go index 03d448021..df1ad8998 100644 --- a/incubator/hnc/pkg/reconcilers/setup.go +++ b/incubator/hnc/pkg/reconcilers/setup.go @@ -41,6 +41,7 @@ func Create(mgr ctrl.Manager, f *forest.Forest, maxReconciles int) error { Log: ctrl.Log.WithName("reconcilers").WithName("HNCConfiguration"), Manager: mgr, Forest: f, + Affected: make(chan event.GenericEvent), HierarchyConfigUpdates: hcChan, } if err := cr.SetupWithManager(mgr); err != nil { diff --git a/incubator/hnc/pkg/reconcilers/suite_test.go b/incubator/hnc/pkg/reconcilers/suite_test.go index 835013f8a..b41be3e7f 100644 --- a/incubator/hnc/pkg/reconcilers/suite_test.go +++ b/incubator/hnc/pkg/reconcilers/suite_test.go @@ -16,7 +16,6 @@ limitations under the License. package reconcilers_test import ( - "context" "path/filepath" "testing" "time" @@ -36,7 +35,6 @@ import ( // +kubebuilder:scaffold:imports api "github.com/kubernetes-sigs/multi-tenancy/incubator/hnc/api/v1alpha1" - "github.com/kubernetes-sigs/multi-tenancy/incubator/hnc/pkg/config" "github.com/kubernetes-sigs/multi-tenancy/incubator/hnc/pkg/forest" ) @@ -59,6 +57,8 @@ func TestAPIs(t *testing.T) { []Reporter{envtest.NewlineReporter{}}) } +// All tests in the reconcilers_test package are in one suite. As a result, they +// share the same test environment (e.g., same api server). var _ = BeforeSuite(func(done Done) { logf.SetLogger(zap.LoggerTo(GinkgoWriter, true)) @@ -92,12 +92,6 @@ var _ = BeforeSuite(func(done Done) { k8sClient = k8sManager.GetClient() Expect(k8sClient).ToNot(BeNil()) - // Setup HNCConfiguration object here because it is a cluster-wide singleton shared by all reconcilers. - hncConfig := newHNCConfig() - Expect(hncConfig).ToNot(BeNil()) - ctx := context.Background() - updateHNCConfig(ctx, hncConfig) - go func() { err = k8sManager.Start(ctrl.SetupSignalHandler()) Expect(err).ToNot(HaveOccurred()) @@ -111,10 +105,3 @@ var _ = AfterSuite(func() { err := testEnv.Stop() Expect(err).ToNot(HaveOccurred()) }) - -func newHNCConfig() *api.HNCConfiguration { - hncConfig := &api.HNCConfiguration{} - hncConfig.ObjectMeta.Name = api.HNCConfigSingleton - hncConfig.Spec = config.GetDefaultConfigSpec() - return hncConfig -} diff --git a/incubator/hnc/pkg/reconcilers/test_helpers_test.go b/incubator/hnc/pkg/reconcilers/test_helpers_test.go index e0e0ecf44..ac909a9b9 100644 --- a/incubator/hnc/pkg/reconcilers/test_helpers_test.go +++ b/incubator/hnc/pkg/reconcilers/test_helpers_test.go @@ -7,6 +7,8 @@ import ( . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + v1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" api "github.com/kubernetes-sigs/multi-tenancy/incubator/hnc/api/v1alpha1" @@ -93,3 +95,58 @@ func getHNCConfigWithOffset(offset int, ctx context.Context) *api.HNCConfigurati }).Should(Succeed()) return config } + +func addToHNCConfig(ctx context.Context, apiVersion, kind string, mode api.SynchronizationMode) { + Eventually(func() error { + c := getHNCConfig(ctx) + spec := api.TypeSynchronizationSpec{APIVersion: apiVersion, Kind: kind, Mode: mode} + c.Spec.Types = append(c.Spec.Types, spec) + return updateHNCConfig(ctx, c) + }).Should(Succeed()) +} + +func makeRole(ctx context.Context, nsName, roleName string) { + role := &v1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: roleName, + Namespace: nsName, + }, + TypeMeta: metav1.TypeMeta{ + Kind: "Role", + APIVersion: "rbac.authorization.k8s.io/v1", + }, + Rules: []v1.PolicyRule{ + // Allow the users to read all secrets, namespaces and configmaps. + { + APIGroups: []string{""}, + Resources: []string{"secrets", "namespaces", "configmaps"}, + Verbs: []string{"get", "watch", "list"}, + }, + }, + } + ExpectWithOffset(1, k8sClient.Create(ctx, role)).Should(Succeed()) +} + +func hasRole(ctx context.Context, nsName, roleName string) func() bool { + // `Eventually` only works with a fn that doesn't take any args + return func() bool { + nnm := types.NamespacedName{Namespace: nsName, Name: roleName} + role := &v1.Role{} + err := k8sClient.Get(ctx, nnm, role) + return err == nil + } +} + +func roleInheritedFrom(ctx context.Context, nsName, roleName string) string { + nnm := types.NamespacedName{Namespace: nsName, Name: roleName} + role := &v1.Role{} + if err := k8sClient.Get(ctx, nnm, role); err != nil { + // should have been caught above + return err.Error() + } + if role.ObjectMeta.Labels == nil { + return "" + } + lif, _ := role.ObjectMeta.Labels["hnc.x-k8s.io/inheritedFrom"] + return lif +}