feat: Add GlobalReplicationGroup resource support

[NicholasBlaskey]

Add support for the ElastiCache GlobalReplicationGroup resource, which backs the Global Datastore feature for cross-region replication.

Implementation notes:

• generator.yaml: resource config, field renames, custom delete/update hooks, and a NodeGroupCount custom field for declarative shard scaling
• hooks.go: multi-phase delete (disassociate secondaries before deleting the global group), custom update (shard scaling plus modify), and Describe output mapping
• E2E tests covering create, modify, shard scale up/down, and invalid-primary terminal handling

Generated with SERVICE=elasticache AWS_SDK_GO_VERSION=v1.41.0 make build-controller from code-generator v0.62.0 (build hash db232581), matching the version recorded in ack-generate-metadata.yaml.

Resolves aws-controllers-k8s/community#1048

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[ack-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: NicholasBlaskey
Once this PR has been reviewed and has the lgtm label, please assign knottnt for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ack-prow]

Hi @NicholasBlaskey. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[NicholasBlaskey]

this test takes around 70 minutes to run. Is that too long for ACK?

[gobos12]

Does the bulk of that come from nodegroup scaling? You shouldn't need to wait for that entire operation in order to make sure that it works, I think confirming that the right scaling call is made once the GRG reaches the modifying state would be enough to ensure that the update for this case is working properly.

[NicholasBlaskey]

Yeah it is node group scaling.

This is a good idea, but unfortunately I think the API requires us to wait for the resource to finish scaling before deleting, so while this would speed up the test I think the overall run would take the same as it waits for the scale to finish in cleanup.

[gobos12]

Then maybe it might be better to test the scaling logic in a unit test rather than an integration test. The other tests already cover the CRUD-reconcile-to-synced path, so we're not losing anything by testing the scaling logic with unit tests.

[gobos12]

Nice work @NicholasBlaskey! Left some comments below.

[gobos12]

Does the bulk of that come from nodegroup scaling? You shouldn't need to wait for that entire operation in order to make sure that it works, I think confirming that the right scaling call is made once the GRG reaches the modifying state would be enough to ensure that the update for this case is working properly.

[gobos12]

Should this always default to true?

[NicholasBlaskey]

default/override_values here I don't believe ends up doing anything here because of the custom implementation required.

Removed override_values block here.

[NicholasBlaskey]

To add some detail here, my understanding is we need a complete custom implementation for the whole method because certain fields on the global replication group cannot be sent in the same update (and others must be sent in the same update)

For example the API rejects sending Description, AutomaticFailoverEnabled, and CacheNodeType with anything else with

│ InvalidParameterValue: Global Replication Group modifications only support modifying one field per request.

[gobos12]

I'm probably just confused on the wording in the comments, but wouldn't the delete get stuck here? It seems it's waiting for the resource to be in both the primary-only and available states, but that would require the resources to have secondaries attached while also not having any secondaries? Some clarification in the comments (whether that be here or in generator.yaml) would be nice.

[NicholasBlaskey]

updated comment to be clearer

[gobos12]

Does changing this affect the e2e tests for replication groups?

[NicholasBlaskey]

Yes. I don't think it caused it to fail but split this up to avoid impacting and causing the existing tests to use larger instance types than needed.

[gobos12]

Are these "read-only" specs necessary? They seem to be a part of the data type and aren't required for any of the API calls, adding them here doesn't make sense to me.

[NicholasBlaskey]

These shouldn't have been added. Removing them.

[gobos12]

/ok-to-test

[gustavodiaz7722]

Reviewed this on top of bb62e8b, and validated the behavior on a live cluster (EKS + this controller image, 60s resync) against real ElastiCache in us-west-2, rather than by reading the diff alone. Build and unit tests are clean, the delete state machine works exactly as described (verified end-to-end, primary correctly retained), and the one-change-per-Modify strategy matches what the API actually accepts.

Five things need fixing, four of them one-liners in generator.yaml. The two substantive ones are that automaticFailoverEnabled and cacheParameterGroupName are currently discarded while the CR reports ACK.ResourceSynced=True. Details inline.

For reference, what I ran live: created a cache.r6g.large / redis 7.0 primary with failover enabled, then a GRG with cacheParameterGroupName set and no nodeGroupCount, waited for primary-only + Synced, then patched automaticFailoverEnabled: false, then patched description + cacheParameterGroupName together, then deleted. nodeGroupCount write-back and the scaling paths I did not exercise live (my primary was cluster-mode disabled, so GlobalNodeGroups came back empty).

[gustavodiaz7722]

automaticFailoverEnabled is silently dropped — the read path never asks for member info.

newListRequestPayload builds DescribeGlobalReplicationGroupsInput without ShowMemberInfo, and AWS only returns Members when that flag is true (// Returns the list of members that comprise the Global datastore.). So in the read path ko.Status.Members is always empty, deriveAutomaticFailoverEnabled always returns nil, and the fallback in CustomDescribeGlobalReplicationGroupsSetOutput sets latest.Spec.AutomaticFailoverEnabled = r.ko.Spec.AutomaticFailoverEnabled — i.e. observed is copied from desired, so the delta can never fire.

Observed live: patched spec.automaticFailoverEnabled: false on a synced GRG. Across 6 reconciles (60s resync) the CR stayed ACK.ResourceSynced=True, status.members stayed null, and AWS still reported the primary member as AutomaticFailover: enabled. describe-global-replication-groups --show-member-info returned the member the whole time.

Two consequences: the field is a no-op in both directions, and status.members is a CRD field that never populates.

This also means test_modify_automatic_failover_is_noop passes for a different reason than the one documented — no Modify is issued because there is no observed state at all, not because the derived value matched.

Suggested fix, declaratively here rather than in a hook (setSDKReadMany honors override_values):

  DescribeGlobalReplicationGroups:
    override_values:
      ShowMemberInfo: aws.Bool(true)
    set_output_custom_method_name: CustomDescribeGlobalReplicationGroupsSetOutput

Worth noting that once member info is actually present, customSetGlobalReplicationGroupOutput becomes a problem: it re-maps Members (which generated sdkFind already mapped) but always takes &afStr, whereas the generated code guards with if f10iter.AutomaticFailover != "". An empty enum then becomes a non-nil "", which deriveAutomaticFailoverEnabled sends to its default: branch and reports false instead of "undeterminable" — the opposite of the "nil status on a member -> nil" case the unit test asserts. Since the generated mapping already covers GlobalReplicationGroupId, GlobalNodeGroups, and Members, dropping that function looks like the cleanest fix.

[NicholasBlaskey]

fixed

[gustavodiaz7722]

cacheParameterGroupName is recorded as "last requested" without ever being sent, so the field is silently discarded.

CreateGlobalReplicationGroupInput has exactly three members — GlobalReplicationGroupIdSuffix, PrimaryReplicationGroupId, GlobalReplicationGroupDescription. There is no parameter group on create, and the generated newCreateRequestPayload confirms it. But this line records spec.cacheParameterGroupName unconditionally, so restoreUnreadableFieldsFromAnnotations then hands back the user's own value as "observed" and the delta never fires. A cacheParameterGroupName set at creation time is therefore ignored, permanently, with no error and no condition.

The same thing happens on update. A CPGN change is not counted in pending (only descChanged, afeChanged, nodeTypeChanged, engineGroupChanged are), so when it accompanies another change the switch takes the other branch, the request goes out without CPGN, and setGlobalReplicationGroupOutput still records it as applied. The comment on modifyGlobalReplicationGroup — "the only annotated field is CacheParameterGroupName, which is only sent when it is the sole pending change" — holds for the requeue path but not for the annotation write.

Observed live: created with cacheParameterGroupName: default.redis7 (annotation written, nothing sent), then patched description + cacheParameterGroupName: default.redis7.cluster.on in one apply. The description landed on AWS, the annotation flipped to default.redis7.cluster.on, no ACK.Terminal, Synced=True — and AWS never received either value. default.redis7.cluster.on would have been rejected for that group had it actually been sent, which is what makes it a clean probe.

Suggested fix: only write the annotation where input.CacheParameterGroupName is actually populated (inside the engineGroupChanged branch, after the call succeeds), and drop the setLastRequestedUnreadableFields call from this create hook. Also fold the CPGN-differs condition into pending so a CPGN change combined with another change still reaches the existing terminal error instead of being swallowed.

[NicholasBlaskey]

fixed

[gustavodiaz7722]

InvalidReplicationGroupState is retryable, not terminal.

This is the error AWS returns while the primary is busy, and the message says so explicitly. Called directly against a primary that was not available:

An error occurred (InvalidReplicationGroupState) when calling the CreateGlobalReplicationGroup
operation: Replication Group is not in a valid state. Please retry after the modification on the
Replication group grg-review-primary is complete.

As a terminal code, applying a GRG manifest alongside a still-creating primary — the normal case when primaryReplicationGroupID is used rather than the ref, which is also what the e2e fixtures do — drives the resource permanently to ACK.Terminal=True and requires deleting and recreating the CR. Same for a GRG update issued while the primary happens to be modifying.

Suggested fix: drop this entry and let it requeue. ReplicationGroupNotFoundFault (line above) already covers the genuinely terminal case that test_invalid_primary_terminal exercises, so the test is unaffected.

[NicholasBlaskey]

fixed

[gustavodiaz7722]

Duplicate STATUS printer column.

This field-level print: and the additional_columns STATUS entry below both emit .status.status, so the CRD carries the column twice:

  - additionalPrinterColumns:
    - jsonPath: .status.status
      name: STATUS
    - jsonPath: .status.status
      name: STATUS

Live kubectl get:

NAME         STATUS         STATUS         NODE-TYPE   SYNCED   AGE
grg-review   primary-only   primary-only               True     7m

Suggested fix: drop this print: block and keep the additional_columns entry, which carries the index that order_by: index uses.

Minor, in the same two lines: is_read_only: true is redundant here — Status appears only in Describe output, so the generator places it in Status by default. It is the same redundancy already removed from the other six fields in the last revision, so with the print: block gone the whole Status: entry can go.

[NicholasBlaskey]

fixed

[gustavodiaz7722]

NODE-TYPE column points at a path that is always empty.

CacheNodeType is generated into Spec, not Status — generated sdkFind does ko.Spec.CacheNodeType = elem.CacheNodeType (it is configured from: ModifyGlobalReplicationGroup). So .status.cacheNodeType never resolves and the column renders blank. Live, with spec.cacheNodeType: cache.r6g.large set:

NAME         STATUS         STATUS         NODE-TYPE   SYNCED   AGE
grg-review   primary-only   primary-only               True     7m

Suggested fix: json_path: .spec.cacheNodeType.

[NicholasBlaskey]

fixed

[ack-prow]

@NicholasBlaskey: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
elasticache-kind-e2e	9f22fb8	link	true	/test elasticache-kind-e2e
elasticache-crd-compat-check	9f22fb8	link	true	/test elasticache-crd-compat-check
elasticache-verify-attribution	9f22fb8	link	false	/test elasticache-verify-attribution
elasticache-recommended-policy-test	9f22fb8	link	true	/test elasticache-recommended-policy-test
elasticache-verify-code-gen	9f22fb8	link	true	/test elasticache-verify-code-gen
elasticache-unit-test	9f22fb8	link	true	/test elasticache-unit-test
elasticache-release-test	9f22fb8	link	true	/test elasticache-release-test
elasticache-metadata-file-test	9f22fb8	link	true	/test elasticache-metadata-file-test

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[gustavodiaz7722]

@NicholasBlaskey Can you fix merge conflict? There was a new runtime release that was merged #232

[gustavodiaz7722]

• File: templates/hooks/global_replication_group/sdk_create_post_set_output.go.tpl, generator.yaml
• Issue: The hook is dead code. Generated sdkCreate already assigns both values from the same response before the hook body runs; the hook re-does exactly those two assignments.
• Fix: Delete the template file and the hooks.sdk_create_post_set_output block under GlobalReplicationGroup in generator.yaml, then regenerate.
• Verified fix: Removing both and regenerating deletes exactly 11 lines from sdk.go and changes nothing else anywhere in the tree. The surviving generated code still sets ko.Status.GlobalReplicationGroupID (sdk.go:319) and ko.Status.Status (sdk.go:349). Controller builds.

AI-generated comment (Kiro).

[gustavodiaz7722]

• File: test/e2e/tests/test_global_replication_group.py
• Issue: two of the most intricate new paths are never reached by a test. The suite's full inventory is six tests -- test_create_and_verify, test_invalid_primary_terminal, test_modify_description, test_modify_automatic_failover_is_noop, test_modify_multiple_fields_converges, test_scale_up_and_down -- and every one of them deletes from primary-only. Searching all 767 lines for disassoc|secondary|associate_global|engineVersion|cacheParameterGroup returns exactly one hit: the comment on this line. So delete Phase 2 (disassociate secondaries before deleting the global group, hooks.go:125-153) and the engine-upgrade branch of modifyGlobalReplicationGroup (hooks.go:378-392) are both untested. The engine branch is the one place the Engine + EngineVersion + CacheParameterGroupName grouping rule is applied, and the only path that writes the last-requested-cache-parameter-group-name annotation the resource relies on for delta correctness.
• Fix: add test_engine_version_upgrade to TestGlobalReplicationGroupModify covering a major upgrade with engine, engineVersion, and cacheParameterGroupName patched together, asserting the AWS-side EngineVersion lands, the resource returns to Synced, and the annotation is present on the CR afterward. That is single-region and runnable in CI. For Phase 2, either add a cross-region test gated to the multi-region suite, or note in the PR description that it was validated manually and cannot run in CI.

AI-generated comment (Kiro).