Shared Filesystems: Grant Access by pospispa · Pull Request #346 · gophercloud/gophercloud

pospispa · 2017-05-22T15:02:43Z

Links to the line numbers/files in the OpenStack source code that support the
code in this PR:

https://github.com/openstack/manila/blob/stable/newton/manila/api/v1/shares.py#L393

coveralls · 2017-05-22T15:05:59Z

Coverage decreased (-0.4%) to 69.625% when pulling b83719a on pospispa:grant-access-to-a-share into 5a6e133 on gophercloud:master.

coveralls · 2017-06-05T15:15:19Z

Coverage decreased (-0.4%) to 70.118% when pulling f6c9e98 on pospispa:grant-access-to-a-share into 5acaa3c on gophercloud:master.

coveralls · 2017-06-07T10:33:34Z

Coverage decreased (-0.4%) to 70.11% when pulling e880c52 on pospispa:grant-access-to-a-share into caf34a6 on gophercloud:master.

coveralls · 2017-06-07T11:19:29Z

Coverage decreased (-0.4%) to 70.077% when pulling cf83315 on pospispa:grant-access-to-a-share into caf34a6 on gophercloud:master.

coveralls · 2017-06-08T19:16:23Z

Coverage decreased (-0.04%) to 70.424% when pulling 2212c52 on pospispa:grant-access-to-a-share into caf34a6 on gophercloud:master.

pospispa · 2017-06-09T09:17:29Z

@jtopjian IMHO, this PR is ready for review, PTAL.

coveralls · 2017-06-26T11:50:26Z

Coverage decreased (-0.2%) to 70.858% when pulling 049604d on pospispa:grant-access-to-a-share into 5102b60 on gophercloud:master.

coveralls · 2017-07-20T17:44:22Z

Coverage decreased (-0.2%) to 70.939% when pulling 1b583d8 on pospispa:grant-access-to-a-share into 4af874e on gophercloud:master.

pospispa · 2017-07-25T10:05:47Z

@jtopjian In this PR I started using microversions merged in PR #409
I'll appreciate if you comment on my approach so that we can eventually discuss it.
Thank you in advance.

jrperritt · 2017-07-25T14:59:21Z

@pospispa I appreciate the work you've put into this PR, but something like this can't be merged. You've got several third-party libraries including one of your own. Design changes to Gophercloud are OK sometimes, but they need to be discussed separately. As an example, we won't be using Glide, nor the package for parsing versions, nor your openstackmicroversions package. Indeed, without proper justification, we probably won't be adding any third-party libraries. If you can make this PR small like all the others, we can certainly discuss it, but I think 316 files and 80,000+ lines is unactionable.

jtopjian · 2017-07-25T22:33:12Z

I agree with @jrperritt.

With regard to the openstackmicroversions package, it's small enough that the content of it can be moved to either the internal package or the openstack/utils package. I would recommend doing this as a separate PR and include some unit tests.

For discussion, I think we can simplify this PR down to two topics:

Adding the Grant functionality. This is just a standard feature addition and I don't think there's anything to discuss here.
Restricting Grant to only work when a specific microversion has been set. This deserves some discussion since it'll be a first in Gophercloud.

In order to do 2, you need to check if the client has a microversion set and then compare that microversion with the minimum version needed for the service. I think this check can be turned into a function so that GetAccess (and in theory, Create, Delete, etc) would just have something like this:

func GrantAccess(client *gophercloud.ServiceClient, opts GrantAccessOptsBuilder, id string) (r GrantAccessResult) {
    if err := supportedMicroversion(client) {
        r.Err = err
        return
    }

    ...
}

Maybe there's a better way of doing that, though -- that was just the first idea that came to me.

Edit: Alternatively, we can forgo any microversion check and just let the API service return an error if the client was not configured correctly.

pospispa · 2017-07-26T09:19:48Z

@jrperritt I'm pretty new to gophercloud library and to OpenStack. That's why I decided to implement the features I need first and discuss my approach later even though this approach may result in substantial rework.
I'll split this PR into several small ones.
I'll "copy/paste" the openstackmicroversions code into openstack/utils as suggested by @jtopjian so there will be no need for vendoring.

pospispa · 2017-07-26T10:34:12Z

@jtopjian the below:

Edit: Alternatively, we can forgo any microversion check and just let the API service return an error if the client was not configured correctly.
is a good comment. I've not thought about this option.

I tried to set X-OpenStack-Manila-API-Version header to below values and tried how the Manila API responds:

No X-OpenStack-Manila-API-Version header results in error: "Resource not found"
X-OpenStack-Manila-API-Version header set to "2.1" value results in error: "Resource not found"
X-OpenStack-Manila-API-Version header set to "2.8" value results in error: "Resource not found"
X-OpenStack-Manila-API-Version header set to "2.9" value succeeds in granting access rights.

So firstly, in case the microversion is not checked in the gophercloud library we end up with:

more Manila API calls.
currently, a generic error message that does not give a clue what exactly caused the error.

Secondly, in case we check the microversion in the gophercloud library we end up with:

less Manila API calls.
specific error message that clearly says what caused the error.
quite a lot of code that parses and compares the microversions.

I prefer the second option, i.e. to check the microversions in the gophercloud library.

pospispa · 2017-08-11T12:24:55Z

One other area I'm curious about is the timestamps. The existing Share result has a CreatedAt field that has a custom unmarshaller. Would this be required for the CreatedAt and UpdatedAt fields in the GrantAccessResult, too? I would imagine that the timestamp format being returned is the same.

@jtopjian in constrast with the documentation I used wireshark to capture packet with Grant Access response and there're no created_at and updated_at fields.

So I looked again at manila server code and I didn't found the created_at and updated_at fields here neither.

That's why I think these fields aren't being sent so I suggest I'll remove them.

Please, what's your opinion?

pospispa · 2017-08-11T12:38:57Z

@jtopjian I had to rebase this PR.

I believe I've fixed all your comments except a couple of nits and the CreatedAt and UpdatedAt fields.

Please, let me know if you have further comments.

coveralls · 2017-08-11T12:43:19Z

Coverage increased (+0.03%) to 71.524% when pulling e6ed0d7 on pospispa:grant-access-to-a-share into 4ecdf7c on gophercloud:master.

jtopjian · 2017-08-11T13:19:05Z

in constrast with the documentation I used wireshark to capture packet with Grant Access response and there're no created_at and updated_at fields.

You don't need to use wireshark. You can use:

$ manila --debug ...

Please, what's your opinion?

I appreciate you finishing the due diligence required to make sure this PR has all of the correct fields. If can find no evidence of those fields being returned, then they cannot be included here.

By default shares are inaccessible because they do not have set any access permissions. That's why the Grant Access request is added.

pospispa · 2017-08-11T16:28:54Z

I appreciate you finishing the due diligence required to make sure this PR has all of the correct fields. If can find no evidence of those fields being returned, then they cannot be included here.

@jtopjian I didn't find any evidence of those fields being returned so I deleted them.

Please, check if everything looks good to you.

coveralls · 2017-08-11T16:30:26Z

Coverage increased (+0.03%) to 71.524% when pulling 3736803 on pospispa:grant-access-to-a-share into 8a1afad on gophercloud:master.

jrperritt

Some naming changes; English is often unintuitive.

jrperritt · 2017-08-11T16:37:09Z

+
+// GrantAccessRequest will grant access to a Share based on the values in GrantAccessOpts. To extract
+// the GrantAccess object from the response, call the Extract method on the GrantAccessResult.
+func GrantAccessRequest(client *gophercloud.ServiceClient, id string, opts GrantAccessOptsBuilder) (r GrantAccessResult) {


Adding "Request" (or "Req", or "Res") is a non-starter. We'd like the functions to read well and be succinct. Users will know that this is a request because it starts with a verb ("Grant", which admittedly is very confusing, since "grant" can also be used as a noun). Let's call this GrantAccess.

jrperritt · 2017-08-11T16:42:01Z

 }
+
+// GrantAccess contains all information associated with an OpenStack share Grant Access
+type GrantAccess struct {


Following on from my comment above on GrantAccessRequest, response objects should be nouns. Here, something as simple as Access or AccessRight would work fine (again, admittedly confusing because "access" can be both a noun and a verb).

Perhaps @jtopjian has a better suggestion for the name

@jrperritt thank you for the suggestions. I personally like AccessRight more.

AccessRight works for me as well. I did not consider how nuanced the words in this PR could be, so I apologize about that.

One thing to keep in mind is that Manila has an API call to list access rules, which is a list of all GrantAccess calls that have been performed. Upon first glance, the body of these entries are slightly different than what AccessRight would be. Of course, it's always possible the docs are wrong.

https://developer.openstack.org/api-ref/shared-file-systems/?expanded=list-access-rules-detail#list-access-rules

If that API call gets implemented and new result structs need created, it might be a good idea to carry the AccessRight naming pattern to be consistent. Perhaps AccessRightEntries which is made up of individual AccessRightEntry (assuming AccessRightEntry is different than AccessRight).

Just thinking ahead. No action needs made.

coveralls · 2017-08-11T17:14:30Z

Coverage increased (+0.03%) to 71.524% when pulling 52223dc on pospispa:grant-access-to-a-share into 8a1afad on gophercloud:master.

jtopjian

@pospispa This is looking good. Just a few more comments.

jtopjian · 2017-08-12T00:21:20Z

+// GrantAccessResult contains the result body and error from an GrantAccess request.
+type GrantAccessResult struct {
+	commonResult
+}


@pospispa Take a look at the post-merge discussion that happened in #381 and the additional discussion in #449. I think this should be:

type GrantAccessResult struct { gophercloud.Result }

The name "commonResult" probably made sense when this shares package was only handling "shares", but now that it also handles Export Locations and Access Rights, it might be helpful to think of commonResult as shareResult and only be applicable to Shares. Technically what you have will work, but it's more about naming and "inheritance" consistency.

@jrperritt hopefully I am correct on this one, or else I've just been looking at Gophercloud code too long and am all sorts of mixed up.

jtopjian · 2017-08-12T00:22:51Z

+// Extract will get the GrantAccess object from the commonResult
+func (r GrantAccessResult) Extract() (*AccessRight, error) {
+	var s struct {
+		AccessRightRes *AccessRight `json:"access"`


This should be:

AccessRight AccessRight `json:"access"`

See here for an example.

jtopjian · 2017-08-12T01:15:23Z

+		return
+	}
+	_, r.Err = client.Post(grantAccessURL(client, id), b, &r.Body, &gophercloud.RequestOpts{
+		OkCodes: []int{200, 201},


Do you know when a 201 would be returned? Have you seen a 201 in any manila --debug output? I have only been able to find a default 200 response here.

These return codes are usually the hardest pieces to find. I'm OK leaving both 200 and 201 here unless you're confident it's safe to drop 201.

201 is a copy/paste error from func Create.

Yeah, OkCodes are usually just taken from the API docs, which we know to be incorrect from time to time. However, IMO, return codes are the hardest thing to search for, so I'm not very strict with them when doing a review.

coveralls · 2017-08-14T09:20:33Z

Coverage increased (+0.03%) to 71.524% when pulling 7b137f3 on pospispa:grant-access-to-a-share into 8a1afad on gophercloud:master.

pospispa · 2017-08-14T09:29:24Z

@jtopjian @jrperritt please, does it look good to you? Should I squash the commits?

jtopjian · 2017-08-15T00:50:15Z

+// Extract will get the GrantAccess object from the commonResult
+func (r GrantAccessResult) Extract() (*AccessRight, error) {
+	var s struct {
+		AccessRight *AccessRight `json:"access"`


👍

You're correct with *AccessRight here. I mistyped my last comment.

jtopjian · 2017-08-15T00:59:09Z

+	// The UUID of the share to which you are granted or denied access.
+	ShareID string `json:"share_id"`
+	// The access rule type that can be "ip", "cert" or "user".
+	AccessType string `json:"access_type,omitempty"`


@jrperritt General question: Does omitempty provide any use here?

If I do:

$ grep -R omitempty ~/go/src/github.com/gophercloud/gophercloud | grep result

I see that there are a good number of results.

Does omitempty provide any use here?

No; I think omitempty is only relevant for encoding. But it doesn't hurt either, so... meh.

omitempty is probably useless here.

jtopjian

@pospispa This looks good to me. I have a general question with regard to the omitempty in the result struct, but given there are other result structs that are the same, I'll defer to @jrperritt for whether that should be fixed or not.

Don't squash the commits. Github can do a full squash when this gets merged.

Thank you for your patiences on this PR.

pospispa · 2017-08-15T09:54:24Z

@jtopjian thank you for merging the PR.

* Shared File Systems: Grant Access By default shares are inaccessible because they do not have set any access permissions. That's why the Grant Access request is added. * Added unit tests for POST Grant Access request. * Fixes for the first round of code reviews. * Fixes for the second round of code reviews. * Fixes for the third round of code reviews. * Fixes for the fourth round of code reviews. * Fixes for the fifth round of code reviews.

The total statements covered for the DM code is increased from 24.5-26.5% Signed-off-by: vineela-pachchipulusu <[email protected]>

pospispa mentioned this pull request May 22, 2017

fix microversion header #53

Closed

pospispa changed the title ~~[WIP] Shared Filesystems: Grant Access~~ [WIP] [Pending #342] Shared Filesystems: Grant Access May 29, 2017

pospispa force-pushed the grant-access-to-a-share branch from b83719a to f6c9e98 Compare June 5, 2017 15:12

pospispa mentioned this pull request Jun 5, 2017

Get manila api microversion #342

Closed

pospispa force-pushed the grant-access-to-a-share branch from f6c9e98 to e880c52 Compare June 7, 2017 10:30

pospispa force-pushed the grant-access-to-a-share branch from e880c52 to cf83315 Compare June 7, 2017 11:14

pospispa changed the title ~~[WIP] [Pending #342] Shared Filesystems: Grant Access~~ [Pending #342] Shared Filesystems: Grant Access Jun 8, 2017

pospispa mentioned this pull request Jun 14, 2017

Shared Filesystems: Share Export Locations #381

Merged

pospispa force-pushed the grant-access-to-a-share branch from 2212c52 to 049604d Compare June 26, 2017 11:43

pospispa changed the title ~~[Pending #342] Shared Filesystems: Grant Access~~ [Pending #409] Shared Filesystems: Grant Access Jun 26, 2017

pospispa mentioned this pull request Jun 27, 2017

SharedFileSystems: apiversions package #409

Merged

pospispa force-pushed the grant-access-to-a-share branch from 049604d to 1b583d8 Compare July 20, 2017 17:37

pospispa changed the title ~~[Pending #409] Shared Filesystems: Grant Access~~ Shared Filesystems: Grant Access Jul 20, 2017

jrperritt requested review from jrperritt and jtopjian July 25, 2017 14:50

pospispa changed the title ~~Shared Filesystems: Grant Access~~ [WIP] Shared Filesystems: Grant Access Jul 26, 2017

pospispa changed the title ~~[WIP] Shared Filesystems: Grant Access~~ [WIP] [Pending #435] Shared Filesystems: Grant Access Jul 26, 2017

pospispa force-pushed the grant-access-to-a-share branch from 02f0791 to e6ed0d7 Compare August 11, 2017 12:35

pospispa added 5 commits August 11, 2017 18:13

Shared File Systems: Grant Access

eb4b06b

By default shares are inaccessible because they do not have set any access permissions. That's why the Grant Access request is added.

Added unit tests for POST Grant Access request.

47dc239

Fixes for the first round of code reviews.

92e76cc

Fixes for the second round of code reviews.

5432468

Fixes for the third round of code reviews.

3736803

pospispa force-pushed the grant-access-to-a-share branch from 34b618d to 3736803 Compare August 11, 2017 16:26

jrperritt suggested changes Aug 11, 2017

View reviewed changes

Fixes for the fourth round of code reviews.

52223dc

jtopjian requested changes Aug 12, 2017

View reviewed changes

Fixes for the fifth round of code reviews.

7b137f3

jtopjian reviewed Aug 15, 2017

View reviewed changes

jtopjian approved these changes Aug 15, 2017

View reviewed changes

jrperritt approved these changes Aug 15, 2017

View reviewed changes

jtopjian merged commit b38d5d9 into gophercloud:master Aug 15, 2017

pospispa mentioned this pull request Aug 16, 2017

Discussion: Improvements/Fixes to Grant Access and Share Export Locations Requests #488

Closed

pospispa mentioned this pull request Aug 25, 2017

On Microversions #441

Closed

2 of 8 tasks

msheikh-wind pushed a commit to msheikh-wind/gophercloud that referenced this pull request May 19, 2025

Addition of build files UT cases (gophercloud#346)

2f40114

The total statements covered for the DM code is increased from 24.5-26.5% Signed-off-by: vineela-pachchipulusu <[email protected]>

Conversation

pospispa commented May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pospispa commented Jun 9, 2017

Uh oh!

coveralls commented Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pospispa commented Jul 25, 2017

Uh oh!

jrperritt commented Jul 25, 2017

Uh oh!

jtopjian commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pospispa commented Jul 26, 2017

Uh oh!

pospispa commented Jul 26, 2017

Uh oh!

pospispa commented Aug 11, 2017

Uh oh!

pospispa commented Aug 11, 2017

Uh oh!

coveralls commented Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtopjian commented Aug 11, 2017

Uh oh!

pospispa commented Aug 11, 2017

Uh oh!

coveralls commented Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrperritt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtopjian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

pospispa commented May 22, 2017 •

edited

Loading

coveralls commented May 22, 2017 •

edited

Loading

coveralls commented Jun 5, 2017 •

edited

Loading

coveralls commented Jun 7, 2017 •

edited

Loading

coveralls commented Jun 7, 2017 •

edited

Loading

coveralls commented Jun 8, 2017 •

edited

Loading

coveralls commented Jun 26, 2017 •

edited

Loading

coveralls commented Jul 20, 2017 •

edited

Loading

jtopjian commented Jul 25, 2017 •

edited

Loading

coveralls commented Aug 11, 2017 •

edited

Loading

coveralls commented Aug 11, 2017 •

edited

Loading

coveralls commented Aug 11, 2017 •

edited

Loading

coveralls commented Aug 14, 2017 •

edited

Loading

pospispa commented Aug 14, 2017 •

edited

Loading