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

Control associations across namespaces with ServiceAccount and RBAC #2482

Merged
merged 15 commits into from
Feb 3, 2020

Conversation

barkbay
Copy link
Contributor

@barkbay barkbay commented Jan 29, 2020

Fix #2468

TODO:

cmd/manager/main.go Outdated Show resolved Hide resolved
@@ -6,6 +6,12 @@ kind: ClusterRole
metadata:
name: elastic-operator
rules:
- apiGroups:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only the cluster role of the all-in-one manifest is updated since this feature only makes sense when the operator is watching more than one namespace.

cmd/manager/main.go Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
pkg/apis/apm/v1/apmserver_types.go Show resolved Hide resolved
pkg/utils/rbac/access_review.go Outdated Show resolved Hide resolved
pkg/utils/rbac/access_review.go Outdated Show resolved Hide resolved
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
name: elasticsearch-association
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we should pre-create this role as part of the operator manifest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My gut feeling is that we should create roles only if there are needed but add it as an example in our documentation if the user wants to enable access control. That being said I'm happy to pre-create it if you think it's worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 let's not include it for now and maybe do it later depending on users feedback.

config/samples/associations-rbac/apm_es_kibana_rbac.yaml Outdated Show resolved Hide resolved
config/samples/associations-rbac/apm_es_kibana_rbac.yaml Outdated Show resolved Hide resolved
config/samples/associations-rbac/apm_es_kibana_rbac.yaml Outdated Show resolved Hide resolved
@sebgl
Copy link
Contributor

sebgl commented Jan 30, 2020

It would be nice to have this verified in an E2E test, but it does not really match the current E2E test framework :/
Let's make sure to at least create an issue for it?

@barkbay barkbay marked this pull request as ready for review January 31, 2020 10:37
@barkbay
Copy link
Contributor Author

barkbay commented Jan 31, 2020

Let's make sure to at least create an issue for it?

Agreed, I'll create a follow-up issue to track the e2e test effort for this feature.

Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

Sorry for the drive-by comments, I think this looks good!

pkg/apis/apm/v1/apmserver_types.go Outdated Show resolved Hide resolved
pkg/controller/common/association/rbac.go Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
@barkbay barkbay added v1.1.0 >enhancement Enhancement of existing functionality labels Feb 3, 2020
pkg/controller/common/association/rbac.go Outdated Show resolved Hide resolved
pkg/controller/common/association/rbac.go Outdated Show resolved Hide resolved
)

type Unbinder interface {
Unbind(associated commonv1.Associated) error
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this 👍

@sebgl
Copy link
Contributor

sebgl commented Feb 3, 2020

We should have a dedicated page in the documentation (in this PR or in a follow-up one).

Copy link
Contributor

@sebgl sebgl left a comment

Choose a reason for hiding this comment

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

LGTM once my comments above are considered, great work 👍

I did a test locally, everything seems to work as expected:

  • start the operator without the rbac flag, can establish associations
  • start it with the flag, removes the existing associations
  • setup the correct RBAC, association established
  • set a wrong serviceAccountName, association removed
  • set back a correct serviceAccountName, association established
  • set no serviceAccountName (default), association removed

@barkbay barkbay requested a review from pebrc February 3, 2020 13:36
Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

LGTM. I think this turned out really nice. Curious to see what users make of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement Enhancement of existing functionality v1.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Access control on referenced objects
3 participants