feat(KEP): add service account auto injection KEP for sidecarset #1820
Conversation
magicsong
force-pushed
the
add_service_account_proposal
branch
from
2b41871
to
3c64776
Compare
magicsong
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1820 +/- ##
==========================================
+ Coverage 47.91% 49.94% +2.02%
==========================================
Files 162 192 +30
Lines 23491 24634 +1143
==========================================
+ Hits 11256 12303 +1047
- Misses 11014 11053 +39
- Partials 1221 1278 +57
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
magicsong
force-pushed
the
add_service_account_proposal
branch
from
3c64776
to
f664b5f
Compare
magicsong
changed the title
Signed-off-by: magicsong <[email protected]>
magicsong
force-pushed
the
add_service_account_proposal
branch
from
f664b5f
to
880be6b
Compare
|
|
||
| ### Risks and Mitigations | ||
|
|
||
| - **Risk**: Potential misconfiguration may lead to sidecars running with unintended permissions. |
There was a problem hiding this comment.
consider add a serviceaccount white list similar to https://openkruise.io/docs/user-manuals/sidecarset#inject-pod-metadata-annotations
There was a problem hiding this comment.
I understand that when you mention "whitelist," you are referring to "role," right? In this scenario, it is ultimately the user's responsibility to create the RoleBinding, and Kruise should not manage this Role
There was a problem hiding this comment.
the whitelist is to control which sidecarset can inject which serviceaccount, so as to control the risk that un-intended user of sidecarset inject serviceaccounts.
| ### API Changes | ||
|
|
||
| 1. Add a new `serviceAccountName` field in the SidecarSet specification, allowing users to define a ServiceAccount specifically for the sidecar containers. | ||
| 2. Add a new `errorWhenServiceAccountInjectionConflict` field in the SidecarSet specification to control the behavior when multiple SidecarSets inject different ServiceAccounts into the same pod. |
There was a problem hiding this comment.
it seems reporting an error is the only sane solution when facing injection conflict. it is overly complex and security nightmare if kruise-manager can merge and operate rolebinding/clusterrolebinding
There was a problem hiding this comment.
Why? Merging a role isn't very difficult. However, shouldn't we do it before the user realizes the original role list will be lost?
| metadata: | ||
| name: sidecarset-sample | ||
| spec: | ||
| serviceAccountName: "sidecar-service-account" |
There was a problem hiding this comment.
- what if the user change the serviceAccountName in the SidecarSet, what is the expected behavior?
- what if the service account does not exist in the namespaces to related pods
There was a problem hiding this comment.
- This field is immutable, just like in Kubernetes.
- We handle it during validation; the service account must exist.
|
@magicsong can you attend the bi-weekly community meeting this week, we can discuss the kep in detail |
sure |
|
another possible solution is to add secret backed service account using volumes. |
Ⅰ. Describe what this PR does
a KEP to add service account auto injection for sidecarset
Ⅱ. Does this pull request fix one issue?
Ⅲ. Describe how to verify it
Ⅳ. Special notes for reviews
related issue: #1747