Skip to content

Commit

Permalink
Merge pull request #5062 from pedjak/use-name-generator-for-composites
Browse files Browse the repository at this point in the history 
Use `NameGenerator` for composite name generation
  • Loading branch information
@turkenh
turkenh committed Nov 28, 2023
2 parents 6756027 + 13c2d1f commit eeee5cd
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 eeee5cd

Please sign in to comment.