AWSClusterSpec's "ControlPlaneLoadBalancer" field should be mutable

[seh]

/kind bug

What steps did you take and what happened:

Create a CAPA cluster using the default settings. Observe that the API servers are fronted by a Classic ELB using the "Internet-facing" scheme, with cross-zone load balancing disabled.

Edit the Kubernetes manifest for the AWSCluster object, adding a "controlPlaneLoadBalancer" field (be it in JSON or YAML), with its "crossZoneLoadBalancing" field set to true.

Apply the changes to the API server, and observe that it rejects the attempt, complaining that the "spec.controlPlaneLoadBalancer [...] field is immutable". Note, though, that no matter what I specify here—even for cases where it should match the current configuration once parsed—the API server rejects it. Here are some candidate updates:

• Specify matching values

apiVersion: infrastructure.cluster.x-k8s.io/v1alpha3
kind: AWSCluster
metadata:
  name: aws-test-1
  namespace: aws-test-1
spec:
  controlPlaneLoadBalancer:
    scheme: Internet-facing
    crossZoneLoadBalancing: true
  # ...

• Eliminate one field

apiVersion: infrastructure.cluster.x-k8s.io/v1alpha3
kind: AWSCluster
metadata:
  name: aws-test-1
  namespace: aws-test-1
spec:
  controlPlaneLoadBalancer:
    scheme: Internet-facing
  # ...

• Eliminate the other field

apiVersion: infrastructure.cluster.x-k8s.io/v1alpha3
kind: AWSCluster
metadata:
  name: aws-test-1
  namespace: aws-test-1
spec:
  controlPlaneLoadBalancer:
    crossZoneLoadBalancing: true
  # ...

• Eliminate both fields

apiVersion: infrastructure.cluster.x-k8s.io/v1alpha3
kind: AWSCluster
metadata:
  name: aws-test-1
  namespace: aws-test-1
spec:
  controlPlaneLoadBalancer:
  # ...

Observe that the field doesn't appear to be set on the existing Kubernetes object:

% kubectl --namespace=aws-test-1 get awscluster aws-test-1 --output=jsonpath='{.spec.controlPlaneLoadBalancer}'
<no output>

This is the only mention of the "controlPlaneLoadBalancer" field that the API server accepts:

apiVersion: infrastructure.cluster.x-k8s.io/v1alpha3
kind: AWSCluster
metadata:
  name: aws-test-1
  namespace: aws-test-1
spec:
  controlPlaneLoadBalancer: null
  # ...

What did you expect to happen:

The CAPA controllers would reconfigure the load balancer to allow cross-zone load balancing. At minimum, I'd expect it to accept specified values that match the current values (after defaults are applied).

Anything else you would like to add:
PR #1677 introduced the validation that rejects all mutation of the AWSCluster.Spec.ControlPlaneLoadBalancer field. That may have been in the interest of safety, if there is not yet any way to reconcile these changes to the fields. @benmoss, do you recall this one?

As far as I can tell, it's not possible to change the scheme of a Classic ELB after it's created, but it is possible to change its cross-zone load balancing setting (via the aws tool, or the API with this attribute).

Environment:

• Cluster-api-provider-aws version:
  v0.5.4
• Kubernetes version: (use kubectl version):

Client Version: version.Info{Major:"1", Minor:"18", GitVersion:"v1.18.4", GitCommit:"c96aede7b5205121079932896c4ad89bb93260af", GitTreeState:"clean", BuildDate:"2020-06-18T02:59:13Z", GoVersion:"go1.14.3", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"18", GitVersion:"v1.18.2", GitCommit:"52c56ce7a8272c798dbc29846288d7cd9fbae032", GitTreeState:"clean", BuildDate:"2020-04-30T20:19:45Z", GoVersion:"go1.13.9", Compiler:"gc", Platform:"linux/amd64"}

• OS (e.g. from /etc/os-release):
  This is a kind cluster running on macOS.

NAME="Ubuntu"
VERSION="19.10 (Eoan Ermine)"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 19.10"
VERSION_ID="19.10"
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
VERSION_CODENAME=eoan
UBUNTU_CODENAME=eoan

[seh]

Related to the update attempts I described earlier, if I attempt to create an AWSCluster object specifying the "spec.controlPlaneLoadBalancer" field with a value of null (in YAML), I see the following validation failure:

The AWSCluster "aws-test-1" is invalid: spec.controlPlaneLoadBalancer: Invalid value: "null": spec.controlPlaneLoadBalancer in body must be of type object: "null"

[randomvariable]

If I attempt to create an AWSCluster object specifying the "spec.controlPlaneLoadBalancer" field with a value of null (in YAML), I see the following validation failure.

Maybe that's coming from something inherent in CRDv1, have seen some odd stuff on vsphere too, but it's a weird error message for sure. Not sure it's the webhook's causing it, but it could be.

To the mutability question then, we can scope this down to relaxing constraint to allow cross-zone AZ support being toggaaalable?

[seh]

we can scope this down to relaxing constraint to allow cross-zone AZ support being toggleable?

Yes, that would be good.

It would help too, though, if we were more tolerant of going from no mention of this "ControlPlaneLoadBalancer" to the presence of the field with values that match the existing values. In other words, if we were able to compare the submitted value against the current value with defaults taken into account. You can see above that I wasn't even able to submit an updated manifest that would match the effective values of these fields, because I was specifying values that match the default values.

Likewise, if we had specified these fields with values that match the defaults, and then we remove the fields from our manifests, ideally we'd accept that update, because it induces no need for reconciling a change in intended state.

[benmoss]

We decided to make many fields immutable when the current implementation wasn't handling mutations correctly. There was discussion of this in #1673. I think we are amenable to changing these when the controllers become capable of managing updates.

[detiber]

/milestone v0.5.x
/priority important-soon

[gab-satchi]

/assign

[gab-satchi]

/lifecycle active