diff --git a/internal/lineagecontrollerwebhook/webhook.go b/internal/lineagecontrollerwebhook/webhook.go index a98735da3..b6e996275 100644 --- a/internal/lineagecontrollerwebhook/webhook.go +++ b/internal/lineagecontrollerwebhook/webhook.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/cozystack/cozystack/pkg/lineage" + admissionv1 "k8s.io/api/admission/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -123,7 +124,7 @@ func (h *LineageControllerWebhook) Handle(ctx context.Context, req admission.Req logger.Error(err, "error computing lineage labels") return admission.Errored(500, fmt.Errorf("error computing lineage labels: %w", err)) } - labels, err := h.computeLabels(ctx, obj, owner) + labels, err := h.computeLabels(ctx, obj, owner, req.Operation) if err != nil { logger.Error(err, "error computing lineage labels") return admission.Errored(500, fmt.Errorf("error computing lineage labels: %w", err)) @@ -159,7 +160,7 @@ func (h *LineageControllerWebhook) getOwner(ctx context.Context, o *unstructured return obj, err } -func (h *LineageControllerWebhook) computeLabels(ctx context.Context, obj *unstructured.Unstructured, owner *unstructured.Unstructured) (map[string]string, error) { +func (h *LineageControllerWebhook) computeLabels(ctx context.Context, obj *unstructured.Unstructured, owner *unstructured.Unstructured, op admissionv1.Operation) (map[string]string, error) { if owner == nil { return nil, nil } @@ -194,10 +195,13 @@ func (h *LineageControllerWebhook) computeLabels(ctx context.Context, obj *unstr resourceSelectors := h.getResourceSelectors(obj.GroupVersionKind().GroupKind(), crd) selected := matchResourceToExcludeInclude(ctx, obj.GetName(), templateLabels, obj.GetLabels(), resourceSelectors) - // The object may narrow the verdict and never widen it: a rendered "false" - // is kept even where the definition selects the name, a rendered "true" is - // replaced by the computed verdict. - if obj.GetLabels()[corev1alpha1.TenantResourceLabelKey] == "false" { + // On CREATE the object may narrow the verdict and never widen it: a + // rendered "false" is kept even where the definition selects the name, a + // rendered "true" is replaced. Only on CREATE: a re-admission (UPDATE with + // managed-by-cozystack dropped, as the cacert controller does on a selector + // change) carries the "false" this webhook wrote earlier, and keeping it + // would block a grant. + if op == admissionv1.Create && obj.GetLabels()[corev1alpha1.TenantResourceLabelKey] == "false" { selected = false } labels[corev1alpha1.TenantResourceLabelKey] = "false" diff --git a/internal/lineagecontrollerwebhook/webhook_tenantresource_test.go b/internal/lineagecontrollerwebhook/webhook_tenantresource_test.go index 4fe9f6af8..32d218715 100644 --- a/internal/lineagecontrollerwebhook/webhook_tenantresource_test.go +++ b/internal/lineagecontrollerwebhook/webhook_tenantresource_test.go @@ -16,13 +16,15 @@ import ( ) // The tenantresource verdict is the definition's, but an object may narrow it -// and never widen it: a rendered "false" survives a selector that matches the -// name, a rendered "true" does not survive one that does not. +// on CREATE and never widen it: a rendered "false" survives a selector that +// matches the name, a rendered "true" does not survive one that does not. A +// re-admission UPDATE gets the definition's verdict whatever the label says. func TestHandle_TenantResourceLabelNarrowsOnly(t *testing.T) { tenantResourcePath := "/metadata/labels/" + strings.ReplaceAll(corev1alpha1.TenantResourceLabelKey, "/", "~1") tests := []struct { name string + op admissionv1.Operation secret string rendered string // want is the value the label must hold once the patch is applied; @@ -31,22 +33,41 @@ func TestHandle_TenantResourceLabelNarrowsOnly(t *testing.T) { }{ { name: "a rendered false is kept where the definition selects the name", + op: admissionv1.Create, secret: "harbor-demo-credentials", rendered: "false", want: "", }, { name: "a rendered true is overwritten where the definition does not select the name", + op: admissionv1.Create, secret: "harbor-demo-other", rendered: "true", want: "false", }, { name: "no rendered label gets the definition's verdict", + op: admissionv1.Create, secret: "harbor-demo-credentials", rendered: "", want: "true", }, + { + // The cacert controller's re-admission after a selector change: + // managed-by-cozystack dropped, the old verdict still on the object. + name: "a re-admission grants where the definition now selects the name", + op: admissionv1.Update, + secret: "harbor-demo-credentials", + rendered: "false", + want: "true", + }, + { + name: "a re-admission revokes where the definition no longer selects the name", + op: admissionv1.Update, + secret: "harbor-demo-other", + rendered: "true", + want: "false", + }, } for _, tt := range tests { @@ -78,13 +99,25 @@ func TestHandle_TenantResourceLabelNarrowsOnly(t *testing.T) { t.Fatalf("marshal object: %v", err) } - resp := w.Handle(context.Background(), admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{ + req := admissionv1.AdmissionRequest{ Kind: metav1.GroupVersionKind{Version: "v1", Kind: "Secret"}, Namespace: "tenant-demo", Name: tt.secret, - Operation: admissionv1.Create, + Operation: tt.op, Object: runtime.RawExtension{Raw: raw}, - }}) + } + if tt.op == admissionv1.Update { + old := obj.DeepCopy() + oldLabels := old.GetLabels() + oldLabels[ManagedObjectKey] = "true" + old.SetLabels(oldLabels) + oldRaw, err := json.Marshal(old) + if err != nil { + t.Fatalf("marshal old object: %v", err) + } + req.OldObject = runtime.RawExtension{Raw: oldRaw} + } + resp := w.Handle(context.Background(), admission.Request{AdmissionRequest: req}) if !resp.Allowed { t.Fatalf("expected allowed response, got %+v", resp.Result) }