Skip to content

Commit 5963f46

Browse files
IvanHuntersIvanHunters
committed
fix(marketplace): address review of tap auto-register
- Do not auto-install a privileged variant on connect (no confirmation exists there); leave it unregistered and surface a Warning Event. - Auto-register only the default variant, with an empty Package.Spec.Variant, so the install target is deterministic instead of the first-listed variant. - Delete the tap-managed registration Package when its PackageSource is pruned or its tap is torn down, so a removed package leaves the catalog. - Key Package ownership on the label AND the tap-source annotation (collision.Owns), so a name a later tap reuses is not adopted or deleted by an earlier one; verify an already-existing Package is ours before treating the registration as done. - Correct the cozypkg tap help (apps register on connect) and log a failed de-register at a visible level. - Tests: privileged skip, untap teardown (extracted runUntap), add idempotency, Package dry-run propagation, and ownership on both delete paths. Signed-off-by: IvanHunters <[email protected]>
1 parent 6032cd1 commit 5963f46

10 files changed

Lines changed: 402 additions & 111 deletions

File tree

‎cmd/cozypkg/cmd/add.go‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -498,22 +498,34 @@ func installPackage(ctx context.Context, k8sClient client.Client, packageSourceN
498498
},
499499
}
500500

501-
if err := k8sClient.Create(ctx, pkg); err != nil {
502-
// A tapped repository registers its apps on connect, so the Package
503-
// may already exist; treat that as done rather than an error.
504-
if apierrors.IsAlreadyExists(err) {
505-
_, _ = fmt.Fprintf(os.Stderr, "Package %s is already registered\n", pkgName)
506-
continue
507-
}
501+
created, err := createPackageIdempotent(ctx, k8sClient, pkg)
502+
if err != nil {
508503
return fmt.Errorf("failed to create Package %s: %w", pkgName, err)
509504
}
505+
if !created {
506+
_, _ = fmt.Fprintf(os.Stderr, "Package %s is already registered\n", pkgName)
507+
continue
508+
}
510509

511510
fmt.Fprintf(os.Stderr, "✓ Added Package %s\n", pkgName)
512511
}
513512

514513
return nil
515514
}
516515

