Skip to content

RDS: Add support for InstanceState - #1794

Merged
turkenf merged 4 commits into
crossplane-contrib:mainfrom
ytsarev:rds_instance_state
Jun 13, 2025
Merged

RDS: Add support for InstanceState#1794
turkenf merged 4 commits into
crossplane-contrib:mainfrom
ytsarev:rds_instance_state

Conversation

@ytsarev

@ytsarev ytsarev commented May 29, 2025

Copy link
Copy Markdown
Contributor

Description of your changes

Add support for https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/rds_instance_state

I have:

  • Read and followed Crossplane's contribution process.
  • Run make generate and committed the results (ideally in a separate commit).
  • Not made any manual changes to generated files, and verified this with make check-diff.

How has this code been tested

k get -f examples/rds/v1beta1/instancestate.yaml
NAME                                                     SYNCED   READY   EXTERNAL-NAME           AGE
instancestate.rds.aws.upbound.io/example-instancestate   True     True    example-instancestate   21m

NAME                                                READY   SYNCED   EXTERNAL-NAME                   AGE
instance.rds.aws.upbound.io/example-instancestate   True    True     db-VZVRXGW5NIADDI3D3VF4MOBHVE   21m

Change state: available to state: stopped in InstanceState spec

apiVersion: rds.aws.upbound.io/v1beta1
kind: InstanceState
metadata:
  annotations:
    meta.upbound.io/example-id: rds/v1beta1/instancestate
  labels:
    testing.upbound.io/example-name: instancestate
  name: example-instancestate
spec:
  forProvider:
    identifierSelector:
      matchLabels:
        testing.upbound.io/example-name: instancestate
    region: us-west-1
    state: stopped <--- HERE

Reapply manifest, observe that the target RDS instance is getting stopped in the console, first Stopping... then eventually getting to Stopped Temporarily

image

@turkenf

turkenf commented May 29, 2025

Copy link
Copy Markdown
Collaborator

/test-examples="examples/rds/v1beta1/instancestate.yaml"

2 similar comments
@turkenf

turkenf commented May 29, 2025

Copy link
Copy Markdown
Collaborator

/test-examples="examples/rds/v1beta1/instancestate.yaml"

@turkenf

turkenf commented May 30, 2025

Copy link
Copy Markdown
Collaborator

/test-examples="examples/rds/v1beta1/instancestate.yaml"

Comment thread config/externalname.go Outdated
// rds
//
// aws_rds_instance_state import format: rds_instance_state-instanceId-12345678
"aws_rds_instance_state": config.TemplatedStringAsIdentifier("", "rds_instance_state-{{ .parameters.identifier }}"),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The import section in the Terraform document seemed a bit contradictory to me. The document gives two different formats for importing this resource.

  1. db-L72FUFBZX2RRXT3HOJSIUQVOKE
  2. rds_instance_state-instanceId-12345678

The second format and the one we are using here seems a bit suspicious to me. We cannot observe the import test because the uptest is currently failing due to a connection error. Did you perform a manual import test?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

And from what I understand from the source code, the correct format seems to be the first one(db-...), can you please try the following configuration. And it would be great to verify this with an import test :)

Suggested change
"aws_rds_instance_state": config.TemplatedStringAsIdentifier("", "rds_instance_state-{{ .parameters.identifier }}"),
"aws_rds_instance_state": config.TemplatedStringAsIdentifier("", "{{ .parameters.identifier }}"),

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@turkenf I updated the simplified configuration according to your suggestion.

Observations:

  • Everything works in general according to the initial PR description. I still see example-instancestate 'internal' name as external name of instance state for some reason
k get -f examples/rds/v1beta1/instancestate.yaml
NAME                                                     SYNCED   READY   EXTERNAL-NAME           AGE
instancestate.rds.aws.upbound.io/example-instancestate   True     True    example-instancestate   15m

NAME                                                READY   SYNCED   EXTERNAL-NAME                   AGE
instance.rds.aws.upbound.io/example-instancestate   True    True     db-5XBUBIXX5VXQQTSMYIJTCTYRGQ   15m
  • Import test. Import works even when crossplane.io/external-name is whatever . The resolved forProvider.identifier takes presedence and drives the import.
k get -f examples/rds/v1beta1/instancestate.yaml
NAME                                                     SYNCED   READY   EXTERNAL-NAME   AGE
instancestate.rds.aws.upbound.io/example-instancestate   True     True    whatever        5m7s

NAME                                                READY   SYNCED   EXTERNAL-NAME                   AGE
instance.rds.aws.upbound.io/example-instancestate   True    True     db-5XBUBIXX5VXQQTSMYIJTCTYRGQ   26m

So from the functional standpoint everything is working, the default extetnal-name propagation is slightly confusing

@ytsarev
ytsarev force-pushed the rds_instance_state branch from 966e20a to 1322ea2 Compare May 30, 2025 13:33
@turkenf

turkenf commented Jun 3, 2025

Copy link
Copy Markdown
Collaborator

/test-examples="examples/rds/v1beta1/instancestate.yaml"

@turkenf

turkenf commented Jun 12, 2025

Copy link
Copy Markdown
Collaborator

/test-examples="examples/rds/v1beta1/instancestate.yaml"

ytsarev and others added 4 commits June 12, 2025 19:10
Signed-off-by: Yury Tsarev <yury@upbound.io>
Signed-off-by: Yury Tsarev <yury@upbound.io>
Signed-off-by: Fatih Türken <turkenf@gmail.com>
Signed-off-by: Fatih Türken <turkenf@gmail.com>
@turkenf
turkenf force-pushed the rds_instance_state branch from 32a768c to 41f50e1 Compare June 12, 2025 16:12
@turkenf

turkenf commented Jun 12, 2025

Copy link
Copy Markdown
Collaborator

/test-examples="examples/rds/v1beta1/instancestate.yaml"

https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/15615730328

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants