Skip to content

Add ServiceAccounts for the ModuleLoader and DevicePlugin workloads #90

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

Merged

Conversation

mresvanis
Copy link
Contributor

@mresvanis mresvanis commented Sep 27, 2022

This PR adds ServiceAccounts for the module loader and the device plugin workloads of a Module, when no ServiceAccounts are specified in the Module.Spec.

This way we avoid using the K8S default ServiceAccount, which may have different permissions than the module loader and/or the device plugin workloads require.

Specifically, this PR:

  • adds the required ServiceAccount permissions to the module reconciler
  • adds the following Module reconciliation steps when the Module.Spec.ModuleLoader.ServiceAccountName is empty:
    • adds a module loader ServiceAccount
    • adds the module loaderServiceAccount to the module loader DaemonSet
  • adds the following Module reconciliation steps when the Module.Spec.DevicePlugin.ServiceAccountName is empty:
    • adds a device plugin ServiceAccount
    • adds the device plugin ServiceAccount to the device plugin DaemonSet

Signed-off-by: Michail Resvanis [email protected]

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 27, 2022
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 27, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @mresvanis. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 27, 2022
@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2022

Codecov Report

Base: 72.90% // Head: 73.46% // Increases project coverage by +0.55% 🎉

Coverage data is based on head (e034df0) compared to base (56b8bb3).
Patch coverage: 86.95% of modified lines in pull request are covered.

❗ Current head e034df0 differs from pull request most recent head 41910e7. Consider uploading reports for the commit 41910e7 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #90      +/-   ##
==========================================
+ Coverage   72.90%   73.46%   +0.55%     
==========================================
  Files          16       17       +1     
  Lines        1705     1771      +66     
==========================================
+ Hits         1243     1301      +58     
- Misses        398      404       +6     
- Partials       64       66       +2     
Impacted Files Coverage Δ
main.go 4.38% <0.00%> (-0.04%) ⬇️
controllers/module_reconciler.go 66.10% <30.00%> (-1.60%) ⬇️
internal/daemonset/daemonset.go 97.94% <100.00%> (+0.04%) ⬆️
internal/rbac/rbac.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@qbarrand
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 28, 2022
@qbarrand
Copy link
Contributor

This LGTM. Please fix the linting issue and squash into one commit.
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mresvanis, qbarrand

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2022
@mresvanis mresvanis force-pushed the add-default-serviceaccounts branch 5 times, most recently from 4892a75 to 3f95a85 Compare September 28, 2022 10:08
@mresvanis mresvanis marked this pull request as ready for review September 28, 2022 10:14
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 28, 2022
@mresvanis mresvanis force-pushed the add-default-serviceaccounts branch from 3f95a85 to d60b38a Compare September 28, 2022 10:19
@mresvanis
Copy link
Contributor Author

@ybettan @yevgeny-shnaidman this PR is now ready for review, whenever you have some time it would be great to take a look into it. Thank you!

@mresvanis mresvanis force-pushed the add-default-serviceaccounts branch from e034df0 to 41910e7 Compare September 28, 2022 14:05
This change adds ServiceAccounts for the module loader and the device plugin
workloads of a Module, when the ServiceAccounts are not defined in the
Module.Spec.ModuleLoader.ServiceAccountName and
Module.Spec.DevicePlugin.ServiceAccountName respectively.

This way we avoid using the K8S default ServiceAccount, which may have different
permissions that the module loader and/or the device plugin workloads require.

Specifically, this change:

- adds the required ServiceAccount permissions to the module reconciler
- adds the following Module reconciliation steps when the
  Module.Spec.ModuleLoader.ServiceAccountName is empty:
  - adds a module loader ServiceAccount
  - adds the module loaderServiceAccount to the module loader DaemonSet
- adds the following Module reconciliation steps when the
  Module.Spec.DevicePlugin.ServiceAccountName is empty:
  - adds a device plugin ServiceAccount
  - adds the device plugin ServiceAccount to the device plugin DaemonSet

Signed-off-by: Michail Resvanis <[email protected]>
@mresvanis mresvanis force-pushed the add-default-serviceaccounts branch from 41910e7 to c64e103 Compare September 28, 2022 14:45
@ybettan
Copy link
Contributor

ybettan commented Sep 28, 2022

/lgtm

1 similar comment
@ybettan
Copy link
Contributor

ybettan commented Sep 28, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 28, 2022
@k8s-ci-robot k8s-ci-robot merged commit 51ee5e0 into kubernetes-sigs:main Sep 28, 2022
@mresvanis mresvanis deleted the add-default-serviceaccounts branch September 29, 2022 07:40
qbarrand pushed a commit to qbarrand/kernel-module-management that referenced this pull request Jun 2, 2023
…etes-sigs#90)

In order to run only the `filter` package tests for example, use:
```
make unit-test TEST=github.com/qbarrand/oot-operator/internal/filter
```

Signed-off-by: Yoni Bettan <[email protected]>
qbarrand pushed a commit to qbarrand/kernel-module-management that referenced this pull request Jun 2, 2023
…sigs#90) (#56)

This change adds ServiceAccounts for the module loader and the device plugin
workloads of a Module, when the ServiceAccounts are not defined in the
Module.Spec.ModuleLoader.ServiceAccountName and
Module.Spec.DevicePlugin.ServiceAccountName respectively.

This way we avoid using the K8S default ServiceAccount, which may have different
permissions that the module loader and/or the device plugin workloads require.

Specifically, this change:

- adds the required ServiceAccount permissions to the module reconciler
- adds the following Module reconciliation steps when the
  Module.Spec.ModuleLoader.ServiceAccountName is empty:
  - adds a module loader ServiceAccount
  - adds the module loaderServiceAccount to the module loader DaemonSet
- adds the following Module reconciliation steps when the
  Module.Spec.DevicePlugin.ServiceAccountName is empty:
  - adds a device plugin ServiceAccount
  - adds the device plugin ServiceAccount to the device plugin DaemonSet

Signed-off-by: Michail Resvanis <[email protected]>

Signed-off-by: Michail Resvanis <[email protected]>
qbarrand pushed a commit to qbarrand/kernel-module-management that referenced this pull request Jun 2, 2023
…-sigs#90)

* Bump sigs.k8s.io/controller-runtime from 0.12.3 to 0.13.1

Bumps [sigs.k8s.io/controller-runtime](https://github.com/kubernetes-sigs/controller-runtime) from 0.12.3 to 0.13.1.
- [Release notes](https://github.com/kubernetes-sigs/controller-runtime/releases)
- [Changelog](https://github.com/kubernetes-sigs/controller-runtime/blob/master/RELEASE.md)
- [Commits](kubernetes-sigs/controller-runtime@v0.12.3...v0.13.1)

---
updated-dependencies:
- dependency-name: sigs.k8s.io/controller-runtime
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* fixup! Bump sigs.k8s.io/controller-runtime from 0.12.3 to 0.13.1

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Michail Resvanis <[email protected]>
yevgeny-shnaidman pushed a commit to yevgeny-shnaidman/kernel-module-management-upstream that referenced this pull request Jul 31, 2023
…etes-sigs#90)

In order to run only the `filter` package tests for example, use:
```
make unit-test TEST=github.com/qbarrand/oot-operator/internal/filter
```

Signed-off-by: Yoni Bettan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants