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

Fix cisProfile option with ignition format #402

Merged
merged 4 commits into from
Aug 23, 2024

Conversation

hardys
Copy link
Contributor

@hardys hardys commented Aug 12, 2024

Currently there are some stray tabs which break the rendered output, replace these with spaces and add some unit test coverage.

kind/bug

What this PR does / why we need it:

Currently the cisProfile option is broken when selecting ignition format for Leap/SLEMicro deployments

Fixes: #401

Checklist:

  • squashed commits into logical changes
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@hardys hardys marked this pull request as draft August 12, 2024 10:36
@hardys
Copy link
Contributor Author

hardys commented Aug 12, 2024

Review welcome but marking this as a draft until I've tested it e2e locally

@furkatgofurov7
Copy link
Contributor

@hardys let's rebase before re-running e2e tests on this one, thanks!

@hardys
Copy link
Contributor Author

hardys commented Aug 13, 2024

@hardys let's rebase before re-running e2e tests on this one, thanks!

Will do, just want to complete some more local testing then will rebase and remove the draft status

Also looks like we'll need #301 so good to see that may be getting revived

@hardys
Copy link
Contributor Author

hardys commented Aug 15, 2024

After testing locally I found another issue:

	converting base config to Ignition: error converting to Ignition: warning at $.storage.files.1.mode, line 41 col 13: unreasonable mode would be reasonable if specified in octal; remember to add a leading zero

I think that's because the mode constant here needs updating, will add to this PR and re-test

@hardys
Copy link
Contributor Author

hardys commented Aug 16, 2024

I found some more issues which should now be resolved:

  • The CIS prepration script was copies to /opt which isn't writable for Leap/SLEMicro, moved to /etc for both ignition/cloud-init cases so it's alongside the configuration files
  • The script was running too early in the ignition flow, prior to the RKE2 installation, so the sysctl file was not yet present
  • The script logic to find the sysctl file did not include the path used by the RKE2 installer, now added

@hardys
Copy link
Contributor Author

hardys commented Aug 22, 2024

The CIS prepration script was copies to /opt which isn't writable for Leap/SLEMicro, moved to /etc for both ignition/cloud-init cases so it's alongside the configuration files

The latest iteration takes a different approach and mounts the /opt subvolume as described in https://en.opensuse.org/Portal:MicroOS/Ignition#Mounts - I'm still not sure writing the file to /opt is ideal, but with this approach I can avoid changing any of the existing cloud-init related code.

@hardys hardys marked this pull request as ready for review August 22, 2024 14:59
@hardys
Copy link
Contributor Author

hardys commented Aug 22, 2024

@alexander-demicev @furkatgofurov7 this is passing CI and now ready for review - please take a look when you get a moment, thanks!

This causes a `unreasonable mode` error because it's specified as a
string not octal like the default mode

Signed-off-by: Steven Hardy <[email protected]>
Otherwise the script fails, this aligns with how the cloud-init
script works.

Signed-off-by: Steven Hardy <[email protected]>
The default path using the script installer writes this to
/opt/rke2/share/rke2/rke2-cis-sysctl.conf (on Leap/SLEMicro at least)

Signed-off-by: Steven Hardy <[email protected]>
This is not mounted by default, therefore we cannot write the
/opt/rke2-cis-script.sh script.

So add configuration to mount the subvolume, as described in
https://en.opensuse.org/Portal:MicroOS/Ignition#Mounts

Signed-off-by: Steven Hardy <[email protected]>
auto-merge was automatically disabled August 23, 2024 10:47

Head branch was pushed to by a user without write access

@furkatgofurov7 furkatgofurov7 merged commit bc6cb16 into rancher:main Aug 23, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cisProfile option broken with ignition format
3 participants