How to shoehorn Appmesh Virtual Gateway / Virtual Node into the CC resource pattern given it has no "id" and given that "arn" and "createdDate" are not top level fields.

[Johnlon]

I'm still having some problems fitting "appmesh" into CC.

I hope the following makes sense...

If we look at the QueryResourceManager class it says ...

":param id: Names the field in the enum_spec response that contains the identifier to use"

The doco is telling us that the intention the id field "is identifier to use" on other calls to the same api such as the describe functions.

However, the single "id" thing doesn't make sense in the context of Appmesh Virtual Gateway where the api's calls take both meshName and virtualGateway name to drive them.

Other CC resource types seem to get away with a single id field that can be used but not the appmesh types.

So what do I put in the id field for Appmesh VirtualGateway?

In my current impl I notice that I've put the value "arn" in the 'id' field but that's rubbish as there is no "arn" at the top level of the resource description - obviously the fact I've put a junk value in the 'id' hasn't hurt anything visible yet in my tests and I thought my test cases are pretty decent. (I've since add a test case for reporting that shows up the problems).

The arn is actually at this path .. "metadata.arn"

But in anycase putting an "arn" in the id field doesn't conform the doco "names the field in the enum_spec respons" so what to I do?
Forget the 'id' field and leave some junk in there?

Anyway I wrote a further test case around reporting because whilst a junk value in "id" doesn't harm the detection functionality (my tests pass) I found that that bogus "id" does show up in the report output.

I tried the "metadata.arn" as the Id and the dot doesn't break cloud custodian and if I do this then the ARN does show up in the report view and unsurprisingly the header values are ['metadata.arn', 'virtualGatewayName'] .

But the only reason the dot isn't blowing up in CC is because in my extension I've overridden the get_arns function that would otherwise have fallen over here '_id = r[id_key]' . I a previous PR I changed this to permit a path but that was reverted by the reviewer but I think that flexibility is warranted as it seems an unecessary obstacle not allowing a path. There is a small change to make this processing flexible to paths.

In any case I have overrided get_arns and so with that setting then the arn shows up in the report.
BUT that is not a valid configuration according to the "id" documentation.

? So what should the documentation say to improve upon this situation as it's obviously a little more complicated than the one liner thats there at the moment?
? What should I put in the "id" field for a type that doesn't have a singleton id field (ie virtual gateway)?

---

So I wrote a test for the report to expose this problem and when I did that I discovered a bug that hadnn't shown up before. I wonder is it only my extension that benefits from a reporting test case?

This time the bug is due to the restrictions on the "date" field value.
In appmesh the createdDate valuie is at "metadata.createdDate" and ... you guessed it ... I can't have a path.

So I have to leave date=None or CC blows up.

?can we change date to allow a path?

So to summarise.

• What do I put in the "id" field for virtualGateway?
  There isn't an option thrat I can see that actually conforms to the field doco for "id".
  • if it is going to be the ARN, then we need to figure out an amendment to the doco for the id field to explain this exception
  • but if it's the ARN then the id field needs to accept a path without blowing up if I've not overriden get_arns()
    also ..
• can we change "date" to a path so I can get my createdDate into the report?

---

BTW I scanned the code base for another resource that might be a bit like AppMesh virtual gateway and came up empty handed. Is there anything similar?

---

Also I'd like to come back to the question of allowing dots in the id field.

As far as I can see all that's needed is to change get_arns() to be tolerant ...
Existing impl ... https://github.com/cloud-custodian/cloud-custodian/blob/main/c7n/query.py#L617

M proposal .... just call 'jmespath_search' to get the id (but only if needed) ...

    def get_arns(self, resources):
        arns = []

        m = self.get_model()
        arn_key = getattr(m, 'arn', None)
        if arn_key is False:
            raise ValueError("%s do not have arns" % self.type)

        for r in resources:
            if arn_key:
                arns.append(r[arn_key])
            else:
                id_key = m.id
                # get the id field path efficiently - only use a search if necessary
                if '.' in id_key:
                    id_value = jmespath_search(id_key, r)
                else:
                    id_value = r[id_key]

                if 'arn' in id_value[:3]:
                    arns.append(id_value)
                else:
                    arns.append(self.generate_arn(id_value))
        return arns

This change has the added benefit that if I DO set my id to be the arn value that I no longer need to override the get_arns fucntion in my extension....
See .. https://github.com/cloud-custodian/cloud-custodian/blob/main/c7n/resources/appmesh.py#L218

I think that all around it's just a better approach to permit a path in the id field.

But it still doesn't answer what the documentation for "id" ought to say in query.py

---

To be honest - the arn key in get_arns ALSO ought to be a path imho with the same efficiency guard.
As I've said the arn in Appmesh is "metadata.arn" and at present I have not defined the ARN.
But as we've seen whilst filtering works this kinf of limitaiton leads to inconsistencies, and extra code I hare to write, and reporting issues.

I don't see why all field refs cannot just allow paths - the whole thing will be more consistent and predictable - a good outcome IMHO.

---

Lots of stuff to discuss!
Any takers.

Please concentrate mainly on the what do I do about appmesh however as it's a square peg in a round hold trying to fit it into CC.

[Johnlon]

Impl of this in #9290

[ajkerrigan]

More comments on the PR, thanks again for working on this and thinking about the bigger picture in the process ❤️ .

I don't see why all field refs cannot just allow paths - the whole thing will be more consistent and predictable - a good outcome IMHO.

Agreed, I can see adding the possibility of jmespath lookups anywhere we might have an expression instead of a simple field name. Using jmespath_search() across the board and optimizing away unnecessary searches seems clean, helpful and predictable.

That said, I do think if we have a property in tens/hundreds of resource types and it's always been a top-level field, it's not really surprising that there was no prior effort to make it an expression.

Please concentrate mainly on the what do I do about appmesh however as it's a square peg in a round hold trying to fit it into CC.

If we really want to keep it focused on appmesh, avoiding the higher level changes and overriding get_arns() to get your ARN seems reasonable. The higher level changes around documentation, testing, ARN searching, etc are thought-provoking and welcome, but that does necessarily make the discussion less focused.

[Johnlon]

Hi the latest pr has the changes I needed to get appmesh in without changing the data model to fit cc. A lose end of "Id" which for child resources doesn't make sense as a single value for appmesh and so I stuffed the child arn into that field even tho the APIs won't ever reference it.

A more general approach to id that is compatible with appmesh would be for the id field to be a list of the field names that make up the id.

I guess the further generaliations would be where ever the id maps to eg enum/describe or whatever because it's not a single API param but multiple in the same order as the fields in the id.

I think a generalisation to cover all types is possible but obviously its work.

In the meantime as long as my pr is acceptable then I'm stuck with the id field doco saying it's a field that provides a value that will be pushed into API calls ,but that's not accurate for appmesh. Is there any language that could be added to make it clearer?

I'm not expert enough to know the right words.

---

Also from.my experiences of testing, I think a more extensive guided is warranted.

Oh and take a look in my pr at the ApiCallCaptor and how I use it to complement the placebo so that I am sure the right calls are being made. Sadly placebo has no such strength tho in their forum that have discussed a lot without outcomes.