Skip to content

BUILD-348: share discoverability ('use' verb, namespace scope roles); inject build-284 in repo#51

Merged
openshift-merge-robot merged 3 commits intoopenshift:masterfrom
gabemontero:chg-sar-verb
Sep 22, 2021
Merged

BUILD-348: share discoverability ('use' verb, namespace scope roles); inject build-284 in repo#51
openshift-merge-robot merged 3 commits intoopenshift:masterfrom
gabemontero:chg-sar-verb

Conversation

@gabemontero
Copy link
Copy Markdown
Contributor

/assign @adambkaplan
/assign @coreydaley
@deads2k FYI (if anything else, for api godoc)

So this PR proves out our recent discussions in slack and @coreydaley 's openshift/api#979

Highlights:

  • switches to only a 'use' verb when seeing if the Pod SA can use / mount a SharedResource
  • I replaced use of ClusterRole/ClusterRoleBinding in tests/examples to vet out the discussion point around allowing namespace admins to grant permissions to SA's in their namespaces for shares they can discover based on separate roles etc. around getting and listing SharedResources
  • I pulled in @coreydaley 's PR change into this repo's current local API def, including updating the godoc based on the simplification @deads2k noted because of the switch to the 'use' verb , and updated all the code/tests with the changes from that
  • presumably when @coreydaley 's PR merges his openshift/api vendor into this repo should be greatly simplified
  • I've deferred on the naming discussion @adambkaplan is having with Jan on the groupname ... left it as is from @coreydaley 's PR for now

I'm still going to put up a openshift/enhancements update, but I am currently of the opinion this PR progresses us on the path, could be merged after review, and then follow up PRs can come if the EP merge or api merge results in addition adjustments.

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 16, 2021
@gabemontero
Copy link
Copy Markdown
Contributor Author

all green e2e's @adambkaplan @coreydaley

Copy link
Copy Markdown
Contributor

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

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

/approve

Nits, but otherwise looks good

Comment thread README.md Outdated
# OpenShift Shared Resource CSI Driver

The OpenShift Projected Resource CSI Driver allows for the controlled (via Kubernetes RBAC) sharing of Kubernetes Secrets and ConfigMaps across
The OpenShift Sharead Resource CSI Driver allows for the controlled (via Kubernetes RBAC) sharing of Kubernetes Secrets and ConfigMaps across
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

spelling nit:

Suggested change
The OpenShift Sharead Resource CSI Driver allows for the controlled (via Kubernetes RBAC) sharing of Kubernetes Secrets and ConfigMaps across
The OpenShift Shared Resource CSI Driver allows for the controlled (via Kubernetes RBAC) sharing of Kubernetes Secrets and ConfigMaps across

Comment thread docs/simple-example.md Outdated
- storage.openshift.io
resources:
- shares
- sharedresourcess
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

important nit:

Suggested change
- sharedresourcess
- sharedresources

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yep ... fortunately it is correct in the actual yaml files in the examples subdir ;-)

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Sep 21, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, gabemontero

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

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [adambkaplan,gabemontero]

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

// volumeAttributes:
// sharedResource: my-share
//
// For the mount to be successful, the pod's service account must be granted permission to 'use' the named SharedResource object
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

so starting here @coreydaley was my attempt to take what @deads2k and I discussed in slack in reference to your openshift/api PR and apply "corrections" in the godoc

// `oc create rolebinding shared-resource-my-share --role=shared-resource-my-share --serviceaccount=my-namespace:default`
//
// Administrators can create separate Roles and RoleBindings for their users to be able the list and/or view the
// available cluster scoped `SharedResources` objects.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is the end of where I tried to update godoc @coreydaley in reference to discussion with @deads2k in slack

ideally you can replicate this in your openshift/api PR, and then, with the resolution of the group name that @adambkaplan will be striving for with @jsafrane and @deads2k your openshift/api PR will be "unblocked", or at least at the point where we are just iterating on grammar and word choice vs. major concepts and architectural viability

@gabemontero
Copy link
Copy Markdown
Contributor Author

spelling nits updated and pushed @adambkaplan thanks

@coreydaley tried to highlight the godoc updates that I hope should help your openshift/api PR

pending further feedback from you @coreydaley I think this one is good to go

@gabemontero
Copy link
Copy Markdown
Contributor Author

getting 500s from CIs registry on startup of all jobs

will sit tight and retry tests later

@gabemontero
Copy link
Copy Markdown
Contributor Author

/retest

@coreydaley
Copy link
Copy Markdown

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2021
@openshift-merge-robot openshift-merge-robot merged commit a284a86 into openshift:master Sep 22, 2021
@gabemontero gabemontero deleted the chg-sar-verb branch September 22, 2021 19:40
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. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants