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

Allow yip configs #751

Merged
merged 1 commit into from
Jun 17, 2024
Merged

Allow yip configs #751

merged 1 commit into from
Jun 17, 2024

Conversation

anmazzotti
Copy link
Contributor

@anmazzotti anmazzotti commented Jun 12, 2024

Closes rancher/elemental#1437

@davidcassany This is what I had in mind.

(Still have to test it, but it should work)

Whenever the user can define a "cloud config", the elemental-operator codebase always assumed it was cloud-init style, therefore it always prefixed the #cloud-config header.

With this patch we try to figure out if this is a yip config (technically if at least one stage has been defined), before applying or not the #cloud-config header.

I also removed the custom mapping code and used sigs.k8s.io/yaml which contrary to our code, works when working with maps (as stages is in yip)

@github-actions github-actions bot added area/operator operator related changes area/tests test related changes area/build build related changes labels Jun 12, 2024
Signed-off-by: Andrea Mazzotti <andrea.mazzotti@suse.com>
@anmazzotti
Copy link
Contributor Author

Finally managed to test this PR.

Any existing cloud-init config style works as expected. However now you have the option to use yip like config.
For example given a registration like:

apiVersion: elemental.cattle.io/v1beta1
kind: MachineRegistration
metadata:
  name: fire-nodes
  namespace: fleet-default
spec:
  machineName: test-btrfs-${System Information/UUID}
  config:
    cloud-config:
      name: "A registration driven config"
      stages:
        initramfs:
        - name: "Setup users"
          ensure_entities:
          - path: /etc/shadow
            entity: |
                kind: "shadow"
                username: "root"
                password: "root"
        fs:
        - name: "Create dummy file"
          files:
          - path: /oem/test
            content: |
                      test                    
            permissions: 0777
            owner: 1000
            group: 100

The system gets initialized as expected and the resulting config (/oem/91_custom.yaml) installed looks like this:

name: A registration driven config
stages:
  fs:
  - files:
    - content: "test                    \n"
      group: 100
      owner: 1000
      path: /oem/test
      permissions: 511
    name: Create dummy file
  initramfs:
  - ensure_entities:
    - entity: |
        kind: "shadow"
        username: "root"
        password: "root"
      path: /etc/shadow
    name: Setup users

@anmazzotti anmazzotti marked this pull request as ready for review June 13, 2024 09:32
@anmazzotti anmazzotti requested a review from a team as a code owner June 13, 2024 09:32
@anmazzotti anmazzotti self-assigned this Jun 13, 2024
Copy link
Contributor

@davidcassany davidcassany left a comment

Choose a reason for hiding this comment

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

LGTM
Just a comment thinking if we could bring it a step further by adapting yip to run strict validation of the schema. We could make it toggable at yip side to keep yip backward compatible.

// Determine whether this is a yip config or a cloud-init one.
// Since all fields are optional in yip, we test whether any stage has been defined,
// as an indication that this is indeed a yip config.
yipConfig := &schema.YipConfig{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Given we are embracing schema adoption in server side I am wondering if should we get full benefit of it. We making stronger the coupling between elemental-operator and elemental-toolkit through yip, so I am wondering if shouldn't we consider strict schema use. I am, providing a validation on the schema to detect wrong syntax on server side.
We could consider adapting yip for a strict validation. Then here we could simply try yip syntax and on error try cloud-init syntax and on error simply error out and make it visible through the resource state something went wrong. I think this would be a great improvement to prevent non supported cloud-init syntax. What you think?

Copy link
Contributor Author

@anmazzotti anmazzotti Jun 17, 2024

Choose a reason for hiding this comment

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

Would indeed be nice to have validation. Right now indeed you could write anything in there and it will fail during registration/install, which is way too late.

This however seem to suggest we need a validation webhook, so we should first implement that, then rebase this PR to it (plus the strict validation as you suggested)

Edit: Nevermind, I did miss your approval. I opened #755 to improve on this.

@anmazzotti anmazzotti merged commit 9c8550f into rancher:main Jun 17, 2024
22 checks passed
anmazzotti added a commit to anmazzotti/elemental-operator that referenced this pull request Jun 18, 2024
Signed-off-by: Andrea Mazzotti <andrea.mazzotti@suse.com>
anmazzotti added a commit to anmazzotti/elemental-operator that referenced this pull request Jun 18, 2024
Signed-off-by: Andrea Mazzotti <andrea.mazzotti@suse.com>
anmazzotti added a commit that referenced this pull request Jun 18, 2024
Signed-off-by: Andrea Mazzotti <andrea.mazzotti@suse.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build build related changes area/operator operator related changes area/tests test related changes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Support Yip syntax and cloud-init syntax all together
2 participants