Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use NameGenerator for composite name generation #5062

Merged
merged 1 commit into from
Nov 28, 2023
Merged

Use NameGenerator for composite name generation #5062

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 15 additions & 16 deletions internal/controller/apiextensions/claim/configurator.go
View file
Open in desktop
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be clear here: this is the same in the old code, @pedjak just made it clearer here in the comment.

return c.GenerateName(ctx, cp)
}

return nil
Expand Down
86 changes: 15 additions & 71 deletions internal/controller/apiextensions/claim/configurator_test.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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