appmesh - added many tests - fixed big appmesh bug - extended CC to allow appmesh model to fit

[Johnlon]

Update: my tests and class doco are sorted now

Moved some of my doco into query.py

Good to go.

#9307

[ajkerrigan]

There are some good questions in the comments here - although any helpful/clarifying bits might make more sense on the parent TypeInfo docstring rather than defined for appmesh specifically.

I dig the idea of trying to make tests more useful, though for some of the points we're using SDK info / shape details to do best effort validation rather than making everything a functional test. As far as I know that's a conscious design decision.

[ajkerrigan]

arn_type helps custodian build an ARN when the list/describe response doesn't provide one directly. Since we get this resource back in the arn property of a list response and you've already defined arn = "arn", you can remove arn_type.

[Johnlon]

Ack - I've added a comment.
The other fields in QueryResourceManager.generate_arn are probably the same , apart maybe from ID.

[ajkerrigan]

These types are used in a few places: (config compliance filters, config execution modes, the awscc provider...)

If there is Config/CloudFormation support for a resource type, these values are usually the same (which is why you often see patterns like cfn_type = config_type = 'AWS::AppMesh::Mesh'. There are exceptions like ACM certs, so it's still worth having separate types.

Custodian relies on shape data from the SDK and periodically updated value lists to validate these type properties, rather than adding functional tests for every resource type.

[Johnlon]

Yeah the PolicyMetaLint does check that the config_type value is know too AWS.

Documentation error in the parent class...
The existing doc comments say these are both optional but actually 'config_type' is mandatory

If cfn_type is optional then under what circumstances must it be provided?

Thanks

[ajkerrigan]

If CloudFormation supports a resource, we should record that in cfn_type. If that resource has explicit Config support, we should record that in config_type.

Custodian's Config filters/modes look for a config_type first, and can fall back to cfn_type.

cfn_type pops up in a couple extra spots though. A non-exhaustive list there would be the awscc provider and iam-analyzer filter.

I wouldn't expect the TypeInfo comments to always contains a complete list of where those properties are used, though examples are always neat. It's pretty common that Config or CloudFormation add support well after Custodian supports a resource type, so adding config_type or cfn_type down the road is fine too.`

[Johnlon]

Understood.

[ajkerrigan]

get_arns() pops up in a few places (like building a set of resource ARNs for bulk tagging operations).

That method tries to get an ARN for a resource type somehow, which means:

• Do we already have an arn from the list/describe response? Cool, use that
• Do we have an arn_type defined? Cool, we can use that to build an ARN from the parts we have

[Johnlon]

for AppmeshVirtualGateway this wont work anyway as "arn" cannot be a path (no dots) and the ARN isn't top level in the datastruture.

[kapilt]

overriding get_arns means the metadata for arn isn't used.

[Johnlon]

yep - got that one thanks

[Johnlon]

There are some good questions in the comments here - although any helpful/clarifying bits might make more sense on the parent TypeInfo docstring rather than defined for appmesh specifically.

I dig the idea of trying to make tests more useful, though for some of the points we're using SDK info / shape details to do best effort validation rather than making everything a functional test. As far as I know that's a conscious design decision.

Agree the comments should be moved - I'm just aggregating them here to sort them out.

DONE - most of the comments are not on in the parent class.

[Johnlon]

See also https://github.com/orgs/cloud-custodian/discussions/9306

[ajkerrigan]

Thanks for working on this @Johnlon . Apologies for the delays reviewing, going to have to just chip in with partial comments here because there are a lot of different things going on here.

(Also I know you've asked multiple times to keep things focused on appmesh specifically, but it's hard to do that given the proposed changes.)

[ajkerrigan]

It's a convenient side effect of @thisisshi 's work in #8533 that we call custodian's own jmespath_search() rather than directly using jmespath.search(). So if we're going to optimize away unnecessary calls, we handle that inside jmespath_search() which is already the most common pattern for extracting things that might be JMESPath expressions or simple top-level field keys.

(That could definitely be handled in a separate PR if we want to keep this one more focused, but this one is already operating at a pretty high/broad level in many of its changes)

[Johnlon]

Hi I only did this because I a previous commit I was told that the Search was expensive. Welcome better alternative that allows a path.

[Johnlon]

Yeah thanks. Re the request for specificity. The earlier commit didnt include the core changes I made in the final commit. So there's a leap there. Apols

[Johnlon]

@ajkerrigan should switch to jmespath_search() and optimise it?

[Johnlon]

@ajkerrigan - hi do you want me to do something with jmespath_search as you suggested?
Thanks John

[ajkerrigan]

No, thanks

[Johnlon]

I took a look at trying what you suggested however I am doubtful about the approach an my ability to make it float.

I thought this naive approach would work ....

def jmespath_search(*args, **kwargs):
    # DROP OUT IF ITS A FIELD 
    path = args[0]
    resource = args[1]
    if '.' not in path:
        return resource[path]

   # Otherwise it's a path...
   return jmespath.search(
           *args,
           **kwargs,
           options=jmespath.Options(custom_functions=C7NJmespathFunctions())
    )

But, the 3rd options passed to the jmespath.search mean my shortcut might interfer with existing usage if people are already passing simple fields to it.

Also I cam across cases where there were more than 2 args being passed to the wrapper "jmespath_search" meaning again I am uncertain whether my shortcut interfers with existing usage.

Any ideas? (BTW I am not a python programmer and not even sure how *args and *kwargs actually work or why one would need them)

[ajkerrigan]

I dig the idea of avoiding unnecessary searches, even if I'm not sure how much time we actually spend there (probably worth some profiling before we just assume savings).

That said, it's probably safer to use jmespath_compile() (caching the results where we can), and use a direct resource.get(key) if the resulting expression has type == 'field'. For example:

jmespath_compile("ResourceId")
{'type': 'field', 'children': [], 'value': 'ResourceId'}

jmespath_compile("&ResourceId")
{'type': 'expref', 'children': [{'type': 'field', 'children': [], 'value': 'ResourceId'}]}

jmespath_compile("ResourceIds[0]")
{'type': 'index_expression', 'children': [{'type': 'field', 'children': [], 'value': 'ResourceIds'}, {'type': 'index', 'value': 0, 'children': []}]}

jmespath_compile("split(',', Resources)")
{'type': 'function_expression', 'children': [{'type': 'literal', 'value': ',', 'children': []}, {'type': 'field', 'children': [], 'value': 'Resources'}], 'value': 'split'}

jmespath_compile("Resource.Id")
{'type': 'subexpression', 'children': [{'type': 'field', 'children': [], 'value': 'Resource'}, {'type': 'field', 'children': [], 'value': 'Id'}]}

And again, it seems helpful to generally treat this case as a "maybe-JMESPath-expression" and handle that at a higher level than this ARN key lookup specifically.

[Johnlon]

@ajkerrigan sorry wasn't sure if you want me to make this optimisation or not?
RSVP thanks

[Johnlon]

TBH Thiis is beyond my python skills :(

[Johnlon]

Would it be permissible to have the original code I suggested since it doesn't raise as many questions as I don't think I'm up to the reorg you are suggesting.

[ajkerrigan]

Addressed in more depth later, but it seems totally reasonable to have this represent a JMESPath expression, and then optimize away unnecessary jmespath.search() calls as we like within custodian's derived jmespath_search() method.

[Johnlon]

Thanks - that helps

[ajkerrigan]

Bit of a judgement call here, but some of this feels like it goes beyond explaining the parameters and into "recommendations for creating new resources". This extra level of detail feels more appropriate in something like the Adding a New Resource documentation that is focused on helping contributors.

[Johnlon]

The Intention is to verify the API calls that my config and code is making cc perform, and. Not testing boto. Adding this allows.me to be sure that my apps behave as Intended in a way that placebo does not. For example placebo alone is incapable of doing verification of the values passed to the APIs or even the number of calls made really. This is simply a better mock of the boto API to confirm behaviour

[Johnlon]

@ajkerrigan I understand your point about moving the comment to the "new resource" content . I was just concerned about it getting separated from the related comments in the preceding paragraph.

Thoughts?

[ajkerrigan]

This is a cool use of botocore's history recording 👍 . I do want to be a bit careful though, since tests have both a value and a cost, and I don't want to get too hung up on testing botocore's underlying activity. It seems very easy to end up with a glut of tests at a finer grained level of detail than we want to maintain for the value we get out of them.

[Johnlon]

See previous comment re this being a test of my code and cc config rather than the APIs

I found this really useful and found some unexpected issues. I think it's therefore a useful approach that authors can choose to adopt or ignore. I can move the ApiCallCaptor back into my appmesh test but I can some other services n the pipeline and I'll want to use it there too.

[ajkerrigan]

I like this level of detail from an observer standpoint, but I'm not sure what it functionally buys us to assert this during every test run. if we've already asserted that the shape/content of the policy results match expectations, do we need to also tick off each botocore call that got us there?

(This is a question not just for you, but for other maintainers. I can honestly see it from both sides, it's just a tradeoff.)

[Johnlon]

It found bugs in my code even though I had other tests. Defo a best practice imho.of course it's up to the committer I guess to.decide.the.level.of. Confidencd they want about their implementations behaviour

--

You can tell that I am big on testing. Those who know me will know my mantra that if there isn't a failable test then it's best to assume that the test subject is either broken or doesn't even exist. I can't work any other way and feel responsible :)

[Johnlon]

Hi @ajkerrigan any news on the review please.
Appreciated John

[ajkerrigan]

Thanks for the updates 👍

[Johnlon]

@ajkerrigan thanks for the help.

Please also take a look at this ..
https://github.com/Johnlon/cloud-custodian/blob/main/docs/johns-unofficial-dummys-guide.md

This is for my own benefit but also anyone else.
I'm not sure it's appropriate to commit it to CC as I also don't want to change it away from the current format which suits my purposes right now. But let me know your thougths.
I had it in a diffferent project up until now.

John

[ajkerrigan]

@ajkerrigan thanks for the help.

Please also take a look at this .. https://github.com/Johnlon/cloud-custodian/blob/main/docs/johns-unofficial-dummys-guide.md

This is for my own benefit but also anyone else. I'm not sure it's appropriate to commit it to CC as I also don't want to change it away from the current format which suits my purposes right now. But let me know your thougths. I had it in a diffferent project up until now.

John

Thanks for the write-up. It's helpful to have an opinionated and detailed run through a bunch of contribution steps 👍 . I'm not sure we'd want to embed that whole thing in the docs, but perhaps could link to it as an external reference if you had it in a post somewhere?

I think the ApiCallCaptor will be very handy in certain spots. I'm looking forward to playing with it locally myself next time I'm deep in the botocore weeds, to see how it feels in practice compared to other patterns of inspecting call patterns. But I also want to be cautious about overusing that. As a concrete example, your example voice connector resource needs to list / get / fetch tags on the way to building a full Custodian resource. Rather than verifying all of the botocore calling patterns on the way, I would rather see folks assert that the expected properties exist on the final resource. If they don't, then by all means dig into the calls to find out why. It's neat to have a few tests showcasing that level of tracking, and validating that abstractions like universal tagging generally behave as expected. But doing it by default for every new resource feels like it carries more noise than signal.

[Johnlon]

Dummys guide moved here : https://gist.github.com/Johnlon/98e1e4b9aaedfedead428c89e7dab5bb

[Johnlon]

Hi thanks. I will find a home for it.
Re the tagging test , the way I look at it is if there isn't a test then either it's broke or doesn't exist. A psychology stemming from early years of being optimistic and regretting it. I find that strong standardized patterns lead to easier to navigate , understand, trust , maintain code.