Skip to content

objects: partially revert e54703519#2160

Merged
jtopjian merged 2 commits intogophercloud:masterfrom
cardoe:fix-object-with-slash
Jun 8, 2021
Merged

objects: partially revert e54703519#2160
jtopjian merged 2 commits intogophercloud:masterfrom
cardoe:fix-object-with-slash

Conversation

@cardoe
Copy link
Copy Markdown
Contributor

@cardoe cardoe commented May 12, 2021

Since this change you are no longer able to create objects with a / in
them to create pseudo-folders. This behavior is relied upon by kOps
documented as broken in kubernetes/kops#9933. fixes #2159.

@coveralls
Copy link
Copy Markdown

coveralls commented May 12, 2021

Coverage Status

Coverage decreased (-0.002%) to 79.916% when pulling dc08882 on cardoe:fix-object-with-slash into 9abac83 on gophercloud:master.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented May 13, 2021

Build failed.

@jtopjian
Copy link
Copy Markdown
Contributor

It looks like the failure is with the TestObjectsBulkDelete test:

2021-05-13 00:17:02.588362 | ubuntu-bionic |     convenience.go:35: �[1;31mFailure in objects_test.go, line 316: unexpected error �[0m�[1;33m"Expected HTTP response code [] when accessing [DELETE http://192.168.0.18:8080/v1/AUTH_6b79870e7d4b4022a9a51dc246cf1174/test%26happy%3F-eX1Hu1AF], but got 409 instead\n<html><h1>Conflict</h1><p>There was a conflict when trying to complete your request.</p></html>"�[0m�[1;31m�[0m
2021-05-13 00:17:02.640192 | ubuntu-bionic | --- FAIL: TestObjectsBulkDelete (3.14s)

I'm thinking that url. QueryEscape should be removed from all occurrences of being used with objectName. It feels inconsistent to only remove it for the Create API call.

@cardoe cardoe force-pushed the fix-object-with-slash branch from 8c9ee12 to 3cb4af3 Compare June 7, 2021 20:45
@cardoe
Copy link
Copy Markdown
Contributor Author

cardoe commented Jun 7, 2021

Rebased and updated based on your suggestion @jtopjian

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Jun 7, 2021

Build failed.

@cardoe
Copy link
Copy Markdown
Contributor Author

cardoe commented Jun 7, 2021

@jtopjian it seems it couldn't find a host to test against? Just amending my commit to make it try again.

Since this change you are no longer able to create objects with a / in
them to create pseudo-folders. This behavior is relied upon by kOps
documented as broken in kubernetes/kops#9933. fixes gophercloud#2159.
@cardoe cardoe force-pushed the fix-object-with-slash branch from 3cb4af3 to ba5ba63 Compare June 7, 2021 21:31
@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Jun 7, 2021

@cardoe Yes, there's been some hiccups with OpenLab since this PR first opened. If you can, please do a git pull from the master branch and then rebase this branch off of the updated master. Once you do a force-push, testing should work again. Let me know if you have any problems, though. I apologize about the hassle.

@cardoe
Copy link
Copy Markdown
Contributor Author

cardoe commented Jun 7, 2021

The failure happened when I was rebased atop of 9abac83 which is the current master commit.

@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Jun 7, 2021

ah - well that's interesting! Let's hope it was a one-off issue. I'll dig into it if it fails again. Additionally, I'll look at testing this in a different environment and try to get this reviewed/merged quickly for you.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Jun 7, 2021

Build failed.

@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Jun 7, 2021

The errors indicate that Devstack is still being created with Ubuntu Bionic, though there's no more mention of Bionic in the repo. I wonder if OpenLab/Zuul is not picking up the rebase somehow...

I'll take a look at this shortly and try to run the tests in a separate environment.

@cardoe
Copy link
Copy Markdown
Contributor Author

cardoe commented Jun 7, 2021

Thank you for the help with this.

@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Jun 8, 2021

@cardoe I've added a commit with additional changes.

I tested this on Vexxhost and ran into errors with the QueryEscape calls in general - I'm not sure how this is/was passing in OpenLab. I decided to just remove all QueryEscape calls from within Gophercloud. A dev/user is still able to escape the paths if they like by performing a url.QueryEscape on the name before calling the various Object Storage functions - there's no reason for Gophercloud to mandate it.

Let me know your thoughts. I'm happy to merge this.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Jun 8, 2021

Build failed.

@kayrus
Copy link
Copy Markdown
Contributor

kayrus commented Jun 8, 2021

A dev/user is still able to escape the paths if they like by performing a url.QueryEscape on the name before calling the various Object Storage functions - there's no reason for Gophercloud to mandate it.

Let's hope other dependent apps won't fail :\

@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Jun 8, 2021

There's a good possibility this will affect some applications - this is a breaking change. However, leaving this in is breaking functionality in other areas. From the behaviour I observed on Vexxhost, it's best to leave the URL encoding to the application so it can handle these situations in a more controlled and fine-grained manner.

@cardoe
Copy link
Copy Markdown
Contributor Author

cardoe commented Jun 8, 2021

So I definitely feel that encoding the object name is wrong and bad. Playing with this further shows a number of valid objects that cannot be properly manipulated with GopherCloud. I'm less opinionated about the container name being escaped.

I see with your changes I missed a spot in the bulk delete with the object name when it was in a loop which would have caused test failures with my change.

@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Jun 8, 2021

OK, I'm going to merge this.

I do understand that this is a breaking change. For anyone who reads this - I apologize. And if this behaviour is still not working correctly, please let me know and I'll do what I can to fix it.

For anyone who was relying on URL encoded container or object names, you can still do this by encoding the name before passing the name to the object storage functions.

@jtopjian jtopjian merged commit b7d5b2c into gophercloud:master Jun 8, 2021
@cardoe cardoe deleted the fix-object-with-slash branch June 8, 2021 20:40
cardoe added a commit to cardoe/kops that referenced this pull request Jun 16, 2021
Since the v1.19.0 release of kOps the OpenStack integration has been
broken because gophercloud made a change to start escaping all path
names for files stored in swift. kOps used a file with a slash in it
which was getting escaped and then not handled correctly. This has been
fixed in upstream gophercloud with gophercloud/gophercloud#2160 which
was included in gophercloud v0.18.0. Bump to that version to fix kubernetes#9933.
cardoe added a commit to cardoe/kops that referenced this pull request Jun 16, 2021
Since the v1.19.0 release of kOps the OpenStack integration has been
broken because gophercloud made a change to start escaping all path
names for files stored in swift. kOps used a file with a slash in it
which was getting escaped and then not handled correctly. This has been
fixed in upstream gophercloud with gophercloud/gophercloud#2160 which
was included in gophercloud v0.18.0. Bump is for issue kubernetes#9933.
cardoe added a commit to cardoe/kops that referenced this pull request Jun 16, 2021
Since the v1.19.0 release of kOps the OpenStack integration has been
broken because gophercloud made a change to start escaping all path
names for files stored in swift. kOps used a file with a slash in it
which was getting escaped and then not handled correctly. This has been
fixed in upstream gophercloud with gophercloud/gophercloud#2160 which
was included in gophercloud v0.18.0. Bump is for issue kubernetes#9933.
cardoe added a commit to cardoe/kops that referenced this pull request Jun 21, 2021
Since the v1.19.0 release of kOps the OpenStack integration has been
broken because gophercloud made a change to start escaping all path
names for files stored in swift. kOps used a file with a slash in it
which was getting escaped and then not handled correctly. This has been
fixed in upstream gophercloud with gophercloud/gophercloud#2160 which
was included in gophercloud v0.18.0. Bump is for issue kubernetes#9933.
cardoe added a commit to cardoe/kops that referenced this pull request Jun 21, 2021
Since the v1.19.0 release of kOps the OpenStack integration has been
broken because gophercloud made a change to start escaping all path
names for files stored in swift. kOps used a file with a slash in it
which was getting escaped and then not handled correctly. This has been
fixed in upstream gophercloud with gophercloud/gophercloud#2160 which
was included in gophercloud v0.18.0. Bump is for issue kubernetes#9933.

(cherry picked from commit 80da992)
mandre added a commit to shiftstack/installer that referenced this pull request Oct 27, 2022
Update the pin to gophercloud v0.18.0 so that we get a fix that came
with gophercloud/gophercloud#2160.

Also use a replacement for tencentcloud-sdk-go as they removed their
tags and broke all downstream projects in the process
(https://github.com/hashicorp/terraform#29021). This is needed in
order to run `go mod vendor`.
mandre added a commit to shiftstack/installer that referenced this pull request Nov 3, 2022
Update the pin to gophercloud v0.18.0 so that we get a fix that came
with gophercloud/gophercloud#2160.
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.

objects: unable to create files with / in the name as a pseudo folder

4 participants