Skip to content

Upgrade Kured to v1.15.1#2160

Merged
Zash merged 1 commit intomainfrom
simonl/kured-upgrade
Jun 12, 2024
Merged

Upgrade Kured to v1.15.1#2160
Zash merged 1 commit intomainfrom
simonl/kured-upgrade

Conversation

@lunkan93
Copy link
Contributor

@lunkan93 lunkan93 commented Jun 4, 2024

Warning

This is a public repository, ensure not to disclose:

  • personal data beyond what is necessary for interacting with this pull request, nor
  • business confidential information, such as customer names.

What kind of PR is this?

Required: Mark one of the following that is applicable:

  • kind/feature
  • kind/improvement
  • kind/deprecation
  • kind/documentation
  • kind/clean-up
  • kind/bug
  • kind/other

Optional: Mark one or more of the following that are applicable:

Important

Breaking changes should be marked kind/admin-change or kind/dev-change depending on type
Critical security fixes should be marked with kind/security

  • kind/admin-change
  • kind/dev-change
  • kind/security
  • kind/adr

What does this PR do / why do we need this PR?

Upgraded kured to Chart Version 5.4.5 and App Version 1.15.1

Information to reviewers

Two security-related default-values were improved:

  • hostNetwork is set to false by default now.
  • readOnlyRootFilesystem is set to true by default now.

Also kured now wants to mount a hostPath for /var/run, for the reboot-sentinel-file, by default. Any opinions on this?

Checklist

  • Proper commit message prefix on all commits
  • Change checks:
    • The change is transparent
    • The change is disruptive
    • The change requires no migration steps
    • The change requires migration steps
    • The change upgrades CRDs
    • The change updates the config and the schema
  • Metrics checks:
    • The metrics are still exposed and present in Grafana after the change
    • The metrics names didn't change (Grafana dashboards and Prometheus alerts are not affected)
    • The metrics names did change (Grafana dashboards and Prometheus alerts were fixed)
  • Logs checks:
    • The logs do not show any errors after the change
  • Pod Security Policy checks:
    • Any changed pod is covered by Pod Security Admission
    • Any changed pod is covered by Gatekeeper Pod Security Policies
    • The change does not cause any pods to be blocked by Pod Security Admission or Policies
  • Network Policy checks:
    • Any changed pod is covered by Network Policies
    • The change does not cause any dropped packets in the NetworkPolicy Dashboard
  • Audit checks:
    • The change does not cause any unnecessary Kubernetes audit events
    • The change requires changes to Kubernetes audit policy
  • Falco checks:
    • The change does not cause any alerts to be generated by Falco
  • Bug checks:
    • The bug fix is covered by regression tests

@lunkan93 lunkan93 self-assigned this Jun 4, 2024
@lunkan93 lunkan93 added the kind/improvement Improvement of existing features, e.g. code cleanup or optimizations. label Jun 4, 2024
@OlleLarsson
Copy link
Contributor

OlleLarsson commented Jun 5, 2024

Also kured now wants to mount a hostPath for /var/run, for the reboot-sentinel-file, by default.

The conclusion here was that the default behavior changed from using nsenter to read the file (still uses nsenter to execute the reboot command as far as I understand) on the host to mounting a hostPath instead.

It would be interesting to look at using the signal-reboot mode as that seem to remove the need for Kured to run in privileged mode!

@lunkan93
Copy link
Contributor Author

lunkan93 commented Jun 5, 2024

Tested out the signal method with the config changes from the latest commit.
Noteworthy is that I had to add the annotation container.apparmor.security.beta.kubernetes.io/kured: unconfined as mentioned here, as kured was getting:

time="2024-06-05T07:11:07Z" level=info msg="Emit reboot-signal for node: simonl-dev-mc-worker-nova-4018765f-9m8kl"
time="2024-06-05T07:11:07Z" level=fatal msg="Signal of SIGRTMIN+5 failed: permission denied"

Thoughts?

@OlleLarsson
Copy link
Contributor

Tested out the signal method with the config changes from the latest commit. Noteworthy is that I had to add the annotation container.apparmor.security.beta.kubernetes.io/kured: unconfined as mentioned here, as kured was getting:

time="2024-06-05T07:11:07Z" level=info msg="Emit reboot-signal for node: simonl-dev-mc-worker-nova-4018765f-9m8kl"
time="2024-06-05T07:11:07Z" level=fatal msg="Signal of SIGRTMIN+5 failed: permission denied"

Thoughts?

I unfortunately can't say what implications it has to disable apparmor vs running it as privileged, but my initial hunch is that it is more secure to run the container without privileged and apparmor than it is to run it privileged.

@Zash Zash self-assigned this Jun 11, 2024
@Zash Zash force-pushed the simonl/kured-upgrade branch from fcaa17e to 25dc614 Compare June 11, 2024 12:06
@Zash
Copy link
Contributor

Zash commented Jun 11, 2024

I moved the signal method out of this branch for now, pending further investigation into the apparmor issue.

@Zash Zash merged commit 87d4ae2 into main Jun 12, 2024
@Zash Zash deleted the simonl/kured-upgrade branch June 12, 2024 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/improvement Improvement of existing features, e.g. code cleanup or optimizations.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[2] Upgrade Kured to app v1.14.1 and chart 5.3.1

5 participants