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

📖 Document OLMv1 permission model #1380

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

rashmi43
Copy link
Contributor

@rashmi43 rashmi43 commented Oct 16, 2024

Description

Document OLMv1 permission model
aims to clarify #1376

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@rashmi43 rashmi43 requested a review from a team as a code owner October 16, 2024 06:24
Copy link

netlify bot commented Oct 16, 2024

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 65aece4
🔍 Latest deploy log https://app.netlify.com/sites/olmv1/deploys/6716637ca07ea40008303aab
😎 Deploy Preview https://deploy-preview-1380--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.12%. Comparing base (b7674d8) to head (65aece4).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1380      +/-   ##
==========================================
- Coverage   74.84%   73.12%   -1.73%     
==========================================
  Files          42       42              
  Lines        2516     3166     +650     
==========================================
+ Hits         1883     2315     +432     
- Misses        449      663     +214     
- Partials      184      188       +4     
Flag Coverage Δ
e2e 54.64% <ø> (-2.12%) ⬇️
unit 52.52% <ø> (-0.18%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: rashmi_kh <rashmi.khanna@in.ibm.com>
Signed-off-by: rashmi_kh <rashmi.khanna@in.ibm.com>
@@ -0,0 +1,13 @@
#### OLMv1 Permission Model

Here we aim to describe the OLMv1 permission model. OLMv1 itself does not have permission to manage the installation and lifecycle of cluster extensions. Rather, it requires that each cluster extension specifies a service account that will be used to manage its bundle contents.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Here we aim to describe the OLMv1 permission model. OLMv1 itself does not have permission to manage the installation and lifecycle of cluster extensions. Rather, it requires that each cluster extension specifies a service account that will be used to manage its bundle contents.
Here we aim to describe the OLMv1 permission model. OLMv1 itself does not have permission to manage the installation and lifecycle of cluster extensions. Rather, it requires that each cluster extension specify a service account that will be used to manage its bundle contents.

Here we aim to describe the OLMv1 permission model. OLMv1 itself does not have permission to manage the installation and lifecycle of cluster extensions. Rather, it requires that each cluster extension specifies a service account that will be used to manage its bundle contents.


1) The purpose of the service account specified in the ClusterExtension spec, which is to manage everything in (2) below.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the structure of this section is a little bit unclear. Maybe we could call attention to the fact that the installer service account and the service account for the cluster extension's Deployment have different purposes: the first manages the lifecycle of the cluster extensions - so it needs to be able to create/modify the resources packed in the bundle, and assign RBAC to the extension's Deployment SA. And, the second, gives the extension controller the RBAC it needs to do its business.

I think most of that information is already here, we should just restructure it a bit to make clear this distinction and the roles of each of the SAs

docs/concepts/permission-model.md Outdated Show resolved Hide resolved
docs/concepts/permission-model.md Outdated Show resolved Hide resolved
Signed-off-by: rashmi_kh <rashmi.khanna@in.ibm.com>

To understand the permission model, lets see the scope of the the service accounts associated with and part of the ClusterExtension deployment:

1) The ClusterExtension CR defines a service account to deploy and manage the ClusterExtension lifecycle and can be derived using the [document](../howto/dervice-service-account.md). It is specified in the ClusterExtension [yaml](../tutorials/install-extension#L71) while deploying a ClusterExtension.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1) The ClusterExtension CR defines a service account to deploy and manage the ClusterExtension lifecycle and can be derived using the [document](../howto/dervice-service-account.md). It is specified in the ClusterExtension [yaml](../tutorials/install-extension#L71) while deploying a ClusterExtension.
1) The ClusterExtension CR defines a service account to deploy and manage the ClusterExtension lifecycle and can be derived using the [document](../howto/derive-service-account.md). It is specified in the ClusterExtension [yaml](../tutorials/install-extension#L71) while deploying a ClusterExtension.

To understand the permission model, lets see the scope of the the service accounts associated with and part of the ClusterExtension deployment:

1) The ClusterExtension CR defines a service account to deploy and manage the ClusterExtension lifecycle and can be derived using the [document](../howto/dervice-service-account.md). It is specified in the ClusterExtension [yaml](../tutorials/install-extension#L71) while deploying a ClusterExtension.
2) The purpose of the service account specified in the ClusterExtension spec is to manage the cluster extension lifecycle. Its permissions is the cumulative of permissions required for managing the cluster extension lifecycle and the RBAC for the operator bundle.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
2) The purpose of the service account specified in the ClusterExtension spec is to manage the cluster extension lifecycle. Its permissions is the cumulative of permissions required for managing the cluster extension lifecycle and the RBAC for the operator bundle.
2) The purpose of the service account specified in the ClusterExtension spec is to manage the cluster extension lifecycle. Its permissions is the cumulative of permissions required for managing the cluster extension lifecycle and any RBAC that may be included in the extension bundle.


1) The ClusterExtension CR defines a service account to deploy and manage the ClusterExtension lifecycle and can be derived using the [document](../howto/dervice-service-account.md). It is specified in the ClusterExtension [yaml](../tutorials/install-extension#L71) while deploying a ClusterExtension.
2) The purpose of the service account specified in the ClusterExtension spec is to manage the cluster extension lifecycle. Its permissions is the cumulative of permissions required for managing the cluster extension lifecycle and the RBAC for the operator bundle.
3) The contents of the operator bundle may contain more service accounts and RBAC. Since the operator bundle contains its own RBAC, it means the ClusterExtension service account requires either:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
3) The contents of the operator bundle may contain more service accounts and RBAC. Since the operator bundle contains its own RBAC, it means the ClusterExtension service account requires either:
3) The contents of the extension bundle may contain more service accounts and RBAC. Since the extension bundle contains its own RBAC, it means the ClusterExtension service account requires either:

3) The contents of the operator bundle may contain more service accounts and RBAC. Since the operator bundle contains its own RBAC, it means the ClusterExtension service account requires either:
- the same set of permissions that are defined in the RBAC that it is trying to create.
- bind/escalate verbs for RBAC, see https://kubernetes.io/docs/reference/access-authn-authz/rbac/#privilege-escalation-prevention-and-bootstrapping
4) The OLMv1 operator-controller generates a service account with the required RBAC for the extension controller based on the contents of the ClusterServiceVersion in much the same way that OLMv0 does. In the ArgoCD example, the [controller service account](https://github.com/argoproj-labs/argocd-operator/blob/da6b8a7e68f71920de9545152714b9066990fc4b/deploy/olm-catalog/argocd-operator/0.6.0/argocd-operator.v0.6.0.clusterserviceversion.yaml#L1124) permissions allow the operator to manage and run the controller logic.
Copy link
Member

Choose a reason for hiding this comment

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

I'm on the fence about how important it is to include bullet (4). It seems that at a conceptual level, but bullet could be left out and the document would stand on its own, such that it describes the concept without delving into implementation specifics.

On the otherhand, concrete examples can be useful to illustrate a concept. Two alternative suggestions:

  1. Re-write this to be more of an example rather than another bullet point.
  2. Going along with (1), is there a place in the docs that we talk about registry+v1 specifics? If so, maybe we insert some details there about how OLMv1 generates service accounts and RBAC? Then we could link there from there.

Maybe we drop bullet (4) for now, and do an example in a follow-up?


Note: The ClusterExtension permissions are not propogated to the deployment. The ClusterExtension service account and the bundle's service accounts have different purposes and naming conflicts between the two service accounts can lead to failure of ClusterExtension deployment.

The ClusterExtension permissions needs to be manually derived based on the details listed above and specified when deploying a ClsuterExtension.
Copy link
Member

Choose a reason for hiding this comment

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

Rather than stating that RBAC needs to be manually derived, I think I would drop this sentence. We already link to the "derive-service-account" doc above. In the future, we'll make this UX better, and I think it will be easier to catch the necessary doc change if we keep the details about deriving a service account in a single place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants