Use separate cloud.config file for in-tree vs out-of-tree components

[rifelpet]

Seeing if this fixes kubernetes/cloud-provider#51

My thinking is that if we dont provide a --cloud-config, apiserver's PV admission plugin will not be enabled (or will be a no-op) and instead the EBS CSI Driver controller will provide the equivalent labels on PVs.

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[rifelpet]

/test pull-kops-e2e-ipv6-ci

[rifelpet]

(oops wrong one)
/test pull-kops-e2e-aws-cloud-controller-manager

EDIT: sorry I was getting mixed up. the ipv6-ci job is the relevant job for this issue because the issue occurs when NodeIPFamilies is set in the cloud.config file.

[olemarkus]

The errors you are seeing there are expected, I think. We see those in periodics as well.

[rifelpet]

Do you think the correct behavior is to not set apiserver's --cloud-config when --cloud-provider == external? I'm having a hard time finding documentation that mentions what these values should be exactly.

[hakman]

The failures for pull-kops-e2e-ipv6-ci are quite more compared to the periodic test:
https://testgrid.k8s.io/kops-ipv6#kops-grid-scenario-ipv6-calico

Not sure how well this is documented, but we should find someone to chat with, because our change to CCM is not very compatible with the tests:

can't store data at section "global", variable "NodeIPFamilies"

[olemarkus]

As mentioned on slack, all cloud provider config we provide specifically targets external CCMs and are not used by the other components. So I think we should not set this on the other components. That API server even tries to read this when CCM is external is probably a bug.

[rifelpet]

Seeing if pinning the az helps with

https://kubernetes.slack.com/archives/C8MKE2G5P/p1632913993215300?thread_ts=1632581337.212700&channel=C8MKE2G5P&message_ts=1632913993.215300

/test pull-kops-e2e-ipv6-ci

[rifelpet]

trying again to confirm the volume tests pass when --cloud-config isnt set:

/test pull-kops-e2e-ipv6-ci

[hakman]

Much better now :)

[rifelpet]

Agreed. I'm testing the removal of the admission plugin altogether given that it shouldn't be able to function without in-tree cloud provider code and --cloud-config.

/test pull-kops-e2e-ipv6-ci

[hakman]

I may be wrong, but looks like less tests are failing compared to periodic ones.

[rifelpet]

Yes I think we should proceed with both removing --cloud-config and the admission plugin when using external CCM but I'm going to double check in the sig-cloud-provider meeting today

[rifelpet]

/test pull-kops-e2e-ipv6-ci

[rifelpet]

/test pull-kops-e2e-ipv6-ci

[rifelpet]

The suggestion from the sig-cloud-provider meeting today was to use separate cloud.config files for in-tree vs out-of-tree components. This way we keep the PV Label admission plugin enabled until CCMs have webhook support (kubernetes/enhancements#2928 KEP slated for alpha in 1.23, plus implementation lag for each provider) and we don't have to rely on any upstream in-tree changes being made.

I updated this PR to create a separate in-tree.cloud.config file that is used by kube-apiserver, kube-controller-manager, and kubelet, keeping the original cloud.config file for use by any external CCM pods.

I'm open to supporting this logic a different way (conditionally creating the second file only if using an external CCM, or only on AWS since that is the only provider experiencing this issue so far, or only on AWS and IPv6, etc.) I figured using a different filename for in-tree sets us up for long-term use of the original cloud.config filename once in-tree is fully removed.

[olemarkus]

At some point we may move the external cloudconfig file to the individual CCMs. I suspect CCMs will stop using the generic CCMs at some point too and rather rely on configmaps or CRDs.

The current implementation here looks good to me.

[olemarkus]

/test pull-kops-e2e-ipv6-ci

[hakman]

Looks ok to me too, except for the name. Either we should go with 'external.cloud.config' for the other or switch to 'in-tree-cloud.config' to avoid creating a new file extension.
But just a nit and I don't mind if we keep it as is.
/lgtm

[rifelpet]

/test pull-kops-e2e-ipv6-ci

[k8s-ci-robot]

@rifelpet: 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
pull-kops-e2e-ipv6-ci	1f6e3ea	link	true	/test pull-kops-e2e-ipv6-ci

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

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 [hakman]

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

[hakman]

For reference --node-ip=:: fixes the test because of kubernetes/kubernetes#103307.