fix: treat absent and disabled EKS Auto Mode fields as equivalent

[michaelhtm]

Description of changes:
DescribeCluster always reports elasticLoadBalancing and omits computeConfig and
storageConfig on a cluster that never opted in, so a CR that left those unset or
declared them disabled read as drift and drove an UpdateClusterConfig that EKS
rejects. The compare hook now treats absent and disabled as the same state per
capability, the Auto Mode request always carries the full compute/storage/load
balancing tuple without the creation-only ipFamily and serviceIPv4CIDR, and a
request EKS rejects is terminal instead of retried forever.

Resolves aws-controllers-k8s/community#3046

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 APPROVED

This pull-request has been approved by: michaelhtm

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [michaelhtm]

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

[michaelhtm]

/label release/patch

[knottnt]

Thanks @michaelhtm. Left a couple comments. Main concern is that we're moving what seems like service side validation into the controller. If a user specifies an invalid combination for compute, storage, and networking I think its okay for us to let the user get an error from the API.

[knottnt]

Should be asserting False? Or does the API also return None in this case?

[knottnt]

I'd suggest delegating this validation to the to the service API instead of performing it client-side. It is possible that this client side validation drifts from what the service actually allows.

[gustavodiaz7722]

The stated premise is wrong for computeConfig / storageConfig, and seven late_initialize entries are dead config**

• Files: generator.yaml:260-284, commit message

• Issue: the commit message opens with "DescribeCluster returns the Auto Mode defaults on clusters that never opted in". The API does not do that:

  Cluster	computeConfig	storageConfig	kubernetesNetworkConfig
  reftest-eks (non-Auto-Mode)	null	null	{serviceIpv4Cidr, ipFamily, elasticLoadBalancing:{enabled:false}}
  adoption-cluster-4vxp7… (non-Auto-Mode)	null	null	{serviceIpv4Cidr, ipFamily, elasticLoadBalancing:{enabled:false}}
  pr255-live (non-Auto-Mode, created by the PR build)	null	null	{serviceIpv4Cidr, ipFamily, elasticLoadBalancing:{enabled:false}}
  ack-dev-auto (Auto Mode, control)	{enabled:true, nodePools:[general-purpose,system], nodeRoleArn:…}	{blockStorage:{enabled:true}}	{…, elasticLoadBalancing:{enabled:true}}

  Only elasticLoadBalancing.enabled: false comes back on a non-Auto-Mode cluster. The mirrorDisabledAutoMode comment (hook.go:206-208) states this correctly and contradicts the commit message; the comment is right.

  Confirmed end to end: on pr255-live, running the PR build, the stored CR spec shows spec.computeConfig: null and spec.storageConfig: null. The seven ComputeConfig* / StorageConfig* late_initialize entries had nothing to adopt.

  Where they would fire — Auto Mode clusters — they adopt AWS-assigned nodePools and nodeRoleARN into the user's spec. A user who deliberately left nodePools unset would have [general-purpose, system] pinned into their CR, which goes stale when EKS changes its defaults and then drives an Auto Mode update. That is an unrequested behaviour change on the resource's most sensitive field.

[gustavodiaz7722]

The new terminal error leaves ACK.ResourceSynced=True

• File: pkg/resource/cluster/hook.go:447

