Skip to content
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

Feature/elasticache user group association #1648

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

ido-re
Copy link

@ido-re ido-re commented Jan 24, 2025

Description of your changes

Allow Elasticach users to associate with Elasticach Usergroup.
This resource will allow you to create Elasticach (Redis) User and associate with an existing user group

Fixes #1620

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

@ido-re ido-re force-pushed the feature/elasticache_user_group_association branch from 152bdba to 6a85467 Compare January 24, 2025 10:42
@ido-re
Copy link
Author

ido-re commented Jan 24, 2025

/test-examples="examples/elasticache/v1beta1/usergroupassociation.yaml"

@ido-re
Copy link
Author

ido-re commented Jan 24, 2025

@ulucinar @sergenyalcin @turkenf @mbbush - I think the PR is read for review :)

@turkenf
Copy link
Collaborator

turkenf commented Jan 24, 2025

/test-examples="examples/elasticache/v1beta1/usergroupassociation.yaml"

@ido-re
Copy link
Author

ido-re commented Jan 25, 2025

@turkenf I see the uptest has failed, do I need to do something?

@turkenf
Copy link
Collaborator

turkenf commented Jan 27, 2025

Hi @ido-re,
Congratulations on your first contribution 🎉

@turkenf I see the uptest has failed, do I need to do something?

You can check why it failed in the uptest logs. The test failed with the error below, sometimes the connection error can be temporary. I will trigger the test again. If it fails with the same error, please try using the v1beta1 versions of all resources in the example.

case.go:363: conversion webhook for elasticache.aws.upbound.io/v1beta2, Kind=User failed: Post "https://provider-aws-elasticache.upbound-system.svc:9443/convert?timeout=30s": dial tcp 10.96.250.111:9443: connect: connection refused
    case.go:363: conversion webhook for elasticache.aws.upbound.io/v1beta2, Kind=User failed: Post "https://provider-aws-elasticache.upbound-system.svc:9443/convert?timeout=30s": dial tcp 10.96.250.111:9443: connect: connection refused

@turkenf
Copy link
Collaborator

turkenf commented Jan 27, 2025

/test-examples="examples/elasticache/v1beta1/usergroupassociation.yaml"

@ido-re ido-re force-pushed the feature/elasticache_user_group_association branch from edaa4da to 09b69d9 Compare January 27, 2025 10:13
@ido-re
Copy link
Author

ido-re commented Jan 27, 2025

@turkenf - thank you, Following your suggestion, I changed the example files to use v1beta1

@turkenf
Copy link
Collaborator

turkenf commented Jan 27, 2025

@turkenf - thank you, Following your suggestion, I changed the example files to use v1beta1.

Thanks for the update @ido-re,

I think you have accidentally updated the generated example here. This is not required since we are not using the generated examples when testing.

Can you please change the default User resource version under the examples folder instead?

@ido-re ido-re force-pushed the feature/elasticache_user_group_association branch from b518b4c to 9e34318 Compare January 27, 2025 16:32
@ido-re
Copy link
Author

ido-re commented Jan 27, 2025

@turkenf - thank you, Following your suggestion, I changed the example files to use v1beta1.

Thanks for the update @ido-re,

I think you have accidentally updated the generated example here. This is not required since we are not using the generated examples when testing.

Can you please change the default User resource version under the examples folder instead?

ho I missed it, fixed it now @turkenf

@turkenf
Copy link
Collaborator

turkenf commented Jan 27, 2025

/test-examples="examples/elasticache/v1beta1/usergroupassociation.yaml"

@ido-re ido-re force-pushed the feature/elasticache_user_group_association branch from 8d49958 to 5d95111 Compare January 28, 2025 06:58
@ido-re
Copy link
Author

ido-re commented Jan 28, 2025

/test-examples="examples/elasticache/v1beta1/usergroupassociation.yaml"

@turkenf is it possible to run again the automation test?
I rollbacked the changes I made on examples-generated/elasticache/v1beta1/usergroupassociation.yaml following your comment and because the diff test was failed

@turkenf
Copy link
Collaborator

turkenf commented Jan 28, 2025

/test-examples="examples/elasticache/v1beta1/usergroupassociation.yaml"

@ido-re
Copy link
Author

ido-re commented Jan 28, 2025

@turkenf I see the last failer is:
uptest-v0.13.0: error: cannot run e2e tests successfully: cannot execute tests: kuttl failed: exit status 1
Do you have suggestions for solving the issue?

@turkenf
Copy link
Collaborator

turkenf commented Jan 28, 2025

@turkenf I see the last failer is:
uptest-v0.13.0: error: cannot run e2e tests successfully: cannot execute tests: kuttl failed: exit status 1
Do you have suggestions for solving the issue?

If you scroll through the logs in the Uptest run, you can see messages in the conditions section of the resources about why the test failed. For example, I see the following message in the UserGroupAssociation conditions:

    logger.go:42: 10:04:35 | case/0-apply |     - lastTransitionTime: "2025-01-28T10:04:03Z"
    logger.go:42: 10:04:35 | case/0-apply |       message: 'create failed: async create failed: failed to create the resource:
    logger.go:42: 10:04:35 | case/0-apply |         [{0 creating ElastiCache User Group Association (example,example): operation
    logger.go:42: 10:04:35 | case/0-apply |         error ElastiCache: ModifyUserGroup, https response error StatusCode: 404,
    logger.go:42: 10:04:35 | case/0-apply |         RequestID: 9c32e451-ecc9-45bb-be9c-fb43aaf1b603, UserNotFound: User example
    logger.go:42: 10:04:35 | case/0-apply |         not found.  []}]'

My advice to you here is to first test/validate the resource manually. Please see: Running Uptest locally, Manual Test

@ido-re ido-re force-pushed the feature/elasticache_user_group_association branch from d8caf26 to 6d28138 Compare February 5, 2025 11:58
@ido-re
Copy link
Author

ido-re commented Feb 5, 2025

2025-01-28T10:04:03Z

@turkenf - sorry for the late response - I tested it locally and I think the issue was found, can you please rerun the uptest?
I changed the user to IAM user type to avoid using more dependencies like secrete in the test.
Also because I need user-group, I copied the user-group test example from the existing one from "examples/elasticache/v1beta1/usergroup.yaml"

@jeanduplessis
Copy link
Collaborator

/test-examples="examples/elasticache/v1beta1/usergroupassociation.yaml"

@ido-re
Copy link
Author

ido-re commented Feb 5, 2025

@turkenf @jeanduplessis - I see failers of different tests even I rebase my commits, I'll be happy to get your direction

@ido-re ido-re force-pushed the feature/elasticache_user_group_association branch from 2b27291 to 7c97698 Compare February 6, 2025 07:45
@ido-re
Copy link
Author

ido-re commented Feb 6, 2025

@turkenf @jeanduplessis - I see failers of different tests even I rebase my commits, I'll be happy to get your direction

okay - I rebased again - @turkenf @jeanduplessis is it possible to rerun the tests?

@jeanduplessis
Copy link
Collaborator

/test-examples="examples/elasticache/v1beta1/usergroupassociation.yaml"

@ido-re ido-re force-pushed the feature/elasticache_user_group_association branch from be434b8 to 24e5bab Compare February 6, 2025 13:40
@turkenf
Copy link
Collaborator

turkenf commented Feb 6, 2025

@ido-re, for the check-diff error, please make sure you run make generate and push the changes.

@ido-re
Copy link
Author

ido-re commented Feb 6, 2025

@ido-re, for the check-diff error, please make sure you run make generate and push the changes.

Yes - I did it, thanks

@ido-re
Copy link
Author

ido-re commented Feb 6, 2025

@ido-re, for the check-diff error, please make sure you run make generate and push the changes.

@turkenf can you please explain the steps? because I ran "make generate" before my last commit and still get the check-diff error
image

@turkenf
Copy link
Collaborator

turkenf commented Feb 6, 2025

@ido-re, please ignore the check-diff error for now; once this PR is merged, you can rebase your branch, and the error will go away. I will trigger a test with a comment below, please do not make any changes until the uptest is completed to see the results.

@turkenf
Copy link
Collaborator

turkenf commented Feb 6, 2025

/test-examples="examples/elasticache/v1beta1/usergroupassociation.yaml"

@turkenf
Copy link
Collaborator

turkenf commented Feb 6, 2025

@turkenf - sorry for the late response - I tested it locally and I think the issue was found, can you please rerun the uptest?

If you have successfully tested the resource manually, it would be very helpful if you could fill in the How has this code been tested section. Thank you 🙏

@ido-re
Copy link
Author

ido-re commented Feb 7, 2025

@turkenf - sorry for the late response - I tested it locally and I think the issue was found, can you please rerun the uptest?

If you have successfully tested the resource manually, it would be very helpful if you could fill in the How has this code been tested section. Thank you 🙏

