-
Notifications
You must be signed in to change notification settings - Fork 16
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.
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
base: master
Are you sure you want to change the base?
Changes from 2 commits
859d741
5bc1cf7
76f51ce
dae1852
9afde90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -15,10 +15,26 @@ type AutoNamingConfig struct { | |||||||||
RandomSuffixMinLength int `json:"randomSuffixMinLength"` | ||||||||||
} | ||||||||||
|
||||||||||
// EngineAutonamingConfiguration contains autonaming parameters passed to the provider from the engine. | ||||||||||
type EngineAutonamingConfiguration struct { | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious, why are we not reusing the autonaming structs and enum from pu-sdk? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great question. I started there but then went for keeping the |
||||||||||
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, | ||||||||||
engineAutonaming EngineAutonamingConfiguration, | ||||||||||
olds, | ||||||||||
news resource.PropertyMap, | ||||||||||
config *AutoNamingConfig, | ||||||||||
|
@@ -27,11 +43,13 @@ func ApplyAutoNaming( | |||||||||
return nil | ||||||||||
} | ||||||||||
// Auto-name fields if not already specified | ||||||||||
val, err := getDefaultName(randomSeed, urn, spec, olds, news, config) | ||||||||||
val, ok, err := getDefaultName(urn, engineAutonaming, spec, olds, news, config) | ||||||||||
if err != nil { | ||||||||||
return err | ||||||||||
} | ||||||||||
news[resource.PropertyKey(spec.SdkName)] = val | ||||||||||
if ok { | ||||||||||
news[resource.PropertyKey(spec.SdkName)] = *val | ||||||||||
} | ||||||||||
return nil | ||||||||||
} | ||||||||||
|
||||||||||
|
@@ -41,29 +59,57 @@ 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, | ||||||||||
engineAutonaming EngineAutonamingConfiguration, | ||||||||||
autoNamingSpec *metadata.AutoNamingSpec, | ||||||||||
olds, | ||||||||||
news resource.PropertyMap, | ||||||||||
config *AutoNamingConfig, | ||||||||||
) (resource.PropertyValue, error) { | ||||||||||
) (*resource.PropertyValue, bool, error) { | ||||||||||
mikhailshilkov marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
sdkName := autoNamingSpec.SdkName | ||||||||||
|
||||||||||
// Prefer explicitly specified name | ||||||||||
if v, ok := news[resource.PropertyKey(sdkName)]; ok { | ||||||||||
return v, nil | ||||||||||
return &v, true, nil | ||||||||||
} | ||||||||||
|
||||||||||
// Fallback to previous name if specified/set. | ||||||||||
if v, ok := olds[resource.PropertyKey(sdkName)]; ok { | ||||||||||
return v, nil | ||||||||||
return &v, true, nil | ||||||||||
} | ||||||||||
|
||||||||||
// Generate naming trivia for the resource. | ||||||||||
namingTriviaApplies, namingTrivia, err := CheckNamingTrivia(sdkName, news, autoNamingSpec.TriviaSpec) | ||||||||||
if err != nil { | ||||||||||
return resource.PropertyValue{}, err | ||||||||||
return nil, false, err | ||||||||||
} | ||||||||||
|
||||||||||
if engineAutonaming.AutonamingMode != nil { | ||||||||||
switch *engineAutonaming.AutonamingMode { | ||||||||||
case EngineAutonamingModeDisable: | ||||||||||
return nil, false, nil | ||||||||||
case EngineAutonamingModeEnforce: | ||||||||||
v := resource.NewStringProperty(engineAutonaming.ProposedName) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't we also validate the proposed name against the Same question for the naming trivia. If that is a requirement should we error here early? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's the intended semantics of enforce, as described in the spec. It can be used to overcome an erroneous autonaming configuration of the provider and override it to follow through to the AWS API. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is gonna be tricky for users right now. After reading pulumi/pulumi#17592 I couldn't find a way users could configure to shorten a logical name. IMO one of the reasons users would want to configure the autonaming behavior in aws-native is to get around naming constraints that are missing from the CFN schema. Usually that's a missing maxLength constraint. A substring expression would be great for autonaming. Something like this maybe? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, they can't shorten a logical name right now. I haven't seen this concern anywhere in the original issue or RFC discussion, but if it comes up often enough, we can add it later. The good news is that it will then work for all providers equally, not just AWS Native. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ok so it's like an escape hatch that users can use until the upstream bug is fixed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is an escape hatch too. More broadly, a lot of users expressed strong opinions that they want Pulumi to stop mangling with names, period. Now they can do that easily and we guarantee that users have full control. |
||||||||||
return &v, true, nil | ||||||||||
case EngineAutonamingModePropose: | ||||||||||
proposedName := engineAutonaming.ProposedName | ||||||||||
|
||||||||||
// Apply naming trivia to the generated name. | ||||||||||
if namingTriviaApplies { | ||||||||||
proposedName = ApplyTrivia(namingTrivia, proposedName) | ||||||||||
} | ||||||||||
|
||||||||||
// Validate the proposed name against the length constraints. | ||||||||||
if autoNamingSpec.MaxLength > 0 && len(proposedName) > autoNamingSpec.MaxLength { | ||||||||||
return nil, false, fmt.Errorf("proposed name %q exceeds max length of %d", proposedName, autoNamingSpec.MaxLength) | ||||||||||
} | ||||||||||
if autoNamingSpec.MinLength > 0 && len(proposedName) < autoNamingSpec.MinLength { | ||||||||||
return nil, false, fmt.Errorf("proposed name %q is shorter than min length of %d", proposedName, autoNamingSpec.MinLength) | ||||||||||
} | ||||||||||
|
||||||||||
v := resource.NewStringProperty(proposedName) | ||||||||||
return &v, true, nil | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
var autoTrim bool | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 pulumi-aws-native/provider/pkg/schema/gen.go Lines 188 to 191 in 11ac7b3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My original take was a no, but now I think I misunderstood There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I'm still not sure about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without/pre
It sounds like the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to what Cory said. I feel like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No
Could you please explain why this distinction? If a user configures a pattern like 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
But we can also keep it simple and throw an error for now. Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
yes |
||||||||||
|
@@ -94,7 +140,7 @@ func getDefaultName( | |||||||||
if left <= 0 && autoTrim { | ||||||||||
autoTrimMaxLength := autoNamingSpec.MaxLength - namingTrivia.Length() - randomSuffixMinLength | ||||||||||
if autoTrimMaxLength <= 0 { | ||||||||||
return resource.PropertyValue{}, fmt.Errorf("failed to auto-generate value for %[1]q."+ | ||||||||||
return nil, false, 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) | ||||||||||
|
@@ -106,12 +152,12 @@ func getDefaultName( | |||||||||
|
||||||||||
if left <= 0 { | ||||||||||
if namingTrivia.Length() > 0 { | ||||||||||
return resource.PropertyValue{}, fmt.Errorf("failed to auto-generate value for %[1]q."+ | ||||||||||
return nil, false, 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) | ||||||||||
} else { | ||||||||||
return resource.PropertyValue{}, fmt.Errorf("failed to auto-generate value for %[1]q."+ | ||||||||||
return nil, false, 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) | ||||||||||
} | ||||||||||
|
@@ -123,17 +169,18 @@ func getDefaultName( | |||||||||
} | ||||||||||
|
||||||||||
// Resource name is URN name + "-" + random suffix. | ||||||||||
random, err := resource.NewUniqueName(randomSeed, prefix, randLength, maxLength, nil) | ||||||||||
random, err := resource.NewUniqueName(engineAutonaming.RandomSeed, prefix, randLength, maxLength, nil) | ||||||||||
if err != nil { | ||||||||||
return resource.PropertyValue{}, err | ||||||||||
return nil, false, 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, true, nil | ||||||||||
} | ||||||||||
|
||||||||||
// trimName will trim the prefix to fit within the max length constraint. | ||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is auto generated, we'll need to change it here: https://github.com/pulumi/ci-mgmt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do this - I guess my first question is whether I should be configuring credentials here and why I'm the first one to need this. Should I be using some other place for end-2-end tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to do that for now. They already include upgrade tests and while they do not use any invokes right now, they might in the future and those need creds:
pulumi-aws-native/provider/pkg/provider/provider_2e2_test.go
Lines 19 to 31 in 11ac7b3
(Just saw that the upgrade tests are missing
-short
flag, I'll add that separately)Long term I'd like us to have a more standard test setup where we have a single go.mod on the root and then separate directories for tests in order to keep slower-running integration tests separate from unit tests. This would enable us to run unit tests without needing to remember
-short
or build tags. But that's all unrelated to your changes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working on CI in pulumi/ci-mgmt#1239 but also discussing offline what the best approach could be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pulumi/ci-mgmt#1239 is typically how we'd unblock this, but we're trying to do a couple things right now in the opposite direction:
For those reasons I'll suggest an alternative approach. Basically just perform the auth in-process as part of your test's setup. Actually, before you do that maybe just plumb the environment variables through, since the provider should login for you? So the test would get an environmnet
Otherwise, if you do need to perform the auth, something like this pseudocode:
Then pass those environment variables to the e2e test via
Env:
, or (worse) if you're lazy useos.Setenv
.If
provider/pkg/provider/provider_2e2_test.go
is the only place where you need these credentials, then I suggest defining a helper there likeloginToAWS(t)
. You can put it behind async.Once
to prevent logging in more than you need to.Give it a try, and if it turns into a rabbit hole we can merge the ci-mgmt PR and clean it up later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can give this a try... This would make it unusable in a local dev environment unless one replicates all env vars, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd personally vote for the
ci-mgmt
option because it doesn't mess with the local dev setup. The changes toci-mgmt
are not that big IMO and the aws-native test structure will have to change for the consolidation anywaysThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I prefer
ci-mgmt
too... I un-drafted that PR, happy to ship it and remove the CI change here after it percolates.