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

Implement autonaming configuration protocol #1919

Merged
merged 4 commits into from
Jan 7, 2025
Merged

Implement autonaming configuration protocol #1919

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7a4a29f
Implement autonaming configuration protocol
mikhailshilkov Dec 18, 2024
7841fba
Simplify getDefaultName signature
mikhailshilkov Dec 19, 2024
cb51b7f
Conflict if both engine and provider config are specified. Also, unif…
mikhailshilkov Dec 20, 2024
d3f09bc
Skip e2e test if short
mikhailshilkov Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 85 additions & 32 deletions provider/pkg/autonaming/application.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,28 +10,47 @@ import (
"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
)

type AutoNamingConfig struct {
// ProviderAutoNamingConfig contains autonaming parameters configured for the provider.
type ProviderAutoNamingConfig struct {
AutoTrim bool `json:"autoTrim"`
RandomSuffixMinLength int `json:"randomSuffixMinLength"`
}

// EngineAutoNamingConfig contains autonaming parameters passed to the provider from the engine.
type EngineAutoNamingConfig struct {
RandomSeed []byte
AutonamingMode *EngineAutonamingMode
ProposedName string
}

// EngineAutonamingMode is the mode of autonaming to apply to the resource.
type EngineAutonamingMode int32

const (
EngineAutonamingModePropose EngineAutonamingMode = iota
EngineAutonamingModeEnforce
EngineAutonamingModeDisable
)

func ApplyAutoNaming(
spec *metadata.AutoNamingSpec,
urn resource.URN,
randomSeed []byte,
engineConfig EngineAutoNamingConfig,
providerConfig *ProviderAutoNamingConfig,
olds,
news resource.PropertyMap,
config *AutoNamingConfig,
) error {
if spec == nil {
return nil
}
// Auto-name fields if not already specified
val, err := getDefaultName(randomSeed, urn, spec, olds, news, config)
val, err := getDefaultName(urn, engineConfig, providerConfig, spec, olds, news)
if err != nil {
return err
}
news[resource.PropertyKey(spec.SdkName)] = val
if val != nil {
news[resource.PropertyKey(spec.SdkName)] = *val
}
return nil
}

Expand All @@ -41,38 +60,71 @@ func ApplyAutoNaming(
// based on its URN name, It ensures the name meets the length constraints, if known.
// Defaults to the name followed by 7 random hex characters separated by a '-'.
func getDefaultName(
randomSeed []byte,
urn resource.URN,
autoNamingSpec *metadata.AutoNamingSpec,
engineConfig EngineAutoNamingConfig,
providerConfig *ProviderAutoNamingConfig,
propertySpec *metadata.AutoNamingSpec,
olds,
news resource.PropertyMap,
config *AutoNamingConfig,
) (resource.PropertyValue, error) {
sdkName := autoNamingSpec.SdkName
) (*resource.PropertyValue, error) {
sdkName := propertySpec.SdkName

// Prefer explicitly specified name
if v, ok := news[resource.PropertyKey(sdkName)]; ok {
return v, nil
return &v, nil
}

// Fallback to previous name if specified/set.
if v, ok := olds[resource.PropertyKey(sdkName)]; ok {
return v, nil
return &v, nil
}

// Generate naming trivia for the resource.
namingTriviaApplies, namingTrivia, err := CheckNamingTrivia(sdkName, news, autoNamingSpec.TriviaSpec)
namingTriviaApplies, namingTrivia, err := CheckNamingTrivia(sdkName, news, propertySpec.TriviaSpec)
if err != nil {
return resource.PropertyValue{}, err
return nil, err
}

if engineConfig.AutonamingMode != nil {
// Engine autonaming conflicts with provider autonaming. If both are specified, return an error.
if providerConfig != nil {
return nil, fmt.Errorf("pulumi:autonaming conflicts with provider autonaming configuration, please specify only one")
}

switch *engineConfig.AutonamingMode {
case EngineAutonamingModeDisable:
return nil, nil
case EngineAutonamingModeEnforce:
v := resource.NewStringProperty(engineConfig.ProposedName)
return &v, nil
case EngineAutonamingModePropose:
proposedName := engineConfig.ProposedName

// Apply naming trivia to the generated name.
if namingTriviaApplies {
proposedName = ApplyTrivia(namingTrivia, proposedName)
}

// Validate the proposed name against the length constraints.
if propertySpec.MaxLength > 0 && len(proposedName) > propertySpec.MaxLength {
return nil, fmt.Errorf("proposed name %q exceeds max length of %d", proposedName, propertySpec.MaxLength)
}
if propertySpec.MinLength > 0 && len(proposedName) < propertySpec.MinLength {
return nil, fmt.Errorf("proposed name %q is shorter than min length of %d", proposedName, propertySpec.MinLength)
}

v := resource.NewStringProperty(proposedName)
return &v, nil
}
}

var autoTrim bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way that we can get the new engine autonaming to work with the existing autoNaming config? It seems like it could work with EngineAutonamingModePropose. And then maybe we should throw errors if you try to configure it with Enforce or Disable?

pulumi-aws-native/provider/pkg/schema/gen.go

Lines 188 to 191 in 11ac7b3

"autoNaming": {
Description: "The configuration for automatically naming resources.",
TypeSpec: pschema.TypeSpec{Ref: "#/types/aws-native:config:AutoNaming"},
},

Copy link
Member Author

Choose a reason for hiding this comment

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

My original take was a no, but now I think I misunderstood autoTrim. It could potentially apply to the Propose mode. randomSuffixMinLength won't work because the suffix (or prefix, or whatever format) is already baked into the proposed name.

Copy link
Member Author

@mikhailshilkov mikhailshilkov Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, I'm still not sure about autoTrim. Do we have any docs about what its supposed to do exactly? Reading the code, its semantics is roughly "if a random suffix makes the generated name too long, we can trim that random suffix down to N characters, where N is either 1 or whatever the user configured". I don't think there is any way to apply that to the proposed name, given it has an unknown structure. I assume we don't want to be trimming non-random portions of the name, and there is no way to ensure the min suffix length rule.

Copy link
Contributor

Choose a reason for hiding this comment

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

Without/preautoTrim the behavior was to allow the random suffix to be as little as 1 character if that allowed the name to fit within the maxLength requirements.

autoTrim introduced a couple of new behaviors.

  1. randomSuffixMinLength could be used to enforce a minimum length of the random suffix
  2. autoTrim: true. If the length of the name exceeded the maxLength (including the random suffix) then the name would be trimmed to fix within the maxLength. The part of the name that is trimmed is taken from the middle of the name in order to keep uniqueness.

It sounds like the randomSuffixMinLength might conflict with the new engine functionality. In that case we should throw an error if both are used. Since autoTrim removes from the middle it's possible it could work with the new engine behavior, but I think it would also be fine if these two features are mutually exclusive for now (especially if the autotrim behavior could be added to the engine)

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to what Cory said.
Are we able to make a distinction between whether engine level autonaming is configured for the stack/provider or only for a certain resource.

I feel like autoTrim + stack/provider level autonaming config should be an invalid case, but autoTrim + resource level autonaming config should be valid because it is essentially an escape hatch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are we able to make a distinction between whether engine level autonaming is configured for the stack/provider or only for a certain resource?

No

I feel like autoTrim + stack/provider level autonaming config should be an invalid case, but autoTrim + resource level autonaming config should be valid because it is essentially an escape hatch.

Could you please explain why this distinction? If a user configures a pattern like ${stack}-${name}-${hex(6)} on the provider level for all AWS Native resources vs. does so for a give aws-native:foo:Bar - how does that change the expectation of the user of whether some middle part of the generated name will be trimmed or not?

In any case, the question is a bit hypothetical because the provider can't tell the difference with the current design. I think I'm inclined to do the following for Propose mode:

  1. If RandomSuffixMinLength is configured, throw an error, we can't do anything with it.
  2. If AutoTrim is true and autoNamingSpec.MaxLength is defined, call trimName(ProposedName, autoNamingSpec.MaxLength) and hope it does what the user wanted.

But we can also keep it simple and throw an error for now.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine for now to just say that you can't configure both the engine autonaming and the provider autoName config and throw an error/warning if you have explicitly configured both.

I can create an issue for supporting a autoTrim type feature in the engine, would that be in pu/pu?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added returning an error if both engine and provider configs are specified. Also, renamed a few types and variables to make it consistent and more clear.

I can create an issue for supporting a autoTrim type feature in the engine, would that be in pu/pu?

yes

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please explain why this distinction? If a user configures a pattern like ${stack}-${name}-${hex(6)} on the provider level for all AWS Native resources vs. does so for a give aws-native:foo:Bar - how does that change the expectation of the user of whether some middle part of the generated name will be trimmed or not?

My reasoning is that users might want to apply the trimming by default to ensure their resources fit AWS limitations, but then overwrite it for a particular resource type as an escape hatch (e.g. Bucket only allowing lowercase chars).
But it's fine for now because the provider can't detect this atm.

// resource.NewUniqueName does not allow for a random suffix shorter than 1.
randomSuffixMinLength := 1
if config != nil {
autoTrim = config.AutoTrim
if config.RandomSuffixMinLength != 0 {
randomSuffixMinLength = config.RandomSuffixMinLength
if providerConfig != nil {
autoTrim = providerConfig.AutoTrim
if providerConfig.RandomSuffixMinLength != 0 {
randomSuffixMinLength = providerConfig.RandomSuffixMinLength
}
}

Expand All @@ -83,21 +135,21 @@ func getDefaultName(
if randomSuffixMinLength > randLength {
randLength = randomSuffixMinLength
}
if len(prefix)+namingTrivia.Length()+randLength < autoNamingSpec.MinLength {
randLength = autoNamingSpec.MinLength - len(prefix) - namingTrivia.Length()
if len(prefix)+namingTrivia.Length()+randLength < propertySpec.MinLength {
randLength = propertySpec.MinLength - len(prefix) - namingTrivia.Length()
}

maxLength := 0
if autoNamingSpec.MaxLength > 0 {
left := autoNamingSpec.MaxLength - len(prefix) - namingTrivia.Length()
if propertySpec.MaxLength > 0 {
left := propertySpec.MaxLength - len(prefix) - namingTrivia.Length()

if left <= 0 && autoTrim {
autoTrimMaxLength := autoNamingSpec.MaxLength - namingTrivia.Length() - randomSuffixMinLength
autoTrimMaxLength := propertySpec.MaxLength - namingTrivia.Length() - randomSuffixMinLength
if autoTrimMaxLength <= 0 {
return resource.PropertyValue{}, fmt.Errorf("failed to auto-generate value for %[1]q."+
return nil, fmt.Errorf("failed to auto-generate value for %[1]q."+
" Prefix: %[2]q is too large to fix max length constraint of %[3]d"+
" with required suffix length %[4]d. Please provide a value for %[1]q",
sdkName, prefix, autoNamingSpec.MaxLength, randomSuffixMinLength)
sdkName, prefix, propertySpec.MaxLength, randomSuffixMinLength)
}
prefix = trimName(prefix, autoTrimMaxLength)
randLength = randomSuffixMinLength
Expand All @@ -106,14 +158,14 @@ func getDefaultName(

if left <= 0 {
if namingTrivia.Length() > 0 {
return resource.PropertyValue{}, fmt.Errorf("failed to auto-generate value for %[1]q."+
return nil, fmt.Errorf("failed to auto-generate value for %[1]q."+
" Prefix: %[2]q is too large to fix max length constraint of %[3]d"+
" with required suffix %[4]q. Please provide a value for %[1]q",
sdkName, prefix, autoNamingSpec.MaxLength, namingTrivia.Suffix)
sdkName, prefix, propertySpec.MaxLength, namingTrivia.Suffix)
} else {
return resource.PropertyValue{}, fmt.Errorf("failed to auto-generate value for %[1]q."+
return nil, fmt.Errorf("failed to auto-generate value for %[1]q."+
" Prefix: %[2]q is too large to fix max length constraint of %[3]d. Please provide a value for %[1]q",
sdkName, prefix, autoNamingSpec.MaxLength)
sdkName, prefix, propertySpec.MaxLength)
}
}
if left < randLength {
Expand All @@ -123,17 +175,18 @@ func getDefaultName(
}

// Resource name is URN name + "-" + random suffix.
random, err := resource.NewUniqueName(randomSeed, prefix, randLength, maxLength, nil)
random, err := resource.NewUniqueName(engineConfig.RandomSeed, prefix, randLength, maxLength, nil)
if err != nil {
return resource.PropertyValue{}, err
return nil, err
}

// Apply naming trivia to the generated name.
if namingTriviaApplies {
random = ApplyTrivia(namingTrivia, random)
}

return resource.NewStringProperty(random), nil
v := resource.NewStringProperty(random)
return &v, nil
}

// trimName will trim the prefix to fit within the max length constraint.
Expand Down
Loading
Loading