-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Add support for generating bootstrap data in Ignition format to CABPK #4172
✨ Add support for generating bootstrap data in Ignition format to CABPK #4172
Conversation
Welcome @invidian! |
Hi @invidian. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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 don't see a CAEP (proposal) for this. |
/ok-to-test |
I'd appreciate some help with CI, I have no idea why is it failing 😞 |
I believe they are failing here: cluster-api/bootstrap/kubeadm/api/v1alpha3/conversion_test.go Lines 29 to 37 in 2dd66d6
So basically CRD conversion is broken for kubeadm types. |
Thanks. I managed to figure that out. I guess some manual conversion code is missing, but I didn't figure out yet where to add it. Another weird thing is that running |
Hey @neolit123, have you seen #3430 (comment) ? I think I contained the proposal in this comment. Isn't it providing enough context? |
As @bgilbert noted over the old PR (#3437 (comment)), there are newer Ignition versions (2.x) with an incompatible spec (3.x) and a different config transpiler yaml format (FCCT). I'd like to make sure this is taken into account here. Edit: I just saw #3430 (comment) which seems to take it into account 👍 |
@vincepri @neolit123 @JoelSpeed @justinsb tests are now passing. Please have a look 🙏 |
As users usually do not create KubeadmConfig directly, but rather create KubeadmConfigTemplate and now KubeadmConfig has more validation rules, it's good if users sees those errors directly. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io> Signed-off-by: Johanan Liebermann <jliebermann@microsoft.com>
KubeadmConfigSpec has now more validation rules, so we should trigger them, as KubeadmConfigSpec is embedded in KubeadmControlPlane object. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io> Signed-off-by: Johanan Liebermann <jliebermann@microsoft.com>
Maybe the linter issues point to the test failure cause |
Allow provisioning CAPD machines using Ignition as an alternative to cloud-init. So far, cloud-init has been hardcoded as the only provisioning format supported by CAPD. - Add a package called "provisioning" which contains the interfaces and common logic for all supported provisioning formats. - Move the existing cloud-init code under the new package. - Add a new provisioning implementation for Ignition. Co-authored-by: Suraj Deshmukh <suraj@kinvolk.io> Signed-off-by: Johanan Liebermann <jliebermann@microsoft.com>
This commit adds the feature gate KubeadmBootstrapFormatIgnition that will control the usage of field Ignition in KubeadmConfig. If user provides ignition field then the webhook config rejects the request with a validation error. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io> Signed-off-by: Johanan Liebermann <jliebermann@microsoft.com>
Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io> Signed-off-by: Johanan Liebermann <jliebermann@microsoft.com>
This file tests functions in kubeadmconfig_webhook_test.go. Signed-off-by: Johanan Liebermann <jliebermann@microsoft.com>
Ignition doesn't support the replace_fs filesystem parameter and the partition filesystem parameter supported by cloud-init. Disallow using these parameters with Ignition. Signed-off-by: Johanan Liebermann <jliebermann@microsoft.com>
For some reason, running While looking at reported linter issues, I also spotted some complexity which could be reduced, so I've done that. Last batch of changes is available to see here: |
/test pull-cluster-api-e2e-full-main |
Thx for providing that link /lgtm 🎉 But let's please wait for green e2e-full results before merge |
That apparently didn't match the regex :) /lgtm |
/lgtm |
@vincepri @fabriziopandini do you want to approve as well? :) I'll remove hold once e2e-full-main is green. |
/lgtm |
[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:
Approvers can indicate their approval by writing |
/unhold 🎉 |
Congrats 🎉 |
What this PR does / why we need it:
This PR provides an alternative solution to PR #3437, described in #3430 (comment).
For manual testing, you can use https://github.com/kinvolk/cluster-api/tree/invidian/ignition-support-next based on
v1alpha3
together with https://github.com/kinvolk/cluster-api-provider-aws/tree/invidian/flatcar-support-format-and-flatcar-specifics.Please let me know what you think about it and what is missing to get this PR merged.
I guess the following things are missing:
format
field, I couldn't figure out where to put it, so I skipped adding new test for now.CC @iaguis @vbatts @dongsupark
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #3430