Merge pull request crossplane#1493 from muvaf/shortilello

Shorter composite and composed resource names

pkg/controller/apiextensions/composite/api.go

@@ -30,6 +30,7 @@ import (
 	"github.com/crossplane/crossplane-runtime/pkg/resource"
 
 	"github.com/crossplane/crossplane/apis/apiextensions/v1alpha1"
+	"github.com/crossplane/crossplane/pkg/controller/apiextensions/composite/composed"
 )
 
 // Error strings.
@@ -217,6 +218,26 @@ func (s *EnforcedCompositionSelector) SelectComposition(_ context.Context, cp re
 	return nil
 }
 
+// NewConfiguratorChain returns a new *ConfiguratorChain.
+func NewConfiguratorChain(l ...Configurator) *ConfiguratorChain {
+	return &ConfiguratorChain{list: l}
+}
+
+// ConfiguratorChain executes the Configurators in given order.
+type ConfiguratorChain struct {
+	list []Configurator
+}
+
+// Configure calls Configure function of every Configurator in the list.
+func (cc *ConfiguratorChain) Configure(ctx context.Context, cp resource.Composite, comp *v1alpha1.Composition) error {
+	for _, c := range cc.list {
+		if err := c.Configure(ctx, cp, comp); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // NewAPIConfigurator returns a Configurator that configures a
 // composite resource using its composition.
 func NewAPIConfigurator(c client.Client) *APIConfigurator {
@@ -252,3 +273,24 @@ func (c *APIConfigurator) Configure(ctx context.Context, cp resource.Composite,
 
 	return errors.Wrap(c.client.Update(ctx, cp), errUpdateComposite)
 }
+
+// NewAPINamingConfigurator returns a Configurator that sets the root name prefixKu
+// to its own name if it is not already set.
+func NewAPINamingConfigurator(c client.Client) *APINamingConfigurator {
+	return &APINamingConfigurator{client: c}
+}
+
+// An APINamingConfigurator sets the root name prefix to its own name if it is not
+// already set.
+type APINamingConfigurator struct {
+	client client.Client
+}
+
+// Configure the supplied composite resource's root name prefix.
+func (c *APINamingConfigurator) Configure(ctx context.Context, cp resource.Composite, _ *v1alpha1.Composition) error {
+	if cp.GetLabels()[composed.LabelKeyNamePrefixForComposed] != "" {
+		return nil
+	}
+	meta.AddLabels(cp, map[string]string{composed.LabelKeyNamePrefixForComposed: cp.GetName()})
+	return errors.Wrap(c.client.Update(ctx, cp), errUpdateComposite)
+}

pkg/controller/apiextensions/composite/api_test.go

@@ -36,6 +36,7 @@ import (
 	"github.com/crossplane/crossplane-runtime/pkg/test"
 
 	"github.com/crossplane/crossplane/apis/apiextensions/v1alpha1"
+	"github.com/crossplane/crossplane/pkg/controller/apiextensions/composite/composed"
 )
 
 var errBoom = errors.New("boom")
@@ -541,3 +542,54 @@ func TestAPIEnforcedCompositionSelector(t *testing.T) {
 		})
 	}
 }