@turkenf Because the failed is for the user creating, I tested it, and something so strange:

  1. From some reason the uptest is running the API on ver v1beta2 when in the example I'm using v1beta1
    ref from uptest:
    ` logger.go:42: 19:54:42 | case/0-apply | - apiVersion: elasticache.aws.upbound.io/v1beta2
    logger.go:42: 19:54:42 | case/0-apply | kind: User
    logger.go:42: 19:54:42 | case/0-apply | metadata:
    logger.go:42: 19:54:42 | case/0-apply | annotations:
    logger.go:42: 19:54:42 | case/0-apply | crossplane.io/external-create-failed: "2025-02-06T19:54:30Z"
    '

  2. In my environment I'm successful in creating the Redis user with the IAM type but here it failed

ref from uptest:
'
logger.go:42: 19:54:42 | case/0-apply | - lastTransitionTime: "2025-02-06T19:41:30Z"
logger.go:42: 19:54:42 | case/0-apply | reason: Creating
logger.go:42: 19:54:42 | case/0-apply | status: "False"
logger.go:42: 19:54:42 | case/0-apply | type: Ready
logger.go:42: 19:54:42 | case/0-apply | - lastTransitionTime: "2025-02-06T19:54:30Z"
logger.go:42: 19:54:42 | case/0-apply | message: 'create failed: async create failed: failed to create the resource:
logger.go:42: 19:54:42 | case/0-apply | [{0 creating ElastiCache User (example): operation error ElastiCache: CreateUser,
logger.go:42: 19:54:42 | case/0-apply | https response error StatusCode: 400, RequestID: 37481c17-2f16-4e78-82f2-ee683d17f1f2,
logger.go:42: 19:54:42 | case/0-apply | InvalidParameterCombination: User Id and User name must be same for authentication
logger.go:42: 19:54:42 | case/0-apply | type: iam []}]'
`

ref from my env:
image

image

@turkenf
Copy link
Collaborator

turkenf commented Feb 7, 2025

Ok @ido-re, thanks for the explanation. When I find time, I will check your PR in detail and test it manually.

@ido-re ido-re force-pushed the feature/elasticache_user_group_association branch from 03a8dc8 to ad4d44d Compare February 10, 2025 10:40
@ido-re
Copy link
Author

ido-re commented Feb 10, 2025

@turkenf finally I finished successfully running the local uptest (was blocked by the security team before) and now it's working properly with the results below:

  • new "test" user-group" created with existing "default" user
  • new "exampleuser" created
  • "exampleuser" attached to the "test" group and the "test" group includes 2 users "default" and "exampleuser"
  • all the new resource terminated in the end of the uptest
    • test on my dev was account
      the successfully example testes are ready for your remote uptest

@turkenf
Copy link
Collaborator

turkenf commented Feb 10, 2025

/test-examples="examples/elasticache/v1beta1/usergroupassociation.yaml"

@ido-re
Copy link
Author

ido-re commented Feb 11, 2025

@turkenf I see all the tests are passed, should I need to do something?

@turkenf
Copy link
Collaborator

turkenf commented Feb 11, 2025

@ido-re, I appreciate your effort here; no for now, thanks 🙏 I will review the changes in the coming days and do my best to add this resource to the minor release we will make at the end of February.

@turkenf
Copy link
Collaborator

turkenf commented Feb 12, 2025

Hello @ido-re,

Before I started reviewing this PR, I noticed the following note in the Terraform documentation, which was a bit annoying:

Screenshot 2025-02-12 at 17 05 10

According to this note, it seems that the aws_elasticache_user_group_association resource may not work properly with the provider in its current state (to be confirmed with the team). It’s a bit frustrating to think that all the effort put into this PR might go to waste, but I appreciate the work done here.

Let me roughly explain the situation; since Crossplane continuously reconciles resources, there is a risk that UserGroup and UserGroupAssociation may conflict with each other. Specifically, UserGroupAssociation modifies the userIds of UserGroup, which Crossplane might then detect as a drift and attempt to revert, which can lead to an endless reconcile loop. I will share with you the results of the manual tests I conducted with the image I produced from this PR, which confirms this situation:

I created the resources User, UserGroup and UserGroupAssociation. The resources were created successfully and I verified in the console that users were added to the UserGroup resource:

Screenshot 2025-02-12 at 16 46 01

After the first reconciliation, encountered the following diff in the logs and the UserGroupAssociation resources were set to Ready=False

