Skip to content
New issue

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

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

Already on GitHub? # to your account

Shorter composite and composed resource names #1493

Merged
merged 4 commits into from
Jun 19, 2020

Conversation

muvaf
Copy link
Member

@muvaf muvaf commented May 9, 2020

Description of your changes

Currently, the name of the composed resources get to be way too long for some provider validations, like crossplane-system-myrequirement-sdas2-dsdh4 while it's not necessary to include the namespace as prefix and also yet another random string as postfix. With this change:

  • Requirement name: myrequirement
  • Composite name: myrequirement-dshn3
  • A composed resource name: myrequirement-hdsn2

However, in Kubernetes, children resources have the name of their parents as prefix like:

  • Deployment: deploymentname
  • ReplicaSet: deploymentname-dsjn3ndshh
  • Pod: deploymentname-dsjn3ndshh-idsjn

So, I think we should keep following the convention for composed resource name and make it myrequirement-dshn3-hdsn2, however, this increases the likeliness of failure in name validation of managed resources since it's longer; see crossplane-contrib/provider-gcp#222 and crossplane/stack-azure-sample#26 . In practice, it's fine if we have only one rand string set but it's against the convention.

I think we should do the truncation for external-name annotation generation as proposed in crossplane-contrib/provider-gcp#222 (comment) . So, I'll keep this in draft until we resolve that discussion. If we do the truncation, then it should be fine to keep the convention.

Fixes #1598

How has this code been tested?

Checklist

I have:

  • Run make reviewable to ensure this PR is ready for review.
  • Ensured this PR contains a neat, self documenting set of commits.
  • Updated any relevant documentation and examples.
  • Reported all new error conditions into the log or as an event, as
    appropriate.

For more about what we believe makes a pull request complete, see our
definition of done.

@muvaf muvaf requested review from negz, kasey and hasheddan May 9, 2020 00:51
Comment on lines 66 to 72
if cp.GetRequirementReference() != nil && cp.GetRequirementReference().Name != "" {
cd.SetGenerateName(cp.GetRequirementReference().Name + "-")
}
Copy link
Member

Choose a reason for hiding this comment

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

Agree that this feels a little weird to sometimes name it one way and sometimes name it another.

@negz
Copy link
Member

negz commented May 12, 2020

@muvaf and I discussed this out of band today, and I wanted to capture an idea we had here.

Historically we've automatically named dynamically provisioned managed resources (which are cluster scoped) claimnamespace-claimname-randm. The thinking being that it makes it easier to tell where dynamically provisioned resources came from at a glance when running (e.g.) kubectl list cloudsqlinstances or browsing the list of CloudSQL resources in the GCP console.

This makes sense in some ways because the name reads left-to-right and thus sorts from least to most specific. Unfortunately it makes the name more likely to need truncation (because including the namespace makes it longer), and harder to truncate cleanly (because we'd truncate the most specific part of the name from the right).

With composition we face the added concern that composites may be nested - is a composite resource can compose a composite resource. So if we stuck with Kubernetes convention we could end up with a name like reqnamespace-reqname-djm2f-os90d-m12kf-mdns2.

I wonder if we can make use of labels to help with this? Perhaps something like:

  • Statically provisioned composite resources are named whatever the user chooses.
  • Dynamically provisioned composite resources are named requirementname-randm.
  • All composed resources are labelled crossplane.io/composite: compositename where compositename is the name of the root/outermost composite resource.
  • All composed resources are named compositename-randm where compositename is the name of the root/outermost composite resource.
  • All composed resources of a dynamically provisioned composite resource are labelled crossplane.io/requirement: reqnamespace-reqname.

I believe this would give us the following properties:

  • Composite and composed resource names would never collide (at the API server level).
  • It would be possible to use kubectl get -l crossplane.io/requirement and kubectl get -l crossplane.io/composite to list resources that were composed by a particular composite and/or requirement.
  • Composed resource names would include at most 12 random chars. A requirement named mycooldb would result in a composite named something like mycooldb-fk1nd, whose composed resources (no matter how nested) would be named something like mycooldb-fk1nd-3d01d.

@muvaf muvaf force-pushed the shortilello branch 3 times, most recently from d4927ea to ab54740 Compare June 12, 2020 17:50
@muvaf muvaf marked this pull request as ready for review June 12, 2020 17:50
@muvaf muvaf force-pushed the shortilello branch 2 times, most recently from cd20896 to d526940 Compare June 12, 2020 17:52
@muvaf
Copy link
Member Author

muvaf commented Jun 12, 2020

Composed resource names would include at most 12 random chars. A requirement named mycooldb would result in a composite named something like mycooldb-fk1nd, whose composed resources (no matter how nested) would be named something like mycooldb-fk1nd-3d01d.

I have implemented this. All composite resources will first check whether they got the prefix label or not. If not, they'll label themselves with their name and use that label as prefix. This enables recursive application of the prefix.

@muvaf
Copy link
Member Author

muvaf commented Jun 12, 2020

Now all of the members of a composition has got crossplane.io/requirement-name and crossplane.io/requirement-namespace labels. @negz should be ready for another look.

All manually tested.

Copy link
Member

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

@muvaf LGTM! Looks like it needs a minor rebase.

@@ -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()
Copy link
Member

Choose a reason for hiding this comment

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

Will a composed resource ever have a namespace?

Copy link
Member Author

Choose a reason for hiding this comment

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

It may or may not but I wanted that logic to be complete; we're setting the identifier in case template has something else and the identifier that can be overwritten is namespace + name here, not necessarily name only. UID could be part of the template but that's a really far away case to cover.

muvaf added 4 commits June 19, 2020 10:53
…efix for the name of the composite resource

Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
set a root name prefix label on composite to be used as prefix
for all composed resources.

Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
… name and namespace as label

Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
…resources

Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
@muvaf muvaf merged commit 2b8762f into crossplane:master Jun 19, 2020
@muvaf muvaf deleted the shortilello branch June 19, 2020 09:10
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Composed member names can get too long
3 participants