Skip to content
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

✨ [WIP] Introduce bootstrap provider for ignition #3437

Closed
wants to merge 1 commit into from
Closed

✨ [WIP] Introduce bootstrap provider for ignition #3437

wants to merge 1 commit into from

Conversation

dongsupark
Copy link
Member

@dongsupark dongsupark commented Jul 31, 2020

An experimental bootstrap provider for ignition, to support OS like Flatcar Container Linux or Ignition.

As discussed with @vincepri , the code is located under exp, so it can be isolated from other parts.

It originally comes from
https://github.com/fossabot/cluster-api-bootstrap-provider-kubeadm-ignition.

Fixes #3430

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 31, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @dongsupark. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 31, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign cecilerobertmichon
You can assign the PR to them by writing /assign @cecilerobertmichon in a comment when ready.

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

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 31, 2020
@ncdc
Copy link
Contributor

ncdc commented Jul 31, 2020

Make sure you add an OWNERS or OWNERS_ALIASES file

@@ -0,0 +1,164 @@
/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably remove the v1alpha2 APIs completely

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably remove the v1alpha2 APIs completely

Removed v1alpha2 completely.

// +kubebuilder:subresource:status

// KubeadmConfig is the Schema for the kubeadmconfigs API
type KubeadmConfig struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this type will need to be renamed to avoid conflicting with the cloud-init based bootstrap provider.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this type will need to be renamed to avoid conflicting with the cloud-init based bootstrap provider.

Right. I renamed related structures to KubeadmIgnition*.

@@ -0,0 +1,9 @@
# Kubeadm types
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these are the same types being used by the cloud-init based bootstrap provider, you might be able to just import those rather than keeping a separate copy here as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these are the same types being used by the cloud-init based bootstrap provider, you might be able to just import those rather than keeping a separate copy here as well.

Right.
I removed unnecessary types, and tried to use existing types outside of exp.
Though types related to systemd units should stay here under exp/kubeadm-ignition/types.

@@ -0,0 +1,54 @@
/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar with these util/secret helpers, if they are the same as the cloud-init based bootstrap provider, might be worth it to just import those instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed util/secret from exp, and just used one in the upper-level.

@fabriziopandini
Copy link
Member

@dongsupark let me know when this one is ready for a first pass

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 4, 2020
@dongsupark
Copy link
Member Author

Make sure you add an OWNERS or OWNERS_ALIASES file

Added exp/kubeadm-ignition/OWNERS.

An experimental bootstrap provider for ignition.

It originally comes from
https://github.com/fossabot/cluster-api-bootstrap-provider-kubeadm-ignition

See also #3430
@ncdc
Copy link
Contributor

ncdc commented Aug 25, 2020

#3430 is currently in the v0.3.9 milestone, due in a few days. Do we anticipate this is ready to merge by then, or should we push this back?

"bytes"
"encoding/json"
"errors"
"github.com/aws/aws-sdk-go/aws"
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand, Ignition needs a source from which it pulls its configuration. This implementation is using S3 at the moment. We may want/need to support multiple sources, and S3 could/would be a reasonable choice when running on AWS. I don't know there's any simple way to keep source-specific code out of the bootstrap provider, unless you went with a plugin model, where the plugins lived in separate repos. That, however, would make deployment a lot more complicated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bootstrap provider that lives in the Cluster API repo being infrastructure agnostic should be a requirement IMO otherwise we're setting a precedent and breaking one of the core principles of Cluster API. If it's not straightforward it should come with a design proposal.

"I don't know there's any simple way to keep source-specific code out of the bootstrap provider" --> sounds like we need to do a bit more research around this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/flatcar-linux/ignition/blob/master/doc/supported-platforms.md gives a good overview of using ignition with different platforms

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 making this agnostic, let's discuss and design a solution that is agnostic. We have prior art with object references, or we could make this based on the infrastructure provider used (infra providers would provide the code that runs here)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would this be okay to switch to a generic S3 API? Like minio?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly 🤔 We try to be as agnostic as possible, what that really comes down to is provider interfaces that are satisfied with third party installations.

For example, we could abstract away this interface to be a metadata provider. Then, the AWS provider could contribute the AWS code, GCP, Azure can do the same with time.

What do you all think?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might should be part of the #3761 design


templateBackend, err := ignition.NewS3TemplateBackend(userdataDir, userDataBucket)
if err != nil {
setupLog.Error(err, "unable to create aws s3 session")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment

cc @vincepri

@dongsupark
Copy link
Member Author

@ncdc

#3430 is currently in the v0.3.9 milestone, due in a few days. Do we anticipate this is ready to merge by then, or should we push this back?

There are a couple of issues left in individual infrastructure providers that have cloud-init-specific implementations.
Let's push back it to the next milestone.

@vincepri
Copy link
Member

/milestone Next

@k8s-ci-robot k8s-ci-robot added this to the Next milestone Aug 26, 2020
@k8s-ci-robot
Copy link
Contributor

@dongsupark: PR needs rebase.

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 26, 2020
@vbatts
Copy link

vbatts commented Dec 2, 2020

WIP for rebasing this on v1alpha4 https://github.com/kinvolk/cluster-api/pull/1

Regardless, I'm hoping we can find a short term way forward while we figure out the architecture needed for #3761


const (
KubernetesDefaultVersion = "v1.17.4"
IngitionSchemaVersion = "2.2.0"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignition config spec 2.x is unmaintained and unsupported upstream. New code should use 3.x, either with or without compatibility code for 2.x. One approach is to generate 3.x structs and then use ign-converter to downconvert when needed.

Copy link

@LorbusChris LorbusChris Dec 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to add here: ign-converter also goes the other way, i.e. we're using it to migrate clusters provisioned with Ign0.x/spec 2.2.0 to Ign2.x/spec 3

@mariusgrigoriu
Copy link

Are the AWS and ignition version issues the only thing blocking merge?

@dongsupark
Copy link
Member Author

Thanks a lot for all comments.
@invidian and I have reworked the whole PR into a new one #4172.
For more contexts, please see the comment.
We will try to address comments in this PR as well.

@dongsupark dongsupark closed this Feb 11, 2021
@dongsupark dongsupark deleted the dongsu/bootstrap-ignition branch February 11, 2021 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ignition support in bootstrap provider