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

feat: support config embedded and external files #1417

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

aabouzaid
Copy link
Contributor

@aabouzaid aabouzaid commented Mar 24, 2024

What

This PR introduces embedded manifests in the k3d config file which boosts the portability of the config and makes it easier to share.

So adding an embedded manifest could be as easy as:

apiVersion: k3d.io/v1alpha5
kind: Simple
metadata:
  name: my-app

manifests:
  - name: my-app-ns.yaml
    manifest: |
      ---
      apiVersion: v1
      kind: Namespace
      metadata:
        name: my-app

This will be mounted under: /var/lib/rancher/k3s/server/manifests/embedded/my-app-ns.yaml.

Why

In addition to the portability, the goal is to make it easy to seed k3d resources, and given that k3s already supports HelmChart it will be even easier to pre-install any custom packages when the cluster is created.

Fixes #536
Also partially fixes #1065 since no need to be in the same dir to mount some seed manifests.

Implications

No expected change in the behavior.

Note

The CLI part is not done yet, I will do it once we agree on the implementation.

Thank you 🙌

@aabouzaid
Copy link
Contributor Author

aabouzaid commented Mar 24, 2024

OK, I've found an issue in my implementation, and now I know no one worked on that feature since 2021 😄 (and the whole topic of needing to use the absolute part rather than the relative path).

So using tmp files doesn't work because if the files are deleted the cluster will not be able to start.
The closest solution is to use the k3d config dir as a base for the embedded manifests ... but any better ideas are welcome 🤔

@aabouzaid aabouzaid force-pushed the feat-introduce-embedded-manifests branch 3 times, most recently from bf92876 to e229304 Compare March 24, 2024 22:43
@aabouzaid
Copy link
Contributor Author

After some thinking, I've found a better way to utilize the tools node to copy the manifests to a volume instead of relying on the files on the host.

I'd consider this the final implementation and if we agree on it, then I will add the CLI args if needed (tbh, I don't see much value of having it in the CLI).

@aabouzaid
Copy link
Contributor Author

