⚠️ Remove redundant fields from CABPK / KCP ClusterConfiguration

[sbueringer]

Signed-off-by: Stefan Büringer [email protected]

What this PR does / why we need it:

The PR drops fields that can be configured by other means and thus don't have to exist redundantly on the ClusterConfiguration struct.

The removed fields are:

• Networking.ServiceSubnet
• Networking.PodSubnet
• Networking.DNSDomain
• KubernetesVersion
• ControlPlaneEndpoint
• ClusterName

WIP for now because it contains #12303

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of #10852

[sbueringer]

/test pull-cluster-api-e2e-conformance-ci-latest-main
/test pull-cluster-api-e2e-conformance-main
/test pull-cluster-api-e2e-latestk8s-main
/test pull-cluster-api-e2e-main
/test pull-cluster-api-e2e-mink8s-main
/test pull-cluster-api-e2e-upgrade-1-33-1-34-main

[sbueringer]

/test pull-cluster-api-e2e-upgrade-1-33-1-34-main

[sbueringer]

/test pull-cluster-api-e2e-upgrade-1-33-1-34-main

[sbueringer]

/test pull-cluster-api-e2e-conformance-ci-latest-main
/test pull-cluster-api-e2e-conformance-main
/test pull-cluster-api-e2e-latestk8s-main
/test pull-cluster-api-e2e-main
/test pull-cluster-api-e2e-mink8s-main
/test pull-cluster-api-e2e-upgrade-1-33-1-34-main

[sbueringer]

/hold
for rebase after #12303 merges.

Only the last commit of this PR will remain (and is ready for review now)

[sbueringer]

/assign @fabriziopandini @chrischdi

[chrischdi]

lgtm for the last commit

[fabriziopandini]

Wondering if we should give a stronger warning about this change (or generalizing all the changes where we loose info in the v1beta1 (old client)->v1beta2 (storage) -> v1beta1 (old client) e.g.

If you are using git ops tools, either migrate to v1beta2 or make sure to drop following fields to prevent issues:

[sbueringer]

/test pull-cluster-api-e2e-conformance-ci-latest-main
/test pull-cluster-api-e2e-conformance-main
/test pull-cluster-api-e2e-latestk8s-main
/test pull-cluster-api-e2e-main
/test pull-cluster-api-e2e-mink8s-main
/test pull-cluster-api-e2e-upgrade-1-33-1-34-main

[fabriziopandini]

/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 712ff84d071d051d01e9bff541d8cdbcf3bf3132

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [fabriziopandini]

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

[Karthik-K-N]

@sbueringer A question regarding setting ControlPlaneEndpoint when using v1beta2 ,

In CAPIBM we use kube-vip and used to set one external IP for control-plane endpoint and internal IP for cluster, The cluster config looks like below

apiVersion: infrastructure.cluster.x-k8s.io/v1beta2
kind: IBMPowerVSCluster
metadata:
  labels:
    cluster.x-k8s.io/cluster-name: ibm-powervs-1
  name: ibm-powervs-1
  namespace: default
spec:
  controlPlaneEndpoint:
    host: 163.68.77.203 // External IP
    port: 6443
  network:
    name: karthik-capi-test
  serviceInstanceID: 10b1000b-da8d-4e18-ad1f-6b2a56a8c130

In kube-vip we use internal ip

apiVersion: controlplane.cluster.x-k8s.io/v1beta2
kind: KubeadmControlPlane
metadata:
  name: ibm-powervs-1-control-plane
  namespace: default
spec:
  kubeadmConfigSpec:
    clusterConfiguration:
      apiServer:
        certSANs:
        - 192.168.169.203
        - 163.68.77.203
        extraArgs:
        - name: cloud-provider
          value: external
      controlPlaneEndpoint: 192.168.169.203:6443 // Internal IP
      controllerManager:
        extraArgs:
        - name: cloud-provider
          value: external
    files:
    - content: |
        apiVersion: v1
        kind: Pod
        metadata:
          creationTimestamp: null
          name: kube-vip
          namespace: kube-system
          .
          .

With this field omission, IIUC the cluster.spec.controlplaneendpoint will be used in kubeadm config spec, So we are not able to use two different IPs. Or is there any other better way to manage this.

[sbueringer]

Wow. I did not know that this is possible

(cc @fabriziopandini)

[mkumatag]

With this field omission, IIUC the cluster.spec.controlplaneendpoint will be used in kubeadm config spec, So we are not able to use two different IPs. Or is there any other better way to manage this.

lets explore if these fields are helpful

cluster-api/api/bootstrap/kubeadm/v1beta2/kubeadm_types.go

Line 97 in 7a22509

LocalAPIEndpoint APIEndpoint `json:"localAPIEndpoint,omitempty"`

cluster-api/api/bootstrap/kubeadm/v1beta2/kubeadm_types.go

Line 539 in 7a22509

LocalAPIEndpoint APIEndpoint `json:"localAPIEndpoint,omitempty"`

[Amulyam24]

lets explore if these fields are helpful
cluster-api/api/bootstrap/kubeadm/v1beta2/kubeadm_types.go
LocalAPIEndpoint APIEndpoint json:"localAPIEndpoint,omitempty"

cluster-api/api/bootstrap/kubeadm/v1beta2/kubeadm_types.go
LocalAPIEndpoint APIEndpoint json:"localAPIEndpoint,omitempty"

It did not work upon configuring with this.

Upon debugging further with @Karthik-K-N, the only work around which worked to get a successful cluster creation was adding a script in preKubeadmCommands to change the controlPlaneEndpoint in /run/kubeadm/kubeadm.yaml

After removing the field from kubeadmConfigSpec. clusterConfiguration, it was set to the external IP which was being taken from IBMPowerVSCluster.spec.controlPlaneEndpoint.

Replacing it with the internal IP which is configured with kube-vip

sed -i 's#controlPlaneEndpoint: {EXTERNAL_IP}:6443#controlPlaneEndpoint: {INTERNAL_IP}:6443#' /run/kubeadm/kubeadm.yaml

@sbueringer, would this be recommended or is there a better way to handle this?

[mkumatag]

It did not work upon configuring with this.

interesting! as per the api doc it should work, may be we need to explore why it is not working?!

[sbueringer]

@mkumatag @Amulyam24 @Karthik-K-N

Hey folks,
thx for the feedback and thx for looking into this. I looked into this with Fabrizio as well. We're basically going to revert the PR for this field (I"ll have a PR open in a bit)

[sbueringer]

@mkumatag @Amulyam24 @Karthik-K-N Please see: #12423

[nikParasyr]

Hello.
Was wondering about the removal of:

• Networking.ServiceSubnet
• Networking.PodSubnet
• Networking.DNSDomain

On v1beta1 we were heavily using these together with RunTimeSDK to:

1. set sane defaults for our users ( and thus keep cluster definition minimal as 90% of our users are not "power-users" )
2. ensure correctness of various subnets through XValidations ( together with the infrastructure node subnet )
3. the above was done in a single "point" (ClusterClass variable) making it easier for us and users to check/configure these settings when needed

With the removal of them on v1beta2 the above are not possible as easily (and the upgrade will be more heavy).
In contrast to the endpoint problem discussed above, I expect we can achieve the same result/functionality using some admissionController (opa-gatekeeper/kyverno/...) but it will require more work and there wont be a single point (spec.clusterNetworking + variables.nodeSubnet for users, opa-gatekeeper policy for these checks + CC Validations for the rest for admins).

So I guess my question would be, is there a "strong" reason for removing them and would you consider re-adding them?
( again i don't expect this to be breaking our use-case, just making it a bit less "friendly" )

thanks a lot :)

[fabriziopandini]

Sorry if this caused impact for you

The main reason we did this was to make sure there is a single source of truth for those information, because having them in two places was confusing and potentially leading to other kind of issues

The approach we are using to keep cluster configuration minimal is to leverage on cluster class, but as of today there is no way in CC to control those cluster settings.

I would suggest to open an issue to track this as a feature request

[sbueringer]

I'm not sure if we should use ClusterClass to write fields on the Cluster spec (which is the only way I could see that ClusterClass could influence the fields on the Cluster)

[fabriziopandini]

yep, it needs discussion, this is why I asked to open an issue

[nikParasyr]

Thank you for the quick replies. This is the related issue i created