InvalidParameterCombination error when creating ReplicationGroup with cluster mode enabled

[matteo-magni]

Describe the bug
A cluster-enabled ReplicationGroup with 2 (or more) nodes would fail if created along with its CacheParameterGroup.
The CacheParameterGroup gets created and, before it can set the actual value for the cluster-enabled parameter, the ACK controller attempts to create the ReplicationGroup. This leads to a non-recoverable error (ACK.Terminal is "True") because the ReplicationGroup must have 2 nodes but the cluster-enabled parameter is still set to no (the default value).

Steps to reproduce

• prepare a k8s cluster with Elasticache ACK controller
• create the elasticache-00 namespace
• apply code from this gist (make sure you do supply the ReplicationGroup resource with the cacheSubnetGroupName and securityGroupIDs correct for your environment)
• see following status condition in ReplicationGroup resource:

  - message: "InvalidParameterCombination: Use a parameter group with cluster-enabled
      parameter to create more than one node group.\n\tstatus code: 400, request id:
      f0c028b5-1097-4a37-819a-f5d8bc3026dc"
    status: "True"
    type: ACK.Terminal

Expected outcome
The ReplicationGroup gets created correctly with cluster mode enabled and exactly 2 nodes.
It might actually fail until the CacheParameterGroup sets the cluster-enabled parameter to yes, but it would eventually reconcile.

Environment

• Kubernetes version: 1.23
• Using EKS: Yes, version v1.23.10-eks-15b7512
• AWS service targeted: Elasticache

Tested with Elasticache ACK v0.0.20 as well as the one compiled off the ack-bot/rt-v0.20.1-codegen-v0.20.1 branch (because I needed the fix to issue #1318 ): same error.

[matteo-magni]

I'll add a bit more of context.

At VMware we’re building some Tanzu packages to enable users to easily consume cloud services (like AWS Elasticache) and provide service bindings for applications.

The AWS Elasticache package is ready but we don't feel confident to ship the cluster-enabled feature because of this issue, so we’ll either delay the release of the package or we’ll allow just the creation of single-node instances until the issue is addressed and fixed.

[ack-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle stale

[matteo-magni]

/remove-lifecycle stale

[RedbackThomson]

Hey @matteo-magni , apologies for the long delay in response to this.
I see that the controller properly sets the CacheParameterGroupName on the SDK request object when calling create - https://github.com/aws-controllers-k8s/elasticache-controller/blob/a318922772c16e6f1829f65f89452c200210d0f5/pkg/resource/replication_group/sdk.go#L922-L924 - so it should be passing the correct name to the service. I imagine that when we call CreateReplicationGroup, the controller has not finished creating the ParameterGroup. One solution would be to tell the code-generator that InvalidParameterCombination is not terminal, and that it should retry. But InvalidParameterCombination is a terminal error in most instances and we should probably treat it like such.

However, there is actually a better solution for this problem, which would be to use resource references. Resource field references allow you to reference a dependent K8s object instead of the concrete value for a field. The controller will look up the dependent object, wait for it to be synced, and then use the value from its spec or status to fill in the field. In this case, you would replace cacheParameterGroupName: redis-00 with

cacheParameterGroupRef:
  from:
    name: redis-00
    namespace: elasticache-00

Once the CacheParameterGroup gets the ResourceSynced: true condition, then the controller will use its name to create the ReplicationGroup.

One current issue with this: The Elasticache controller currently doesn't have resource references. These are pretty easy to add, so if this sounds like a valid solution for you, I can work on adding those

[matteo-magni]

Thanks @RedbackThomson for reaching out and finding some time to investigate.
Indeed, what you just proposed seems like a neat and feasible solution from my point of view. Plus, if it is also easy to implement I would go for it.
Thanks!

[RedbackThomson]

Thanks @RedbackThomson for reaching out and finding some time to investigate. Indeed, what you just proposed seems like a neat and feasible solution from my point of view. Plus, if it is also easy to implement I would go for it. Thanks!

It is super easy, actually! We have a document about how to update the controller generator.yaml so that the code-generator knows how to interpret fields as references - https://aws-controllers-k8s.github.io/community/docs/contributor-docs/code-generator-config/#informing-the-code-generator-that-a-field-refers-to-another-resource

[a-hilaly]

/cc @aws-controllers-k8s/elasticache-maintainer @nmvk @kumargauravsharma