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

RFC: 18 Open context provider framework #167

Closed
wants to merge 1 commit into from
Closed

Conversation

ddneilson
Copy link

@ddneilson ddneilson commented May 27, 2020


title: RFC: #18 Open context provider framework
labels: management/rfc


Rendered version


By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache-2.0 license

@mergify
Copy link
Contributor

mergify bot commented May 27, 2020

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@ddneilson ddneilson mentioned this pull request Jun 3, 2020
7 tasks
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea, but I'd like to see some more detail for the end-to-end story of this feature: the library/provider author and the end-user of the provider construct.

I'd also like to see some concrete use cases you would want to use this feature for.


Instead 3rd party libraries are required to implement workarounds, like CfnCustomResources that resolve during deployment instead of during
synthesis. This increases the size of the Cfn template generated by the app, increases the time required to deploy the stack, adds unnecessary
extra resources to the user's account, and makes it possible for a value generated by the CfnCustomResource to differ from one deployment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. But on the other hand, that's kinda the point of a custom resource. They fill similar, but not THE SAME needs as a context provider. This section is making it sound like context providers are strictly superior to custom resources, which I would argue they are not.

```ts
// Allow the provider to be an arbitrary string
const someValues: thirdpartlib.DoSomethingContextReponse = ContextProvider.getValue(scope, {
provider: 'thidparty-do-something',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought here was an even MORE open-ended system. How about we do the following instead:

provider: 'npm:my-provider-package@3.1.2'

The CLI can then go and fetch that package (version number required!) and call the context provider using a well-defined protocol: potentially the default export of the module, or a function with a well-known name in there, and some agreed-upon arguments.

This seems like the most open-ended system and has the advantage that the user doesn't ultimately have to go cdk --plugin.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, what about an environment where npmjs.com might not be available? (wink wink nudge nudge)

I guess we need a way to tell cdk what npm install command to use, so we can subsitute it in select environments?

(Also note that we can't just rely on your project's package.lock to bring in the plugin, as not everyone will be running an NPM project. Java/Python/C# users will have a global install of the cdk)


```ts
// Allow the provider to be an arbitrary string
const someValues: thirdpartlib.DoSomethingContextReponse = ContextProvider.getValue(scope, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would want more protection around this than simply using ContextProvider.getValue(). At least there will need to be an agreement of a schema and checking on it (potentially using jsonschema, or maybe something simpler that doesn't bring too many dependencies).

If we're going to make an open-ended system, we want clear error detection and reporting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I think I would never require users to call ContextProvider.getValue(). Maybe construct library authors can, but users should have a stronger-typed interface on top.

They would be calling ThirdPartyProvider.fromLookup() or something.


# Drawbacks

This change would alter the internal API in that some fields that are currently defined as `cxschema.ContextProvider` would need to change to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a better way is to add an enum value like cxschema.ContextProvider.CUSTOM and put the actual provider in a separate field (property). That way it's very clear what's going on.


# Unresolved questions

* Is there a precidence for coupling a minumum version of the CDK CLI with the a minimum version of CDK construct library?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, we do this all the time. Our stated compatibility rules at the moment are CLI >= framework.

ContextProvider -- `context-providers/`

* All of the context provider implementations are baked into the CDK CLI app.
* Base: `interface ContextProviderPlugin { getValue(args: {[key: string]: any}): Promise<any>; }` (in `context-providers/provider.ts`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's talk AWS permissions.

Don't forget that the specific instances that implement ContextProviderPlugin have also been initialized with SdkProviders, which allow them to get AWS credentials for any account/region pair.

The context provider is going to have to do AWS calls. Where is it going to get access to an SDK instance? How are we communicating that? How do we define the API between plugin and CDK? Who initializes it? I'm not a fan of just passing an SdkProvider, both for "security" reasons as well as keeping the API surface as minimal as we can keep it and not leaking implementation details.

There needs to be a TypeScript package somewhere that contains just the API definitions for this plugin relationship. What are the types that are going to go in there? Just "whatever the AWS SDK for JS exports"? Or are we going to wrap those clients?

To sidestep that--maybe we just say that external context providers are subprocesses, and we initialize the environment with AWS_REGION and AWS_ACCESS_KEY_ID (etc)?


As an additional wrinkle: right now, we just run the context providers using the "current" AWS credentials, and it's the user's responsibility to make sure those are sufficient for whatever it is they intend to do. In the near future, we will introduce a special role that will be bootstrapped into the target account you will be querying for context in. That role will have a fixed set of permissions, and so what role should your CUSTOM context provider assume? If it wants to do, say, s3:ListBuckets, that role we provide by default won't have the permissions to do that.

At the same time, I'm not sure we can justify attaching aws/ReadOnlyAccess to that role, so I guess the user will just manually have to add policy statements at bootstrap time to satisfy the needs of the context providers they intend to be using?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The context provider could specify the role that it needs (e.g., a PolicyStatement), which the CDK could bootstrap (and validate, if we want to ensure it doesn't require any write permissions). Further, the CDK could ask the user to authorize the creation of those roles (unless --require-approval never is specified).

The context provider would be created with an SDK from SdkProvider::withAssumedRole. If there's a reason not to expose the full SDK, there could be a package exposing all or some services. It's not clear to me there's any value in such a thin wrapper of the SDK.

extra resources to the user's account, and makes it possible for a value generated by the CfnCustomResource to differ from one deployment
to the next.

Unfortunately, these CfnCustomResources required by 3rd party construct libraries may also be a security risk to the user. The lambda that
Copy link
Contributor

@rix0rrr rix0rrr Jun 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This smells a little like FUD. While I personally would have preferred a way to restrict Lambdas Invoke permissions to CloudFormation (which is afaik not possible, and when I asked for it they told me to write the custom resource to do a callback to the Stack and check whether the resource was currently `_IN_PROGRESS, ... TT 0150388604), this should not be the main factor in deciding between custom resource and context provider.

@ddneilson
Copy link
Author

I appreciate the feedback, @rix0rrr . I'm trying to find some time to digest your comments, and dig back into this; I'll aim to have something up within the next week or so.

@eladb eladb changed the title RFC: 18 Open context provider framework RFC: #18 Open context provider framework Jun 23, 2020
@eladb eladb changed the title RFC: #18 Open context provider framework RFC: 0018 Open context provider framework Jun 23, 2020
@eladb eladb changed the title RFC: 0018 Open context provider framework RFC: 18 Open context provider framework Jun 23, 2020
@john-tipper
Copy link

For me, the concrete use case I wish to use this feature for is in resolving multiple accounts for complicated cross-account deployments. We have a lot of AWS Organization accounts, which are identifiable by name, tag etc. I want the CDK to be able to resolve which accounts exist and which stacks need to go into which account at synth time. This will allow me to do things like update IAM resource policies to permit access from new accounts really simply.

I'd like to KISS and be able to do something simple, like we do for credentials, and supply the plugin explicitly on the command line. My implementation of that plugin could then return context (via whatever mechanism my plugin uses: SDK call, REST call, bash script etc) that might give a list of accounts that meet some criteria (e.g. being tagged with some value).

@eladb
Copy link
Contributor

eladb commented Mar 10, 2021

@ddneilson is this something you'd be interested in continuing to work on or should we close for now? (always happy for you to pick this up in the future)

@ddneilson
Copy link
Author

@ddneilson is this something you'd be interested in continuing to work on or should we close for now? (always happy for you to pick this up in the future)

@eladb

Interested, yes. Available bandwidth? No.

I would still love to see it possible for libraries other than the core CDK construct library make use of the provider framework. Synth-time values are important to more than just the CDK.

For instance, we have a construct that works best if we know the account's ELB Limits at synth time -- https://github.com/aws/aws-rfdk/blob/f84097ef4c084916b1affbd3de55a8a11bba1d04/packages/aws-rfdk/lib/core/lib/health-monitor.ts#L187. The construct creates one or more load balancers to run Health Checks, but has to take into account the maximum number of instances a single LB can monitor (which is an account limit) to know how many LBs to create. The only way to get that right now is to ask the customer to query the limits themselves, and manually enter them into their app. It's a horrid customer experience.

@chris-smith-zocdoc
Copy link

One area this would be super useful is having the end user write custom Lookup functions for capabilities that aren't bundled with the cdk.

There are several open issues requesting various lookups that could be written by the end user if the context provider framework was extensible

aws/aws-cdk#8461
aws/aws-cdk#6803
aws/aws-cdk#14032
aws/aws-cdk#1417

Currently the "best practice" as defined by the docs say

If you need some value (from AWS or elsewhere) for which there is no native CDK context provider, we recommend writing a separate script to retrieve the value and write it to a file, then read that file in your CDK app. Run the script only when you want to refresh the stored value, not as part of your regular build process.

which is less than ideal from a workflow perspective

@sciutand
Copy link

sciutand commented Jul 5, 2022

I just came across this. My issue is that I want to use some instance type that are not available in all az. I receive the instance type as a parameter and there is no way for me to find out which az's the instance is available for the given region/account combination.
And I need to pass that to my autoscaling group or it will try to launch an instance in the wrong az and fail miserably.
Upvote on allowing custom providers.

@RichiCoder1
Copy link

Howdy! I know there has been work and there's "sort of" support for this in the CDK right now, but I was wondering if this was still something that was in progress?

If not, something external contributors could help move forward?

@marcogrcr
Copy link

marcogrcr commented Mar 1, 2023

My use case is that I want to create a DynamoEventSource for subscribing a Lambda function for each replica of a global table. However CfnGlobalTable.attrStreamArn and Table.tableStreamArn only return the ARN of the stream for the table in the region of the stack that contains the resource and there's no way to automatically obtain the stream ARNs of the replicas without a CFN custom resource or a custom script.

This, with this feature I could create this instead:

function lookupTableByName(scope: Construct, name: string): ITable

Which uses a context provider plugin that internally invokes dynamodb:DescribeTable.

@marcogrcr
Copy link

@RichiCoder1 it seems the current state of ContextProviderPlugin is very rough:

Users can author plugins as follows:

interface MyPluginQuery {
  /*
   * These fields will be automatically populated using the stack's account/region if the `ContextProvider.getValue()`
   * invocation omits the `includeEnvironment` param field or sets it to `true`.
   */
  readonly account: string;
  readonly region: string;

  // custom fields
  readonly myField: unknown;
}

interface MyPluginContext {
  // custom fields
  readonly myField: unknown;
}

class MyPlugin implements ContextProviderPlugin {
  public async getValue(args: MyPluginQuery): Promise<MyPluginContext> {
    // ...
  }
}

export = {
  version: "1",
  init(host: PluginHost) {
    host.registerContextProviderAlpha("my-plugin", new MyPlugin());
  }
} as Plugin;

And then use them as follows:

type MyPluginProps = Omit<MyPluginQuery, "account" | "region">;

function myPluginLookup(scope: Construct, props: MyPluginProps): MyPluginContext {
  return ContextProvider.getValue(scope, {
    provider: "plugin",
    props: {
      ...props,
      pluginName: "my-plugin"
    },
    includeEnvironment: true, // so account/region are populated from `scope`'s stack
    dummyValue: { myField: "..." },
  }).value;
}

Where:

  • ContextProvider is defined here.
  • ContextProviderPlugin is defined here.
  • Plugin is defined here.
  • PluginHost is defined here.

However, the following limitations apply:

@mrgrain mrgrain closed this Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants