Support Resource Based Policy for DynamoDB #1339

ShayYannay · 2024-06-02T13:22:53Z

Description of your changes

Support Resource Based Policy for DynamoDB tables

I have:

Read and followed Crossplane's [contribution process].
Run make reviewable to ensure this PR is ready for review.
Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Tested locally using Uptest, verified that the resource policy get created for the DynamoDB table:

NAME                                    SYNCED   READY   EXTERNAL-NAME   AGE
table.dynamodb.aws.upbound.io/example   True     True    example         3m7s

NAME                                             SYNCED   READY   EXTERNAL-NAME   AGE
resourcepolicy.dynamodb.aws.upbound.io/example   True     True    example         3m7s

mbbush

Thanks for the contribution @ShayYannay!

I left a couple small suggestions. I'll also trigger our CI test for the new example manifest, which will hopefully provide some more debug output.

mbbush · 2024-06-05T21:52:57Z

examples/dynamodb/v1beta1/resourcepolicy.yaml

+              "dynamodb:DeleteItem"
+            ],
+            "Resource": [
+              "arn:aws:dynamodb:us-east-2:123456789012:table/example"


Suggested change

"arn:aws:dynamodb:us-east-2:123456789012:table/example"

"arn:aws:dynamodb:us-east-2:${data.aws_account_id}:table/example"

This example manifest is used by uptest, our testing pipeline, which will interpolate this pattern against a config file to plug in the current aws account id, so that the ARN is correct and the policy should work.

Thanks @mbbush I will fix that.

On another topic I was able to run the project locally using make run add provider config and create dynamdb table in my aws account but as for resource based policy attached to that table it didn't worked, got this error:
read resource request failed: parsing resource ID: arn: invalid prefix

I think that my changes in config/dynamodb/config.go are ignored when running locally since in config/provider.go the import of the artifact is remotely and not locally.
That can explain why I still seeing synced value of false and the arn parsing error above:

NAME SYNCED READY EXTERNAL-NAME AGE LABELS resourcepolicy.dynamodb.aws.upbound.io/example False 30m testing.upbound.io/example-name=example

And on another note how do you debug the project ? or add some logs to it ? it would have been helpful to understand why the error message that I got occurred (maybe the arn is empty or using the external name of the dynamodb table not sure...)

If I can't relay on local testing to work can I just make sure it passed in Uptest?

Please advise and thanks! for answering.

The error message I see is in the status condition, complaining about being unable to observe the resource.

Look at https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/9395450353, possibly also at the provider pod logs which should be attached to the run as an artifact, which might have more details.

For running locally, I recommend make e2e UPTEST_EXAMPLES_LIST=... There are one or two other env vars you'll have to set in order to get your credentials into the provider config, and your AWS account id into the substitution. I think they're documented in the up test repo? Or maybe just reading the comments in the makefile in this project? This builds the appropriate provider subpackages for the manifest selected, starts a kind cluster, installs crossplane, side loads the built provider images onto the cluster, installs a provider config, and runs uptest.

Thanks, In order to look at the provider pod logs I will need to either run it in uptest locally (since it runs under crossplane k8 deployment) or check the uptest action in github logs , is that correct?
From what I saw when running out of cluster (using `make run) the logs are printed only to the console but they didn't helped much.

BTW saw that you trirgger this action: https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/9397396899
I finished all the commits would like to run the e2e completely ? (saw only debug job ran)

@mbbush I've ran with Uptest locally and still got the same error :(

logger.go:42: 16:03:58 | case/0-apply | status: logger.go:42: 16:03:58 | case/0-apply | atProvider: {} logger.go:42: 16:03:58 | case/0-apply | conditions: logger.go:42: 16:03:58 | case/0-apply | - lastTransitionTime: "2024-06-06T12:57:50Z" logger.go:42: 16:03:58 | case/0-apply | message: 'observe failed: read resource request failed: parsing resource ID: logger.go:42: 16:03:58 | case/0-apply | arn: invalid prefix' logger.go:42: 16:03:58 | case/0-apply | reason: ReconcileError logger.go:42: 16:03:58 | case/0-apply | status: "False" logger.go:42: 16:03:58 | case/0-apply | type: Synced

I've also check the dynamodb provider logs and there wasn't something new, got the same error as running with out of cluster:

2024-06-06T13:15:07Z DEBUG provider-aws Cannot observe external resource {"controller": "managed/dynamodb.aws.upbound.io/v1beta1, kind=resourcepolicy", "request": {"name":"example"}, "uid": "542edb03-cdc2-414c-8991-fd2f4d307f45", "version": "1142", "external-name": "", "error": "read resource request failed: parsing resource ID: arn: invalid prefix", "errorVerbose": "parsing resource ID: arn: invalid prefix\ngithub.com/crossplane/upjet/pkg/controller.getFatalDiagnostics\n\tgithub.com/crossplane/upjet@

BTW for running locally using uptest (in case someone reading this and seeking for help) 2 environment variables need to be set (@mbbush took it from the Makefile, thanks for that)

export UPTEST_CLOUD_CREDENTIALS export UPTEST_EXAMPLE_LIST

Than run make e2e UPTEST_EXAMPLES_LIST="examples/....

So in short i'm stuck any help will be welcomed :)

I'm on-the-go at the moment and can't easily run the tests myself, but one thing I'd try would be kubectl get -o yaml --watch ResourcePolicy before starting your test, so you can see every revision of the object. I wonder if there's some issue with the way the dynamodb table's ARN is getting resolved, leading to an invalid value, that's putting the provider into an error state that it's failing to recover from as it should.

Something else to try would be to create a ResourcePolicy with the full ARN of the dynamodb table in spec.forProvider.resourceArn and/or in the crossplane.io/external-name annotation from the very start, and see if that changes any behavior.

None of these would be proper fixes, but they might help shed some light on what's happening so I can suggest what a proper fix would be.

Regarding the env vars, I may well have misquoted the pluralization of UPTEST_EXAMPLES_LIST (or is it UPTEST_EXAMPLE_LIST?), so go by whatever's in the makefile, not my comment. There's also another env var, UPTEST_DATASOURCE_PATH, which you should set to the path to a yaml file containing aws_account_id: '123456789012' or whatever your account id is.

I set the arn specifically that didn't help as well (with resourceArn), I will try to use watch on the ResourcePolicy maybe that will shed some light.
I've starting to think that maybe the terraform code itself return some other value than just a pure arn which make the provider fail, I'm testing with terraform command to see if there something strange, though status.atProvier.arn for the table look ok on the console logs of Uptest.

I see the same issue of arn logs in Uptest in github action:
https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/9401774623/job/25894339632
So it's not something with my local env.

mbbush · 2024-06-05T21:54:22Z

config/externalname.go

@@ -902,7 +907,7 @@ var TerraformPluginSDKExternalNameConfigs = map[string]config.ExternalName{
 	// DynamoDB Global Tables can be imported using the global table name
 	"aws_dynamodb_global_table": config.NameAsIdentifier,
 	// aws_dynamodb_tag can be imported by using the DynamoDB resource identifier and key, separated by a comma (,)
-	"aws_dynamodb_tag": config.TemplatedStringAsIdentifier("", "{{ .parameters.resource_arn }},{{ .parameters.key }}"),
+	"aws_dynamodb_tag": config.TemplatedStringAsIdentifier("", "{{ .parameters.resource_arn }},{{ .parameters.key }}"),	


oops, looks like some accidentally-committed whitespace.

mbbush · 2024-06-06T05:08:09Z

/test-examples=examples/dynamodb/v1beta1/resourcepolicy.yaml

@ShayYannay Unfortunately you won't be able to trigger this yourself until after your first PR is merged.

mbbush · 2024-06-06T05:10:53Z

/test-examples="examples/dynamodb/v1beta1/resourcepolicy.yaml"

mbbush · 2024-06-06T08:12:09Z

Each individual commit needs to be signed off. The output of the DCO job will tell you how to fix it.

ShayYannay · 2024-06-06T11:53:51Z

/test-examples="examples/dynamodb/v1beta1/resourcepolicy.yaml"

turkenf · 2024-06-06T13:27:53Z

/test-examples="examples/dynamodb/v1beta1/resourcepolicy.yaml"

ShayYannay · 2024-06-08T10:33:49Z

@mbbush I have read some nice documentation on how upjet use cross resources dependencies, from what I understand the generated examples use the terraform registry in order to correlated between the resources (e.g resource policy to table in DynamoDB):
https://github.com/crossplane/upjet/blob/main/docs/configuring-a-resource.md#cross-resource-referencing

Tried to add Type in config.Reference explicitly to DynamoDB table (e.g https://pkg.go.dev/github.com/upbound/provider-aws/apis/dynamodb/v1beta1#Table) saw the make generate did not changed anything since the generated code already automatically inferred the Type
Saw that in the terraform registry for dynamodb policy the example is missing aws_dynamodb_table e.g the generated example for resource policy does not include the table as depended manifest.
I suspect that the external name for resource policy create some trouble correlating between ResourcePolicy and Table managed resources.
I used in my code "aws_dynamodb_resource_policy": config.IdentifierFromProvider, and put it under TerraformPluginFrameworkExternalNameConfigs (saw that its framework and not sdk according to the terraform source code), but could be that this configuration is also wrong since resource policy attached to table is not IAM resource rather than configuration to the Table, another assumption is the terraform state used to parse the arn is missed because of this configuration.

Please advise, thanks!

ShayYannay · 2024-06-08T20:20:13Z

@mbbush Finally figured it out!!
The resource policy id in dynamodb is equal to the dybamodb table arn, e.g the external name should be from template (see my last commit), able to understand that by looking carefully at the terraform state file on the id property.
I've ran Uptest locally and now it pass

mbbush · 2024-06-08T23:42:48Z

I've pieced together what the error message means now.

It strikes me as potentially relevant that none of the existing framework providers use the IdentifierFromProvider external name config. It looks like there's some bug in the upjet Framework client that sends invalid read requests when the resource has not yet been created. I can try to track that down separately from your PR.

As far as what external name configuration you should use, I'd recommend config.TemplatedStringAsIdentifier("", "{{ .parameters.resource_arn }}"),

This will result in the resource not reconciling at all until after it's resolved the reference to the dynamodb table it's referring to, and then as soon as that's done, it will know that's supposed to be the terraform id. The "" is simply to indicate that there's no field in the resource's spec that should be removed and replaced with the external name.

Can you make that change, rebase, and run codegen again, and then I can uptest it in github actions?

One development practice that I try to follow when working on this provider is to put all of the generated changes in separate commits. Then when I'm rebasing, I can just drop all of those commits, not have to deal with conflicts in generated files, and just rerun make generate.

ShayYannay · 2024-06-13T05:35:29Z

Thanks! @mbbush I will follow your recommendations and change that.

ShayYannay · 2024-06-16T06:55:53Z

@mbbush tried changing in config/externalname.go
from "aws_dynamodb_resource_policy": config.TemplatedStringAsIdentifier("name", "arn:aws:dynamodb:{{ .setup.configuration.region }}:{{ .setup.client_metadata.account_id }}:table/{{ .external_name }}"),
to "aws_dynamodb_resource_policy": config.TemplatedStringAsIdentifier("", "{{ .parameters.resource_arn }}"),

However got this error when running make generate:

angryjet: error: error loading packages using pattern ./...: provider-upjet-aws-dynamodb-rbp/apis/dynamodb/v1beta1/zz_generated.resolvers.go:194:62: mg.Spec.InitProvider.ResourceArn undefined (type ResourcePolicyInitParameters has no field or method ResourceArn)
exit status 1
apis/generate.go:34: running "go": exit status 1
09:30:33 [FAIL]
make[1]: *** [go.generate] Error 1
make: *** [generate] Error 2

Do you know why?

mbbush · 2024-06-19T04:32:13Z

If you delete all the generated resolvers files before running make generate, you won't get that error.

ShayYannay · 2024-06-20T05:51:17Z

Got it I will check it out thanks

turkenf · 2024-07-02T18:45:08Z

Hi @ShayYannay,

Many thanks for your effort in this PR. Would you like to move forward with this PR?

If you have not time to work on this, please let us know.

turkenf · 2024-07-05T11:34:44Z

/test-examples="examples/dynamodb/v1beta1/resourcepolicy.yaml"

turkenf · 2024-07-08T17:35:19Z

/test-examples="examples/dynamodb/v1beta1/resourcepolicy.yaml"

Signed-off-by: Shay Yannay <[email protected]>

turkenf · 2024-07-08T20:01:33Z

Covered in #1392

ShayYannay · 2024-07-09T05:34:43Z

@turkenf thanks for reaching closure on this PR, I was busy with other tasks, saw the new PR in here
I'll be glad to contribute to this great project in the future if we stumbled on aws provider features that integrate in our work.

@mbbush many thanks for the guidance in this PR.

ShayYannay mentioned this pull request Jun 2, 2024

DynamoDB Table Resource Based Policy Support #1307

Closed

mbbush reviewed Jun 6, 2024

View reviewed changes

ShayYannay marked this pull request as ready for review June 6, 2024 06:06

ShayYannay requested review from ulucinar, sergenyalcin and turkenf as code owners June 6, 2024 06:06

ShayYannay force-pushed the dynamodb-resource-based-policy branch from ddd209e to e4d4c51 Compare June 6, 2024 08:22

turkenf force-pushed the dynamodb-resource-based-policy branch from f151a44 to 0b8776b Compare July 8, 2024 17:59

ShayYannay added 5 commits July 8, 2024 21:08

Support resource based policy for dynamodb

82c78f1

Signed-off-by: Shay Yannay <[email protected]>

remove whitespace

ab8a8f9

Signed-off-by: Shay Yannay <[email protected]>

use variable for aws account id

57d1aa3

Signed-off-by: Shay Yannay <[email protected]>

Signoff empty commit

2583c39

Signed-off-by: Shay Yannay <[email protected]>

fix resource policy external name

0847d9f

Signed-off-by: Shay Yannay <[email protected]>

turkenf force-pushed the dynamodb-resource-based-policy branch from 0b8776b to 0847d9f Compare July 8, 2024 18:10

turkenf closed this Jul 8, 2024

mergenci mentioned this pull request Jul 10, 2024

Configure ResourcePolicy.dynamodb resource and add example #1392

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Resource Based Policy for DynamoDB #1339

Support Resource Based Policy for DynamoDB #1339

ShayYannay commented Jun 2, 2024 •

edited

Loading

mbbush left a comment

mbbush Jun 5, 2024

ShayYannay Jun 6, 2024

mbbush Jun 6, 2024

ShayYannay Jun 6, 2024

ShayYannay Jun 6, 2024

mbbush Jun 6, 2024

mbbush Jun 6, 2024

ShayYannay Jun 6, 2024 •

edited

Loading

mbbush Jun 5, 2024

mbbush commented Jun 6, 2024

mbbush commented Jun 6, 2024

mbbush commented Jun 6, 2024

ShayYannay commented Jun 6, 2024

turkenf commented Jun 6, 2024

ShayYannay commented Jun 8, 2024

ShayYannay commented Jun 8, 2024

mbbush commented Jun 8, 2024

ShayYannay commented Jun 13, 2024

ShayYannay commented Jun 16, 2024

mbbush commented Jun 19, 2024

ShayYannay commented Jun 20, 2024

turkenf commented Jul 2, 2024

turkenf commented Jul 5, 2024

turkenf commented Jul 8, 2024

turkenf commented Jul 8, 2024

ShayYannay commented Jul 9, 2024

	"arn:aws:dynamodb:us-east-2:123456789012:table/example"
	"arn:aws:dynamodb:us-east-2:${data.aws_account_id}:table/example"

Support Resource Based Policy for DynamoDB #1339

Support Resource Based Policy for DynamoDB #1339

Conversation

ShayYannay commented Jun 2, 2024 • edited Loading

Description of your changes

How has this code been tested

mbbush left a 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

ShayYannay Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbbush commented Jun 6, 2024

mbbush commented Jun 6, 2024

mbbush commented Jun 6, 2024

ShayYannay commented Jun 6, 2024

turkenf commented Jun 6, 2024

ShayYannay commented Jun 8, 2024

ShayYannay commented Jun 8, 2024

mbbush commented Jun 8, 2024

ShayYannay commented Jun 13, 2024

ShayYannay commented Jun 16, 2024

mbbush commented Jun 19, 2024

ShayYannay commented Jun 20, 2024

turkenf commented Jul 2, 2024

turkenf commented Jul 5, 2024

turkenf commented Jul 8, 2024

turkenf commented Jul 8, 2024

ShayYannay commented Jul 9, 2024

ShayYannay commented Jun 2, 2024 •

edited

Loading

ShayYannay Jun 6, 2024 •

edited

Loading