Skip to content
This repository was archived by the owner on Feb 6, 2025. It is now read-only.

Add kustomize support for addons #858

Merged
merged 1 commit into from
Mar 3, 2020
Merged

Add kustomize support for addons #858

merged 1 commit into from
Mar 3, 2020

Conversation

ereslibre
Copy link
Contributor

@ereslibre ereslibre commented Nov 27, 2019

Submitted for early feedback. Still WIP.

Why is this PR needed?

With kustomize, we can evolve our base manifests, applying the user
provided changes (patches) on top of them.

Implements: https://github.com/SUSE/caasp-rfc/pull/46
Fixes: https://github.com/SUSE/avant-garde/issues/941

What does this PR do?

This PR creates a new directory structure as defined by the linked RFC, so it's possible for users to provide their kustomize patches for every addon.

This allows us to respect user customizations when running skuba addon upgrade apply, because user patches will always be placed on top of our base manifests (that can freely evolve). If any of the user kustomize patch is outdated enough to not cleanly apply, skuba will report that error to the user.

Info for QA

It's needed to check different things:

  • You can add your own kustomize patches after skuba cluster init in the given directories, and these patches are respected when applied to the cluster, when doing skuba node bootstrap.

  • When you have patches, when running skuba addon upgrade apply, the user patches will be applied along with our evolved base to the cluster, so user customizations are kept at all times.

  • If you have incompatible patches (e.g. the resource you are patching was renamed in the base manifests, and what you are trying to patch no longer exists), then skuba will write an error message stating the error.

Docs

Documentation needs to be written, because a user action is required. They have to evolve the cluster definition folder from:

Old cluster definition folder
.
├── addons
│   ├── cilium
│   │   └── cilium.yaml
│   ├── cri
│   │   └── default_flags
│   ├── dex
│   │   └── dex.yaml
│   ├── gangway
│   │   └── gangway.yaml
│   ├── kured
│   │   └── kured.yaml
│   └── psp
│       └── psp.yaml
├── kubeadm-init.conf
└── kubeadm-join.conf.d
    ├── master.conf.template
    └── worker.conf.template

to

New cluster definition folder
.
├── addons
│   ├── cilium
│   │   ├── base
│   │   │   └── cilium.yaml
│   │   └── patches
│   ├── cri
│   │   └── default_flags
│   ├── dex
│   │   ├── base
│   │   │   └── dex.yaml
│   │   └── patches
│   ├── gangway
│   │   ├── base
│   │   │   └── gangway.yaml
│   │   └── patches
│   ├── kured
│   │   ├── base
│   │   │   └── kured.yaml
│   │   └── patches
│   └── psp
│       ├── base
│       │   └── psp.yaml
│       └── patches
├── kubeadm-init.conf
└── kubeadm-join.conf.d
    ├── master.conf.template
    └── worker.conf.template

Merge restrictions

(Please do not edit this)

We are in v4-maintenance phase, so we will restrict what can be merged to prevent unexpected surprises:

What can be merged (merge criteria):
    2 approvals:
        1 developer: code is fine
        1 QA: QA is fine
    there is a PR for updating documentation (or a statement that this is not needed)

@@ -275,6 +341,47 @@ func (addon Addon) Images(imageTag string) []string {
return images
}

