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

Add "cis "to the cis-profile enum to support 1.29+ #301

Merged
merged 3 commits into from
Aug 26, 2024

Conversation

AshleyDumaine
Copy link
Contributor

@AshleyDumaine AshleyDumaine commented Apr 18, 2024

What this PR does / why we need it: For RKE2 version >=1.29, the cis profile flag needs to be "cis" (https://docs.rke2.io/security/hardening_guide#generic-cis-configuration), but this isn't currently in the CRD enums.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer: I'm not sure if a v1beta2 will be needed for this since this since this is a CRD change but no fields are being added/removed from the spec.

Checklist:

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

@richardcase richardcase added area/api Indicates an issue or PR relates to the APIs area/bootstrap Indicates an issue or PR related to the bootstrap provider kind/feature New feature or request labels Apr 19, 2024
@richardcase
Copy link
Contributor

Thanks for this @AshleyDumaine . Would you be able to look at the CI failure?

auto-merge was automatically disabled April 19, 2024 17:36

Head branch was pushed to by a user without write access

@AshleyDumaine
Copy link
Contributor Author

Thanks for this @AshleyDumaine . Would you be able to look at the CI failure?

It looks like controller-gen for v0.13.0 is segfaulting, but I don't see this issue when running with v0.14.0. Is it okay to bump controller-gen in this PR?

@richardcase
Copy link
Contributor

It looks like controller-gen for v0.13.0 is segfaulting, but I don't see this issue when running with v0.14.0. Is it okay to bump controller-gen in this PR?

Yes i think so 👍

@AshleyDumaine
Copy link
Contributor Author

It looks like controller-gen for v0.13.0 is segfaulting, but I don't see this issue when running with v0.14.0. Is it okay to bump controller-gen in this PR?

Yes i think so 👍

Okay sounds good! I regenerated the CRDs and make verify is coming back clean locally, can you please retrigger the CI when you get the chance? Thanks!

@AshleyDumaine
Copy link
Contributor Author

@richardcase I also had to bump conversion-gen to 0.30.0 since the conversion files were failing to generate as well, but I had to account for this issue in conversion.go: kubernetes/code-generator#94 (comment)

Copy link

github-actions bot commented Aug 2, 2024

This PR is stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 2, 2024
@AshleyDumaine
Copy link
Contributor Author

Hey @richardcase would you be able to retrigger the CI for this? 🙏 Just updated for the repo being moved out of the sandbox project.

@github-actions github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 3, 2024
@richardcase
Copy link
Contributor

@AshleyDumaine - i'm afraid i can no longer trigger the CI. Perhaps @alexander-demicev , @Danil-Grigorev or @furkatgofurov7 can?

Danil-Grigorev
Danil-Grigorev previously approved these changes Aug 5, 2024
@furkatgofurov7
Copy link
Contributor

@AshleyDumaine hey, sorry for late reply on this. Can you please rebase the PR on the latest main and then we can re-trigger e2e tests.

--build-tag=ignore_autogenerated_rk2_control_plane \
--output-file-base=zz_generated.conversion $(ROOT_DIR) \
--go-header-file=./hack/boilerplate.go.txt
--output-file=zz_generated.conversion.go $(ROOT_DIR)/$(CAPRKE2_DIR) \
Copy link
Contributor

@hardys hardys Aug 14, 2024

Choose a reason for hiding this comment

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

I think the --build-tag here need to be removed like the bootstrap?

make generate works for me locally with that removed, and we can see unknown flag: --build-tag in the CI results

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated 👍

@hardys
Copy link
Contributor

hardys commented Aug 15, 2024

The test failure now looks unrelated to the PR

Error: No space left on device : '/home/runner/runners/2.319.0/_diag/pages/d64410bf-7d25-4684-bd7f-9ce1f1fc6dc4_30a67a00-fcd0-50a9-e885-da1abe87be01_1.log'

@furkatgofurov7 is it worth rechecking as that may be an issue with a specific runner?

@furkatgofurov7
Copy link
Contributor

The test failure now looks unrelated to the PR

Error: No space left on device : '/home/runner/runners/2.319.0/_diag/pages/d64410bf-7d25-4684-bd7f-9ce1f1fc6dc4_30a67a00-fcd0-50a9-e885-da1abe87be01_1.log'

@furkatgofurov7 is it worth rechecking as that may be an issue with a specific runner?

@hardys sure re-triggering it, I can't find last e2e run logs when checking it so looks like an infra issue with GH runner (seeing this frequently these days).

@hardys
Copy link
Contributor

hardys commented Aug 21, 2024

@furkatgofurov7 I know there were some issues with the e2e test recently which appear to have caused the most recent failure, are those now resolved so this can be rechecked again?

@hardys
Copy link
Contributor

hardys commented Aug 21, 2024

I can't approve but this lgtm - I tested locally in conjunction with #402 and all works as expected when deploying RKE2 1.30.3, hopefully we can resolve the e2e issues soon as AFAICS they aren't related to this PR

@furkatgofurov7
Copy link
Contributor

furkatgofurov7 commented Aug 22, 2024

@hardys @AshleyDumaine sorry for the CI problems unrelated to your PRs. We are still investigating it, meanwhile we have some assumptions around what could be the root cause based on the latest runs on this PR (TL;DR is, scaling up workers to 3 and applying an upgrade to newer version of k8s does not work due to some errors on CP never being upgraded to new k8s version
stderr: Job for rke2-server.service failed because the control process exited with error code.)

@furkatgofurov7
Copy link
Contributor

@hardys @AshleyDumaine sorry for the CI problems unrelated to your PRs. We are still investigating it, meanwhile we have some assumptions around what could be the root cause based on the latest runs on this PR (TL;DR is, scaling up workers to 3 and applying an upgrade to newer version of k8s does not work due to some errors on CP never being upgraded to new k8s version

stderr: Job for rke2-server.service failed because the control process exited with error code.)

Possible e2e fix #413 is merged, can you please rebase once again and we rerun the tests? Thank you.

@AshleyDumaine
Copy link
Contributor Author

@hardys @AshleyDumaine sorry for the CI problems unrelated to your PRs. We are still investigating it, meanwhile we have some assumptions around what could be the root cause based on the latest runs on this PR (TL;DR is, scaling up workers to 3 and applying an upgrade to newer version of k8s does not work due to some errors on CP never being upgraded to new k8s version
stderr: Job for rke2-server.service failed because the control process exited with error code.)

Possible e2e fix #413 is merged, can you please rebase once again and we rerun the tests? Thank you.

Should be good to run now, just rebased. Thanks!

Copy link
Contributor

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

Thank you very much for your work and patience @AshleyDumaine!

@alexander-demicev
Copy link
Member

@AshleyDumaine We have a check that requires all commits to be signed, can I ask you to sign them?

@furkatgofurov7
Copy link
Contributor

@AshleyDumaine We have a check that requires all commits to be signed, can I ask you to sign them?

@alexander-demicev all commits are signed AFAICT

@alexander-demicev
Copy link
Member

@hardys
Copy link
Contributor

hardys commented Aug 23, 2024

@furkatgofurov7 Commits have to be signed with a GPG key https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification

I just followed this guide and had issues with the GPG key local signing, but configuring for SSH key signing worked (note you have to upload the public key again even if it's already used for verification, since the key upload UI has a dropdown for key type which must be signing)

Also @AshleyDumaine you might find it convenient when configured to run git rebase --exec 'git commit --amend --no-edit -n -S' -i origin to sign all the commits on your branch

auto-merge was automatically disabled August 26, 2024 14:04

Head branch was pushed to by a user without write access

@AshleyDumaine
Copy link
Contributor Author

@furkatgofurov7 Commits have to be signed with a GPG key https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification

I just followed this guide and had issues with the GPG key local signing, but configuring for SSH key signing worked (note you have to upload the public key again even if it's already used for verification, since the key upload UI has a dropdown for key type which must be signing)

Also @AshleyDumaine you might find it convenient when configured to run git rebase --exec 'git commit --amend --no-edit -n -S' -i origin to sign all the commits on your branch

Thanks, should be good to go now.

@furkatgofurov7 furkatgofurov7 merged commit 1954b23 into rancher:main Aug 26, 2024
4 checks passed
@AshleyDumaine AshleyDumaine deleted the cis-enum branch August 27, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Indicates an issue or PR relates to the APIs area/bootstrap Indicates an issue or PR related to the bootstrap provider kind/feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants