Add SNS tests.

[Micheletto]

This is a work in progress to add some SNS checks. It currently has a single test, not yet working, which asserts that no SNS Subscriptions should exist with PendingVerified set to true. We no doubt could use other SNS related tests, such as asserting that no SNS topics should have zero subscriptions and the like.

The current state of this test is failing on the SNS get_subscription_attributes call, and as near as I can tell it has something to do with sending the request to the wrong AWS region. There are no doubt other problems, stylistic and otherwise.

Latest Traceback:

==================================== ERRORS ====================================
____________ ERROR collecting aws/sns/test_sns_pending_verified.py _____________
aws/sns/test_sns_pending_verified.py:8: in <module>
    sns_subscription_attributes(),
aws/sns/resources.py:23: in sns_subscription_attributes
    for subscription in sns_subscriptions()
aws/sns/resources.py:23: in <listcomp>
    for subscription in sns_subscriptions()
aws/client.py:220: in get
    debug_cache=self.debug_cache,
aws/client.py:149: in get_aws_resource
    raise error
aws/client.py:145: in get_aws_resource
    result = full_results(client, call.method, call.args, call.kwargs)
aws/client.py:71: in full_results
    return getattr(client, method)(*args, **kwargs)
venv/lib/python3.7/site-packages/botocore/client.py:357: in _api_call
    return self._make_api_call(operation_name, kwargs)
venv/lib/python3.7/site-packages/botocore/client.py:661: in _make_api_call
    raise error_class(parsed_response, operation_name)
E   botocore.errorfactory.NotFoundException: An error occurred (NotFound) when calling the GetSubscriptionAttributes operation: Subscription does not exist
!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!!
=========================== 1 error in 15.39 seconds ===========================

[smarnach]

I'm sorry I didn't get to this today. I should be able to look into it tomorrow before you start your day.

[smarnach]

I can confirm your diagnosis here. When running this test locally, the first subscription in the list is in eu-central-1, but the later call to retrieve the attributes happens in us-east-1. From looking at the client code, I understand what the problem is – the client code iterates all regions for all requests, so we need to restrict it to the right region for these requests. This may involve adding some new methods for this use case. I've got some ideas, but I won't have time to implement them before leaving for the pony farm, but I can spend more time on this later today.

I dislike that we need to make so many API calls, but from a quick look at the API documentation, I can't see a way of retrieving the attributes in bulk. I'll further debug this once I'm back from the pony farm.

[g-k]

It's gross, but the way we get attributes for an existing resource and avoid 404s elsewhere is to pass the profile and region along from the previous list resource call e.g.

frost/aws/s3/resources.py

Lines 14 to 29 in fafff36

	def s3_buckets_cors_rules():
	"http://botocore.readthedocs.io/en/latest/reference/services/s3.html#S3.Client.get_bucket_cors"
	return [
	botocore_client.get(
	"s3",
	"get_bucket_cors",
	[],
	{"Bucket": bucket["Name"]},
	profiles=[bucket["__pytest_meta"]["profile"]],
	regions=[bucket["__pytest_meta"]["region"]],
	result_from_error=lambda error, call: {"CORSRules": None},
	)
	.extract_key("CORSRules")
	.values()[0]
	for bucket in s3_buckets()
	]

[Micheletto]

It's gross, but the way we get attributes for an existing resource and avoid 404s elsewhere is to pass the profile and region along from the previous list resource call e.g.

Ahh... I asked about that inarticulately in the chat channel.

can I just add this magic __pytest_meta region value?

Is there a long term plan to do this behind the scenes somewhere?

[g-k]

Ahh... I asked about that inarticulately in the chat channel.

Yeah, I realized that later. Completely forgot about this hack until I ran into for the desync test.

Is there a long term plan to do this behind the scenes somewhere?

#286 would remove the need for a profile option and I'm inclined to require one region too, since fetching resources for multiple profiles and regions is crazy slow and we haven't written cross-account or region tests AFAIK.

[smarnach]

I personally think the fix should be something else. We have two different kinds of requests here. One is "get all resources of this kind, across whatever dimensions like regions and profiles". The other one is "make a single boto API call". These simply shouldn't be calls to the same function. The latter may simply be a direct call to the boto API, without going through our client wrapper. I'll take a look at the code again now and prototype something for further discussion.

[g-k]

Yeah, I'd like to remove the existing clients & cache code and decouple it from the test logic. We could use boto3 if CloudOps is more familiar with that.

[Micheletto]

This latest version works. I incorporated the __pytest_meta magic suggested by @g-k and fixes from @smarnach's PR plus a .values()[0] slice as seen in the aws/rds/resources code.

I think, in this PR, we should stick to the current conventions. We may want to make client changes, but probably in a separate PR.

[smarnach]

Nice! I agree that the client changes should go in a different PR.

Running this on the dev account takes almost 10 minutes for me, but all tests pass:

====== 2731 passed in 564.59s (0:09:24) ======

The main problem is that we get rate limited when making so many API requests. There doesn't seem to be an alternative, though – if we want to implement this test, we need to make all these API calls, since we need one API call to retrieve the attributes for each subscription. There doesn't seem to be a way to retrieve them in bulk.

I ran black to reformat the code and run the actual tests. The tests on this branch pass, but the Travis tests on the merged branch fail. This is due to the changes required for the new pytest version. You will probably need to register the "sns" mark in aws/conftest.py, and use helpers.get_param_id() for the ids function to fix the errors and warnings.

[g-k]

Yeah, for perf and to reduce "time to check" for AWS it'd be nicer to run stuff in an aws config callback: https://docs.aws.amazon.com/config/latest/developerguide/evaluate-config_develop-rules_example-events.html

…

On Thu, Sep 17, 2020 at 4:43 AM Sven Marnach ***@***.***> wrote: Nice! I agree that the client changes should go in a different account. Running this on the dev account takes almost 10 minutes for me, but all tests pass: ====== 2731 passed in 564.59s (0:09:24) ====== The main problem is that we get rate limited when making so many API requests. There doesn't seem to be an alternative, though – if we want to implement this test, we need to make all these API calls, since we need one API call to retrieve the attributes for each subscription. There doesn't seem to be a way to retrieve them in bulk. I ran black to reformat the code and run the actual tests. The tests on this branch pass, but the Travis tests on the merged branch fail. This is due to the changes required for the new pytest version. You will probably need to register the "sns" mark in aws/conftest.py, and use helpers.get_param_id() for the ids function to fix the errors and warnings. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#334 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABXKLJTSNXWVOCZUABT4WTSGHD3VANCNFSM4RMC5BWQ> .

[ajvb]

LGTM!

@Micheletto is the capturing of the basic auth url no longer a concern?

[Micheletto]

@Micheletto is the capturing of the basic auth url no longer a concern?

It is being captured in subscription_attrs and passed to the test. We aren't specifically referring to it. Is it in danger of possibly showing up in a traceback? Could I nuke it with a .delete()? Most subscriptions aren't going to have the key.

[smarnach]

Is it in danger of possibly showing up in a traceback?

Not really. If we were sending errors to Sentry, it could theoretically show up, but I don't think we do that.

Could I nuke it with a .delete()? Most subscriptions aren't going to have the key.

You could – it's either del my_dict["key"] if you know the key exists or my_dict.pop("key") if you want to delete it regardless of whether the key exists. You'd need to change the list comprehension in sns_subscription_attributes() to an imperative for loop, store each dict in a variable, delete the offending key and .append() it to a results list. I don't think it's necessary to do that if it's not printed anywhere.