Skip to content
This repository was archived by the owner on Mar 31, 2026. It is now read-only.

feature: V4 Post policies - #87

Merged
crwilcox merged 43 commits into
googleapis:masterfrom
MaxxleLLC:v4_post_policies
Apr 1, 2020
Merged

feature: V4 Post policies#87
crwilcox merged 43 commits into
googleapis:masterfrom
MaxxleLLC:v4_post_policies

Conversation

@IlyaFaer

Copy link
Copy Markdown

Towards #8

@IlyaFaerIlyaFaer added the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label Mar 16, 2020
@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 16, 2020
Comment threadgoogle/cloud/storage/_signing.py
Comment threadgoogle/cloud/storage/client.py Outdated
Comment threadtests/unit/test_client.py
@IlyaFaer
IlyaFaer requested a review from frankynMarch 16, 2020 15:07
@IlyaFaer
IlyaFaer marked this pull request as ready for review March 16, 2020 15:07
@IlyaFaer

IlyaFaer commented Mar 16, 2020

Copy link
Copy Markdown
Author

@frankyn, I've pushed a real PR. Only system tests are failing with Anonymous caller does not have storage.objects.create access to <bucket-name>/<file-name> (the same Jonathan Lui got)

Everything except this seems to be fine

@frankyn

Copy link
Copy Markdown
Contributor

Thanks @IlyaFaer, I'll review. I'll update group in V4 Sig Hangouts Chat.

@frankyn
frankyn requested a review from crwilcoxMarch 17, 2020 06:01

@frankynfrankyn left a comment

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.

Small nits

Comment threadgoogle/cloud/storage/_signing.py Outdated
Comment threadgoogle/cloud/storage/client.py Outdated
Comment threadgoogle/cloud/storage/client.py

@frankynfrankyn left a comment

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.

Thanks for your patience @IlyaFaer, added a few more comments.

Comment threadgoogle/cloud/storage/_signing.py Outdated
Comment threadgoogle/cloud/storage/client.py Outdated
Comment threadgoogle/cloud/storage/client.py
Comment threadgoogle/cloud/storage/client.py

@frankynfrankyn left a comment

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.

One more nit, and will ping @crwilcox for his Python expertise.

Comment threadgoogle/cloud/storage/client.py
@IlyaFaerIlyaFaer added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 23, 2020
Comment threadgoogle/cloud/storage/client.py Outdated
@IlyaFaer

Copy link
Copy Markdown
Author

@frankyn, @crwilcox, I've moved conformance tests into this PR. I assume you'd like me to add the conformance tests data as well!? Locally I've added policy conformance tests data into url_signer_v4_test_data.json, and used it while running the tests. Is it a correct place?

Безымянный

@frankyn

Copy link
Copy Markdown
Contributor

@IlyaFaer yes, keep it in the same file to reduce complexity of conformance tests.

@frankyn

Copy link
Copy Markdown
Contributor

@IlyaFaer could you update conformance tests to follow the exact format of source conformance tests: https://github.com/googleapis/conformance-tests/blob/master/storage/v1/v4_signatures.json

It can be a subsequent PR to reduce complexity for this one. I'm trying to prevent snowflakes.

@frankynfrankyn left a comment

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.

I missed a few nits in yesterday's review. Thanks for consolidating conformance tests @IlyaFaer

Comment threadgoogle/cloud/storage/client.py
Comment threadgoogle/cloud/storage/client.py Outdated
@IlyaFaerIlyaFaer added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 31, 2020
@yoshi-kokoroyoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 31, 2020

@frankynfrankyn left a comment

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.

Few more nits.

Comment threadtests/unit/test_client.py Outdated
Comment threadgoogle/cloud/storage/client.py Outdated

@frankynfrankyn left a comment

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.

LGTM, pending KokoroCI passing and approval from @crwilcox

@IlyaFaer

IlyaFaer commented Mar 31, 2020

Copy link
Copy Markdown
Author

follow the exact format of source conformance tests

@frankyn, you mean, use these signingV4Tests and postPolicyV4Tests fields on a first level of the json? That's not a problem, though I assume some code should be changed in test__signing.py as well, so yes, that probably should be done in another PR.

@crwilcox, @frankyn, I think, I saw random conformance tests are flakely failing in Python 2.7 and 3.5 while it all was in progress. I'll take a closer look.

@frankyn

Copy link
Copy Markdown
Contributor

Hi @IlyaFaer,

@frankyn, you mean, use these signingV4Tests and postPolicyV4Tests fields on a first level of the json? That's not a problem, though I assume some code should be changed in test__signing.py as well, so yes, that probably should be done in another PR.

Yes, that's correct, could you please open a tracking issue for it as well? Thank you.

I think, I saw random conformance tests are flakely failing in Python 2.7 and 3.5. Will take a closer look.

Conformance tests should not have flakes, please keep us posted.

@crwilcox

Copy link
Copy Markdown
Contributor

It seems there are a few kokoro failures. @IlyaFaer are these expected or is this work to do yet?

@frankyn

Copy link
Copy Markdown
Contributor

@crwilcox, @IlyaFaer raised that are some flakes. It looks like system tests and not conformance tests, but I might be incorrect. Pending follow-up.

Thanks for reviewing this PR @crwilcox!

@IlyaFaer

IlyaFaer commented Apr 1, 2020

Copy link
Copy Markdown
Author

@frankyn, @crwilcox, I've found out what was causing flaky: dict.items() returns result in different order sometimes:

Безымянный1
Безымянный2

So I've added sorted() for fields - didn't detect any new failing on 2.7/3.5 so far.

I've fixed system tests as well (they were failing with Anonymous requests are not allowed for some time, but now they're okay, so I've fixed a couple of my bads).

I hope nothing is forgotten. All checks are green now 🎉

@crwilcox

Copy link
Copy Markdown
Contributor

Awesome. Yep @IlyaFaer ordered dicts weren't in 2.7 at all or even older versions of 3.

@crwilcox
crwilcox merged commit b451e2d into googleapis:masterApr 1, 2020
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
* feat: add POST policies building method
* add comments, ignoring x-ignore fields and required fields validation
* fix docs style, add virtual hosted style URLs
* add bucket_bound_hostname support
* cosmetic changes
* add unit tests
* Revert "add unit tests"
This reverts commit f56440b.
* add few lines from the old implementation for consistency
* add some system tests
* move system tests into separate class
* fix credentials scope URL mistake
* fix unit tests
* fix algorithm name
* add an example
* add access token support
* add credentials as an argument
* rename method
* add conformance tests into client unit tests
* align conformance tests with test data
* add an ability to set expiration as integer
* update conformance tests to avoid problems with json spaces and timestamp Z-symbol violation
* update implementation to avoid Z symbol isoformat violation and json whitespaces encoding
* fix error with bounded hostnames
* fix problem with bounded hostnames in implementation
* fix conformance tests
* fix problems: ascii encoding of signature and fields order
* change asserts order
* fix conformance tests
* fix encoding issues
* cosmetic changes and adding conformance tests
* fix russion "C" letter in comment
* add conformance tests data
* cosmetic changes
* cosmetic changes
* add fields sorting
* fix system tests
Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
* feat: add POST policies building method
* add comments, ignoring x-ignore fields and required fields validation
* fix docs style, add virtual hosted style URLs
* add bucket_bound_hostname support
* cosmetic changes
* add unit tests
* Revert "add unit tests"
This reverts commit f56440b.
* add few lines from the old implementation for consistency
* add some system tests
* move system tests into separate class
* fix credentials scope URL mistake
* fix unit tests
* fix algorithm name
* add an example
* add access token support
* add credentials as an argument
* rename method
* add conformance tests into client unit tests
* align conformance tests with test data
* add an ability to set expiration as integer
* update conformance tests to avoid problems with json spaces and timestamp Z-symbol violation
* update implementation to avoid Z symbol isoformat violation and json whitespaces encoding
* fix error with bounded hostnames
* fix problem with bounded hostnames in implementation
* fix conformance tests
* fix problems: ascii encoding of signature and fields order
* change asserts order
* fix conformance tests
* fix encoding issues
* cosmetic changes and adding conformance tests
* fix russion "C" letter in comment
* add conformance tests data
* cosmetic changes
* cosmetic changes
* add fields sorting
* fix system tests
Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
@release-pleaserelease-pleaseBot mentioned this pull request Jan 12, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yesThis human has signed the Contributor License Agreement.type: feature request‘Nice-to-have’ improvement, new feature or different behavior or design.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@IlyaFaer@frankyn@crwilcox@googlebot@yoshi-kokoro