@iwilltry42 it would be great if you could take a look at this PR 🙌 (please ignore the first 2 commits, I just left them to show what I tried and didn't work).

@iwilltry42
Copy link
Member

Hi @aabouzaid !
Thanks for putting this up and sorry for letting you wait.
I'm thinking that maybe a more generic approach for arbitrary files may be better.
Like

files:
  - path: k3s-manifests/foo/bar.yaml # k3s-manifests as magic word that gets expanded like we have it in volume mounts
    content: |
      File contents

What do you think?

@aabouzaid
Copy link
Contributor Author

@iwilltry42 sounds like a pretty good idea 👌

Just to ensure that I got it right before heading to the implementation 😁

The path: k3s-manifests/foo/bar.yaml will be copied under /var/lib/rancher/k3s/server/manifests/embedded/foo/bar.yaml
Is that what you mean, right?

@iwilltry42
Copy link
Member

iwilltry42 commented Mar 31, 2024

Yeah, sorry, I'm on my phone only - k3s-manifests is like a shortcut to the manifests directory.

We have a few of those "magic paths" for volume mounts. I can't find the respective issue/release right now though.

Edit: #916

@aabouzaid
Copy link
Contributor Author

aabouzaid commented Apr 3, 2024

@iwilltry42 When I started working on this feature I tried to make it as tiny as possible.

If you are in favor of making it a bit generic, what do you think about making it for any kind of manifest (embedded or in an external file)? That will fix the issue no. #1065 completely in addition to #536

I will think of it like this:

1. Embedded Manifest

manifests:
  - destination: k3s-manifests/foo/bar.yaml
    manifest: |
      ---
      apiVersion: v1
      kind: Namespace
      metadata:
        name: my-app

2. External Manifest - Relative path

manifests:
  - source: foo/bar.yaml
    destination: k3s-manifests/foo/bar.yaml

The destination will support 3 cases:

  • Absolute path: /var/lib/rancher/k3s/server/manifests/foo/bar.yaml
  • Magic path: k3s-manifests-custom/foo/bar.yaml (will be mounted under: /var/lib/rancher/k3s/server/manifests/custom/foo/bar.yaml)
  • Default relative path: foo/bar.yaml (will be mounted under: /var/lib/rancher/k3s/server/manifests/custom/foo/bar.yaml)

The source will support 2 cases (from the host machine):

  • Absolute path: /tmp/bar.yaml
  • Config file relative path: bar.yaml

And manifest which will be embedded in the config file, and it will be mutually exclusive with source (or even remove the manifest and only keep source by parsing if it's a file path or content).


What do you think? 🤔 (I hope I didn't scratch it too much 😅 but that way we cover more use cases).

@iwilltry42
Copy link
Member

@aabouzaid yes, that's what I envisioned 👍
Let's call it files though, so it not only applies to the auto-deploy manifests.
So

files:
  - source: ./relative/path/bar.yaml
    destination: k3s-manifests/magic/path/bar.yaml
  - source: /absolute/path/foo.yaml
    destination: /some/path/in/container/foo.yaml
  - destination: /some/dest/path/baz.yaml
     source: |
      # some yaml file
      abc:
        def:
          - a
          - b

Default relative path: foo/bar.yaml (will be mounted under: /var/lib/rancher/k3s/server/manifests/custom/foo/bar.yaml)

I'd rather not do this, since I think magic paths (shortcuts) are good enough and not too much to type - it may be confusing for the more generic files approach.

We've got to make it clear (docs and help texts) that the source is always copied into the container at start time, not mounted like volumes.

I agree, that relative paths should always be considered relative to the config file - for everything that should be relative to the current workdir, one can use ${PWD} in the config file and it should be substituted by the current workdir.

Are you ok with this?

@iwilltry42 iwilltry42 added this to the v5.7.0 milestone Apr 3, 2024
@iwilltry42 iwilltry42 added enhancement New feature or request component/main labels Apr 3, 2024
@aabouzaid aabouzaid force-pushed the feat-introduce-embedded-manifests branch 5 times, most recently from adc077c to 471f121 Compare April 6, 2024 23:21
Copy link
Contributor Author

@aabouzaid aabouzaid left a comment

Choose a reason for hiding this comment

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

@iwilltry42 I'm done with the main part.

But there are a couple of places I still need your opinion about them 🙌

cmd/cluster/clusterCreate.go Show resolved Hide resolved
pkg/config/v1alpha5/schema.json Outdated Show resolved Hide resolved
pkg/util/files.go Outdated Show resolved Hide resolved
@aabouzaid aabouzaid changed the title feat: support config embedded manifests feat: support config embedded and external files Apr 7, 2024
Copy link
Member

@iwilltry42 iwilltry42 left a comment

Choose a reason for hiding this comment

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

Do you think it'd be nice to have (optional) nodeFilters here as well to scope the files to specific nodes only (i.e., it would be fine if manifests are only deployed to server nodes).

Overall, I like the implementation, good job :)

pkg/util/files.go Outdated Show resolved Hide resolved
pkg/util/files.go Outdated Show resolved Hide resolved
cmd/cluster/clusterCreate.go Show resolved Hide resolved
pkg/config/v1alpha5/schema.json Outdated Show resolved Hide resolved
pkg/config/v1alpha5/types.go Outdated Show resolved Hide resolved
@aabouzaid aabouzaid force-pushed the feat-introduce-embedded-manifests branch from 521316a to c1fd768 Compare April 16, 2024 07:55
@aabouzaid aabouzaid requested a review from iwilltry42 April 16, 2024 07:56
@aabouzaid
Copy link
Contributor Author

@iwilltry42 Please give it a shot, now both embedded and referenced files read from source, and also added nodeFilters.
Thanks for your support 🙌

Copy link
Member

@iwilltry42 iwilltry42 left a comment

Choose a reason for hiding this comment

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

Looks nice and clean, awesome! :)

@iwilltry42 iwilltry42 force-pushed the feat-introduce-embedded-manifests branch from c1fd768 to 1e4c076 Compare April 16, 2024 08:24
@iwilltry42
Copy link
Member

@aabouzaid thanks for this!
Are you good with merging this in and we can follow-up with a CLI equivalent later?

@aabouzaid aabouzaid force-pushed the feat-introduce-embedded-manifests branch from 1e4c076 to bc47c84 Compare April 16, 2024 08:40
@aabouzaid aabouzaid force-pushed the feat-introduce-embedded-manifests branch from bc47c84 to 6ab5fc7 Compare April 16, 2024 08:42
@aabouzaid
Copy link
Contributor Author

@iwilltry42 Sure, let's do it 🙌
And I'd be happy to work on the CLI part next.

@aabouzaid
Copy link
Contributor Author

@iwilltry42 BTW, any docs should be updated at this stage? Like this page: https://k3d.io/v5.6.0/usage/configfile/#all-options-example

@iwilltry42 iwilltry42 force-pushed the feat-introduce-embedded-manifests branch from 6ab5fc7 to b4333cc Compare April 16, 2024 08:58
@iwilltry42 iwilltry42 merged commit 9eaa6c4 into k3d-io:main Apr 16, 2024
@iwilltry42
Copy link
Member

Docs can be added any time before the next release 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/main enhancement New feature or request
Projects
None yet
2 participants