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

BUILD-353: name changes, 2 CRDs instead of one for secrets and configmaps #55

Merged

Conversation

gabemontero
Copy link
Contributor

in repro API should be drag and drop back to openshift/api

the 2 CRDs resulted in removing support for switching between configmaps and secrets for a "share" (a good thing IMO in hindsight)

/assign @coreydaley
/assign @adambkaplan

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 27, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabemontero

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

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

we have green e2e's on the latest rename @coreydaley @adambkaplan

@gabemontero gabemontero changed the title name changes, 2 CRDs instead of one for secrets and configmaps BUILD-353: name changes, 2 CRDs instead of one for secrets and configmaps Sep 28, 2021
@@ -42,6 +42,7 @@ provided by this CSI driver.
Allowing the disabling processing of updates, or switching the default for the system as not dealing with
updates, but then allowing for opting into updates, is also under consideration.

//TODO do we punt on this with the separate CRDs now
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep thanks

docs/csi.md Outdated
@@ -3,13 +3,13 @@
So let's take each part of the [CSIVolumeSource](https://github.com/kubernetes/api/blob/71efbb18d63cd30604981514ac623a6be1d413bb/core/v1/types.go#L1743-L1771):

- for the `Driver` string field, it needs to be ["csi-driver-projected-resource.openshift.io"](https://github.com/openshift/csi-driver-shared-resource/blob/1fcc354faa31f624086265ea2228661a0fc2e7b1/pkg/client/client.go#L28).
- for the `VolumeAttributes` map, this driver currently adds the "share" key (which maps the the `SharedResource` instance your `Pod` wants to use) in addition to the
- for the `VolumeAttributes` map, this driver currently adds the "share" key (which maps the the `SharedConfigMap` OR `SharedSecret` instance your `Pod` wants to use) in addition to the
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
- for the `VolumeAttributes` map, this driver currently adds the "share" key (which maps the the `SharedConfigMap` OR `SharedSecret` instance your `Pod` wants to use) in addition to the
- for the `VolumeAttributes` map, this driver currently adds the "share" key (which maps the `SharedConfigMap` OR `SharedSecret` instance your `Pod` wants to use) in addition to the

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah actually the key name is not correct as well

it is sharedConfigMap or sharedSecret

I'll make that changes as well here

docs/install.md Outdated
oc apply -f ./deploy/0000_10_sharedconfigmap.crd.yaml
customresourcedefinition.apiextensions.k8s.io/sharedconfigmaps.sharedresource.openshift.io created
deploying hostpath components
./deploy/0000_10_sharedsecrets.crd.yaml
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
./deploy/0000_10_sharedsecrets.crd.yaml
./deploy/0000_10_sharedsecret.crd.yaml

Comment on lines +117 to +124
oc apply -f --filename https://raw.githubusercontent.com/openshift/csi-driver-shared-resource/release-4.10/deploy/00-namespace.yaml
oc apply -f --filename https://raw.githubusercontent.com/openshift/csi-driver-shared-resource/release-4.10/deploy/0000_10_sharedconfigmap.crd.yaml
oc apply -f --filename https://raw.githubusercontent.com/openshift/csi-driver-shared-resource/release-4.10/deploy/0000_10_sharedsecret.crd.yaml
oc apply -f --filename https://raw.githubusercontent.com/openshift/csi-driver-shared-resource/release-4.10/deploy/01-service-account.yaml
oc apply -f --filename https://raw.githubusercontent.com/openshift/csi-driver-shared-resource/release-4.10/deploy/02-cluster-role.yaml
oc apply -f --filename https://raw.githubusercontent.com/openshift/csi-driver-shared-resource/release-4.10/deploy/03-cluster-role-binding.yaml
oc apply -f --filename https://raw.githubusercontent.com/openshift/csi-driver-shared-resource/release-4.10/deploy/csi-hostpath-driverinfo.yaml
oc apply -f --filename https://raw.githubusercontent.com/openshift/csi-driver-shared-resource/release-4.10/deploy/csi-hostpath-plugin.yaml
Copy link
Member

Choose a reason for hiding this comment

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

Should we replace release-4.10 here with master instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, as master could be unstable, once a given release goes out.

Copy link
Member

Choose a reason for hiding this comment

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

So we will have to update this readme every release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lesser of the evils IMO ... at least for now ... and if it lags, it is not the worse thing ... and it is easy enough for someone to figure our they can tweak that to another branch

possible this just goes away or morphs into a "dev only" mode at some point, in which case we switch to master

I'd rather hold off on that while we are in this intermediate state

//
// spec:
// volumes:
// - name: shared-secret
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
// - name: shared-secret
// - name: shared-configmap

@@ -0,0 +1,73 @@
package v1alpha1
Copy link
Member

Choose a reason for hiding this comment

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

Should this file be named shared_configmap_type.go ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah and the secret one too

@gabemontero
Copy link
Contributor Author

pushed updates from your comments as separate commits @coreydaley - thanks !

@@ -79,7 +78,9 @@ func init() {

rootCmd.Flags().AddGoFlagSet(flag.CommandLine)
rootCmd.Flags().StringVar(&endPoint, "endpoint", "unix://tmp/csi.sock", "CSI endpoint")
rootCmd.Flags().StringVar(&driverName, "drivername", string(v1.SharedResourcesCSIDriver), "name of the driver")
//rootCmd.Flags().StringVar(&driverName, "drivername", string(v1.SharedResourcesCSIDriver), "name of the driver")
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this commented out line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to leave it as per the TODO comment below we'll swtich back to it once you've updated that driver name with your openshift/api PR

@@ -97,7 +92,7 @@ $ kubectl apply -f ./examples
namespace/my-csi-app-namespace created
role.rbac.authorization.k8s.io/shared-resource-my-share created
rolebinding.rbac.authorization.k8s.io/shared-resource-my-share created
share.sharedresource.storage.openshift.io/my-share created
sharedconfig.sharedresource.openshift.io/my-share created
Copy link
Member

Choose a reason for hiding this comment

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

Should this be sharedconfigmap ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep

docs/faq.md Outdated

You'll see an event like:

```bash
$ oc get events
0s Warning FailedMount pod/my-csi-app MountVolume.SetUp failed for volume "my-csi-volume" : rpc error: code = InvalidArgument desc = the csi driver volumeAttribute 'share' reference had an error: sharedresource.storage.openshift.io "my-share" not found
0s Warning FailedMount pod/my-csi-app MountVolume.SetUp failed for volume "my-csi-volume" : rpc error: code = InvalidArgument desc = the csi driver volumeAttribute 'share' reference had an error: sharedconfig.sharedresource.openshift.io "my-share" not found
Copy link
Member

Choose a reason for hiding this comment

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

Should this be sharedconfigmap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep

@gabemontero
Copy link
Contributor Author

ok @coreydaley squashed the previous review commits, add a new one for the next round of corrections from you - thanks !

@@ -0,0 +1,22 @@
# this is the boilerplate crd def that controller-gen reads and modifies with the
# contents from share_secret_type.go
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
# contents from share_secret_type.go
# contents from shared_secret_type.go

@@ -0,0 +1,104 @@
# this is the boilerplate crd def that controller-gen reads and modifies with the
# contents from share_configmap_type.go
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
# contents from share_configmap_type.go
# contents from shared_configmap_type.go

@@ -1,27 +1,27 @@
# this is the boilerplate crd def that controller-gen reads and modifies with the
# contents from share_type.go
# contents from share_secret_type.go
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
# contents from share_secret_type.go
# contents from shared_secret_type.go

// +genclient:nonNamespaced
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

// SharedSecret allows a Secret o be shared across namespaces.
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
// SharedSecret allows a Secret o be shared across namespaces.
// SharedSecret allows a Secret to be shared across namespaces.

// sharedConfigMap: my-share
//
// For the mount to be successful, the pod's service account must be granted permission to 'use' the named SharedConfigMap object
// within its namespace with an appropriate Role and RoleBinding. For compactness, here are example `oc` invocations for creating
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
// within its namespace with an appropriate Role and RoleBinding. For compactness, here are example `oc` invocations for creating
// within its namespace with an appropriate Role and RoleBinding. For compactness, here are example `oc` invocations for creating

@@ -44,10 +44,10 @@ var (
// a given secret; when possible we range over this list vs. secrets
secretsWithShare = sync.Map{}
// sharesWaitingOnSecrets conversely is for when a share has been created that references a secret, but that
// secret has not been recognized by the controller; quite possibly timing events on when we learn of shares
// secret has not been recognized by the controller; quite possibly timing events on when we learn of shareConfigMaps
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
// secret has not been recognized by the controller; quite possibly timing events on when we learn of shareConfigMaps
// secret has not been recognized by the controller; quite possibly timing events on when we learn of sharedSecrets

secretKey := BuildKey(br)
secretsWithShare.Store(secretKey, secret)
//NOTE: share update ranger will store share in shares sync.Map
//NOTE: share update ranger will store share in shareConfigMaps sync.Map
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
//NOTE: share update ranger will store share in shareConfigMaps sync.Map
//NOTE: share update ranger will store share in shareSecrets sync.Map

// and we are supplying only this specific share to the csi driver update range callbacks.
shareUpdateCallbacks.Range(buildRanger(buildCallbackMap(share.Name, share)))
shareConfigMapsUpdateCallbacks.Range(buildRanger(buildCallbackMap(share.Name, share)))
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
shareConfigMapsUpdateCallbacks.Range(buildRanger(buildCallbackMap(share.Name, share)))
shareSecretsUpdateCallbacks.Range(buildRanger(buildCallbackMap(share.Name, share)))

@@ -117,7 +117,7 @@ func DelSecret(secret *corev1.Secret) {
// storage
func RegisterSecretUpsertCallback(volID string, f func(key, value interface{}) bool) {
secretUpsertCallbacks.Store(volID, f)
// cycle through the secrets with shares, where if the share associated with the volID CSI volume mount references
// cycle through the secrets with shareConfigMaps, where if the share associated with the volID CSI volume mount references
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
// cycle through the secrets with shareConfigMaps, where if the share associated with the volID CSI volume mount references
// cycle through the secrets with sharedSecrets, where if the share associated with the volID CSI volume mount references

func AddSharedSecret(share *sharev1alpha1.SharedSecret) {
br := share.Spec.Secret
key := BuildKey(br)
klog.V(4).Infof("AddSharedConfigMap key %s", key)
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
klog.V(4).Infof("AddSharedConfigMap key %s", key)
klog.V(4).Infof("AddSharedSecret key %s", key)


c.informerFactory.Start(stopCh)
c.shareInformerFactory.Start(stopCh)
c.sharedConfigMapInformerFactory.Start(stopCh)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I missed it, but I don't see where you are starting the sharedSecretInformerFactory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep I missed it - thanks

event := client.Event{
Object: s,
Verb: verb,
}
c.shareWorkqueue.Add(event)
c.sharedConfigMapWorkqueue.Add(event)
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
c.sharedConfigMapWorkqueue.Add(event)
c.sharedSecretWorkqueue.Add(event)

for {
obj, shutdown := c.shareWorkqueue.Get()
obj, shutdown := c.sharedConfigMapWorkqueue.Get()
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
obj, shutdown := c.sharedConfigMapWorkqueue.Get()
obj, shutdown := c.sharedSecretMapWorkqueue.Get()

if shutdown {
return
}

func() {
defer c.shareWorkqueue.Done(obj)
defer c.sharedConfigMapWorkqueue.Done(obj)
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
defer c.sharedConfigMapWorkqueue.Done(obj)
defer c.sharedSecretWorkqueue.Done(obj)


event, ok := obj.(client.Event)
if !ok {
c.shareWorkqueue.Forget(obj)
c.sharedConfigMapWorkqueue.Forget(obj)
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
c.sharedConfigMapWorkqueue.Forget(obj)
c.sharedSecretWorkqueue.Forget(obj)

sShare, err = client.GetListers().SharedSecrets.Get(secretShareName)
if err != nil {
return nil, nil, status.Errorf(codes.InvalidArgument,
"the csi driver volumeAttribute '%s' reference had an error: %s", secretShareName, err.Error())
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
"the csi driver volumeAttribute '%s' reference had an error: %s", secretShareName, err.Error())
"the csi driver volumeAttribute %q reference had an error: %s", secretShareName, err.Error())

if cmShare != nil {
if len(strings.TrimSpace(cmShare.Spec.ConfigMap.Namespace)) == 0 {
return nil, nil, status.Errorf(codes.InvalidArgument,
"the SharedConfigMap %s backing resource namespace needs to be set", configMapShareName)
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
"the SharedConfigMap %s backing resource namespace needs to be set", configMapShareName)
"the SharedConfigMap %q backing resource namespace needs to be set", configMapShareName)

}
if len(strings.TrimSpace(cmShare.Spec.ConfigMap.Name)) == 0 {
return nil, nil, status.Errorf(codes.InvalidArgument,
"the SharedConfigMap %s backing resource name needs to be set", configMapShareName)
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
"the SharedConfigMap %s backing resource name needs to be set", configMapShareName)
"the SharedConfigMap %q backing resource name needs to be set", configMapShareName)

kind = sharev1alpha1.ResourceReferenceTypeSecret
if len(strings.TrimSpace(sShare.Spec.Secret.Namespace)) == 0 {
return nil, nil, status.Errorf(codes.InvalidArgument,
"the SharedSecret %s backing resource namespace needs to be set", secretShareName)
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
"the SharedSecret %s backing resource namespace needs to be set", secretShareName)
"the SharedSecret %q backing resource namespace needs to be set", secretShareName)

}
if len(strings.TrimSpace(sShare.Spec.Secret.Name)) == 0 {
return nil, nil, status.Errorf(codes.InvalidArgument,
"the SharedSecret %s backing resource name needs to be set", secretShareName)
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
"the SharedSecret %s backing resource name needs to be set", secretShareName)
"the SharedSecret %q backing resource name needs to be set", secretShareName)

@gabemontero gabemontero force-pushed the chg-type-names-yet-again branch 2 times, most recently from 8ff140a to a8d44b3 Compare September 29, 2021 15:46
@gabemontero
Copy link
Contributor Author

ok @coreydaley I believe I have addressed the latest set of comments in 2 "core-3" commits (where I squashed the previous review commits

unit tests pass for me locally but I'm still waiting on my cluster to come up to try the e2e's

I'll run those locally when the cluster comes up if I see any failures here in the PR

@sbose78
Copy link

sbose78 commented Sep 29, 2021

Should we update the corresponding enhancement proposal with this change ?

@gabemontero
Copy link
Contributor Author

gabemontero commented Sep 29, 2021

Should we update the corresponding enhancement proposal with this change ?

yes I'm including that as one of the to-do's for BUILD-353

just getting the code ready first

@gabemontero
Copy link
Contributor Author

aws error with slow

/test e2e-aws-csi-driver-slow

@gabemontero
Copy link
Contributor Author

ok the e2e's are passing for me locally ... would anticipate the slow e2e passing if it gets past unrelated flakes

@gabemontero
Copy link
Contributor Author

and the e2e's are green as well @coreydaley

assuming there is nothing else, and @adambkaplan deferral to us in scrum/office hours today, I'll squash and then we can lgtm / merge

And you can start mapping the api changes back to openshift/api#979

Copy link
Member

@coreydaley coreydaley left a comment

Choose a reason for hiding this comment

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

I think these are the last two issues.
If you want to squash your commits and then I think we can get this merged and I can proceed with the openshift/api stuff.

README.md Outdated
specified in a `Pod` that uses this repository's CSI Driver.
- separately, any `User` can discover cluster scoped `SharedResources` based on the permissions granted to them by their cluster
or namespace administrator.

The full definition of the `SharedResource` custom resource can be found [here](deploy/0000_10_sharedresource.crd.yaml).
The full definition of the `SharedConfigMap` can be found [here](deploy/0000_10_sharedconfigmap.crd.yaml)or `SharedSecret` custom resource can be found [here](deploy/0000_10_sharedsecret.crd.yaml).
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
The full definition of the `SharedConfigMap` can be found [here](deploy/0000_10_sharedconfigmap.crd.yaml)or `SharedSecret` custom resource can be found [here](deploy/0000_10_sharedsecret.crd.yaml).
The full definition of the `SharedConfigMap` can be found [here](deploy/0000_10_sharedconfigmap.crd.yaml) or `SharedSecret` custom resource can be found [here](deploy/0000_10_sharedsecret.crd.yaml).


c.informerFactory.Start(stopCh)
c.shareInformerFactory.Start(stopCh)
c.sharedConfigMapInformerFactory.Start(stopCh)
c.sharedConfigMapInformerFactory.Start(stopCh)
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
c.sharedConfigMapInformerFactory.Start(stopCh)
c.sharedSecretInformerFactory.Start(stopCh)

doc updates
update simple example
controller updates
driver rename
add SharedSecret and SharedConfigMap
generate-crd
e2e tests, including replacing any configmap to secret change tests
@gabemontero
Copy link
Contributor Author

corrections pulled in and pushed, commits squashed @coreydaley thanks !!

@coreydaley
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 1, 2021
@openshift-merge-robot openshift-merge-robot merged commit b2fe5d2 into openshift:master Oct 1, 2021
@gabemontero gabemontero deleted the chg-type-names-yet-again branch October 1, 2021 21:18
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.

None yet

5 participants