• Issue: the uniform-tuple enforcement works — requesting Auto Mode with computeConfig.enabled: true only produced the intended terminal condition and made no AWS call (computeConfig stayed null throughout, so the tuple check correctly short-circuits before UpdateClusterConfig). But the resource simultaneously reports itself synced, across 8 consecutive polls:

  ACK.ResourceSynced   True
  ACK.Terminal         True   failed to update AutoMode config: EKS requires spec.computeConfig.enabled,
                              spec.storageConfig.blockStorage.enabled and
                              spec.kubernetesNetworkConfig.elasticLoadBalancing.enabled to all be set to the same value
  Ready                False  (same message)
  

  That contradicts the runtime's own stated contract — the comment at reconciler.go:779-785 says "A terminal condition is a stable state for a resource… Thus, ACK.ResourceSynced must be False." The suppression never fires because the check is an identity comparison against a sentinel:

  if reconcileErr == ackerr.Terminal {   // reconciler.go:781

  ackerr.NewTerminalError(...) returns a *ackerr.TerminalError, which is never == the ackerr.Terminal sentinel. The resource reconciler does not use errors.As here (the field-export reconciler does, at field_export_reconciler.go:484,683). This PR compounds it by wrapping the terminal error a second time:

  return nil, fmt.Errorf("failed to update AutoMode config: %w", err)

  so even an errors.As-based check upstream would be the only thing able to recover it. The consequence is that a permanently broken spec passes any automation gating on ACK.ResourceSynced — including the PR's own e2e tests, which assert wait_on_condition(ref, "ACK.ResourceSynced", "True") and would therefore pass against a terminally broken resource.

[gustavodiaz7722]

is_immutable on ipFamily / serviceIPv4CIDR is not immutability, and the drift it fails to block is reported as synced**

• Files: generator.yaml:291-300, pkg/resource/cluster/hook.go:439-442, apis/v1alpha1/types.go:932,934

• Issue: three facts compose into a silent-drift bug. All three were observed on the deployed PR build.

  (a) The CEL guard is bypassable in two patches. rule: self == oldSelf is a transition rule, and Kubernetes skips transition rules when oldSelf does not exist — so on an optional field it only blocks a single-step change. Against the live CRD:

  Transition	Result
  ipv4 → ipv6, one patch (control)	rejected — Value is immutable once set
  ipv4 → remove → ipv6, two patches	both accepted
  absent → value	accepted
  wrong value at create	accepted

  (b) Nothing downstream reconciles or rejects the result. This commit narrows the Auto Mode trigger to …ElasticLoadBalancing and deletes the only code that ever sent IpFamily/ServiceIpv4Cidr (the old updateComputeConfig). is_immutable emits only the CRD marker — there is no runtime immutability guard in pkg/resource/cluster/. The controller's own log shows it seeing the drift and doing nothing:

  {"msg":"desired resource state has changed","generation":4,
   "diff":[{"Path":{"Parts":["Spec","KubeControllerManagerConfig","PodGCControllerConfig"]},…},
           {"Path":{"Parts":["Spec","KubernetesNetworkConfig","IPFamily"]},"A":"ipv6","B":"ipv4"}]}

  No Auto Mode or ipFamily update follows. Root cause: ackcompare.Path.Contains (runtime@v0.64.0/pkg/compare/path.go:63-77) returns false when the subject has more segments than the difference path, so a difference at Spec.KubernetesNetworkConfig.IPFamily matches none of the 23 delta.DifferentAt(...) subjects in customUpdate. A standalone harness against the PR's own newResourceDelta confirms 0 of 23 match, with a passing control showing an elasticLoadBalancing drift is dispatched.

  (c) The resource reports healthy. customUpdate falls through to rm.setStatusDefaults(updatedRes.ko); return updatedRes, nil (hook.go:524-527), and Cluster.IsSynced is hardcoded return true, nil (manager.go:441-449). Observed over 120 s of reconciles:

  t+10s   CR.ipFamily=ipv6   AWS.ipFamily=ipv4   Synced=True
  …
  t+120s  CR.ipFamily=ipv6   AWS.ipFamily=ipv4   Synced=True
  

  Before this commit the same drift produced a visible AWS error.

[gustavodiaz7722]

test_disable_auto_mode_converges mutates a shared fixture and asserts on a race**

• File: test/e2e/tests/test_cluster_automode.py:153-180

• Issue: two independent problems in one test.

  Fixture contamination. auto_mode_cluster is @pytest.fixture(scope="class") (line 67). Verified definition order, which is pytest's execution order: test_create_auto_mode_cluster → test_disable_auto_mode_converges → test_finalizer_retained_during_deletion. The new test disables Auto Mode on the shared cluster and contains no re-enable step, so the finalizer test runs against a cluster that is no longer in Auto Mode.

  Race. Verified post-patch sequence is wait_for_cluster_active → time.sleep(CHECK_STATUS_WAIT_SECONDS) → describe_cluster → assert computeConfig.enabled is not True. Reconciliation is asynchronous, so at the waiter's first poll the controller has not yet issued UpdateClusterConfig and the cluster is still ACTIVE; the waiter returns immediately and contributes no synchronisation. The assertion then rests entirely on a fixed 240-second sleep covering a multi-minute Auto Mode teardown, with no wait for the CR to leave Synced=False first.

[michaelhtm]

/retest

[ack-prow]

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

Test name	Commit	Details	Required	Rerun command
eks-kind-e2e	9a6cced	link	true	/test eks-kind-e2e

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.