func (addon Addon) listPatches() ([]string, error) {
result := []string{}
sourcePatches, err := filepath.Glob(filepath.Join(addon.patchResourcesDir(addon.addonDir()), "*.yaml"))
Copy link
Contributor

@mssola mssola Nov 29, 2019

Choose a reason for hiding this comment

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

Nitpicking: what if the user has files ending with ".yml" (it's a valid yaml extension)? Something like "*.y?ml" should work, right?

@evrardjp
Copy link
Contributor

evrardjp commented Dec 2, 2019

I don't have the opportunity (time) to propose an alternative approach, so I think this one would do. It's an improvement over what we do. I will vote positively on this approach, just for this.

I discussed this with Rafa, and we have a different view :)
I would tend to say it would be easier to have a different folder structure, on which everything is visible for the users. That folder structure would contain, at its root, a user modified kustomization yaml file, and it would load the different bases (1 per software). All our software manifests would be in a subfolder, named "bases" (so for example, dex would be in /bases/dex/).

Reasons:

  • We can then avoid the code that generates the manifests in a temp location
  • It's clear for the users that the folder they are manipulating contains the unique source of truth.
  • It's easier for users to write their changes: They can see the manifests we have updated by looking into the base folder (which is not the case here).
    This would mean that we need to drop new files/manifests into /base/ before applying them, which isn't too bad (can be done on init like we do now, on the plan, or in a new command "prepare" after plan).

This also marks a path forward where the source of truth is in the cluster, as the source of truth (the cluster definition folder) would simply be uploaded in the cluster (changing what an eventual "prepare" new command would do behind the scenes).

TL:DR; I have the impression my alternative is easier to maintain, and would be more user friendly (because users would see the base manifests).

@evrardjp
Copy link
Contributor

evrardjp commented Dec 2, 2019

As said above, I would +1 assuming this is out of its current WIP state.

Copy link
Contributor

@Itxaka Itxaka left a comment

Choose a reason for hiding this comment

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

+1 from me, looks good. Some nits here and there but seems like we could iterate on those later. Having this in seems more important so we can start building on top.

@jordimassaguerpla
Copy link
Member

Hi!
New code has been merged in master regarding terraform 0.12. Please rebase your branch to take this changes in.
Happy hacking

@evrardjp
Copy link
Contributor

hey, should we get people from the field tell us what they think of this here?
@ereslibre do you want me to update that PR, removing the WIP and rebasing it?

@ereslibre
Copy link
Contributor Author

hey, should we get people from the field tell us what they think of this here?

Field input is always welcome. This is implementing the RFC described above and since it got merged I'd try to not deviate too much from it, we can always evolve if the field thinks this as is is not enough.

do you want me to update that PR, removing the WIP and rebasing it?

This has been sitting for a while on WIP, mainly because I wasn't sure if using templating was the best approach, as opposed to using the Kustomize API directly. However, I think we can move forward with this, and improve later (maybe removing some templating in favour of using Kustomize API if it's something we want).

I'll rebase and update this PR tomorrow so we can continue working on this, and finally get rid of the WIP. We also will have to document on how to migrate a "cluster definition folder" to the new one (will be a set of mkdir's and mv's).

@evrardjp
Copy link
Contributor

hey, should we get people from the field tell us what they think of this here?

Field input is always welcome. This is implementing the RFC described above and since it got merged I'd try to not deviate too much from it, we can always evolve if the field thinks this as is is not enough.

That's correct, but field didn't really get the chance to review the rfc.

do you want me to update that PR, removing the WIP and rebasing it?

This has been sitting for a while on WIP, mainly because I wasn't sure if using templating was the best approach, as opposed to using the Kustomize API directly. However, I think we can move forward with this, and improve later (maybe removing some templating in favour of using Kustomize API if it's something we want).

I think templating is fine. I would have preferred if this was less obscure to the deployers, because here the files are actively in a temp space which can then be removed. But again, this is an implementation of the rfc.

I'll rebase and update this PR tomorrow so we can continue working on this, and finally get rid of the WIP. We also will have to document on how to migrate a "cluster definition folder" to the new one (will be a set of mkdir's and mv's).

Ok, if you tackle it, I will help on something else.

@ereslibre ereslibre changed the title WIP: Add kustomize support for addons Add kustomize support for addons Feb 27, 2020
@ereslibre
Copy link
Contributor Author

This will need documentation to migrate from the current cluster definition to the new one.

@jenting jenting self-requested a review March 2, 2020 11:14
With kustomize, we can evolve our base manifests, applying the user
provided changes (patches) on top of them.
@ereslibre ereslibre requested review from Itxaka, mssola and evrardjp March 3, 2020 11:58
@ereslibre
Copy link
Contributor Author

I think we can get this one merged, and evolve over this.

Copy link

@chentex chentex left a comment

Choose a reason for hiding this comment

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

👍

@ereslibre ereslibre merged commit 9309ada into SUSE:master Mar 3, 2020
@ereslibre ereslibre deleted the kustomize branch March 3, 2020 12:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants