Implement autonaming configuration protocol #1919

mikhailshilkov · 2024-12-18T15:49:41Z

This PR implement the AWS Native part of pulumi/pulumi#1518. See pulumi/pulumi#17592 for the full design.

In short, pulumi/pulumi#17810 introduced the protobuf changes required for the provider-side implementation. With those, a provider can:

Declare that it supports autonaming configurations with a response flag in Configure
Accept two extra properties in CheckRequest: a proposed name and a mode to apply it

This PR applies those parameters to auto-name calculation if they are specified:

In Disabled mode, we don't calculate auto-names and let the resource validation fail if an explicit name is not specified. For that, I had to thread an extra bool flag through autonaming machinery.
In Enforce mode, we use whatever ProposedName is supplied by the engine
In Propose mode, we take the ProposedName but can also apply trivia (e.g. add a suffix, as in the e2e test), or check the max/min length

I added unit tests to cover most autonaming cases. Also, added an end-to-end test in YAML. Apparently, end-2-end tests had no AWS credentials configured, so I added those. Let me know if that's against the intention.

Resolves #1901

github-actions · 2024-12-18T15:50:54Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

codecov · 2024-12-18T16:27:41Z

Codecov Report

Attention: Patch coverage is 84.33735% with 13 lines in your changes missing coverage. Please review.

Project coverage is 51.10%. Comparing base (11ac7b3) to head (9afde90).

Files with missing lines	Patch %	Lines
provider/pkg/autonaming/application.go	91.07%	5 Missing ⚠️
provider/pkg/resources/mock_custom_resource.go	0.00%	4 Missing ⚠️
provider/pkg/provider/provider.go	88.23%	1 Missing and 1 partial ⚠️
provider/pkg/resources/extension_resource.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1919      +/-   ##
==========================================
+ Coverage   50.35%   51.10%   +0.75%     
==========================================
  Files          48       48              
  Lines        7000     7044      +44     
==========================================
+ Hits         3525     3600      +75     
+ Misses       3226     3192      -34     
- Partials      249      252       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

provider/pkg/autonaming/application.go

corymhall · 2024-12-18T18:01:47Z

provider/pkg/autonaming/application.go

+		case EngineAutonamingModeDisable:
+			return nil, false, nil
+		case EngineAutonamingModeEnforce:
+			v := resource.NewStringProperty(engineAutonaming.ProposedName)


Why don't we also validate the proposed name against the autoNamingSpec? I'm assuming that in the current case if the engineAutonaming.ProposedName > autoNamingSpec.MaxLength then the error will be thrown by the AWS service?

Same question for the naming trivia. If that is a requirement should we error here early?

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.

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?${substr(name, 56)}-${alphanum(7)}

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.

. It can be used to overcome an erroneous autonaming configuration of the provider and override it to follow through to the AWS API.

Ok so it's like an escape hatch that users can use until the upstream bug is fixed?

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.

corymhall · 2024-12-18T18:04:56Z

provider/pkg/autonaming/application.go

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

 	var autoTrim bool


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"},

},

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.

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.

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.

randomSuffixMinLength could be used to enforce a minimum length of the random suffix

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)

+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.

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:

If RandomSuffixMinLength is configured, throw an error, we can't do anything with it.

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?

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?

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

flostadler · 2024-12-19T09:19:12Z

.github/workflows/run-acceptance-tests.yml

@@ -140,6 +140,15 @@ jobs:
      with:
        name: pulumi-${{ env.PROVIDER }}-provider.tar.gz
        path: ${{ github.workspace }}/bin/provider.tar.gz
+    - name: Configure AWS Credentials


This file is auto generated, we'll need to change it here: https://github.com/pulumi/ci-mgmt

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?

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

func TestE2eSnapshots(t *testing.T) {

t.Run("logGroup-0.94.0", func(t *testing.T) {

t.Parallel()

test := newAwsTest(t, filepath.Join("testdata", "logGroup"))

testUpgradeFrom(t, test, "0.94.0")

})

t.Run("webAcl-0.94.0", func(t *testing.T) {

t.Parallel()

test := newAwsTest(t, filepath.Join("testdata", "webAcl"))

testUpgradeFrom(t, test, "0.94.0")

})

}

(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.

Working on CI in pulumi/ci-mgmt#1239 but also discussing offline what the best approach could be

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:

we want to reduce the number of exceptions for native providers in order to make it easier to consolidate them with bridged workflows

and we want to make CI tests more reproducible locally, which means reducing the reliance on CI-specific setup steps.

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

Env: []string{"AWS_ROLE_SESSION_NAME=" + os.Getenv("PROVIDER"), "AWS_ROLE_TO_ASSUME=" + os.Getenv("AWS_CI_ROLE_ARN"), ...}

Otherwise, if you do need to perform the auth, something like this pseudocode:

cfg := aws.Config{ Credentials: credentials.NewStaticCredentialsProvider(accessKey, secretKey, ""), Region: region, } c := sts.NewFromConfig(cfg) creds, err := c.AssumeRole(ctx, &sts.AssumeRoleInput{ RoleArn: aws.String(os.Getenv("AWS_CI_ROLE_ARN")), RoleSessionName: aws.String(os.Getenv("AWS_ROLE_SESSION_NAME")), }) require.NoError(t, err) // Set environment with creds

Then pass those environment variables to the e2e test via Env:, or (worse) if you're lazy use os.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 like loginToAWS(t). You can put it behind a sync.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.

I can give this a try... This would make it unusable in a local dev environment unless one replicates all env vars, right?

I'd personally vote for the ci-mgmt option because it doesn't mess with the local dev setup. The changes to ci-mgmt are not that big IMO and the aws-native test structure will have to change for the consolidation anyways

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.

flostadler · 2024-12-19T09:20:56Z

provider/pkg/autonaming/application.go

@@ -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 {


Curious, why are we not reusing the autonaming structs and enum from pu-sdk?

Great question. I started there but then went for keeping the autonaming module self-contained. It feels like a fairly complicated piece of "domain logic", so I wasn't sure I wanted to mix in RPC types. I'm not sure how much I bought by doing so - can be persuaded the other way.

provider/pkg/autonaming/application.go

flostadler · 2024-12-19T09:45:11Z

provider/pkg/autonaming/application.go

+		case EngineAutonamingModeDisable:
+			return nil, false, nil
+		case EngineAutonamingModeEnforce:
+			v := resource.NewStringProperty(engineAutonaming.ProposedName)


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?${substr(name, 56)}-${alphanum(7)}

flostadler · 2024-12-19T12:15:24Z

provider/pkg/provider/provider_2e2_test.go

@@ -30,6 +31,19 @@ func TestE2eSnapshots(t *testing.T) {
 	})
 }

+func TestAutonaming(t *testing.T) {


we should skip this test when running in short mode (i.e. unit tests only)

…y naming

) In pulumi/pulumi-aws-native#1919 I want to run an actual end-2-end test with `pulumitest` in AWS Native, which requires AWS credentials. In understand that a larger consolidation may be coming with #1101, but this change would allow me to land my test ahead of that. Please let me know if there is a better way of achieving the same outcome.

mikhailshilkov force-pushed the mikhailshilkov/autonaming-configuration branch from d6ef0f1 to 54c962e Compare December 18, 2024 16:09

Implement autonaming configuration protocol

859d741

mikhailshilkov force-pushed the mikhailshilkov/autonaming-configuration branch from dbaaf27 to a98c215 Compare December 18, 2024 16:45

mikhailshilkov marked this pull request as ready for review December 18, 2024 16:45

mikhailshilkov requested review from flostadler, corymhall and t0yv0 December 18, 2024 16:45

corymhall requested changes Dec 18, 2024

View reviewed changes

Add AWS credentials to run end2end tests

5bc1cf7

mikhailshilkov force-pushed the mikhailshilkov/autonaming-configuration branch from a98c215 to 5bc1cf7 Compare December 18, 2024 18:10

mikhailshilkov requested a review from corymhall December 18, 2024 18:53

flostadler reviewed Dec 19, 2024

View reviewed changes

Simplify getDefaultName signature

76f51ce

flostadler reviewed Dec 19, 2024

View reviewed changes

mikhailshilkov mentioned this pull request Dec 19, 2024

AWS Native: Add AWS credentials before Test Provider Library step pulumi/ci-mgmt#1239

Merged

mikhailshilkov added 2 commits December 20, 2024 15:19

Conflict if both engine and provider config are specified. Also, unif…

dae1852

…y naming

Skip e2e test if short

9afde90

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement autonaming configuration protocol #1919

Implement autonaming configuration protocol #1919

mikhailshilkov commented Dec 18, 2024 •

edited

Loading

github-actions bot commented Dec 18, 2024

codecov bot commented Dec 18, 2024 •

edited

Loading

corymhall Dec 18, 2024

mikhailshilkov Dec 18, 2024

flostadler Dec 19, 2024

mikhailshilkov Dec 19, 2024

corymhall Dec 19, 2024

mikhailshilkov Dec 19, 2024

corymhall Dec 18, 2024

mikhailshilkov Dec 18, 2024

mikhailshilkov Dec 18, 2024 •

edited

Loading

corymhall Dec 19, 2024

flostadler Dec 19, 2024

mikhailshilkov Dec 19, 2024

corymhall Dec 20, 2024

mikhailshilkov Dec 20, 2024

flostadler Dec 19, 2024

mikhailshilkov Dec 19, 2024

flostadler Dec 19, 2024

mikhailshilkov Dec 19, 2024 •

edited

Loading

blampe Dec 19, 2024

mikhailshilkov Dec 19, 2024

flostadler Dec 20, 2024

mikhailshilkov Dec 20, 2024

flostadler Dec 19, 2024

mikhailshilkov Dec 19, 2024

flostadler Dec 19, 2024

flostadler Dec 19, 2024

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

	func TestE2eSnapshots(t *testing.T) {
	t.Run("logGroup-0.94.0", func(t *testing.T) {
	t.Parallel()
	test := newAwsTest(t, filepath.Join("testdata", "logGroup"))
	testUpgradeFrom(t, test, "0.94.0")
	})

	t.Run("webAcl-0.94.0", func(t *testing.T) {
	t.Parallel()
	test := newAwsTest(t, filepath.Join("testdata", "webAcl"))
	testUpgradeFrom(t, test, "0.94.0")
	})
	}

Implement autonaming configuration protocol #1919

Are you sure you want to change the base?

Implement autonaming configuration protocol #1919

Conversation

mikhailshilkov commented Dec 18, 2024 • edited Loading

github-actions bot commented Dec 18, 2024

Does the PR have any schema changes?

codecov bot commented Dec 18, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikhailshilkov Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikhailshilkov Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikhailshilkov commented Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 18, 2024 •

edited

Loading

mikhailshilkov Dec 18, 2024 •

edited

Loading

mikhailshilkov Dec 19, 2024 •

edited

Loading