+
+func TestAPINamingConfigurator(t *testing.T) {
+	type args struct {
+		kube client.Client
+		cp   resource.Composite
+	}
+	type want struct {
+		cp  resource.Composite
+		err error
+	}
+
+	cases := map[string]struct {
+		reason string
+		args
+		want
+	}{
+		"LabelAlreadyExists": {
+			reason: "No operation should be done if the name prefix is already given",
+			args: args{
+				cp: &fake.Composite{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{composed.LabelKeyNamePrefixForComposed: "given"}}},
+			},
+			want: want{
+				cp: &fake.Composite{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{composed.LabelKeyNamePrefixForComposed: "given"}}},
+			},
+		},
+		"AssignedName": {
+			reason: "Its own name should be used as name prefix if it is not given",
+			args: args{
+				kube: &test.MockClient{
+					MockUpdate: test.NewMockUpdateFn(nil),
+				},
+				cp: &fake.Composite{ObjectMeta: metav1.ObjectMeta{Name: "cp"}},
+			},
+			want: want{
+				cp: &fake.Composite{ObjectMeta: metav1.ObjectMeta{Name: "cp", Labels: map[string]string{composed.LabelKeyNamePrefixForComposed: "cp"}}},
+			},
+		},
+	}
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			c := NewAPINamingConfigurator(tc.args.kube)
+			err := c.Configure(context.Background(), tc.args.cp, nil)
+			if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
+				t.Errorf("\n%s\nConfigure(...): -want, +got:\n%s", tc.reason, diff)
+			}
+			if diff := cmp.Diff(tc.want.cp, tc.args.cp); diff != "" {
+				t.Errorf("\n%s\nConfigure(...): -want, +got:\n%s", tc.reason, diff)
+			}
+		})
+	}
+}

pkg/controller/apiextensions/composite/composed/api.go

@@ -25,16 +25,25 @@ import (
 	"k8s.io/apimachinery/pkg/util/json"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
+	"github.com/crossplane/crossplane-runtime/pkg/meta"
 	"github.com/crossplane/crossplane-runtime/pkg/reconciler/managed"
 	"github.com/crossplane/crossplane-runtime/pkg/resource"
 
 	"github.com/crossplane/crossplane/apis/apiextensions/v1alpha1"
 )
 
 const (
-	errUnmarshal = "cannot unmarshal base template"
-	errFmtPatch  = "cannot apply the patch at index %d"
-	errGetSecret = "cannot get connection secret of composed resource"
+	errUnmarshal  = "cannot unmarshal base template"
+	errFmtPatch   = "cannot apply the patch at index %d"
+	errGetSecret  = "cannot get connection secret of composed resource"
+	errNamePrefix = "name prefix is not found in labels"
+)
+
+// Label keys.
+const (
+	LabelKeyNamePrefixForComposed = "crossplane.io/composite"
+	LabelKeyRequirementName       = "crossplane.io/requirement-name"
+	LabelKeyRequirementNamespace  = "crossplane.io/requirement-namespace"
 )
 
 // ConfigureFn is a function that implements Configurator interface.
@@ -54,14 +63,25 @@ func (*DefaultConfigurator) Configure(cp resource.Composite, cd resource.Compose
 	// Any existing name will be overwritten when we unmarshal the template. We
 	// store it here so that we can reset it after unmarshalling.
 	name := cd.GetName()
+	namespace := cd.GetNamespace()
 	if err := json.Unmarshal(t.Base.Raw, cd); err != nil {
 		return errors.Wrap(err, errUnmarshal)
 	}
+	if cp.GetLabels()[LabelKeyNamePrefixForComposed] == "" {
+		return errors.New(errNamePrefix)
+	}
+	// This label will be used if composed resource is yet another composite.
+	meta.AddLabels(cd, map[string]string{
+		LabelKeyNamePrefixForComposed: cp.GetLabels()[LabelKeyNamePrefixForComposed],
+		LabelKeyRequirementName:       cp.GetLabels()[LabelKeyRequirementName],
+		LabelKeyRequirementNamespace:  cp.GetLabels()[LabelKeyRequirementNamespace],
+	})
 	// Unmarshalling the template will overwrite any existing fields, so we must
 	// restore the existing name, if any. We also set generate name in case we
 	// haven't yet named this composed resource.
-	cd.SetGenerateName(cp.GetName() + "-")
+	cd.SetGenerateName(cp.GetLabels()[LabelKeyNamePrefixForComposed] + "-")
 	cd.SetName(name)
+	cd.SetNamespace(namespace)
 	return nil
 }
 

pkg/controller/apiextensions/composite/composed/api_test.go

