search

pplu / aws-sdk-perl

A community AWS SDK for Perl Programmers
Other
171 stars 94 forks source link

Requirements for stabilisation? #244

Open shadowcat-mst opened 6 years ago

shadowcat-mst commented 6 years ago

So, a number of components in Paws have something like this -

https://metacpan.org/source/JLMARTIN/Paws-0.36/lib/Paws/S3.pm#L2

Is there a list anywhere of what's required to get to the point where that warning can go away? If not, how would we go about getting to a point where there was such a list?

pplu commented 6 years ago

There are three services marked with warnings: S3, Route53 and Glacier. They basically pertain to the RestXML set of services. These services are a bit more complex than the others, each one having their special corner cases, and maybe the code even needs to be specialized for each service to get everything working correctly.

Having a comprehensive set of tests to verify if anything breaks would be a must (note: there are already some tests for stuff that was getting reported) I've revisited open issues that would need looking at before considering each one of these stable.

castaway commented 6 years ago

Hi @pplu I've visited each one of the items listed above (and in some cases left comments), I also made a plan for a set of tests to use to verify fixes for each one, which we'll be working on implementing next. I see the tests generally use mocking for any checks against data sent to the APIs, we will use more of that.

Have you looked at / considered any end to end testing? I discovered https://github.com/localstack/localstack and wondered if we could use it, in a special subset of tests that only run if localstack is installed (indicated by an environment variable or similar) - any thoughts? Alternately we could run the existing set of tests against it with some tweaks based on env vars, and then maybe against AWS itself too, if the tester wants to.

pplu commented 6 years ago

I also made a plan for a set of tests to use to verify fixes for each one, which we'll be working on implementing next. I see the tests generally use mocking for any checks against data sent to the APIs, we will use more of that.

Great! If you want some help around the test suite or have doubts about how to test a specific case, don't doubt in contacting me.

Have you looked at / considered any end to end testing? I discovered https://github.com/localstack/localstack and wondered if we could use it, in a special subset of tests that only run if localstack is installed (indicated by an environment variable or similar) - any thoughts? Alternately we could run the existing set of tests against it with some tweaks based on env vars, and then maybe against AWS itself too, if the tester wants to.

I'd love end to end testing to happen. Most of the test suite is derived from real calls and real responses, so you could say that the tooling is in place. There is a pluggable caller called Paws::Net::MockCaller that accepts two modes: RECORD, where it actually calls the services, but records the input parameters and the response from the backend service. When in REPLAY mode it only accepts the same calling sequence, with the same parameters for each call, and instead of doing the HTTP call, it reads the recorded HTTP responses (so all the response handling logic is invoked). If you want to develop test suites against localstack I'd be happy to have them.

castaway commented 6 years ago

Having just gotten a bit confused with it - it seems if you "record" with credentials set to Test::CustomCredentials, it doesn't send creds that work, at least not for me, I get "The AWS Access Key Id you provided does not exist in our records.".. it records fine if I comment out that setting.

Is it supposed to work with them set? Do we need the CustomCredentials, since it seems to be the REPLAY/RECORD setting that tells it whether we are calling the live backend or not.

pplu commented 6 years ago

The MockCaller is supposed to be used with valid AWS credentials when in record mode (just using the default credential provider, developing with creds in ~/.aws/credentials or in ENV vars. In Replay mode the credential provider doesn't matter (the TestCredentials are useful in test so that Paws need anything special in the environment)

pplu commented 6 years ago

@castaway @piratefinn @shadowcat-mst : Tell me if you feel some of the branches can be merged for an upcoming release (as I want to merge them to release/0.39 as soon as possible). I'm kind of lost inside all the branches, so I ask you to please mention me what branches you think can be merged when you have them "ready" (I'm not actively trying to know if your branches are ready for merge or not).

piratefinn commented 6 years ago

@pplu After some discussion with @castaway, we decided #265 and #261 are good to pull. I'm closing 266 for now until we have tinkered with other elements more.

pplu commented 6 years ago

Hi,

261 is passing all tests. I'll merge it to release/0.39

265 is failing tests https://travis-ci.org/pplu/aws-sdk-perl/builds/404615248 so I won't merge it yet. I understand that #265 is a bunch of tests, with no functionality changes.

castaway commented 6 years ago

Darn, they were supposed to be all TODO unless passing. We will recheck

castaway commented 6 years ago

Hi @pplu , the following PRs have been tested against tests/stabilisation and can be merged:

278 #276 #275 #261

265 should be merged first, that one is currently failing due to the botocore update, the fixes are in #278

Jess

pplu commented 6 years ago

Hi!