516+
// createPackageIdempotent creates pkg and reports whether it created a new
517+
// object. A tapped repository registers its apps on connect, so the Package may
518+
// already exist; that is treated as done (created=false) rather than an error.
519+
func createPackageIdempotent(ctx context.Context, k8sClient client.Client, pkg *cozyv1alpha1.Package) (bool, error) {
520+
if err := k8sClient.Create(ctx, pkg); err != nil {
521+
if apierrors.IsAlreadyExists(err) {
522+
return false, nil
523+
}
524+
return false, err
525+
}
526+
return true, nil
527+
}
528+
517529
// selectVariantInteractive prompts user to select a variant
518530
func selectVariantInteractive(ps *cozyv1alpha1.PackageSource) (string, error) {
519531
if len(ps.Spec.Variants) == 0 {

‎cmd/cozypkg/cmd/tap.go‎

Lines changed: 58 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package cmd
1919
import (
2020
"context"
2121
"fmt"
22+
"io"
2223
"regexp"
2324
"strings"
2425
"time"
@@ -209,9 +210,11 @@ var tapCmd = &cobra.Command{
209210
creates a Flux OCIRepository pointing at the artifact and the PackageSource(s)
210211
the artifact carries, under the names the repository declares. A name that
211212
clashes with a core component (or another tap) is rejected, so an external
212-
package can never silently shadow an official one. Nothing is installed until
213-
'cozypkg add'. Tapping is idempotent. Creating cluster-scoped resources
214-
requires cluster-admin.
213+
package can never silently shadow an official one. The repository's applications
214+
register in the catalog automatically (their registration components install),
215+
except a variant with privileged install components, which the operator registers
216+
manually with 'cozypkg add --allow-privileged'. Tapping is idempotent. Creating
217+
cluster-scoped resources requires cluster-admin.
215218
216219
With --secret the OCIRepository is given a pull-credential secretRef (the
217220
admin pre-creates the Secret in cozy-system), so a private repository taps in
@@ -342,64 +345,69 @@ unmanaged until the repository is tapped again. Only tapped sources (marked
342345
with the marketplace-tap label) can be untapped; official sources are refused.`,
343346
Args: cobra.ExactArgs(1),
344347
RunE: func(cmd *cobra.Command, args []string) error {
345-
ctx := context.Background()
346-
name := args[0]
347-
348348
k8sClient, err := newClusterClient(tapCmdFlags.kubeconfig)
349349
if err != nil {
350350
return err
351351
}
352+
return runUntap(context.Background(), k8sClient, args[0], untapConfirmFlag, cmd.OutOrStdout(), cmd.ErrOrStderr())
353+
},
354+
}
352355

353-
ps := &cozyv1alpha1.PackageSource{}
354-
if err := k8sClient.Get(ctx, client.ObjectKey{Name: name}, ps); err != nil {
355-
return fmt.Errorf("failed to get PackageSource %s: %w", name, err)
356-
}
357-
// A tap is identified by its marker label, not by a name prefix.
358-
if ps.GetLabels()[tapconst.Label] != "true" {
359-
return fmt.Errorf("refusing to untap %q: not a tapped repository (missing the %s label); official sources cannot be untapped", name, tapconst.Label)
360-
}
356+
// runUntap removes a tapped PackageSource, its tap-managed registration Package,
357+
// and its Flux source. It is the testable body of untapCmd; see that command's
358+
// Long for the behaviour and the guard.
359+
func runUntap(ctx context.Context, k8sClient client.Client, name string, allowYes bool, out, errOut io.Writer) error {
360+
ps := &cozyv1alpha1.PackageSource{}
361+
if err := k8sClient.Get(ctx, client.ObjectKey{Name: name}, ps); err != nil {
362+
return fmt.Errorf("failed to get PackageSource %s: %w", name, err)
363+
}
364+
// A tap is identified by its marker label, not by a name prefix.
365+
if ps.GetLabels()[tapconst.Label] != "true" {
366+
return fmt.Errorf("refusing to untap %q: not a tapped repository (missing the %s label); official sources cannot be untapped", name, tapconst.Label)
367+
}
361368

362-
// The registration Package (auto-created on connect, tap-managed)
363-
// de-registers the repository's apps and is removed with the untap.
364-
// Deleting it garbage-collects its registration HelmReleases and the
365-
// ApplicationDefinitions they render. A Package NOT managed by this tap is
366-
// left in place and blocks the untap without --yes.
367-
pkg := &cozyv1alpha1.Package{}
368-
if err := k8sClient.Get(ctx, client.ObjectKey{Name: name}, pkg); err == nil {
369-
if pkg.GetLabels()[tapconst.Label] == "true" {
370-
if err := k8sClient.Delete(ctx, pkg); err != nil {
371-
return fmt.Errorf("failed to delete registration Package %s: %w", name, err)
372-
}
373-
_, _ = fmt.Fprintf(cmd.OutOrStdout(), "Removed Package/%s\n", name)
374-
} else if !untapConfirmFlag {
375-
return fmt.Errorf("package %s is still installed from this source; delete it with 'cozypkg del %s' first, or pass --yes to untap anyway (the Package stays installed)", name, name)
376-
}
377-
}
369+
srcName := ""
370+
if ps.Spec.SourceRef != nil {
371+
srcName = ps.Spec.SourceRef.Name
372+
}
378373

379-
srcName := ""
380-
if ps.Spec.SourceRef != nil {
381-
srcName = ps.Spec.SourceRef.Name
374+
// The registration Package (auto-created on connect, tap-managed)
375+
// de-registers the repository's apps and is removed with the untap. Deleting
376+
// it garbage-collects its registration HelmReleases and the
377+
// ApplicationDefinitions they render. It is removed only when it belongs to
378+
// THIS source (label AND source annotation); a Package a later tap created
379+
// under a reused name, or a foreign Package, is left in place and blocks the
380+
// untap without --yes.
381+
pkg := &cozyv1alpha1.Package{}
382+
if err := k8sClient.Get(ctx, client.ObjectKey{Name: name}, pkg); err == nil {
383+
if collision.Owns(pkg, srcName) {
384+
if err := k8sClient.Delete(ctx, pkg); err != nil {
385+
return fmt.Errorf("failed to delete registration Package %s: %w", name, err)
386+
}
387+
_, _ = fmt.Fprintf(out, "Removed Package/%s\n", name)
388+
} else if !allowYes {
389+
return fmt.Errorf("package %s is still installed from this source; delete it with 'cozypkg del %s' first, or pass --yes to untap anyway (the Package stays installed)", name, name)
382390
}
391+
}
383392

384-
if err := k8sClient.Delete(ctx, ps); err != nil {
385-
return fmt.Errorf("failed to delete PackageSource %s: %w", name, err)
386-
}
387-
fmt.Fprintf(cmd.OutOrStdout(), "Removed PackageSource/%s\n", name)
388-
389-
// Delete the Flux source only if it is a tap we own (marker label) and
390-
// no other PackageSource still references it.
391-
if srcName != "" && !sourceStillReferenced(ctx, k8sClient, srcName, name) {
392-
oci := &sourcev1.OCIRepository{}
393-
if err := k8sClient.Get(ctx, client.ObjectKey{Name: srcName, Namespace: cozySystemNamespace}, oci); err == nil && oci.GetLabels()[tapconst.Label] == "true" {
394-
if err := k8sClient.Delete(ctx, oci); err != nil {
395-
fmt.Fprintf(cmd.ErrOrStderr(), "warning: could not delete OCIRepository/%s: %v\n", srcName, err)
396-
} else {
397-
fmt.Fprintf(cmd.OutOrStdout(), "Removed OCIRepository/%s\n", srcName)
398-
}
393+
if err := k8sClient.Delete(ctx, ps); err != nil {
394+
return fmt.Errorf("failed to delete PackageSource %s: %w", name, err)
395+
}
396+
_, _ = fmt.Fprintf(out, "Removed PackageSource/%s\n", name)
397+
398+
// Delete the Flux source only if it is a tap we own (marker label) and no
399+
// other PackageSource still references it.
400+
if srcName != "" && !sourceStillReferenced(ctx, k8sClient, srcName, name) {
401+
oci := &sourcev1.OCIRepository{}
402+
if err := k8sClient.Get(ctx, client.ObjectKey{Name: srcName, Namespace: cozySystemNamespace}, oci); err == nil && oci.GetLabels()[tapconst.Label] == "true" {
403+
if err := k8sClient.Delete(ctx, oci); err != nil {
404+
_, _ = fmt.Fprintf(errOut, "warning: could not delete OCIRepository/%s: %v\n", srcName, err)
405+
} else {
406+
_, _ = fmt.Fprintf(out, "Removed OCIRepository/%s\n", srcName)
399407
}
400408
}
401-
return nil
402-
},
409+
}
410+
return nil
403411
}
404412

405413
// objectExists reports whether obj already exists on the cluster, so an

‎cmd/cozypkg/cmd/tap_test.go‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package cmd
1919
import (
2020
"context"
2121
"errors"
22+
"io"
2223
"testing"
2324

2425
sourcev1 "github.com/fluxcd/source-controller/api/v1"
@@ -182,3 +183,98 @@ func TestRewritePackageSourceForTap(t *testing.T) {
182183
t.Errorf("empty path should default to /, got %q", ps2.Spec.SourceRef.Path)
183184
}
184185
}
186+
187+
func untapScheme(t *testing.T) *runtime.Scheme {
188+
t.Helper()
189+
s := runtime.NewScheme()
190+
if err := clientgoscheme.AddToScheme(s); err != nil {
191+
t.Fatal(err)
192+
}
193+
if err := cozyv1alpha1.AddToScheme(s); err != nil {
194+
t.Fatal(err)
195+
}
196+
if err := sourcev1.AddToScheme(s); err != nil {
197+
t.Fatal(err)
198+
}
199+
return s
200+
}
201+
202+
func labeledPS(name, src string) *cozyv1alpha1.PackageSource {
203+
return &cozyv1alpha1.PackageSource{
204+
ObjectMeta: metav1.ObjectMeta{Name: name, Labels: map[string]string{tapconst.Label: "true"}},
205+
Spec: cozyv1alpha1.PackageSourceSpec{
206+
SourceRef: &cozyv1alpha1.PackageSourceRef{Kind: "OCIRepository", Name: src, Namespace: "cozy-system"},
207+
},
208+
}
209+
}
210+
211+
func ownedPkg(name, src string) *cozyv1alpha1.Package {
212+
return &cozyv1alpha1.Package{ObjectMeta: metav1.ObjectMeta{
213+
Name: name,
214+
Labels: map[string]string{tapconst.Label: "true"},
215+
Annotations: map[string]string{tapconst.SourceAnnotation: src},
216+
}}
217+
}
218+
219+
func TestRunUntapRemovesRegistrationPackageAndSource(t *testing.T) {
220+
s := untapScheme(t)
221+
oci := &sourcev1.OCIRepository{ObjectMeta: metav1.ObjectMeta{
222+
Name: "tap-acme-hello", Namespace: "cozy-system", Labels: map[string]string{tapconst.Label: "true"},
223+
}}
224+
cl := fake.NewClientBuilder().WithScheme(s).
225+
WithObjects(labeledPS("acme.hello", "tap-acme-hello"), ownedPkg("acme.hello", "tap-acme-hello"), oci).Build()
226+
227+
if err := runUntap(context.Background(), cl, "acme.hello", false, io.Discard, io.Discard); err != nil {
228+
t.Fatalf("runUntap: %v", err)
229+
}
230+
if err := cl.Get(context.Background(), client.ObjectKey{Name: "acme.hello"}, &cozyv1alpha1.Package{}); !apierrors.IsNotFound(err) {
231+
t.Errorf("registration Package must be removed, got %v", err)
232+
}
233+
if err := cl.Get(context.Background(), client.ObjectKey{Name: "acme.hello"}, &cozyv1alpha1.PackageSource{}); !apierrors.IsNotFound(err) {
234+
t.Errorf("PackageSource must be removed, got %v", err)
235+
}
236+
if err := cl.Get(context.Background(), client.ObjectKey{Name: "tap-acme-hello", Namespace: "cozy-system"}, &sourcev1.OCIRepository{}); !apierrors.IsNotFound(err) {
237+
t.Errorf("OCIRepository must be removed, got %v", err)
238+
}
239+
}
240+
241+
func TestRunUntapRefusesOfficial(t *testing.T) {
242+
s := untapScheme(t)
243+
official := &cozyv1alpha1.PackageSource{ObjectMeta: metav1.ObjectMeta{Name: "official"}}
244+
cl := fake.NewClientBuilder().WithScheme(s).WithObjects(official).Build()
245+
if err := runUntap(context.Background(), cl, "official", false, io.Discard, io.Discard); err == nil {
246+
t.Error("expected refusal to untap an unlabeled (official) source")
247+
}
248+
}
249+
250+
func TestRunUntapKeepsForeignPackage(t *testing.T) {
251+
s := untapScheme(t)
252+
// A Package owned by a DIFFERENT source: untap must not delete it, and
253+
// without --yes it blocks.
254+
cl := fake.NewClientBuilder().WithScheme(s).
255+
WithObjects(labeledPS("acme.hello", "tap-acme-hello"), ownedPkg("acme.hello", "tap-other")).Build()
256+
257+
if err := runUntap(context.Background(), cl, "acme.hello", false, io.Discard, io.Discard); err == nil {
258+
t.Error("a foreign Package must block untap without --yes")
259+
}
260+
if err := runUntap(context.Background(), cl, "acme.hello", true, io.Discard, io.Discard); err != nil {
261+
t.Fatalf("with --yes untap should proceed: %v", err)
262+
}
263+
if err := cl.Get(context.Background(), client.ObjectKey{Name: "acme.hello"}, &cozyv1alpha1.Package{}); err != nil {
264+
t.Errorf("the foreign Package must be left in place, got %v", err)
265+
}
266+
}
267+
268+
func TestCreatePackageIdempotent(t *testing.T) {
269+
s := untapScheme(t)
270+
cl := fake.NewClientBuilder().WithScheme(s).Build()
271+
272+
created, err := createPackageIdempotent(context.Background(), cl, &cozyv1alpha1.Package{ObjectMeta: metav1.ObjectMeta{Name: "x"}})
273+
if err != nil || !created {
274+
t.Fatalf("first create: created=%v err=%v", created, err)
275+
}
276+
created, err = createPackageIdempotent(context.Background(), cl, &cozyv1alpha1.Package{ObjectMeta: metav1.ObjectMeta{Name: "x"}})
277+
if err != nil || created {
278+
t.Fatalf("second create must be a no-op: created=%v err=%v", created, err)
279+
}
280+
}

‎internal/marketplace/collision/collision.go‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,19 +14,22 @@ import (
1414
"fmt"
1515

1616
apierrors "k8s.io/apimachinery/pkg/api/errors"
17+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1718
"sigs.k8s.io/controller-runtime/pkg/client"
1819

1920
cozyv1alpha1 "github.com/cozystack/cozystack/api/v1alpha1"
2021
"github.com/cozystack/cozystack/internal/marketplace/tapconst"
2122
)
2223

23-
// Owns reports whether an existing PackageSource is the given tap source's own
24-
// materialization, identified by the marketplace-tap label and the source
25-
// annotation. Such an object is safe to overwrite on an idempotent re-tap; a
26-
// foreign object of the same name is never owned.
27-
func Owns(ps *cozyv1alpha1.PackageSource, sourceName string) bool {
28-
return ps.GetLabels()[tapconst.Label] == "true" &&
29-
ps.GetAnnotations()[tapconst.SourceAnnotation] == sourceName
24+
// Owns reports whether an object (a PackageSource or the registration Package)
25+
// belongs to the given tap source, identified by BOTH the marketplace-tap label
26+
// and the source annotation. Keying on the label alone is not enough: a leftover
27+
// object from an earlier tap keeps the label, so a later tap that reuses the
28+
// name would otherwise adopt or delete an object it did not create. A foreign
29+
// object of the same name is never owned.
30+
func Owns(obj metav1.Object, sourceName string) bool {
31+
return obj.GetLabels()[tapconst.Label] == "true" &&
32+
obj.GetAnnotations()[tapconst.SourceAnnotation] == sourceName
3033
}
3134

3235
// PackageSourceName returns an error if a PackageSource named name already

0 commit comments

Comments
 (0)