@@ -71,15 +71,35 @@ func TestConfigure(t *testing.T) {
 				err: errors.Wrap(errors.New("invalid character 'o' looking for beginning of value"), errUnmarshal),
 			},
 		},
+		"NoLabel": {
+			reason: "The name prefix label has to be set",
+			args: args{
+				cp: &fake.Composite{},
+				cd: &fake.Composed{ObjectMeta: metav1.ObjectMeta{Name: "cd"}},
+				t:  v1alpha1.ComposedTemplate{Base: runtime.RawExtension{Raw: tmpl}},
+			},
+			want: want{
+				cd:  &fake.Composed{ObjectMeta: metav1.ObjectMeta{Name: "cd"}},
+				err: errors.New(errNamePrefix),
+			},
+		},
 		"Success": {
 			reason: "Configuration should result in the right object with correct generateName",
 			args: args{
-				cp: &fake.Composite{ObjectMeta: metav1.ObjectMeta{Name: "cp"}},
+				cp: &fake.Composite{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{
+					LabelKeyNamePrefixForComposed: "ola",
+					LabelKeyRequirementName:       "rola",
+					LabelKeyRequirementNamespace:  "rolans",
+				}}},
 				cd: &fake.Composed{ObjectMeta: metav1.ObjectMeta{Name: "cd"}},
 				t:  v1alpha1.ComposedTemplate{Base: runtime.RawExtension{Raw: tmpl}},
 			},
 			want: want{
-				cd: &fake.Composed{ObjectMeta: metav1.ObjectMeta{Name: "cd", GenerateName: "cp-"}},
+				cd: &fake.Composed{ObjectMeta: metav1.ObjectMeta{Name: "cd", GenerateName: "ola-", Labels: map[string]string{
+					LabelKeyNamePrefixForComposed: "ola",
+					LabelKeyRequirementName:       "rola",
+					LabelKeyRequirementNamespace:  "rolans",
+				}}},
 			},
 		},
 	}

pkg/controller/apiextensions/composite/reconciler.go

@@ -177,7 +177,7 @@ func NewReconciler(mgr manager.Manager, of resource.CompositeKind, opts ...Recon
 
 		composite: compositeResource{
 			CompositionSelector: NewAPILabelSelectorResolver(kube),
-			Configurator:        NewAPIConfigurator(kube),
+			Configurator:        NewConfiguratorChain(NewAPINamingConfigurator(kube), NewAPIConfigurator(kube)),
 			ConnectionPublisher: NewAPIFilteredSecretPublisher(kube, []string{}),
 		},
 

pkg/controller/apiextensions/requirement/configurator.go

@@ -26,17 +26,23 @@ import (
 	"github.com/crossplane/crossplane-runtime/pkg/resource"
 	"github.com/crossplane/crossplane-runtime/pkg/resource/unstructured/composite"
 	"github.com/crossplane/crossplane-runtime/pkg/resource/unstructured/requirement"
+
+	"github.com/crossplane/crossplane/pkg/controller/apiextensions/composite/composed"
 )
 
 // Configure the supplied composite resource. The composite resource name is
-// derived from the supplied requirement, as {namespace}-{name}-{random-string}.
+// derived from the supplied requirement, as {name}-{random-string}.
 // The requirement's external name annotation, if any, is propagated to the
 // composite resource.
 func Configure(_ context.Context, rq resource.Requirement, cp resource.Composite) error {
-	cp.SetGenerateName(fmt.Sprintf("%s-%s-", rq.GetNamespace(), rq.GetName()))
+	cp.SetGenerateName(fmt.Sprintf("%s-", rq.GetName()))
 	if meta.GetExternalName(rq) != "" {
 		meta.SetExternalName(cp, meta.GetExternalName(rq))
 	}
+	meta.AddLabels(cp, map[string]string{
+		composed.LabelKeyRequirementName:      rq.GetName(),
+		composed.LabelKeyRequirementNamespace: rq.GetNamespace(),
+	})
 
 	urq, ok := rq.(*requirement.Unstructured)
 	if !ok {