@castaway I've merged all the branches you mentioned onto the tests/stabilization branch. After regenerating all the service classes, I got a failure in t/s3/xml_creation.t that was basically because of the XML being generated in an undeterministic manner due to get_all_attributes not returning attributes in the same order (https://travis-ci.org/pplu/aws-sdk-perl/builds/410815936 shows the behavior). I modified the caller (https://github.com/pplu/aws-sdk-perl/commit/0cae73c3f9f62bf90bac1ed2eeecab40a1229f9a) but now the route53 tests are broken (https://travis-ci.org/pplu/aws-sdk-perl/builds/410831075).

What way do you recommend fixing the route53 XML tests? They seem to be using XML::Compare, but I don't know if that is helping or not (it doesn't help you find why the test isn't passing).

pplu commented 6 years ago

BTW: 💃 🎉 🎈 @shadowcat-mst @castaway @piratefinn

castaway commented 6 years ago

Hi.. huh, they passed for me last I tried - the use of XML::Compare was an attempt to have it not care which order internal XML elements were in, though I'm not sure if it worked.. I would say for now, if you've managed to have them output in a known order (alphabetical?) then update the tests to match, assuming the XML is generally correct..

pplu commented 6 years ago

I've just pushed a commit to tests/stabilization that fixes the route53 tests, so we're all green :smile:

@castaway, @piratefinn Can you please add items to the Changes file for what you consider fixed?

castaway commented 6 years ago

Yes, now done + pushed to our tests/stabilisation branch (after pulling your changes)

castaway commented 5 years ago

hi @pplu, where did we get to on merging tests/stabilisation (#265) ? I've lost track..

pplu commented 5 years ago

I lost track too :(

castaway commented 5 years ago

I will rerun it against current release branch..

byterock commented 5 years ago

I am still playing with th S3 side of things. Working my way though 'BucketAnalyticsConfigurations' PUT/LIST/GET/Delete etc

After I am finished those I will see if I can get the S3 you memtioned above done.

Would love to get rid of that warning ;)

castaway commented 5 years ago

We're working on them ourselves, I was just trying to see where we'd gotten to (still retesting the branch, as rebase left a failing test)

byterock commented 5 years ago

I am about 80% complete on the code side of things on S3 API side of things you can check out my changes on this branch/pull of paws https://github.com/byterock/aws-sdk-perl/tree/s3ObjectTagging and boto

https://github.com/byterock/botocore/tree/s3-changes

so far I have gotten Put/Get/List/Delete of BucketAnalyticsConfiguration BucketCors BucketEncryption BucketInventoryConfiguration

all working 100%, in scripts

no tests yet in the /t yet as most of the problems have been with the 'Request' side of things

Which of the S3s are you working on so we do not bash each other code

castaway commented 5 years ago

I've lost track as we started last year and then trailed off a bit - would suggest you check items against tests/stabilisation (#265) - which also contains fixes as all the bits were merged together.. before starting on them.

Hopefully will have that rebased on 0.42 soon, and we can merge it and go from there

castaway commented 5 years ago

Right, thats #265 rebased and tests all passing.. now to try and figure out which of the above list it covers.

castaway commented 5 years ago

I am about 80% complete on the code side of things on S3 API side of things you can check out my changes on this branch/pull of paws https://github.com/byterock/aws-sdk-perl/tree/s3ObjectTagging and boto

https://github.com/byterock/botocore/tree/s3-changes

so far I have gotten Put/Get/List/Delete of BucketAnalyticsConfiguration BucketCors BucketEncryption BucketInventoryConfiguration

all working 100%, in scripts

no tests yet in the /t yet as most of the problems have been with the 'Request' side of things

Which of the S3s are you working on so we do not bash each other code

Hey soo two things:

1) Are those boto patches adding Content-MD5 etc to the botocore service-1.json etc? Have you seen this issue in which they didnt want such a patch? https://github.com/boto/botocore/issues/1302

2) It looks like so far we've covered/fixed all the items at the top of this issue apart from #149 and #92 (both of which are similar issues to what you seem to be looking at)

byterock commented 5 years ago

Ok I havea few more in the pipe one which is very interting and I thing will get rid of the flatten problems

will checkout out your branch and see how it matches up with my test set

castaway commented 5 years ago

Ok I havea few more in the pipe one which is very interting and I thing will get rid of the flatten problems

will checkout out your branch and see how it matches up with my test set

I have a feeling we may have fixed those in the branch.. will see!

Btw which issue # is the one about /?version (query params with no values) etc not working? I couldnt find it

byterock commented 5 years ago

I was just going systematically though all the 'actions' of S3 and fixing them as I went along. I added them to the bug list as I fixed them.

castaway commented 5 years ago

hmm annoying, Im sure there was one

byterock commented 5 years ago

What woudl be the most up to date branch??

byterock commented 5 years ago

Ok I fighre that out

tests/stabilisation

So far These bugs have not been Fixed on that branch

