diff --git a/cmd/milo/controller-manager/controllermanager.go b/cmd/milo/controller-manager/controllermanager.go index da06b760e..6c4c7e3a3 100644 --- a/cmd/milo/controller-manager/controllermanager.go +++ b/cmd/milo/controller-manager/controllermanager.go @@ -589,11 +589,8 @@ func Run(ctx context.Context, c *config.CompletedConfig, opts *Options) error { klog.FlushAndExit(klog.ExitFlushTimeout, 1) } - subjectBindingReaperCtrl := resourcemanagercontroller.SubjectBindingReaperController{ - Client: ctrl.GetClient(), - } - if err := subjectBindingReaperCtrl.SetupWithManager(ctrl); err != nil { - logger.Error(err, "Error setting up subject binding reaper controller") + if err := setupPolicyBindingLifecycle(ctrl); err != nil { + logger.Error(err, "Error setting up the policy binding lifecycle controllers") klog.FlushAndExit(klog.ExitFlushTimeout, 1) } diff --git a/cmd/milo/controller-manager/policybindings.go b/cmd/milo/controller-manager/policybindings.go new file mode 100644 index 000000000..494476cfd --- /dev/null +++ b/cmd/milo/controller-manager/policybindings.go @@ -0,0 +1,30 @@ +package app + +import ( + "fmt" + + controllerruntime "sigs.k8s.io/controller-runtime" + + iamcontroller "go.miloapis.com/milo/internal/controllers/iam" + resourcemanagercontroller "go.miloapis.com/milo/internal/controllers/resourcemanager" +) + +// setupPolicyBindingLifecycle sets up the controllers that end a PolicyBinding with what it names: the subject reaper +// deletes a binding whose User is gone, and the adopter makes a binding a dependent of its target, so the garbage +// collector deletes it with that target. +func setupPolicyBindingLifecycle(mgr controllerruntime.Manager) error { + reaper := resourcemanagercontroller.SubjectBindingReaperController{Client: mgr.GetClient()} + if err := reaper.SetupWithManager(mgr); err != nil { + return fmt.Errorf("subject binding reaper: %w", err) + } + adopter := iamcontroller.PolicyBindingAdopter{ + Client: mgr.GetClient(), + APIReader: mgr.GetAPIReader(), + Mapper: mgr.GetRESTMapper(), + Scheme: mgr.GetScheme(), + } + if err := adopter.SetupWithManager(mgr); err != nil { + return fmt.Errorf("policy binding adopter: %w", err) + } + return nil +} diff --git a/config/controller-manager/overlays/core-control-plane/rbac/role.yaml b/config/controller-manager/overlays/core-control-plane/rbac/role.yaml index c8a2013ff..a7dd1fe37 100644 --- a/config/controller-manager/overlays/core-control-plane/rbac/role.yaml +++ b/config/controller-manager/overlays/core-control-plane/rbac/role.yaml @@ -4,6 +4,13 @@ kind: ClusterRole metadata: name: milo-controller-manager rules: +- apiGroups: + - "" + resources: + - events + verbs: + - create + - patch - apiGroups: - "" resources: diff --git a/internal/controllers/garbagecollector/gctest/gctest.go b/internal/controllers/garbagecollector/gctest/gctest.go new file mode 100644 index 000000000..00597628d --- /dev/null +++ b/internal/controllers/garbagecollector/gctest/gctest.go @@ -0,0 +1,66 @@ +// Package gctest runs Milo's garbage collector against a test API server, so a test can prove what the collector +// deletes. It follows the setup of Kubernetes' own garbage collector integration tests +// (test/integration/garbagecollector/garbage_collector_test.go, setupWithServer). +package gctest + +import ( + "context" + "fmt" + "time" + + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/discovery/cached/memory" + "k8s.io/client-go/informers" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/metadata" + "k8s.io/client-go/metadata/metadatainformer" + "k8s.io/client-go/rest" + "k8s.io/client-go/restmapper" + "k8s.io/controller-manager/pkg/informerfactory" + + "go.miloapis.com/milo/internal/controllers/garbagecollector" +) + +// Options sets how the collector runs. The zero value runs it as Kubernetes' controller manager does by default. +type Options struct { + // Workers is the number of delete and orphan workers; 0 means 20, kube-controller-manager's --concurrent-gc-syncs. + Workers int + // SyncPeriod is how often the REST mapper is reset and discovery is read again; 0 means 5 s, as the upstream tests. + SyncPeriod time.Duration +} + +// Start runs the garbage collector against cfg until ctx is done. It returns once the collector is constructed; the +// collector's own first sync waits for its informers. +func Start(ctx context.Context, cfg *rest.Config, opts Options) error { + workers, period := opts.Workers, opts.SyncPeriod + if workers == 0 { + workers = 20 + } + if period == 0 { + period = 5 * time.Second + } + clientSet, err := kubernetes.NewForConfig(cfg) + if err != nil { + return fmt.Errorf("garbage collector clientset: %w", err) + } + metadataClient, err := metadata.NewForConfig(cfg) + if err != nil { + return fmt.Errorf("garbage collector metadata client: %w", err) + } + restMapper := restmapper.NewDeferredDiscoveryRESTMapper(memory.NewMemCacheClient(clientSet.Discovery())) + restMapper.Reset() + started := make(chan struct{}) + close(started) + gc, err := garbagecollector.NewGarbageCollector(ctx, clientSet, metadataClient, restMapper, + garbagecollector.DefaultIgnoredResources(), + informerfactory.NewInformerFactory(informers.NewSharedInformerFactory(clientSet, 0), + metadatainformer.NewSharedInformerFactory(metadataClient, 0)), + started) + if err != nil { + return fmt.Errorf("garbage collector: %w", err) + } + go wait.Until(restMapper.Reset, period, ctx.Done()) + go gc.Run(ctx, workers, period) + go gc.Sync(ctx, clientSet.Discovery(), period) + return nil +} diff --git a/internal/controllers/iam/policybinding_adopter.go b/internal/controllers/iam/policybinding_adopter.go new file mode 100644 index 000000000..bb7e51f2b --- /dev/null +++ b/internal/controllers/iam/policybinding_adopter.go @@ -0,0 +1,147 @@ +package iam + +import ( + "context" + "errors" + "time" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/tools/record" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/predicate" + + iamv1alpha1 "go.miloapis.com/milo/pkg/apis/iam/v1alpha1" +) + +const ( + // targetStoreWindow is how long after a binding's creation its target can still be stored. The API server stamps + // a new object's uid before admission, and the Project webhook writes the creator's binding inside that + // admission, so the binding can exist before its target. The target is stored within its create request's timeout + // (60 s, the API server's default) or never; one more timeout covers clock error between the API server and this + // controller and a write that commits after its request answered 504. + targetStoreWindow = 120 * time.Second + // targetPollInterval is how often an absent target is read again inside the window. + targetPollInterval = time.Second +) + +// errTargetNamespace is a resourceRef that names a namespaced kind without its namespace. +var errTargetNamespace = errors.New("resourceRef names a namespaced kind without a namespace") + +// PolicyBindingAdopter makes a PolicyBinding a dependent of the one object its resourceRef names, so the garbage +// collector deletes the binding with that object. +type PolicyBindingAdopter struct { + Client client.Client + // APIReader reads the target from the API server, not the cache, so a re-created target is never mistaken for + // the one the binding names. + APIReader client.Reader + // Mapper resolves the kind a resourceRef names, so any kind the API server serves can be a binding's owner. + Mapper meta.RESTMapper + Scheme *runtime.Scheme + EventRecorder record.EventRecorder + // now is the clock the target's creation window is measured with; nil means time.Now. + now func() time.Time +} + +// +kubebuilder:rbac:groups=iam.miloapis.com,resources=policybindings,verbs=get;list;watch;patch +// +kubebuilder:rbac:groups=iam.miloapis.com,resources=users;userpreferences;userinvitations,verbs=get +// +kubebuilder:rbac:groups=resourcemanager.miloapis.com,resources=projects;organizations;organizationmemberships,verbs=get +// +kubebuilder:rbac:groups="",resources=events,verbs=create;patch + +// Reconcile adds a binding's target as its owner once the target is stored with the uid the binding names. +func (r *PolicyBindingAdopter) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + var pb iamv1alpha1.PolicyBinding + if err := r.Client.Get(ctx, req.NamespacedName, &pb); err != nil || !adoptable(&pb) { + return ctrl.Result{}, client.IgnoreNotFound(err) + } + return r.adoptLiveTarget(ctx, &pb) +} + +// adoptable reports whether the binding names one target and has no owner. A binding with any owner is left alone: +// the garbage collector keeps a dependent while any owner exists, so a second owner would extend its life. +func adoptable(pb *iamv1alpha1.PolicyBinding) bool { + return pb.Spec.ResourceSelector.ResourceRef != nil && pb.DeletionTimestamp == nil && len(pb.OwnerReferences) == 0 +} + +func (r *PolicyBindingAdopter) adoptLiveTarget(ctx context.Context, pb *iamv1alpha1.PolicyBinding) (ctrl.Result, error) { + target, err := r.liveTarget(ctx, pb.Spec.ResourceSelector.ResourceRef) + switch { + case apierrors.IsNotFound(err) || meta.IsNoMatchError(err): + return r.pollWhileStorable(pb), nil + case errors.Is(err, errTargetNamespace): + r.EventRecorder.Event(pb, corev1.EventTypeWarning, "OwnerNotAllowed", err.Error()) + return ctrl.Result{}, nil + case err != nil: + return ctrl.Result{}, err + } + return ctrl.Result{}, r.adoptOnto(ctx, pb, target) +} + +// liveTarget reads the target's metadata from the API server, through the REST mapper, so any served kind works. +func (r *PolicyBindingAdopter) liveTarget(ctx context.Context, ref *iamv1alpha1.ResourceReference) (*metav1.PartialObjectMetadata, error) { + mapping, err := r.Mapper.RESTMapping(schema.GroupKind{Group: ref.APIGroup, Kind: ref.Kind}) + if err != nil { + return nil, err + } + key := client.ObjectKey{Name: ref.Name} + if mapping.Scope.Name() == meta.RESTScopeNameNamespace { + if ref.Namespace == "" { + return nil, errTargetNamespace + } + key.Namespace = ref.Namespace + } + target := &metav1.PartialObjectMetadata{} + target.SetGroupVersionKind(mapping.GroupVersionKind) + return target, r.APIReader.Get(ctx, key, target) +} + +// adoptOnto sets the target as the binding's owner when it is the object the binding names and is not being deleted. +// A deleting target is never adopted onto: an orphaning delete has just stripped its dependents' references. +func (r *PolicyBindingAdopter) adoptOnto(ctx context.Context, pb *iamv1alpha1.PolicyBinding, target *metav1.PartialObjectMetadata) error { + if string(target.GetUID()) != pb.Spec.ResourceSelector.ResourceRef.UID || target.GetDeletionTimestamp() != nil { + return nil + } + base := pb.DeepCopy() + if err := controllerutil.SetOwnerReference(target, pb, r.Scheme); err != nil { + r.EventRecorder.Event(pb, corev1.EventTypeWarning, "OwnerNotAllowed", err.Error()) + return nil + } + return r.Client.Patch(ctx, pb, client.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{})) +} + +// pollWhileStorable reads an absent target again while it can still be stored, and leaves the binding after that. +func (r *PolicyBindingAdopter) pollWhileStorable(pb *iamv1alpha1.PolicyBinding) ctrl.Result { + now := time.Now + if r.now != nil { + now = r.now + } + if now().Sub(pb.CreationTimestamp.Time) < targetStoreWindow { + return ctrl.Result{RequeueAfter: targetPollInterval} + } + return ctrl.Result{} +} + +// ownerReferencesChanged passes an update that changes the owner references, so a removed reference is set again. +var ownerReferencesChanged = predicate.Funcs{UpdateFunc: func(e event.UpdateEvent) bool { + return !equality.Semantic.DeepEqual(e.ObjectOld.GetOwnerReferences(), e.ObjectNew.GetOwnerReferences()) +}} + +// SetupWithManager sets up the controller with the Manager. A status-only update does not reconcile: the provider +// writes a binding's status on every reconcile of its own. +func (r *PolicyBindingAdopter) SetupWithManager(mgr ctrl.Manager) error { + r.EventRecorder = mgr.GetEventRecorderFor("policybinding-adopter") + return ctrl.NewControllerManagedBy(mgr). + For(&iamv1alpha1.PolicyBinding{}, + builder.WithPredicates(predicate.Or(predicate.GenerationChangedPredicate{}, ownerReferencesChanged))). + Named("policybinding-adopter"). + Complete(r) +} diff --git a/internal/controllers/iam/policybinding_adopter_envtest_test.go b/internal/controllers/iam/policybinding_adopter_envtest_test.go new file mode 100644 index 000000000..78f2a4baf --- /dev/null +++ b/internal/controllers/iam/policybinding_adopter_envtest_test.go @@ -0,0 +1,494 @@ +package iam + +import ( + "context" + "fmt" + "os" + "path/filepath" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/uuid" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/util/retry" + "k8s.io/utils/ptr" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + ctrlconfig "sigs.k8s.io/controller-runtime/pkg/config" + "sigs.k8s.io/controller-runtime/pkg/envtest" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" + + "go.miloapis.com/milo/internal/controllers/garbagecollector/gctest" + iamv1alpha1 "go.miloapis.com/milo/pkg/apis/iam/v1alpha1" + resourcemanagerv1alpha1 "go.miloapis.com/milo/pkg/apis/resourcemanager/v1alpha1" +) + +const ( + adopterNamespace = "organization-acme" + adopterOther = "organization-other" + // testHold keeps a deleted object in storage, deleting, until the test releases it. + testHold = "test.miloapis.com/hold" + // settle is how long a binding must keep its state to count as kept: longer than the garbage collector takes to + // delete a dependent whose owner is absent, which is milliseconds once its graph holds the dependent. + settle = 5 * time.Second +) + +type adopterEnv struct { + ctx context.Context + client client.Client // writes and reads through the API server, as a user would +} + +// TestPolicyBindingAdopter runs the adopter in a manager against an API server with Milo's iam and resourcemanager +// CRDs, with Milo's own garbage collector deleting dependents. Each case is one row of the deletion-path table in the +// design note for milo#845. +func TestPolicyBindingAdopter(t *testing.T) { + env := startAdopterEnv(t, nil) + + t.Run("D1 a binding on a Project is adopted and goes with its Project", func(t *testing.T) { + project := env.project(t, "d1") + pb := env.binding(t, "d1", env.refTo(project), nil) + env.waitOwnedBy(t, pb, project.UID) + require.Nil(t, pb.OwnerReferences[0].Controller, "the target is an owner, not the binding's controller") + require.Nil(t, pb.OwnerReferences[0].BlockOwnerDeletion, "the binding never holds its target's deletion") + require.NoError(t, env.client.Delete(env.ctx, project)) + env.waitGone(t, pb) + }) + + t.Run("D9 a binding on a namespaced object of a core kind in its namespace is adopted and goes with it", func(t *testing.T) { + cm := env.configMap(t, adopterNamespace, "d9") + pb := env.binding(t, "d9", iamv1alpha1.ResourceReference{ + Kind: "ConfigMap", Name: cm.Name, UID: string(cm.UID), Namespace: adopterNamespace, + }, nil) + env.waitOwnedBy(t, pb, cm.UID) + require.NoError(t, env.client.Delete(env.ctx, cm)) + env.waitGone(t, pb) + }) + + t.Run("D8 a namespaced target in another namespace is never an owner", func(t *testing.T) { + cm := env.configMap(t, adopterOther, "d8") + pb := env.binding(t, "d8", iamv1alpha1.ResourceReference{ + Kind: "ConfigMap", Name: cm.Name, UID: string(cm.UID), Namespace: adopterOther, + }, nil) + env.keepsOwners(t, pb, nil) + }) + + t.Run("D19 a status-only update does not reconcile: one Warning for a cross-namespace binding, none per status write", func(t *testing.T) { + cm := env.configMap(t, adopterOther, "d19") + pb := env.binding(t, "d19", iamv1alpha1.ResourceReference{ + Kind: "ConfigMap", Name: cm.Name, UID: string(cm.UID), Namespace: adopterOther, + }, nil) + env.eventually(t, func() bool { return env.warnings(t, pb) == 1 }, "one Warning for the create") + for i := range 3 { + env.writeStatus(t, pb, fmt.Sprintf("status write %d", i)) + } + time.Sleep(settle) + require.Equal(t, int32(1), env.warnings(t, pb), "no Warning for a status-only update") + }) + + t.Run("D4 a binding owned by another object is never adopted and goes with that object", func(t *testing.T) { + project := env.project(t, "d4") + holder := env.configMap(t, adopterNamespace, "d4-holder") + owner := metav1.OwnerReference{APIVersion: "v1", Kind: "ConfigMap", Name: holder.Name, UID: holder.UID} + pb := env.binding(t, "d4", env.refTo(project), []metav1.OwnerReference{owner}) + env.keepsOwners(t, pb, []types.UID{holder.UID}) + require.NoError(t, env.client.Delete(env.ctx, holder)) + env.waitGone(t, pb) // the Project lives on: a second owner would have kept a removed member's grant + }) + + t.Run("D5 a binding naming another uid of a live target is never adopted", func(t *testing.T) { + project := env.project(t, "d5") + ref := env.refTo(project) + ref.UID = string(uuid.NewUUID()) + pb := env.binding(t, "d5", ref, nil) + env.keepsOwners(t, pb, nil) + }) + + t.Run("D3 a binding naming a target never stored is never adopted and stays", func(t *testing.T) { + pb := env.binding(t, "d3", iamv1alpha1.ResourceReference{ + APIGroup: resourcemanagerv1alpha1.GroupVersion.Group, Kind: "Project", Name: "d3-never", UID: string(uuid.NewUUID()), + }, nil) + env.keepsOwners(t, pb, nil) + }) + + t.Run("D2 a binding on a target being deleted is never adopted, and stays once the target is gone", func(t *testing.T) { + project := env.project(t, "d2", testHold) + require.NoError(t, env.client.Delete(env.ctx, project)) + pb := env.binding(t, "d2", env.refTo(project), nil) + env.keepsOwners(t, pb, nil) + env.release(t, project) + env.keepsOwners(t, pb, nil) + }) + + t.Run("D6 an orphaning delete of the target is not undone", func(t *testing.T) { + project := env.project(t, "d6", testHold) + pb := env.binding(t, "d6", env.refTo(project), nil) + env.waitOwnedBy(t, pb, project.UID) + require.NoError(t, env.client.Delete(env.ctx, project, client.PropagationPolicy(metav1.DeletePropagationOrphan))) + env.waitOwners(t, pb, nil) // the garbage collector orphans it; the hold keeps the Project deleting + env.keepsOwners(t, pb, nil) + env.release(t, project) + env.keepsOwners(t, pb, nil) + }) + + t.Run("D7 a foreground delete of the target deletes the binding and does not wait on it", func(t *testing.T) { + project := env.project(t, "d7") + pb := env.binding(t, "d7", env.refTo(project), nil) + env.waitOwnedBy(t, pb, project.UID) + require.NoError(t, env.client.Delete(env.ctx, project, client.PropagationPolicy(metav1.DeletePropagationForeground))) + env.waitGone(t, pb) + env.waitGone(t, project) + }) + + t.Run("D11 an owner reference a person removes is added back", func(t *testing.T) { + project := env.project(t, "d11") + pb := env.binding(t, "d11", env.refTo(project), nil) + env.waitOwnedBy(t, pb, project.UID) + require.NoError(t, retry.RetryOnConflict(retry.DefaultRetry, func() error { + if err := env.client.Get(env.ctx, client.ObjectKeyFromObject(pb), pb); err != nil { + return err + } + pb.OwnerReferences = nil + return env.client.Update(env.ctx, pb) + })) + env.waitOwnedBy(t, pb, project.UID) + }) + + t.Run("D13 a subject edited while the binding is adopted keeps both changes", func(t *testing.T) { + project := env.project(t, "d13") + pb := env.binding(t, "d13", env.refTo(project), nil) + require.NoError(t, retry.RetryOnConflict(retry.DefaultRetry, func() error { + if err := env.client.Get(env.ctx, client.ObjectKeyFromObject(pb), pb); err != nil { + return err + } + pb.Spec.Subjects = append(pb.Spec.Subjects, iamv1alpha1.Subject{Kind: "User", Name: "d13-second", UID: string(uuid.NewUUID())}) + return env.client.Update(env.ctx, pb) + })) + env.waitOwnedBy(t, pb, project.UID) + require.Len(t, pb.Spec.Subjects, 2, "the subject edit survives the adoption") + }) + + t.Run("D21 a User's two self-manage bindings have one owner each and go with the User through its UserPreference", func(t *testing.T) { + user := &iamv1alpha1.User{ObjectMeta: metav1.ObjectMeta{Name: "d21"}, Spec: iamv1alpha1.UserSpec{Email: "d21@example.com"}} + require.NoError(t, env.client.Create(env.ctx, user)) + // The User controller makes the User the owner of its UserPreference; this test stands in for it. + pref := &iamv1alpha1.UserPreference{ + ObjectMeta: metav1.ObjectMeta{Name: "userpreference-d21", OwnerReferences: []metav1.OwnerReference{ + {APIVersion: iamv1alpha1.SchemeGroupVersion.String(), Kind: "User", Name: user.Name, UID: user.UID}, + }}, + Spec: iamv1alpha1.UserPreferenceSpec{UserRef: iamv1alpha1.UserReference{Name: user.Name}}, + } + require.NoError(t, env.client.Create(env.ctx, pref)) + selfManage := env.bindingIn(t, "milo-system", "user-self-manage-d21", iamv1alpha1.ResourceReference{ + APIGroup: iamv1alpha1.SchemeGroupVersion.Group, Kind: "User", Name: user.Name, UID: string(user.UID), + }) + prefSelfManage := env.bindingIn(t, "milo-system", "userpreference-self-manage-d21", iamv1alpha1.ResourceReference{ + APIGroup: iamv1alpha1.SchemeGroupVersion.Group, Kind: "UserPreference", Name: pref.Name, UID: string(pref.UID), + }) + env.waitOwnedBy(t, selfManage, user.UID) + env.waitOwnedBy(t, prefSelfManage, pref.UID) + require.NoError(t, env.client.Delete(env.ctx, user)) + env.waitGone(t, pref) + env.waitGone(t, selfManage) + env.waitGone(t, prefSelfManage) + }) + + t.Run("stress: a burst of 60 bindings, made serially and in parallel, are all adopted and none is deleted", func(t *testing.T) { + const n = 30 + var wg sync.WaitGroup + results := make(chan error, 2*n) + race := func(name string) { + defer wg.Done() + results <- env.projectThenBinding(name) + } + for i := range n { + wg.Add(1) + race(fmt.Sprintf("burst-serial-%d", i)) + } + for i := range n { + wg.Add(1) + go race(fmt.Sprintf("burst-parallel-%d", i)) + } + wg.Wait() + close(results) + for err := range results { + require.NoError(t, err) + } + time.Sleep(settle) + env.allOwnedByTheirProjects(t, "burst-") + }) +} + +// TestPolicyBindingAdopterAdoptsExistingBindings is row D15: bindings made before the adopter runs are adopted by its +// first list, and the garbage collector deletes none of them. +func TestPolicyBindingAdopterAdoptsExistingBindings(t *testing.T) { + const n = 50 + env := startAdopterEnv(t, func(env *adopterEnv) { + for i := range n { + project := env.project(t, fmt.Sprintf("d15-%d", i)) + env.binding(t, fmt.Sprintf("d15-%d", i), env.refTo(project), nil) + } + }) + env.eventually(t, func() bool { return env.countOwned(t, "d15-") == n }, "every existing binding is adopted") + time.Sleep(settle) + require.Equal(t, n, env.countOwned(t, "d15-"), "none deleted after adoption") +} + +// startAdopterEnv starts an API server with Milo's iam and resourcemanager CRDs, Milo's garbage collector, and a +// manager running the adopter. preload, when set, runs after the collector starts and before the manager does. +func startAdopterEnv(t *testing.T, preload func(*adopterEnv)) *adopterEnv { + t.Helper() + if os.Getenv("ADOPTER_ENV_LOG") != "" { + ctrl.SetLogger(zap.New(zap.UseDevMode(true), zap.WriteTo(os.Stderr))) + } + if os.Getenv("KUBEBUILDER_ASSETS") == "" { + if os.Getenv("CI") != "" { + t.Fatal("KUBEBUILDER_ASSETS must be set in CI; this test needs an envtest API server") + } + t.Skip("KUBEBUILDER_ASSETS is not set; run setup-envtest to provide an envtest API server") + } + root := filepath.Join("..", "..", "..") + testEnv := &envtest.Environment{ + CRDDirectoryPaths: []string{ + filepath.Join(root, "config", "crd", "bases", "iam"), + filepath.Join(root, "config", "crd", "bases", "resourcemanager"), + }, + ErrorIfCRDPathMissing: true, + } + cfg, err := testEnv.Start() + require.NoError(t, err) + t.Cleanup(func() { _ = testEnv.Stop() }) + + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, iamv1alpha1.AddToScheme(scheme)) + require.NoError(t, resourcemanagerv1alpha1.AddToScheme(scheme)) + direct, err := client.New(cfg, client.Options{Scheme: scheme}) + require.NoError(t, err) + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + require.NoError(t, gctest.Start(ctx, cfg, gctest.Options{})) + env := &adopterEnv{ctx: ctx, client: direct} + for _, ns := range []string{adopterNamespace, adopterOther, "milo-system"} { + require.NoError(t, direct.Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}})) + } + require.NoError(t, direct.Create(ctx, &resourcemanagerv1alpha1.Organization{ + ObjectMeta: metav1.ObjectMeta{Name: "acme"}, Spec: resourcemanagerv1alpha1.OrganizationSpec{Type: "Standard"}, + })) + if preload != nil { + preload(env) + } + + mgr, err := ctrl.NewManager(cfg, ctrl.Options{ + Scheme: scheme, + Metrics: metricsserver.Options{BindAddress: "0"}, + HealthProbeBindAddress: "0", + Controller: ctrlconfig.Controller{SkipNameValidation: ptr.To(true)}, + }) + require.NoError(t, err) + adopter := &PolicyBindingAdopter{Client: mgr.GetClient(), APIReader: mgr.GetAPIReader(), Mapper: mgr.GetRESTMapper(), Scheme: scheme} + require.NoError(t, adopter.SetupWithManager(mgr)) + go func() { _ = mgr.Start(ctx) }() + require.True(t, mgr.GetCache().WaitForCacheSync(ctx)) + return env +} + +func (e *adopterEnv) project(t *testing.T, name string, finalizers ...string) *resourcemanagerv1alpha1.Project { + t.Helper() + p := &resourcemanagerv1alpha1.Project{ + ObjectMeta: metav1.ObjectMeta{Name: name, Finalizers: finalizers}, + Spec: resourcemanagerv1alpha1.ProjectSpec{ + OwnerRef: resourcemanagerv1alpha1.OwnerReference{Kind: "Organization", Name: "acme"}, + }, + } + require.NoError(t, e.client.Create(e.ctx, p)) + return p +} + +func (e *adopterEnv) configMap(t *testing.T, namespace, name string) *corev1.ConfigMap { + t.Helper() + cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name}} + require.NoError(t, e.client.Create(e.ctx, cm)) + return cm +} + +func (e *adopterEnv) refTo(p *resourcemanagerv1alpha1.Project) iamv1alpha1.ResourceReference { + return iamv1alpha1.ResourceReference{ + APIGroup: resourcemanagerv1alpha1.GroupVersion.Group, Kind: "Project", Name: p.Name, UID: string(p.UID), + } +} + +func newBinding(name string, ref iamv1alpha1.ResourceReference, owners []metav1.OwnerReference) *iamv1alpha1.PolicyBinding { + return &iamv1alpha1.PolicyBinding{ + ObjectMeta: metav1.ObjectMeta{Namespace: adopterNamespace, Name: name, OwnerReferences: owners}, + Spec: iamv1alpha1.PolicyBindingSpec{ + RoleRef: iamv1alpha1.RoleReference{Name: "editor", Namespace: "milo-system"}, + Subjects: []iamv1alpha1.Subject{{Kind: "User", Name: name + "-user", UID: string(uuid.NewUUID())}}, + ResourceSelector: iamv1alpha1.ResourceSelector{ResourceRef: &ref}, + }, + } +} + +// bindingIn creates an unowned binding in a namespace other than the tests' organization namespace. +func (e *adopterEnv) bindingIn(t *testing.T, namespace, name string, ref iamv1alpha1.ResourceReference) *iamv1alpha1.PolicyBinding { + t.Helper() + pb := newBinding(name, ref, nil) + pb.Namespace = namespace + require.NoError(t, e.client.Create(e.ctx, pb)) + return pb +} + +func (e *adopterEnv) binding(t *testing.T, name string, ref iamv1alpha1.ResourceReference, owners []metav1.OwnerReference) *iamv1alpha1.PolicyBinding { + t.Helper() + pb := newBinding(name, ref, owners) + require.NoError(t, e.client.Create(e.ctx, pb)) + return pb +} + +// projectThenBinding writes a Project and, at once, a binding naming it. The webhook's order, the binding stored first, +// cannot be made here: only the API server knows a Project's uid before it is stored. That race is row D16, tested on +// the adopter's poll with a stubbed reader and on kind against the Project webhook. +func (e *adopterEnv) projectThenBinding(name string) error { + p := &resourcemanagerv1alpha1.Project{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: resourcemanagerv1alpha1.ProjectSpec{OwnerRef: resourcemanagerv1alpha1.OwnerReference{Kind: "Organization", Name: "acme"}}, + } + if err := e.client.Create(e.ctx, p); err != nil { + return err + } + return e.client.Create(e.ctx, newBinding(name, e.refTo(p), nil)) +} + +// writeStatus writes a condition to the binding's status, as the authorization provider does on its reconciles. +func (e *adopterEnv) writeStatus(t *testing.T, pb *iamv1alpha1.PolicyBinding, message string) { + t.Helper() + require.NoError(t, retry.RetryOnConflict(retry.DefaultRetry, func() error { + if err := e.client.Get(e.ctx, client.ObjectKeyFromObject(pb), pb); err != nil { + return err + } + meta.SetStatusCondition(&pb.Status.Conditions, metav1.Condition{ + Type: "Ready", Status: metav1.ConditionFalse, Reason: "Test", Message: message, + }) + return e.client.Status().Update(e.ctx, pb) + })) +} + +// warnings counts the adopter's OwnerNotAllowed Warnings on the binding; the recorder folds repeats into one Event's +// count. +func (e *adopterEnv) warnings(t *testing.T, pb *iamv1alpha1.PolicyBinding) int32 { + t.Helper() + var events corev1.EventList + require.NoError(t, e.client.List(e.ctx, &events, client.InNamespace(pb.Namespace))) + var count int32 + for _, ev := range events.Items { + if ev.InvolvedObject.Name == pb.Name && ev.Reason == "OwnerNotAllowed" { + count += max(ev.Count, 1) + } + } + return count +} + +func (e *adopterEnv) release(t *testing.T, obj client.Object) { + t.Helper() + require.NoError(t, retry.RetryOnConflict(retry.DefaultRetry, func() error { + if err := e.client.Get(e.ctx, client.ObjectKeyFromObject(obj), obj); err != nil { + return err + } + obj.SetFinalizers(nil) + return e.client.Update(e.ctx, obj) + })) + e.waitGone(t, obj) +} + +func ownerUIDs(pb *iamv1alpha1.PolicyBinding) []types.UID { + var uids []types.UID + for _, o := range pb.OwnerReferences { + uids = append(uids, o.UID) + } + return uids +} + +func (e *adopterEnv) eventually(t *testing.T, cond func() bool, msg string) { + t.Helper() + require.Eventually(t, cond, 30*time.Second, 100*time.Millisecond, msg) +} + +func (e *adopterEnv) waitOwnedBy(t *testing.T, pb *iamv1alpha1.PolicyBinding, uid types.UID) { + t.Helper() + e.waitOwners(t, pb, []types.UID{uid}) +} + +func (e *adopterEnv) waitOwners(t *testing.T, pb *iamv1alpha1.PolicyBinding, want []types.UID) { + t.Helper() + e.eventually(t, func() bool { + return e.client.Get(e.ctx, client.ObjectKeyFromObject(pb), pb) == nil && equalUIDs(ownerUIDs(pb), want) + }, fmt.Sprintf("%s's owners become %v", pb.Name, want)) +} + +// keepsOwners asserts that the binding exists with exactly these owners for the whole settle period. +func (e *adopterEnv) keepsOwners(t *testing.T, pb *iamv1alpha1.PolicyBinding, want []types.UID) { + t.Helper() + require.Never(t, func() bool { + err := e.client.Get(e.ctx, client.ObjectKeyFromObject(pb), pb) + return err != nil || !equalUIDs(ownerUIDs(pb), want) + }, settle, 100*time.Millisecond, "%s must stay, owned by %v", pb.Name, want) +} + +func (e *adopterEnv) waitGone(t *testing.T, obj client.Object) { + t.Helper() + e.eventually(t, func() bool { + return apierrors.IsNotFound(e.client.Get(e.ctx, client.ObjectKeyFromObject(obj), obj)) + }, fmt.Sprintf("%s is deleted", obj.GetName())) +} + +func (e *adopterEnv) countOwned(t *testing.T, prefix string) int { + t.Helper() + owned := 0 + for _, pb := range e.bindings(t, prefix) { + if len(pb.OwnerReferences) == 1 && string(pb.OwnerReferences[0].UID) == pb.Spec.ResourceSelector.ResourceRef.UID { + owned++ + } + } + return owned +} + +func (e *adopterEnv) allOwnedByTheirProjects(t *testing.T, prefix string) { + t.Helper() + bindings := e.bindings(t, prefix) + require.Len(t, bindings, 60, "no binding lost") + require.Equal(t, len(bindings), e.countOwned(t, prefix), "every binding owned by its Project") +} + +func (e *adopterEnv) bindings(t *testing.T, prefix string) []iamv1alpha1.PolicyBinding { + t.Helper() + var list iamv1alpha1.PolicyBindingList + require.NoError(t, e.client.List(e.ctx, &list, client.InNamespace(adopterNamespace))) + var out []iamv1alpha1.PolicyBinding + for _, pb := range list.Items { + if len(pb.Name) >= len(prefix) && pb.Name[:len(prefix)] == prefix { + out = append(out, pb) + } + } + return out +} + +func equalUIDs(got, want []types.UID) bool { + if len(got) != len(want) { + return false + } + for i := range got { + if got[i] != want[i] { + return false + } + } + return true +} diff --git a/internal/controllers/iam/policybinding_adopter_test.go b/internal/controllers/iam/policybinding_adopter_test.go new file mode 100644 index 000000000..8a67e7ccb --- /dev/null +++ b/internal/controllers/iam/policybinding_adopter_test.go @@ -0,0 +1,172 @@ +package iam + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/tools/record" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + iamv1alpha1 "go.miloapis.com/milo/pkg/apis/iam/v1alpha1" + resourcemanagerv1alpha1 "go.miloapis.com/milo/pkg/apis/resourcemanager/v1alpha1" +) + +// bindingCreated is the binding's creationTimestamp in these cases; the window is measured from it. +var bindingCreated = time.Date(2026, 10, 20, 18, 0, 0, 0, time.UTC) + +// pollCase is one reconcile of a binding whose target the reader answers for. +type pollCase struct { + age time.Duration // the adopter's clock, after the binding's creation + absentReads int // how many reads of the target answer NotFound before it is found + readErr error // an error every read of the target returns instead + unseenKinds int // how many mappings of the target's kind answer no match before the kind is known + beforePatch func(client.WithWatch) // runs once, just before the adopter's patch reaches the API server +} + +// unseenFirst is a REST mapper that has not yet seen the target's kind for its first misses lookups, as the shared +// DeferredDiscoveryRESTMapper answers until its next reset. +type unseenFirst struct { + meta.RESTMapper + misses int +} + +func (m *unseenFirst) RESTMapping(gk schema.GroupKind, versions ...string) (*meta.RESTMapping, error) { + if m.misses > 0 { + m.misses-- + return nil, &meta.NoKindMatchError{GroupKind: gk} + } + return m.RESTMapper.RESTMapping(gk, versions...) +} + +// TestPolicyBindingAdopterWaitsForTheTarget covers rule 4 of the design: a target that is absent when its binding is +// first seen is polled while it can still be stored (the request timeout and a margin, 120 s from the binding's +// creation), and left after that; any other read error is returned, and nothing is written. +func TestPolicyBindingAdopterWaitsForTheTarget(t *testing.T) { + t.Run("D16 absent at the first read, stored within the window: polled, then adopted", func(t *testing.T) { + r, c, project := pollEnv(t, pollCase{age: 10 * time.Second, absentReads: 1}) + res, err := r.Reconcile(context.Background(), bindingRequest()) + require.NoError(t, err) + require.Equal(t, time.Second, res.RequeueAfter, "an absent target inside the window is polled every second") + require.Empty(t, ownersOf(t, c)) + res, err = r.Reconcile(context.Background(), bindingRequest()) + require.NoError(t, err) + require.Zero(t, res.RequeueAfter) + require.Equal(t, []types.UID{project.UID}, ownersOf(t, c)) + }) + + t.Run("D20 a kind the mapper has not seen yet counts as absent: polled, then adopted", func(t *testing.T) { + r, c, project := pollEnv(t, pollCase{age: 10 * time.Second, unseenKinds: 1}) + res, err := r.Reconcile(context.Background(), bindingRequest()) + require.NoError(t, err) + require.Equal(t, time.Second, res.RequeueAfter) + require.Empty(t, ownersOf(t, c)) + _, err = r.Reconcile(context.Background(), bindingRequest()) + require.NoError(t, err) + require.Equal(t, []types.UID{project.UID}, ownersOf(t, c)) + }) + + t.Run("D13 an owner written between the read and the patch is kept: the patch conflicts, the retry leaves it", func(t *testing.T) { + other := metav1.OwnerReference{APIVersion: "v1", Kind: "ConfigMap", Name: "holder", UID: "holder-uid"} + r, c, _ := pollEnv(t, pollCase{age: 10 * time.Second, beforePatch: func(cl client.WithWatch) { + var pb iamv1alpha1.PolicyBinding + require.NoError(t, cl.Get(context.Background(), bindingRequest().NamespacedName, &pb)) + pb.OwnerReferences = append(pb.OwnerReferences, other) + require.NoError(t, cl.Update(context.Background(), &pb)) + }}) + _, err := r.Reconcile(context.Background(), bindingRequest()) + require.True(t, apierrors.IsConflict(err), "the stale patch conflicts: %v", err) + _, err = r.Reconcile(context.Background(), bindingRequest()) + require.NoError(t, err) + require.Equal(t, []types.UID{other.UID}, ownersOf(t, c), "the other owner stays, and no second owner is added") + }) + + t.Run("D3 absent 119 s after the binding's creation: still polled", func(t *testing.T) { + r, c, _ := pollEnv(t, pollCase{age: 119 * time.Second, absentReads: 1 << 20}) + res, err := r.Reconcile(context.Background(), bindingRequest()) + require.NoError(t, err) + require.Equal(t, time.Second, res.RequeueAfter) + require.Empty(t, ownersOf(t, c)) + }) + + t.Run("D3 absent 121 s after the binding's creation: left, not polled", func(t *testing.T) { + r, c, _ := pollEnv(t, pollCase{age: 121 * time.Second, absentReads: 1 << 20}) + res, err := r.Reconcile(context.Background(), bindingRequest()) + require.NoError(t, err) + require.Equal(t, ctrl.Result{}, res) + require.Empty(t, ownersOf(t, c)) + }) + + t.Run("D14 a failed read of the target is returned and writes nothing", func(t *testing.T) { + readErr := apierrors.NewInternalError(context.DeadlineExceeded) + r, c, _ := pollEnv(t, pollCase{age: 10 * time.Second, readErr: readErr}) + _, err := r.Reconcile(context.Background(), bindingRequest()) + require.Error(t, err) + require.Empty(t, ownersOf(t, c)) + }) +} + +// pollEnv returns an adopter over a fake API server holding one Project and one binding naming it. The adopter's +// reads of the target, as metadata through the API reader, answer as the case says. +func pollEnv(t *testing.T, pc pollCase) (*PolicyBindingAdopter, client.Client, *resourcemanagerv1alpha1.Project) { + t.Helper() + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, iamv1alpha1.AddToScheme(scheme)) + require.NoError(t, resourcemanagerv1alpha1.AddToScheme(scheme)) + project := &resourcemanagerv1alpha1.Project{ObjectMeta: metav1.ObjectMeta{Name: "p", UID: "project-uid"}} + pb := newBinding("pb", iamv1alpha1.ResourceReference{ + APIGroup: resourcemanagerv1alpha1.GroupVersion.Group, Kind: "Project", Name: project.Name, UID: string(project.UID), + }, nil) + pb.CreationTimestamp = metav1.NewTime(bindingCreated) + // The fake's default mapper calls every kind namespaced; a Project is cluster-scoped, as Milo serves it. + mapper := meta.NewDefaultRESTMapper([]schema.GroupVersion{resourcemanagerv1alpha1.GroupVersion, iamv1alpha1.SchemeGroupVersion}) + mapper.Add(resourcemanagerv1alpha1.GroupVersion.WithKind("Project"), meta.RESTScopeRoot) + mapper.Add(iamv1alpha1.SchemeGroupVersion.WithKind("PolicyBinding"), meta.RESTScopeNamespace) + reads, patched := 0, false + c := fake.NewClientBuilder().WithScheme(scheme).WithRESTMapper(mapper).WithObjects(project, pb). + WithInterceptorFuncs(interceptor.Funcs{Get: func(ctx context.Context, cl client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, isTarget := obj.(*metav1.PartialObjectMetadata); isTarget { + if pc.readErr != nil { + return pc.readErr + } + if reads++; reads <= pc.absentReads { + return apierrors.NewNotFound(schema.GroupResource{Group: resourcemanagerv1alpha1.GroupVersion.Group, Resource: "projects"}, key.Name) + } + } + return cl.Get(ctx, key, obj, opts...) + }, Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if pc.beforePatch != nil && !patched { + patched = true + pc.beforePatch(cl) + } + return cl.Patch(ctx, obj, patch, opts...) + }}).Build() + return &PolicyBindingAdopter{ + Client: c, APIReader: c, Mapper: &unseenFirst{RESTMapper: mapper, misses: pc.unseenKinds}, Scheme: scheme, + EventRecorder: record.NewFakeRecorder(10), + now: func() time.Time { return bindingCreated.Add(pc.age) }, + }, c, project +} + +func bindingRequest() ctrl.Request { + return ctrl.Request{NamespacedName: types.NamespacedName{Namespace: adopterNamespace, Name: "pb"}} +} + +func ownersOf(t *testing.T, c client.Client) []types.UID { + t.Helper() + var pb iamv1alpha1.PolicyBinding + require.NoError(t, c.Get(context.Background(), bindingRequest().NamespacedName, &pb)) + return ownerUIDs(&pb) +} diff --git a/internal/controllers/iam/user_controller.go b/internal/controllers/iam/user_controller.go index 7c1b56042..8f0e8eacf 100644 --- a/internal/controllers/iam/user_controller.go +++ b/internal/controllers/iam/user_controller.go @@ -33,7 +33,6 @@ type UserController struct { // +kubebuilder:rbac:groups=iam.miloapis.com,resources=users,verbs=get;list;watch;update // +kubebuilder:rbac:groups=iam.miloapis.com,resources=users/status,verbs=update -// +kubebuilder:rbac:groups=iam.miloapis.com,resources=policybindings,verbs=get;list;watch;update;patch // +kubebuilder:rbac:groups=iam.miloapis.com,resources=userpreferences,verbs=get;list;watch;update;patch // +kubebuilder:rbac:groups=iam.miloapis.com,resources=platformaccesses,verbs=get;list;watch // +kubebuilder:rbac:groups=resourcemanager.miloapis.com,resources=organizationmemberships,verbs=list;delete @@ -77,7 +76,7 @@ func (r *UserController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. } } - // Ensure owner references are set on PolicyBinding and UserPreference resources + // Make the User the owner of its UserPreference if err := r.ensureOwnerReferences(ctx, user); err != nil { log.Error(err, "Failed to ensure owner references") return ctrl.Result{}, err @@ -159,11 +158,12 @@ func (r *UserController) reconcileNameReviewAnnotation(ctx context.Context, user return nil } -// ensureOwnerReferences ensures that PolicyBinding and UserPreference resources have proper owner references +// ensureOwnerReferences makes the User the owner of its UserPreference, so the garbage collector deletes the +// preference with the User. The User's self-manage PolicyBindings get their owner from the PolicyBinding adopter, which +// makes each binding a dependent of the object it targets. func (r *UserController) ensureOwnerReferences(ctx context.Context, user *iamv1alpha1.User) error { log := log.FromContext(ctx).WithName("ensure-owner-references") - // Create owner reference for the user ownerRef := metav1.OwnerReference{ APIVersion: iamv1alpha1.SchemeGroupVersion.String(), Kind: "User", @@ -171,27 +171,9 @@ func (r *UserController) ensureOwnerReferences(ctx context.Context, user *iamv1a UID: user.UID, } - // Update PolicyBinding for user self-management - policyBindingName := fmt.Sprintf("user-self-manage-%s", user.Name) - policyBinding := &iamv1alpha1.PolicyBinding{} - err := r.Client.Get(ctx, types.NamespacedName{Name: policyBindingName, Namespace: "milo-system"}, policyBinding) - if apierrors.IsNotFound(err) { - // PolicyBinding doesn't exist, webhook should have created it - log.Info("PolicyBinding not found, skipping (webhook should create it)", "user", user.Name, "policyBinding", policyBindingName) - } else if err != nil { - return fmt.Errorf("failed to get policy binding: %w", err) - } else if !hasOwnerReference(policyBinding.OwnerReferences, ownerRef) { - policyBinding.OwnerReferences = append(policyBinding.OwnerReferences, ownerRef) - if err := r.Client.Update(ctx, policyBinding); err != nil { - return fmt.Errorf("failed to update policy binding with owner reference: %w", err) - } - log.Info("Updated PolicyBinding with owner reference", "user", user.Name) - } - - // Update UserPreference userPreferenceName := fmt.Sprintf("userpreference-%s", user.Name) userPreference := &iamv1alpha1.UserPreference{} - err = r.Client.Get(ctx, types.NamespacedName{Name: userPreferenceName}, userPreference) + err := r.Client.Get(ctx, types.NamespacedName{Name: userPreferenceName}, userPreference) if apierrors.IsNotFound(err) { // UserPreference doesn't exist, webhook should have created it log.Info("UserPreference not found, skipping (webhook should create it)", "user", user.Name, "userPreference", userPreferenceName) @@ -205,23 +187,6 @@ func (r *UserController) ensureOwnerReferences(ctx context.Context, user *iamv1a log.Info("Updated UserPreference with owner reference", "user", user.Name) } - // Update UserPreference PolicyBinding for user preference management - userPreferencePolicyBindingName := fmt.Sprintf("userpreference-self-manage-%s", user.Name) - userPreferencePolicyBinding := &iamv1alpha1.PolicyBinding{} - err = r.Client.Get(ctx, types.NamespacedName{Name: userPreferencePolicyBindingName, Namespace: "milo-system"}, userPreferencePolicyBinding) - if apierrors.IsNotFound(err) { - // UserPreference PolicyBinding doesn't exist, webhook should have created it - log.Info("UserPreference PolicyBinding not found, skipping (webhook should create it)", "user", user.Name, "policyBinding", userPreferencePolicyBindingName) - } else if err != nil { - return fmt.Errorf("failed to get user preference policy binding: %w", err) - } else if !hasOwnerReference(userPreferencePolicyBinding.OwnerReferences, ownerRef) { - userPreferencePolicyBinding.OwnerReferences = append(userPreferencePolicyBinding.OwnerReferences, ownerRef) - if err := r.Client.Update(ctx, userPreferencePolicyBinding); err != nil { - return fmt.Errorf("failed to update user preference policy binding with owner reference: %w", err) - } - log.Info("Updated UserPreference PolicyBinding with owner reference", "user", user.Name) - } - return nil } diff --git a/internal/controllers/iam/user_owner_references_test.go b/internal/controllers/iam/user_owner_references_test.go new file mode 100644 index 000000000..236daa2d9 --- /dev/null +++ b/internal/controllers/iam/user_owner_references_test.go @@ -0,0 +1,49 @@ +package iam + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + iamv1alpha1 "go.miloapis.com/milo/pkg/apis/iam/v1alpha1" +) + +// TestEnsureOwnerReferencesLeavesBindingsToTheAdopter: the User controller makes the User the owner of its +// UserPreference and writes no owner on the User's self-manage bindings. The PolicyBinding adopter is their one writer, +// so a binding's owner does not depend on which controller runs first. +func TestEnsureOwnerReferencesLeavesBindingsToTheAdopter(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, iamv1alpha1.AddToScheme(scheme)) + user := &iamv1alpha1.User{ObjectMeta: metav1.ObjectMeta{Name: "u", UID: "user-uid"}} + pref := &iamv1alpha1.UserPreference{ObjectMeta: metav1.ObjectMeta{Name: "userpreference-u"}} + selfManage := selfManageBinding("user-self-manage-u") + prefSelfManage := selfManageBinding("userpreference-self-manage-u") + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(user, pref, selfManage, prefSelfManage).Build() + + require.NoError(t, (&UserController{Client: c}).ensureOwnerReferences(context.Background(), user)) + + require.NoError(t, c.Get(context.Background(), client.ObjectKeyFromObject(pref), pref)) + require.Equal(t, []types.UID{user.UID}, preferenceOwners(pref), "the User owns its UserPreference") + for _, pb := range []*iamv1alpha1.PolicyBinding{selfManage, prefSelfManage} { + require.NoError(t, c.Get(context.Background(), client.ObjectKeyFromObject(pb), pb)) + require.Empty(t, pb.OwnerReferences, "%s's owner is the adopter's to write", pb.Name) + } +} + +func selfManageBinding(name string) *iamv1alpha1.PolicyBinding { + return &iamv1alpha1.PolicyBinding{ObjectMeta: metav1.ObjectMeta{Namespace: "milo-system", Name: name}} +} + +func preferenceOwners(pref *iamv1alpha1.UserPreference) []types.UID { + var uids []types.UID + for _, o := range pref.OwnerReferences { + uids = append(uids, o.UID) + } + return uids +} diff --git a/internal/webhooks/resourcemanager/v1alpha1/project_webhook.go b/internal/webhooks/resourcemanager/v1alpha1/project_webhook.go index 649547dbe..8ea47f00d 100644 --- a/internal/webhooks/resourcemanager/v1alpha1/project_webhook.go +++ b/internal/webhooks/resourcemanager/v1alpha1/project_webhook.go @@ -231,18 +231,12 @@ func (v *ProjectValidator) createOwnerPolicyBinding(ctx context.Context, project Namespace: fmt.Sprintf("organization-%s", project.Spec.OwnerRef.Name), Labels: map[string]string{ // Lets the subject-binding reaper controller find this binding - // when foundUser is deleted; the OwnerReference below only - // covers cleanup when the Project itself is deleted. + // when foundUser is deleted. v1alpha1.SubjectUserNameLabel: foundUser.Name, }, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: v1alpha1.GroupVersion.String(), - Kind: "Project", - Name: project.Name, - UID: project.UID, - }, - }, + // No owner reference: the Project is not stored yet, so the garbage + // collector would find no owner and delete the binding. The + // PolicyBinding adopter sets it once the Project is stored. }, Spec: iamv1alpha1.PolicyBindingSpec{ RoleRef: iamv1alpha1.RoleReference{ diff --git a/test/iam/user-deletion-garbage-collection/assertions/user-preference-policy-binding-with-owner-ref.yaml b/test/iam/user-deletion-garbage-collection/assertions/user-preference-policy-binding-with-owner-ref.yaml index 15bf307f8..635baf4d3 100644 --- a/test/iam/user-deletion-garbage-collection/assertions/user-preference-policy-binding-with-owner-ref.yaml +++ b/test/iam/user-deletion-garbage-collection/assertions/user-preference-policy-binding-with-owner-ref.yaml @@ -5,5 +5,5 @@ metadata: namespace: milo-system ownerReferences: - apiVersion: iam.miloapis.com/v1alpha1 - kind: User - name: test-gc-user \ No newline at end of file + kind: UserPreference + name: userpreference-test-gc-user diff --git a/test/iam/user-deletion-garbage-collection/chainsaw-test.yaml b/test/iam/user-deletion-garbage-collection/chainsaw-test.yaml index 6b19f0ebe..73ed84415 100644 --- a/test/iam/user-deletion-garbage-collection/chainsaw-test.yaml +++ b/test/iam/user-deletion-garbage-collection/chainsaw-test.yaml @@ -50,8 +50,9 @@ spec: timeout: 30s for: jsonPath: + # The binding's target, the UserPreference, is its owner; the User owns the UserPreference. path: '{.metadata.ownerReferences[0].name}' - value: test-gc-user + value: userpreference-test-gc-user - name: delete-user description: Delete the User resource and verify associated resources are garbage collected