First cut of adding support for appmesh #9260
Conversation
c7n/resources/appmesh.py
Outdated
| # skip the enum call entirely. | ||
| # | ||
| # TODO: Propose an improvement where we allow skipping of the | ||
| # enum_spec call self.source.get_resources(ids) |
There was a problem hiding this comment.
a source subclass is free to do that, normally this is not in a query manager, but a describe source subclass, you can grep DescribeSource for extant examples .
There was a problem hiding this comment.
I removed my logic as it meant the pull and event models differred.
|
Thanks for the pr. there's a couple of failing tests, since config is supported for app mesh the overrides on resources/get_resources need to go into a describe source subclass, also since supported by aws config, cfn_type should be defined in resource_type. it also appears some of the arn_types are invalid, fwiw, we test this against the aws iam documentation re valid arn types. you can run tests locally w/ make test and lint w/ make lint |
|
looking at the arn type issue, the arn_type per custodian is always mesh, but their is a subtype per child, per resource arns in the iam docs https://docs.aws.amazon.com/service-authorization/latest/reference/list_awsappmesh.html#awsappmesh-resources-for-iam-policies as a result you'll likely need to override get_arns in the child resources. |
|
UPDATE : I've given is a crack fixing the tests - please take look at my commits hi @kapilt I can see test_config_resource_support has failed however there isn't any documentation or other commentary in the test to explain it's objectives. What is the test attempting to verify. I can make the test pass by removing stuff from 'whitelist', but I don't know what I'm doing. Also what is the meaning of this comment in the same test... And this comment and the others like it ... |
|
I can't run all the test as the moment as they blow up in random places when I run "make test". I don't know why my win11 build blows up in some random tests but I'll look at the windows server build below. |
Hi I see "likely" - is there some doubt about needing to implement? Also is there some API documentation and hopefully some specific acceptance test for my classes (or specifically get_arns) to tdd my solution? Apologies but I can run some of the tests locally but not the full "make" set. I can show you a log of what I see if needed. UPDATE: I found the windows build. Sorry the GitHub UI has a scroll bar below that I hadn't noticed that hides the windows stuff. I will look. FURTHER UPDATE: I changed core query.py to allow "arn" and "id" fields to be paths so that I didn't need to implement get_arns() - please check my logic - I couldn't find tests for query.py to edit - please let me know. Thanks |
…on value not just a field name. ... to allow .... - Changed appmesh virtual gateway "arn" attribute to be a path to the necessary attribute Also ... - Removed my override of get_resources because it resulted in the models in pull and in event modes to be different meaning policies would not be portable. - Resolved some failing tests.
c7n/query.py
Outdated
| @@ -625,9 +635,9 @@ def get_arns(self, resources): | |||
| id_key = m.id | |||
|
|
|||
| for r in resources: | |||
| _id = r[id_key] | |||
| _id = jmespath_search(id_key, r) | |||
There was a problem hiding this comment.
preference would be to to implement the get_arns method in the subclass versus putting it on the base, as jmespath execution is actually relatively expensive on cost, versus a key lookup for a high cardinality resources. ie only pay the cost when its actually needed by putting the logic specific to the resource types that need it.
| id = name = 'meshName' | ||
|
|
||
| # if a resource type is supported by resource group tagging api setting this value get tag filters/actions | ||
| universal_taggable = object() |
There was a problem hiding this comment.
fwiw added this to get default tag filters/actions
| class DescribeMesh(DescribeSource): | ||
| # override default describe augment to get tags | ||
| def augment(self, resources): | ||
| return universal_augment(self.manager, resources) |
There was a problem hiding this comment.
custodian defaults to fetching tags for resources, this describe source customization ensure that for this resource type.
c7n/resources/appmesh.py
Outdated
|
|
||
| class DescribeGatewayDefinition(ChildDescribeSource): | ||
| def __init__(self, manager): | ||
| super().__init__(manager) |
There was a problem hiding this comment.
fwiw, override isn't needed, describe virtual gateway includes the mesh name / parent id.
|
@kapilt Hi I found that the developer instructions here https://cloudcustodian.io/docs/developer/installing.html#developer-installing Work fine on a Linux but the Makefile doesn't work on Windows as it seems to contain a bunch of non-portable stuff. Are there some instructions for building it and running all the tests for Windows? Thanks |
|
Wow thanks How do we get it into next build? |
|
Like I said I have a few more to do and need a bit more understanding on the changes you made and in fact what some of it means. Btw have some more doco changes I want to record in appmesh before pull |
|
Pushed some.commets |
|
Btw perhaps make a note that python 12 has a problem that prevents it being used in debugger at least on window. I used 3.11. |
|
Windows needs things like ms ask too |
|
Hi I need help writing proper tests for this extension. I suspect part (not all) of the problem is that the placebo library is deficient in that the test library makes no attempt to match the API params to the test data files. It just dumbly replays the files. There isn't even a good way to be absolutely sure the API isn't making excessive calls because it round robins the files. There is no way to be strict in placebo. To be honest placebo is pretty suboptimal compared to proper mocking of responses Vs request params. Any thoughts? There are probably other things I'm not understanding that would help me thoroughly functional test all the params on the extension. Any ideas? At the moment the coverage is clearly very gappy. |
|
@kapilt Hi need so help if you could. I haver this branch of appmesh where you can see I have corrupted several of the field with a "!!!" in the values... I did this because I was writing much more comprehensive tests include around the API calls made and I want to be sure that every cfg value has purpose and is referenced by a test or influences the outcome of a test, What tests if any should be emplyed to demonstrate something that relies on these values, as the fields seem completely redundant right now? Please also take alook at test_appmesh.py for some clues on the extended testing. |
Added basic appmesh support.
Also incidentally attempts to provide some useful documentation in the code on how to use the API's and write such an extension.
Perhaps in a later PR this could be written up as a primer or something.