Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AWS SigV4 signing for Apollo Connectors #6144

Merged
merged 10 commits into from
Oct 28, 2024

Conversation

pubmodmatt
Copy link

@pubmodmatt pubmodmatt commented Oct 11, 2024

Allow AWS SigV4 signing for Apollo Connectors requests. This allows connectors to invoke any AWS service through its HTTP API. Signing options can be configured per Connectors source.

Example configuration:

authentication:
  subgraph:
    ... # Subgraph authentication configuration
  connector: # Peer of subgraph
    sources:
      aws.lambda:
        aws_sig_v4:
          default_chain:
            profile_name: "default"
            region: "us-east-1"
            service_name: "lambda"
            assume_role:
              role_arn: "arn:aws:iam::XXXXXXXXXXXX:role/executelambda"
              session_name: "connector"

Other changes:

  • Fix a bug where JSON Selection does not accept the $context variable in apply_with_vars

Note that tests are currently failing due to an unrelated dependency issue.


Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

  • Changes are compatible1
  • Documentation2 completed
  • Performance impact assessed and acceptable
  • Tests added and passing3
    • Unit Tests
    • Integration Tests
    • Manual Tests

Exceptions

Notes

Footnotes

  1. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this.

  2. Configuration is an important part of many changes. Where applicable please try to document configuration examples.

  3. Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions.

@pubmodmatt pubmodmatt self-assigned this Oct 11, 2024
@pubmodmatt pubmodmatt requested review from nicholascioli and a team as code owners October 11, 2024 21:58
@svc-apollo-docs
Copy link
Collaborator

svc-apollo-docs commented Oct 11, 2024

✅ Docs Preview Ready

No new or changed pages found.

@Geal
Copy link
Contributor

Geal commented Oct 14, 2024

why is this done under the connectors configuration and not under the authentication configuration? We can't duplicate the entire configuration under connectors, that will make a confusing UX.

IIRC one of the issues here is that the subgraph name is generated from the source name and other parts from the directive, and that won't make sense for users. So here's a thing that could be done here: extend the SubgraphConfiguration type, that's used to handle configuration overrides per subgraph (there's a lot of unintuitive edge cases to do that properly, so that should not be rewritten from scratch for connectors). A good way to do it would be to use a wrapper type that adds the sources field and adds a #[serde(flatten)] on the SubgraphConfiguration field

// In various parts of the configuration, we need to provide a global configuration for subgraphs,
// with a per subgraph override. This cannot be handled easily with `Default` implementations,
// because to work in an intuitive way, overriding configuration should work per field, not on the
// entire structure.
//
// As an example, let's say we have this subgraph plugin configuration:
//
// ```rust
// use serde::Deserialize;
// use serde::Serialize;
// use schemars::JsonSchema;
//
// #[derive(Debug, Clone, Default, Deserialize, Serialize, JsonSchema)]
// struct PluginConfig {
// #[serde(default = "set_true")]
// a: bool,
// #[serde(default)]
// b: u8,
// }
// ```
//
// If we have this configuration, we expect that all subgraph would have `a = false`, except for the
// "products" subgraph. All subgraphs would have `b = 0`, the default.
// ```yaml
// subgraph:
// all:
// a: false
// subgraphs:
// products:
// a: true
// ```
//
// But now, if we get this configuration:
//
// ```yaml
// subgraph:
// all:
// a: false
// subgraphs:
// products:
// b: 1
// ```
//
// We would expect that:
// - for all subgraphs, `a = false`
// - for all subgraphs, `b = 0`
// - for the "products" subgraph, `b = 1`
//
// Unfortunately, if we used `Default` implementation, we would end up with `a = true` for the
// "products" subgraph.
//
// Another way to handle it is to use `Option` for every field, then handle override when requesting them,
// but this ends up with a configuration schema that does not contain the default values.
//
// This `SubgraphConfiguration` type handles overrides through a custom deserializer that works in three steps:
// - deserialize `all` and `subgraphs` fields to `serde_yaml::Mapping`
// - for each specific subgraph configuration, start from the `all` configuration (or default implementation),
// and replace the overriden fields
// - deserialize to the plugin configuration
/// Configuration options pertaining to the subgraph server component.
#[derive(Default, Serialize, JsonSchema)]
pub(crate) struct SubgraphConfiguration<T>
where
T: Default + Serialize + JsonSchema,
{
/// options applying to all subgraphs
#[serde(default)]
pub(crate) all: T,
/// per subgraph options
#[serde(default)]
pub(crate) subgraphs: HashMap<String, T>,
}

That type could then be used to gradually enable plugins to support connectors

@pubmodmatt
Copy link
Author

pubmodmatt commented Oct 14, 2024

why is this done under the connectors configuration and not under the authentication configuration?

@Geal - all of the connectors settings so far are under preview_connectors currently, including per-source settings. Since we're now at public preview and moving to GA, it makes sense to move these settings into their integration points. There are some open PRs for things like telemetry that already do this. I'll look at moving this into authorization.

The internal subgraph name is a temporary artifact, and not exposed to customers. The current per-source settings, including the ones in this PR, go under the actual name of the subgraph containing the Connector. So rather than a wrapper type, I think sources would just go directly under SubgraphConfiguration.

However, I'm not sure sources alone is enough to convey that these settings are related to connectors, if it moves out from under preview_connectors. It may make more sense under a connector subheading.

Presumably any Connectors settings under subgraph -> all would apply to any sources with the same name in multiple subgraphs. That might be useful if the same REST API were used in multiple subgraphs, but the sources would need to have the same name across all subgraphs.

Another issue this raises is whether a plugin would want to have settings that apply across all connectors, which would require duplicating the pattern for subgraph -> all, subgraphs at the connector level, including the custom serialization. This then creates a combinatorial case which may or may not be intuitive:

  • subgraph -> all -> connector -> all applies to all connectors in all subgraphs
  • subgraph -> subgraphs -> subgraph_name -> connector -> all applies to all connectors in the named subgraph
  • subgraph -> all -> connector -> connectors -> connector_source_name applies to any connector with that name in all subgraphs
  • subgraph -> subgraphs -> subgraph_name -> connector -> connectors -> connector_source_name applies a single connector source in a single subgraph

@pubmodmatt
Copy link
Author

why is this done under the connectors configuration and not under the authentication configuration?

We had some discussion around this, and have a design where the connector settings are at the same level as subgraph, and do not implement an all mechanism. So settings must be provided for an individual connector source, as is coded up in the latest commits here.

@pubmodmatt pubmodmatt merged commit a066d0a into next Oct 28, 2024
12 of 13 checks passed
@pubmodmatt pubmodmatt deleted the pubmodmatt/connectors/aws_sigv4 branch October 28, 2024 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants