Skip to content

Commit

Permalink
Use NameGenerator for composite name generation
Browse files Browse the repository at this point in the history 
The generator is already used for MR name generation.

Changes:

* since used by two controllers, `NameGenerator` moved under `internal/names` package
* `configurator_test.go` updated

Signed-off-by: Predrag Knezevic <predrag.knezevic@upbound.io>
  • Loading branch information
@pedjak
pedjak committed Nov 27, 2023
1 parent 6756027 commit 4ba31ce
Show file tree
Hide file tree
Showing 10 changed files with 320 additions and 318 deletions.
31 changes: 15 additions & 16 deletions internal/controller/apiextensions/claim/configurator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"github.com/crossplane/crossplane-runtime/pkg/resource/unstructured/claim"
"github.com/crossplane/crossplane-runtime/pkg/resource/unstructured/composite"

"github.com/crossplane/crossplane/internal/names"
"github.com/crossplane/crossplane/internal/xcrd"
)

Expand All @@ -40,33 +41,29 @@ const (
errUnsupportedDstObject = "destination object was not valid object"
errUnsupportedSrcObject = "source object was not valid object"

errName = "cannot use dry-run create to name composite resource"
errBindCompositeConflict = "cannot bind composite resource that references a different claim"

errMergeClaimSpec = "unable to merge claim spec"
errMergeClaimStatus = "unable to merge claim status"
)

// An APIDryRunCompositeConfigurator configures composite resources. It may
// perform a dry-run create against an API server in order to name and validate
// the configured resource.
type APIDryRunCompositeConfigurator struct {
client client.Client
type apiCompositeConfigurator struct {
names.NameGenerator
}

// NewAPIDryRunCompositeConfigurator returns a Configurator of composite
// NewAPICompositeConfigurator returns a Configurator of composite
// resources that may perform a dry-run create against an API server in order to
// name and validate the configured resource.
func NewAPIDryRunCompositeConfigurator(c client.Client) *APIDryRunCompositeConfigurator {
return &APIDryRunCompositeConfigurator{client: c}
func NewAPICompositeConfigurator(ng names.NameGenerator) Configurator {
return &apiCompositeConfigurator{NameGenerator: ng}
}

// Configure the supplied composite resource by propagating configuration from
// the supplied claim. Both create and update scenarios are supported; i.e. the
// composite may or may not have been created in the API server when passed to
// this method. The configured composite may be submitted to an API server via a
// dry run create in order to name and validate it.
func (c *APIDryRunCompositeConfigurator) Configure(ctx context.Context, cm resource.CompositeClaim, cp resource.Composite) error { //nolint:gocyclo // Only slightly over (12).
func (c *apiCompositeConfigurator) Configure(ctx context.Context, cm resource.CompositeClaim, cp resource.Composite) error { //nolint:gocyclo // Only slightly over (12).
ucm, ok := cm.(*claim.Unstructured)
if !ok {
return nil
Expand Down Expand Up @@ -144,13 +141,15 @@ func (c *APIDryRunCompositeConfigurator) Configure(ctx context.Context, cm resou
ucp.SetClaimReference(proposed)

if !meta.WasCreated(cp) {
// The API server returns an available name derived from
// generateName when we perform a dry-run create. This name is
// likely (but not guaranteed) to be available when we create
// the composite resource. If the API server generates a name
// that is unavailable it will return a 500 ServerTimeout error.
cp.SetGenerateName(fmt.Sprintf("%s-", cm.GetName()))
return errors.Wrap(c.client.Create(ctx, cp, client.DryRunAll), errName)

// Generated name is likely (but not guaranteed) to be available
// when we create the composite resource. If taken,
// then we are going to update an existing composite,
// hijacking it from another claim. Depending on context/environment
// the consequences could be more or less serious.
// TODO: decide if we must prevent it.
return c.GenerateName(ctx, cp)
}

return nil
Expand Down
86 changes: 15 additions & 71 deletions internal/controller/apiextensions/claim/configurator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package claim

import (
"context"
"strconv"
"strings"
"testing"

Expand All @@ -43,7 +44,6 @@ func TestCompositeConfigure(t *testing.T) {
apiVersion := "v"
kind := "k"
now := metav1.Now()
errBoom := errors.New("boom")

type args struct {
ctx context.Context
Expand All @@ -58,7 +58,6 @@ func TestCompositeConfigure(t *testing.T) {

cases := map[string]struct {
reason string
c client.Client
args args
want want
}{
Expand Down Expand Up @@ -164,70 +163,8 @@ func TestCompositeConfigure(t *testing.T) {
err: errors.New(errBindCompositeConflict),
},
},
"DryRunError": {
reason: "We should return any error we encounter while dry-run creating a dynamically provisioned composite",
c: &test.MockClient{
MockCreate: test.NewMockCreateFn(errBoom),
},
args: args{
cm: &claim.Unstructured{
Unstructured: unstructured.Unstructured{
Object: map[string]any{
"apiVersion": apiVersion,
"kind": kind,
"metadata": map[string]any{
"namespace": ns,
"name": name,
},
"spec": map[string]any{
"coolness": 23,

// These should be preserved.
"compositionRef": "ref",
"compositionSelector": "ref",

// These should be filtered out.
"resourceRef": "ref",
"writeConnectionSecretToRef": "ref",
},
},
},
},
cp: &composite.Unstructured{},
},
want: want{
cp: &composite.Unstructured{
Unstructured: unstructured.Unstructured{
Object: map[string]any{
"metadata": map[string]any{
"generateName": name + "-",
"labels": map[string]any{
xcrd.LabelKeyClaimNamespace: ns,
xcrd.LabelKeyClaimName: name,
},
},
"spec": map[string]any{
"coolness": 23,
"compositionRef": "ref",
"compositionSelector": "ref",
"claimRef": map[string]any{
"apiVersion": apiVersion,
"kind": kind,
"namespace": ns,
"name": name,
},
},
},
},
},
err: errors.Wrap(errBoom, errName),
},
},
"ConfiguredNewXR": {
reason: "A dynamically provisioned composite resource should be configured according to the claim",
c: &test.MockClient{
MockCreate: test.NewMockCreateFn(nil),
},
args: args{
cm: &claim.Unstructured{
Unstructured: unstructured.Unstructured{
Expand Down Expand Up @@ -260,6 +197,7 @@ func TestCompositeConfigure(t *testing.T) {
Object: map[string]any{
"metadata": map[string]any{
"generateName": name + "-",
"name": name + "-1",
"labels": map[string]any{
xcrd.LabelKeyClaimNamespace: ns,
xcrd.LabelKeyClaimName: name,
Expand Down Expand Up @@ -590,9 +528,6 @@ func TestCompositeConfigure(t *testing.T) {
},
"SkipK8sAnnotationPropagation": {
reason: "Claim's kubernetes.io annotations should not be propagated to XR",
c: &test.MockClient{
MockCreate: test.NewMockCreateFn(nil),
},
args: args{
cm: &claim.Unstructured{
Unstructured: unstructured.Unstructured{
Expand Down Expand Up @@ -632,6 +567,7 @@ func TestCompositeConfigure(t *testing.T) {
Object: map[string]any{
"metadata": map[string]any{
"generateName": name + "-",
"name": name + "-1",
"labels": map[string]any{
xcrd.LabelKeyClaimNamespace: ns,
xcrd.LabelKeyClaimName: name,
Expand All @@ -658,9 +594,6 @@ func TestCompositeConfigure(t *testing.T) {
},
"SkipK8sLabelPropagation": {
reason: "Claim's kubernetes.io annotations should not be propagated to XR",
c: &test.MockClient{
MockCreate: test.NewMockCreateFn(nil),
},
args: args{
cm: &claim.Unstructured{
Unstructured: unstructured.Unstructured{
Expand Down Expand Up @@ -700,6 +633,7 @@ func TestCompositeConfigure(t *testing.T) {
Object: map[string]any{
"metadata": map[string]any{
"generateName": name + "-",
"name": name + "-1",
"labels": map[string]any{
xcrd.LabelKeyClaimNamespace: ns,
xcrd.LabelKeyClaimName: name,
Expand All @@ -726,7 +660,7 @@ func TestCompositeConfigure(t *testing.T) {

for name, tc := range cases {
t.Run(name, func(t *testing.T) {
c := NewAPIDryRunCompositeConfigurator(tc.c)
c := NewAPICompositeConfigurator(&mockNameGenerator{})
got := c.Configure(tc.args.ctx, tc.args.cm, tc.args.cp)
if diff := cmp.Diff(tc.want.err, got, test.EquateErrors()); diff != "" {
t.Errorf("Configure(...): %s\n-want error, +got error:\n%s\n", tc.reason, diff)
Expand All @@ -739,6 +673,16 @@ func TestCompositeConfigure(t *testing.T) {

}

type mockNameGenerator struct {
last int
}

func (m *mockNameGenerator) GenerateName(_ context.Context, obj resource.Object) error {
m.last++
obj.SetName(obj.GetGenerateName() + strconv.Itoa(m.last))
return nil
}

func TestClaimConfigure(t *testing.T) {
errBoom := errors.New("boom")
ns := "spacename"
Expand Down
4 changes: 3 additions & 1 deletion internal/controller/apiextensions/claim/reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ import (
"github.com/crossplane/crossplane-runtime/pkg/resource/unstructured"
"github.com/crossplane/crossplane-runtime/pkg/resource/unstructured/claim"
"github.com/crossplane/crossplane-runtime/pkg/resource/unstructured/composite"

"github.com/crossplane/crossplane/internal/names"
)

const (
Expand Down Expand Up @@ -210,7 +212,7 @@ type crComposite struct {

func defaultCRComposite(c client.Client) crComposite {
return crComposite{
Configurator: NewAPIDryRunCompositeConfigurator(c),
Configurator: NewAPICompositeConfigurator(names.NewNameGenerator(c)),
ConnectionPropagator: NewAPIConnectionPropagator(c),
}
}
Expand Down
5 changes: 3 additions & 2 deletions internal/controller/apiextensions/composite/composition_functions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import (
"github.com/crossplane/crossplane-runtime/pkg/resource/unstructured/composite"

"github.com/crossplane/crossplane/apis/apiextensions/fn/proto/v1beta1"
"github.com/crossplane/crossplane/internal/names"
)

// Error strings.
Expand Down Expand Up @@ -99,7 +100,7 @@ type FunctionComposer struct {
}

type xr struct {
NameGenerator
names.NameGenerator
managed.ConnectionDetailsFetcher
ComposedResourceObserver
ComposedResourceGarbageCollector
Expand Down Expand Up @@ -191,7 +192,7 @@ func NewFunctionComposer(kube client.Client, r FunctionRunner, o ...FunctionComp
ConnectionDetailsFetcher: f,
ComposedResourceObserver: NewExistingComposedResourceObserver(kube, f),
ComposedResourceGarbageCollector: NewDeletingComposedResourceGarbageCollector(kube),
NameGenerator: NewAPINameGenerator(kube),
NameGenerator: names.NewNameGenerator(kube),
},

pipeline: r,
Expand Down
7 changes: 4 additions & 3 deletions internal/controller/apiextensions/composite/composition_pt.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (

v1 "github.com/crossplane/crossplane/apis/apiextensions/v1"
"github.com/crossplane/crossplane/internal/controller/apiextensions/usage"
"github.com/crossplane/crossplane/internal/names"
)

// Error strings
Expand Down Expand Up @@ -73,7 +74,7 @@ func WithTemplateAssociator(a CompositionTemplateAssociator) PTComposerOption {

// WithComposedNameGenerator configures how the PTComposer should generate names
// for unnamed composed resources.
func WithComposedNameGenerator(r NameGenerator) PTComposerOption {
func WithComposedNameGenerator(r names.NameGenerator) PTComposerOption {
return func(c *PTComposer) {
c.composed.NameGenerator = r
}
Expand Down Expand Up @@ -105,7 +106,7 @@ func WithComposedConnectionDetailsExtractor(e ConnectionDetailsExtractor) PTComp
}

type composedResource struct {
NameGenerator
names.NameGenerator
managed.ConnectionDetailsFetcher
ConnectionDetailsExtractor
ReadinessChecker
Expand Down Expand Up @@ -134,7 +135,7 @@ func NewPTComposer(kube client.Client, o ...PTComposerOption) *PTComposer {

composition: NewGarbageCollectingAssociator(kube),
composed: composedResource{
NameGenerator: NewAPINameGenerator(kube),
NameGenerator: names.NewNameGenerator(kube),
ReadinessChecker: ReadinessCheckerFn(IsReady),
ConnectionDetailsFetcher: NewSecretConnectionDetailsFetcher(kube),
ConnectionDetailsExtractor: ConnectionDetailsExtractorFn(ExtractConnectionDetails),
Expand Down
17 changes: 9 additions & 8 deletions internal/controller/apiextensions/composite/composition_pt_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import (
"github.com/crossplane/crossplane-runtime/pkg/test"

v1 "github.com/crossplane/crossplane/apis/apiextensions/v1"
"github.com/crossplane/crossplane/internal/names"
)

func TestPTCompose(t *testing.T) {
Expand Down Expand Up @@ -122,7 +123,7 @@ func TestPTCompose(t *testing.T) {
}
return tas, nil
})),
WithComposedNameGenerator(NameGeneratorFn(func(ctx context.Context, cd resource.Object) error { return nil })),
WithComposedNameGenerator(names.NameGeneratorFn(func(ctx context.Context, cd resource.Object) error { return nil })),
WithComposedConnectionDetailsFetcher(ConnectionDetailsFetcherFn(func(ctx context.Context, o resource.ConnectionSecretOwner) (managed.ConnectionDetails, error) {
return nil, nil
})),
Expand Down Expand Up @@ -160,7 +161,7 @@ func TestPTCompose(t *testing.T) {
}}
return tas, nil
})),
WithComposedNameGenerator(NameGeneratorFn(func(ctx context.Context, cd resource.Object) error { return nil })),
WithComposedNameGenerator(names.NameGeneratorFn(func(ctx context.Context, cd resource.Object) error { return nil })),
},
},
args: args{
Expand Down Expand Up @@ -192,7 +193,7 @@ func TestPTCompose(t *testing.T) {
}}
return tas, nil
})),
WithComposedNameGenerator(NameGeneratorFn(func(ctx context.Context, cd resource.Object) error { return nil })),
WithComposedNameGenerator(names.NameGeneratorFn(func(ctx context.Context, cd resource.Object) error { return nil })),
},
},
args: args{
Expand Down Expand Up @@ -224,7 +225,7 @@ func TestPTCompose(t *testing.T) {
}}
return tas, nil
})),
WithComposedNameGenerator(NameGeneratorFn(func(ctx context.Context, cd resource.Object) error { return nil })),
WithComposedNameGenerator(names.NameGeneratorFn(func(ctx context.Context, cd resource.Object) error { return nil })),
WithComposedConnectionDetailsFetcher(ConnectionDetailsFetcherFn(func(ctx context.Context, o resource.ConnectionSecretOwner) (managed.ConnectionDetails, error) {
return nil, errBoom
})),
Expand Down Expand Up @@ -259,7 +260,7 @@ func TestPTCompose(t *testing.T) {
}}
return tas, nil
})),
WithComposedNameGenerator(NameGeneratorFn(func(ctx context.Context, cd resource.Object) error { return nil })),
WithComposedNameGenerator(names.NameGeneratorFn(func(ctx context.Context, cd resource.Object) error { return nil })),
WithComposedConnectionDetailsFetcher(ConnectionDetailsFetcherFn(func(ctx context.Context, o resource.ConnectionSecretOwner) (managed.ConnectionDetails, error) {
return nil, nil
})),
Expand Down Expand Up @@ -297,7 +298,7 @@ func TestPTCompose(t *testing.T) {
}}
return tas, nil
})),
WithComposedNameGenerator(NameGeneratorFn(func(ctx context.Context, cd resource.Object) error { return nil })),
WithComposedNameGenerator(names.NameGeneratorFn(func(ctx context.Context, cd resource.Object) error { return nil })),
WithComposedConnectionDetailsFetcher(ConnectionDetailsFetcherFn(func(ctx context.Context, o resource.ConnectionSecretOwner) (managed.ConnectionDetails, error) {
return nil, nil
})),
Expand Down Expand Up @@ -368,7 +369,7 @@ func TestPTCompose(t *testing.T) {
}}
return tas, nil
})),
WithComposedNameGenerator(NameGeneratorFn(func(ctx context.Context, cd resource.Object) error { return nil })),
WithComposedNameGenerator(names.NameGeneratorFn(func(ctx context.Context, cd resource.Object) error { return nil })),
WithComposedConnectionDetailsFetcher(ConnectionDetailsFetcherFn(func(ctx context.Context, o resource.ConnectionSecretOwner) (managed.ConnectionDetails, error) {
return nil, nil
})),
Expand Down Expand Up @@ -427,7 +428,7 @@ func TestPTCompose(t *testing.T) {
}
return tas, nil
})),
WithComposedNameGenerator(NameGeneratorFn(func(ctx context.Context, cd resource.Object) error {
WithComposedNameGenerator(names.NameGeneratorFn(func(ctx context.Context, cd resource.Object) error {
if cd.GetObjectKind().GroupVersionKind().Kind == "BrokenResource" {
return errBoom
}
Expand Down

0 comments on commit 4ba31ce

Please sign in to comment.