PutBucketAnalyticsConfiguration https://github.com/pplu/aws-sdk-perl/issues/350

The issue is the boto has "requestUri":"/{Bucket}?analytics"

and when run though request we get

/dev.cargotel.paws?id=test1333'

should be

/dev.cargotel.paws?analytics=&id=test1333'

PutBucketCors https://github.com/pplu/aws-sdk-perl/issues/351

Flattening issue

`

http://www.example.com ...` Paws was giving ` http://www.example.com ...` boto change on this one PutBucketLifecycleConfiguration https://github.com/pplu/aws-sdk-perl/issues/353 PutBucktMetricsConfiguration https://github.com/pplu/aws-sdk-perl/issues/354 boto change Missing "ContentMD5" as required by AWS PutBucketPolicy https://github.com/pplu/aws-sdk-perl/issues/355 this isseu here is the API requires JSON not XML These have been fixed GetBucketPolicyOutput https://github.com/pplu/aws-sdk-perl/issues/355
castaway commented 5 years ago

bits of s3 use JSON not XML !? impressive.. The /?analytics one is the URI bug I was looking for (I'm sure someone reported an issue in URI.pm for that) ..

Looks like you found a bunch that hadn't been previously reported (we were working on the list at the top of this issue) .. mebbe we should make a new list?

byterock commented 5 years ago

One response is in JSON and I think one put as well.

I have fixed 'PutBucketAcl ' the https://github.com/pplu/aws-sdk-perl/issues/356

this gets the traits => ['NameInRequest','NameInRequest'], bug (nobody noticed that one. I reuse to fix a flatten/tagging issue and that call is the only one to use XML traits ARG

anway don't have many more to reivew/fix at least for S3.

castaway commented 5 years ago

Hi @pplu, could we get #265 looked at / merged please? Then ideally a review of the changes @byterock has been making would be useful, to know if thats on the right track.

byterock commented 5 years ago

I found a fix for that ' The /?analytics Bug'

you can see i on this chommit https://github.com/byterock/aws-sdk-perl/commit/08d44a3f1588669b2506f7a0b2911ad3d1ff8921

Nothing special I just treat all query params as URI and change the template that is comming in.

No more changes to boto yeah

castaway commented 5 years ago

I found a fix for that ' The /?analytics Bug'

you can see i on this chommit byterock@08d44a3

Nothing special I just treat all query params as URI and change the template that is comming in.

No more changes to boto yeah

You're manually building the query string? Surely this would be better fixed in URI itself, rather than adding extra level code to Paws?

byterock commented 5 years ago

Well the code change is simple enough I treat both ParamInURI and ParamInQuery the same except if when it is a ParamInQuery I add the item to the template.

if ($attribute->does('Paws::API::Attribute::Trait::ParamInQuery')) { my $att_name = $attribute->name; $uri_template .= $joiner.$attribute->query_name."={".$att_name."}"; $vars->{ $att_name } = $call->$att_name

and then let template sort it out.

At the AWs server end doesn't really care if we call a param a URI or Query param as long as in comes along in the end as all of these work

/some.bucket/?analytics&id=10001 /some.bucket/?analytics=&id=10001 /some.bucket/?id=10001&analytics

as a matter of fact I am now getting into a problem with my new test case where the URI and URL are failing at times as the order of the params keeps changes once it goes into a request object

castaway commented 4 years ago

Hi & happy new year, @pplu , would it be possible to get an update on when you'll be able to look at merging the various PRs? Unfortunately we now have some conflicting ones (SC has fixed some things, and @byterock has fixed some in different ways), plus the Moo conversion will need to be updated after the merge (or done first and everything else updated)

byterock commented 4 years ago

I went through all the bugs on the I could find on S3 and the good old

https://github.com/byterock/aws-sdk-perl/tree/s3ObjectTagging

seems to be working for 99% everything.

I am going to look at the tests on this brach and move them into mine to see if any fail and then check my real world tests to see if the newer tests are ok

byterock commented 4 years ago

Ok here is the first one (all tests run against the https://github.com/byterock/aws-sdk-perl/tree/s3ObjectTagging branch)

t/s3/content_headers.t

This test checks for the existance of the 'Content-Length' header

I have done some real world testsing and the 'Contnet-Lenght' header does not need to be included

Below is the request and response from a real test. bless( { 'method' => 'PUT', 'date' => '20200204T221945Z', 'headers' => bless( { 'content-md5' => 'jeZMJlnIjr5T4V2k0kf3sA==', 'authorization' => 'AWS4-HMAC-SHA256 Credential=AKIAJ25UFIFHZ4WML4BQ/20200204/us-east-1/s3/aws4_request,SignedHeaders=content-md5;content-type;date;host;x-amz-content-sha256;x-amz-date,Signature=c8817743aa6fa579894cbeaa996e190eb2ab8869da4007bf9be22e379fd844db', 'x-amz-content-sha256' => '0ddf26e60424f76a462d28de46e32715db6c6a7efe651eb51e441b9b28f81994', 'content-type' => 'application/xml', 'host' => 's3.amazonaws.com', '::std_case' => { 'x-amz-date' => 'X-Amz-Date', 'x-amz-content-sha256' => 'X-Amz-Content-Sha256' }, 'x-amz-date' => '20200204T221945Z', 'date' => '20200204T221945Z' }, 'HTTP::Headers' ), 'content' => '<AccessControlPolicy xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><AccessControlList><Grant><Grantee xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:type="CanonicalUser"><DisplayName>OwnerDisplayName</DisplayName><ID>61fc60cb3564d2688646074ceaa202e56ee65cf6e56c28b3a034eabd193d54c8</ID></Grantee><Permission>FULL_CONTROL</Permission></Grant></AccessControlList><Owner><DisplayName>Nothing</DisplayName><ID>61fc60cb3564d2688646074ceaa202e56ee65cf6e56c28b3a034eabd193d54c8</ID></Owner></AccessControlPolicy>', 'parameters' => { 'AccessControlPolicy.Grants.Grant.1.Grantee.DisplayName' => 'OwnerDisplayName', 'AccessControlPolicy.Owner.ID' => '61fc60cb3564d2688646074ceaa202e56ee65cf6e56c28b3a034eabd193d54c8', 'AccessControlPolicy.Grants.Grant.1.Permission' => 'FULL_CONTROL', 'AccessControlPolicy.Owner.DisplayName' => 'Nothing', 'AccessControlPolicy.Grants.Grant.1.Grantee.ID' => '61fc60cb3564d2688646074ceaa202e56ee65cf6e56c28b3a034eabd193d54c8', 'AccessControlPolicy.Grants.Grant.1.Grantee.Type' => 'CanonicalUser', 'Bucket' => 'oneoffpaws' }, 'uri' => '/oneoffpaws?acl', 'url' => 'https://s3.amazonaws.com/oneoffpaws?acl' }, 'Paws::Net::S3APIRequest' )

I think we can drop this test. Unless I am missing something?

byterock commented 4 years ago

The following tests all pass;

s3/uri_avoid_chars.t s3/uri_encoding.t s3/multipartupload.t

s3/xml_creation.t

is a little flawed The XML is not correct AWS requires the ' xmlns="http://s3.amazonaws.com/doc/2006-03-01/"' in the root tag of both calls

as found in boto "SelectObjectContent":{ ... "input":{ "shape":"SelectObjectContentRequest", "locationName":"SelectObjectContentRequest", "xmlNamespace":{"uri":"http://s3.amazonaws.com/doc/2006-03-01/"} },

as well the XML sorting might be differnt so test can fail randomly

I can correct for the 'xmlNamespace' but not the sort

I only have

t/s3/uri_avoid_chars.t

to look at but I am not sure what that is testing??

castaway commented 4 years ago

Hmm, re the Content-Length headers, this was from #92, and then the AWS docs which say "Length of the message (without the headers) according to RFC 2616. This header is required for PUTs and operations that load XML, such as logging and ACLs." - see: https://docs.aws.amazon.com/AmazonS3/latest/API/RESTCommonRequestHeaders.html

Not really much harm in sending them.. mebbe one day the docs will catch up with reality..

castaway commented 4 years ago

Re xml_creation.t - aha thanks, have you got changes that sort out the xmlNamespace bits? Can we easily extract them? (and matching tests?)

Re: t/s3/uri_avoid_chars.t this is a test to ensure that Paws does not allow users to attempt to create objects using characters which AWS does not allow.

byterock commented 4 years ago

I would just drop the xml_creation.t test as I created a new generic test suite for that

09_requests.t

That should cover all the XML creation and its little odd bits

All the changes are in the

https://github.com/byterock/aws-sdk-perl/tree/s3ObjectTagging

branch

As for 't/s3/uri_avoid_chars.t '

I am not sure if this is a correct test for this level of API. Would be useful for a CLI or alike but nowhere in PAWS is there pre-call data sanitization.

Anyway AWS will fail on creation when you have such a bad character. Better error handling might be what we want here.

I think the only part of Rest XML that is not working or tested in 'S3::SelectObjectContent' with that

"event":true stream.

Will see if I can play with that later today

byterock commented 4 years ago

I attmpeted to run teh 09 and 10 test suites on a few other branches today with rater poor resutls (almost all fails)

What branch apart form mine should we test it agianst?

I found this one

https://github.com/shadow-dot-cat/aws-sdk-perl.git

was rather out of date for the Rest XML bits ??

cheers John

castaway commented 4 years ago

Can we keep this discussion on #372 please?