Make server's AWS node attestor plugin subsume AWS node resolver plugin

[martincapello]

Pull Request check list

• Commit conforms to CONTRIBUTING.md?
• Proper tests/regressions included?
• Documentation updated?

Affected functionality
SPIRE Server's AWS node attestor and node resolver plugins.

Description of change
1) Refactors the code of the aforementioned plugins moving common functionality to the pkg/common/plugin/aws.
2) Adds agent's selectors resolution to the AWS node attestor plugin.
3) Deprecates the AWS node resolver plugin by making it return an empty response and log a warning when configured.
4) Re-refactors to move code that wasn't common any longer.

Which issue this PR fixes
Fixes #683 (comment)

[amartinezfayo]

Thanks @martincapello for this, it is looking great!
I think that we need to update https://github.com/spiffe/spire/blob/master/doc/plugin_server_nodeattestor_aws_iid.md to reflect that the node attestor now discovers the selectors.

[azdagron]

With the attestor plugin now returning the selectors also provided by the resolver, I wonder if the node resolver should be deprecated. We could do this in a few ways:

1. Gut the resolver functionality and emit a warning (when configured) that it is deprecated and will be removed in the future.
2. Keep the resolver intact but emit a warning (when configured) that it is deprecated and will be removed in the future.

[martincapello]

With the attestor plugin now returning the selectors also provided by the resolver, I wonder if the node resolver should be deprecated. We could do this in a few ways:

1. Gut the resolver functionality and emit a warning (when configured) that it is deprecated and will be removed in the future.
2. Keep the resolver intact but emit a warning (when configured) that it is deprecated and will be removed in the future.

Happy to make any change needed once the code owners give me green light.

[amartinezfayo]

With the attestor plugin now returning the selectors also provided by the resolver, I wonder if the node resolver should be deprecated. We could do this in a few ways:

1. Gut the resolver functionality and emit a warning (when configured) that it is deprecated and will be removed in the future.
2. Keep the resolver intact but emit a warning (when configured) that it is deprecated and will be removed in the future.

@azdagron I'm not sure if I understand the semantics of 1. Specifically I'm not sure what "gut the resolver functionality" means in this context. Just wanted to make sure that we don't brake any compatibility guarantees.
In any case I think that it's a good idea to start the work to deprecate the resolver in the future. Having this kind of overlapping functionality doesn't add any value that I can think of, and only contributes to confuse operators.

[azdagron]

The node resolver is more or less tightly coupled with the node attestor. Meaning that if aws_iid type is specified in the attestation data, the attest handler looks up both the node attestor and node resolver with that name. This means that if somebody has both the attestor and resolver configured, that the attestor is now going to do all the same work that the node resolver is doing, and they are going to return duplicate selectors.

When I said "gut the resolver functionality", I meant to remove all of the internals of the resolver, essentially turning it into a no-op. The Configure RPC would do nothing other than log a warning that it is deprecated. The Resolve RPC would return an empty response. This doesn't seem to me that it would break compatibility guarantees unless somebody wrote their own external aws_iid node attestor that they were using the built-in node resolver. I think the risk here is small.

However, if we don't want to risk it, we can keep the resolver as-is and just make sure that the attest code dedups selectors (I don't think it does right now). It does mean that we're now doing 2x the API calls against AWS, which may impact cost (I don't recall how AWS does pricing here). We'd still want to log a warning saying that the resolver will be removed in the future.

[amartinezfayo]

Thanks for the detailed explanation. IMO #1 provides a cleaner solution and I do agree that chances to brake an external aws_iid node attestor that is using the built-in node resolver are really small. Although preserving compatibility in such cases would be desirable, I believe that we don't really commit to guarantee that since we explicitly say that SPIRE cannot make any guarantees around configuration or behavior compatibility for external plugins (https://github.com/spiffe/spire/blob/master/doc/upgrading.md#configuration-and-behavior-compatibility)

[martincapello]

Ok then, can I go ahead and deprecate the node resolver as proposed in the # 1 alternative?

[APTy]

+1 to removing the aws node resolver behavior but logging a warning to users.

[martincapello]

I'll start working on the changes.

[martincapello]

Note that I refactored the code again because there was code that wasn't common any longer. This also allowed to reduce the amount of exported things.

[azdagron]

Looking pretty good. Are there more test cases we can remove (or move over to the attestor tests) from the resolver tests? I noticed there are some Configure tests that are no longer relevant.

[azdagron]

This call may have already happened during the Attest call (albeit with different filters). I think we should refactor the code to always call DescribeInstancesWithContext during the Attest call (instead of conditionally), using these filters, since we're going to be need it to build the selector list anyway.

[martincapello]

Forgot to remove node resolver tests, doing it now...

[azdagron]

I've manually verified this all still works in AWS. I'm good with this change.

Thanks again, @martincapello !