2025-02-12T16:46:56+03:00	DEBUG	provider-aws	Diff detected	{"uid": "7840d46e-f1ba-4945-9559-036328179136", "name": "test", "gvk": "elasticache.aws.upbound.io/v1beta1, Kind=UserGroup", "instanceDiff": "*terraform.InstanceDiff{mu:sync.Mutex{state:0, sema:0x0}, Attributes:map[string]*terraform.ResourceAttrDiff{\"user_ids.#\":*terraform.ResourceAttrDiff{Old:\"3\", New:\"1\", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, \"user_ids.1971754988\":*terraform.ResourceAttrDiff{Old:\"default\", New:\"default\", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, \"user_ids.236312002\":*terraform.ResourceAttrDiff{Old:\"exampleuser\", New:\"\", NewComputed:false, NewRemoved:true, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, \"user_ids.3350137769\":*terraform.ResourceAttrDiff{Old:\"exampleuser2\", New:\"\", NewComputed:false, NewRemoved:true, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}}, Destroy:false, DestroyDeposed:false, DestroyTainted:false, RawConfig:cty.NilVal, RawState:cty.NilVal, RawPlan:cty.NilVal, Meta:map[string]interface {}(nil)}"}

And the ss from the console:

Screenshot 2025-02-12 at 16 48 01

After the next reconciliation, the UserGroupAssociation resource transitioned back to Ready=True, which indicates that it re-applies the association. However, since this cycle repeats on every reconcile, it effectively creates a race condition between UserGroup and UserGroupAssociation, preventing stable resource management. This behavior makes it difficult to use UserGroupAssociation reliably within Crossplane.

As I mentioned above, I will discuss this with the team to clarify the situation and update you accordingly. Thanks!

@ido-re
Copy link
Author

ido-re commented Feb 18, 2025

Hello @ido-re,

Before I started reviewing this PR, I noticed the following note in the Terraform documentation, which was a bit annoying:

Screenshot 2025-02-12 at 17 05 10 According to this note, it seems that the `aws_elasticache_user_group_association` resource may not work properly with the provider in its current state (to be confirmed with the team). It’s a bit frustrating to think that all the effort put into this PR might go to waste, but I appreciate the work done here.

Let me roughly explain the situation; since Crossplane continuously reconciles resources, there is a risk that UserGroup and UserGroupAssociation may conflict with each other. Specifically, UserGroupAssociation modifies the userIds of UserGroup, which Crossplane might then detect as a drift and attempt to revert, which can lead to an endless reconcile loop. I will share with you the results of the manual tests I conducted with the image I produced from this PR, which confirms this situation:

I created the resources User, UserGroup and UserGroupAssociation. The resources were created successfully and I verified in the console that users were added to the UserGroup resource:

Screenshot 2025-02-12 at 16 46 01

After the first reconciliation, encountered the following diff in the logs and the UserGroupAssociation resources were set to Ready=False

2025-02-12T16:46:56+03:00	DEBUG	provider-aws	Diff detected	{"uid": "7840d46e-f1ba-4945-9559-036328179136", "name": "test", "gvk": "elasticache.aws.upbound.io/v1beta1, Kind=UserGroup", "instanceDiff": "*terraform.InstanceDiff{mu:sync.Mutex{state:0, sema:0x0}, Attributes:map[string]*terraform.ResourceAttrDiff{\"user_ids.#\":*terraform.ResourceAttrDiff{Old:\"3\", New:\"1\", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, \"user_ids.1971754988\":*terraform.ResourceAttrDiff{Old:\"default\", New:\"default\", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, \"user_ids.236312002\":*terraform.ResourceAttrDiff{Old:\"exampleuser\", New:\"\", NewComputed:false, NewRemoved:true, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, \"user_ids.3350137769\":*terraform.ResourceAttrDiff{Old:\"exampleuser2\", New:\"\", NewComputed:false, NewRemoved:true, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}}, Destroy:false, DestroyDeposed:false, DestroyTainted:false, RawConfig:cty.NilVal, RawState:cty.NilVal, RawPlan:cty.NilVal, Meta:map[string]interface {}(nil)}"}

And the ss from the console:

Screenshot 2025-02-12 at 16 48 01

After the next reconciliation, the UserGroupAssociation resource transitioned back to Ready=True, which indicates that it re-applies the association. However, since this cycle repeats on every reconcile, it effectively creates a race condition between UserGroup and UserGroupAssociation, preventing stable resource management. This behavior makes it difficult to use UserGroupAssociation reliably within Crossplane.

As I mentioned above, I will discuss this with the team to clarify the situation and update you accordingly. Thanks!

@turkenf is the "initProvider" can help in this case?

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.

Request for aws_elasticache_user_group_association resource
5 participants