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.

Sign up for GitHub

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

Jump to bottom

dynamic allowed service accounts watcher for injector #5898

Merged
merged 31 commits into from Mar 6, 2023
Merged

dynamic allowed service accounts watcher for injector #5898

merged 31 commits into from Mar 6, 2023

Conversation

filintod
Copy link
Contributor

@filintod filintod commented Feb 7, 2023 •
edited

Description

Added capability to dynamically add allowed UIDs by watching against namespace and/or service account name prefixes. When a service account is created or deleted we add/delete the ID of the service account in the injector authUIDs list. The watch can also be set against a string label selector.

A user/operator can provide the extra value allowedServiceAccountsPrefixNames to the sidecar chart definition with star sign at the end to say whether the service account name or the namespace or both are prefixes. We separate each namespace:serviceaccountname tuple with commas (,). The predicates will be ORed.

Service Account Name or Namespace Prefix watch examples

  • Service Account name prefix with fixed namespace
    Watch for service account names prefix with name1-x or name2-x in namespace1, or for name1-x in namepsace-2. 
    allowedServiceAccountsPrefixNames: "namespace1:name1-x*,namespace1:name2-x,namespace2:name1-x*"
  • Namespace name prefix with fixed service account name
    Watch for service account names prefix with name-x in namespace1 
    allowedServiceAccountsPrefixNames: "vc-team-*:name"
  • Namespace name prefix and Service Account name prefix 
    allowedServiceAccountsPrefixNames: "vc-team-*:team-*"

Issue reference

Please reference the issue this PR will close: #5906

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

@filintod filintod changed the title controller scaffold dynamic allowed service accounts watcher for injector Feb 7, 2023
@artursouza artursouza self-assigned this Feb 7, 2023
@artursouza artursouza added this to the v1.11 milestone Feb 7, 2023
@filintod filintod force-pushed the helpers-dynamic-service-account branch 5 times, most recently from 4f90659 to 1e8fe3d Compare February 8, 2023 05:07
@filintod filintod marked this pull request as ready for review February 8, 2023 05:26
@filintod filintod requested review from a team as code owners February 8, 2023 05:26
yaron2
yaron2 previously requested changes Feb 8, 2023
View reviewed changes
cmd/injector/main.go Outdated Show resolved Hide resolved
@yaron2
Copy link
Member

yaron2 commented Feb 8, 2023

From the linked issue:

This will open the possibility of using dapr with vcluster.

I want to make it clear what scenario we're enabling here, as Dapr can in fact run by default inside of VClusters.
Does this enable injecting VCluster data plane sidecars from a control plane that sits outside of a VCluster?

@salaboy
Copy link

salaboy commented Feb 8, 2023

@yaron2 yes this enables VCluster Clusters (Vcluster 0.14.0) to consume a single Dapr installation that is in the Host cluster.
Each VCluster created will create a new service account, which needs to be added to the allowed list manually.
This PR by @filintod is trying to a way to bypass that manual intervention by using a convention and wildcards

@salaboy
Copy link

salaboy commented Feb 8, 2023

@yaron2 I wonder.. can we make this into 1.10? or is it too late?

@filintod filintod force-pushed the helpers-dynamic-service-account branch from 53b798c to 958723f Compare February 8, 2023 19:20
@filintod filintod requested review from yaron2 and removed request for yaron2 February 8, 2023 19:27
@filintod
Copy link
Contributor Author

filintod commented Feb 8, 2023 •
edited

looks like there was a recent commit on 1.10 branch where name:namespace was swap to the more k8s common namespace:name. I guess I should do the same. So expect another change

@yaron2
Copy link
Member

yaron2 commented Feb 8, 2023

@yaron2 I wonder.. can we make this into 1.10? or is it too late?

This will need to go in 1.11.

ItalyPaleAle
ItalyPaleAle requested changes Feb 9, 2023
View reviewed changes
charts/dapr/README.md Outdated Show resolved Hide resolved
cmd/injector/main.go Outdated Show resolved Hide resolved
pkg/injector/allowedsawatcher/controller.go Outdated Show resolved Hide resolved
pkg/injector/allowedsawatcher/controller_test.go Outdated Show resolved Hide resolved
pkg/injector/allowedsawatcher/controller_test.go Outdated Show resolved Hide resolved
pkg/injector/allowedsawatcher/predutil.go Outdated Show resolved Hide resolved
pkg/injector/allowedsawatcher/predutil_test.go Outdated Show resolved Hide resolved
pkg/injector/interfaces/interfaces.go Outdated Show resolved Hide resolved
utils/utils.go Outdated Show resolved Hide resolved
pkg/injector/injector.go Outdated Show resolved Hide resolved
@filintod filintod force-pushed the helpers-dynamic-service-account branch from 8d916e7 to d361ed1 Compare February 10, 2023 02:49
mukundansundar
mukundansundar reviewed Feb 10, 2023
View reviewed changes
pkg/injector/injector.go Outdated Show resolved Hide resolved
pkg/injector/injector.go Outdated Show resolved Hide resolved
pkg/injector/injector_test.go Outdated Show resolved Hide resolved
pkg/injector/namespacednamematcher/namenamespacematcher.go Outdated Show resolved Hide resolved
pkg/injector/namespacednamematcher/namenamespacematcher.go Show resolved Hide resolved
@filintod filintod requested review from ItalyPaleAle and yaron2 and removed request for yaron2 and ItalyPaleAle February 10, 2023 20:23
filintod and others added 16 commits February 21, 2023 08:13
@filintod
add watch verb to injector rbac clusterrole
09a5c3a 
Signed-off-by: Filinto Duran <filinto@diagrid.io>
@filintod
fix double kubeconfig flag set, fix bug in adding uid, add test, refa…
3a540ec 
…ctors

Signed-off-by: Filinto Duran <filinto@diagrid.io>
@filintod
final refactor hopefully, swap format name:namespace to upcoming and …
4e6c56e 
…more k8s frienly namespace:name

Signed-off-by: Filinto Duran <filinto@diagrid.io>
@filintod
fix flake in test slice sort for map to slice
f1dd923 
add comment about order guarantees just in case we forget in the future

Signed-off-by: Filinto Duran <filinto@diagrid.io>
@filintod
simplify code
208f724 
Signed-off-by: Filinto Duran <filinto@diagrid.io>
@filintod
revert using ctrl runtime getconfig
c6af0a7 
Signed-off-by: Filinto Duran <filinto@diagrid.io>
@filintod
refactors
f370999 
Signed-off-by: Filinto Duran <filinto@diagrid.io>
@filintod @mukundansundar
Update pkg/injector/namespacednamematcher/namenamespacematcher.go
d2df12f 
from feedback

Co-authored-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Signed-off-by: Filinto Duran <duranto@gmail.com>
@filintod @mukundansundar
Update pkg/injector/injector.go
e11e379 
Co-authored-by: Mukundan Sundararajan <65565396+mukundansundar@users.noreply.github.com>
Signed-off-by: Filinto Duran <duranto@gmail.com>
@filintod
incorporate feedback, add tests
caa9bca 
Signed-off-by: Filinto Duran <filinto@diagrid.io>
@filintod
add test, fix bug found on test for any sa prefix
16e293a 
Signed-off-by: Filinto Duran <filinto@diagrid.io>
@filintod
simplify allow any namespace
3ad38a7 
Signed-off-by: Filinto Duran <filinto@diagrid.io>
@filintod
incorporate feedback from italypaleale
4bcbcf3 
Signed-off-by: Filinto Duran <filinto@diagrid.io>
@filintod
improve readability of message from feedback
1518312 
Signed-off-by: Filinto Duran <filinto@diagrid.io>
@filintod
improve readability of message from feedback
4e0782d 
Signed-off-by: Filinto Duran <filinto@diagrid.io>
@filintod
fix bug in test added after rebase
e7a903e 
Signed-off-by: Filinto Duran <filinto@diagrid.io>
@filintod filintod force-pushed the helpers-dynamic-service-account branch from 1539ff0 to e7a903e Compare February 21, 2023 14:14
@ItalyPaleAle
Copy link
Contributor

@filintod Please don't rebase your PRs as that breaks our ability to use things like "see changes since your last review", and we need to review the entire PR from scratch every time!

ItalyPaleAle
ItalyPaleAle previously approved these changes Mar 3, 2023
View reviewed changes
@ItalyPaleAle
Merge branch 'master' of https://github.com/dapr/dapr into helpers-dy…
a82ab3d 
…namic-service-account

Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
@ItalyPaleAle ItalyPaleAle requested a review from yaron2 March 3, 2023 16:55
@ItalyPaleAle ItalyPaleAle dismissed yaron2’s stale review March 3, 2023 16:55

Feedback addressed - please re-review

@artursouza artursouza merged commit 8609d5a into dapr:master Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@salaboy salaboy salaboy left review comments

@mukundansundar mukundansundar mukundansundar left review comments

@ItalyPaleAle ItalyPaleAle ItalyPaleAle approved these changes

@yaron2 yaron2 Awaiting requested review from yaron2

Assignees

@artursouza artursouza

Labels
None yet
Projects
None yet
Milestone
v1.11
6 participants
@filintod @yaron2 @salaboy @ItalyPaleAle @mukundansundar @artursouza