[Bug]: appautoscaling Policy does not support / (forward slash) in external name

[jamesdobson]

Is there an existing issue for this?

• I have searched the existing issues

Affected Resource(s)

• appautoscaling.aws.upbound.io/v1beta1 - Policy

Resource MRs required to reproduce the bug

I'm trying to adopt control of an already-existing policy using the following Policy:

apiVersion: appautoscaling.aws.upbound.io/v1beta1
kind: Policy
metadata:
  annotations:
    crossplane.io/external-name: DynamoDBReadCapacityUtilization:table/mytable
  name: mytable-policy
spec:
  forProvider:
    policyType: TargetTrackingScaling
    region: us-east-1
    resourceId: table/mytable
    serviceNamespace: dynamodb
    scalableDimension: dynamodb:table:ReadCapacityUnits
    targetTrackingScalingPolicyConfiguration:
      - predefinedMetricSpecification:
          - predefinedMetricType: DynamoDBReadCapacityUtilization
        targetValue: 70

Steps to Reproduce

1. Create an appautoscaling policy on a DynamoDB table where the policy name has a / in it
2. Try to adopt control of the policy using the above Policy

What happened?

1. The Policy gets "Ready" True but "Synced" False
2. The Policy conditions have the message update failed: async update failed: refuse to update the external resource because the following update requires replacing it: cannot change the value of the argument "name" from "DynamoDBReadCapacityUtilization:table/mytable" to "mytable"

Relevant Error Output Snippet

Crossplane Version

1.18.2

Provider Version

commit 506de87

Kubernetes Version

v1.31.4+k3s1

Kubernetes Distribution

k3d

Additional Info

TL;DR: I understand the problem and have a few ways to fix it. I'm unsure which is best and how to write automated tests for it; guidance is appreciated.

Why Can't You Use Something Other Than / (forward-slash) In The Name?

We have many DynamoDB tables with autoscaling policies that were created by the terraform-aws-dynamodb-table module. It's a project with 106 stars, 181 forks, and has been around for almost 5 years. The last update was in October.

This project uses the resource_id of an aws_appautoscaling_target to create the name for the aws_appautoscaling_policy resources. Since the resource_id will always contain a / (because of what AWS expects), this means that the name of the policy will always contain a / too.

The terraform-aws-dynamodb-table module appears to be popular enough that it's reasonable to assume there are other users out there who will come across this issue when they migrate to Crossplane.

What's Going Wrong?

In this provider the external name for aws_appautoscaling_policy uses config.TemplatedStringAsIdentifier("name", "{{ .parameters.service_namespace }}/{{ .parameters.resource_id }}/{{ .parameters.scalable_dimension }}/{{ .external_name }}").

This means that the ID in the Terraform state will be something like: dynamodb/table/mytable/dynamodb:table:ReadCapacityUnits/DynamoDBReadCapacityUtilization:table/mytable

It should be parsed like this:

• service_namespace: dynamodb
• resource_id: table/mytable
• scalable_dimension: dynamodb:table:ReadCapacityUnits
• external_name: DynamoDBReadCapacityUtilization:table/mytable

However, the logic in GetExternalNameFromTemplated only takes everything after the last /. It would be difficult to make GetExternalNameFromTemplated know which / characters are actually delimiters and which are part of the values.

When the GetExternalNameFn returns mytable instead of DynamoDBReadCapacityUtilization:table/mytable, the provider gets confused.

I've tested that it works if I use a non-slash character in the name of the policy. So it's definitely due to the /.

How Could It Be Fixed?

The name field contains the external name. It was set by SetIdentifierArgumentFn. I feel like GetExternalNameFn should just return the name field.

This can be fixed in this provider by doing something like this:

	"aws_appautoscaling_policy": config.NewExternalNameFrom(
		config.TemplatedStringAsIdentifier("name", "{{ .parameters.service_namespace }}/{{ .parameters.resource_id }}/{{ .parameters.scalable_dimension }}/{{ .external_name }}"),
		config.WithGetExternalNameFn(func(fn config.GetExternalNameFn, tfstate map[string]any) (string, error) {
			name, err := fieldpath.Pave(tfstate).GetString("name")
			if err != nil {
				return "", errors.Wrapf(err, "cannot get %s from tfstate to use as external name", "name")
			}

			return name, nil
		}),
	),

I do wonder why TemplatedStringAsIdentifier doesn't always do this, assuming the nameFieldPath is set. That would be a change to core upjet, but it could look something like this:

		GetExternalNameFn: func(tfstate map[string]any) (string, error) {
			if nameFieldPath != "" {
				name, err := fieldpath.Pave(tfstate).GetString(nameFieldPath)
				if err != nil {
					return "", errors.Wrapf(err, "cannot get %s from tfstate to use as external name", nameFieldPath)
				}

				return name, nil
			} else {
				id, ok := tfstate["id"]
				if !ok {
					return "", errors.New(errIDNotFoundInTFState)
				}

				return config.GetExternalNameFromTemplated(tmpl, id.(string))
			}
		},

Where Do I Need Guidance?

1. Is it ok to use the name field to get the external name, or are there some cases where this won't work, e.g. GetExternalNameFn is called after SetIdentifierArgumentFn?
2. Which approach to fixing is preferred?
3. Pointers for writing automated tests of this. The automated test will adopt an existing policy in AWS by creating a Policy with the external name set. I can't find any existing tests that do something like this, but I don't understand uptest well.

Thanks!