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

chore(cxapi): plugin context provider limited by cx schema #18709

Merged
merged 4 commits into from
Feb 4, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ export enum ContextProvider {
* KMS Key Provider
*/
KEY_PROVIDER = 'key-provider',

/**
* A plugin provider (the actual plugin name will be in the properties)
*/
PLUGIN = 'plugin',
}

/**
Expand Down Expand Up @@ -461,7 +466,24 @@ export interface KeyContextQuery {
* Alias name used to search the Key
*/
readonly aliasName: string;
}

/**
* Query input for plugins
*
* This alternate branch is necessary because it needs to be able to escape all type checking
* we do on on the cloud assembly -- we cannot know the properties that will be used a priori.
*/
export interface PluginContextQuery {
/**
* The name of the plugin
*/
readonly pluginName: string;

/**
* Arbitrary other arguments for the plugin
*/
[key: string]: any;
Copy link
Contributor

Choose a reason for hiding this comment

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

  readonly pluginArguments?: [key: string]: any;

Can we change this to something like this?

Copy link
Contributor Author

@rix0rrr rix0rrr Feb 4, 2022

Choose a reason for hiding this comment

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

We could, but this feels simpler (requires fewer code changes, too).

}

export type ContextQueryProperties = AmiContextQuery
Expand All @@ -473,4 +495,6 @@ export type ContextQueryProperties = AmiContextQuery
| LoadBalancerContextQuery
| LoadBalancerListenerContextQuery
| SecurityGroupContextQuery
| KeyContextQuery;
| KeyContextQuery
| PluginContextQuery;

Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,9 @@
},
{
"$ref": "#/definitions/KeyContextQuery"
},
{
"$ref": "#/definitions/PluginContextQuery"
}
]
}
Expand All @@ -472,6 +475,7 @@
"key-provider",
"load-balancer",
"load-balancer-listener",
"plugin",
"security-group",
"ssm",
"vpc-provider"
Expand Down Expand Up @@ -834,6 +838,20 @@
"region"
]
},
"PluginContextQuery": {
"description": "Query input for plugins\n\nThis alternate branch is necessary because it needs to be able to escape all type checking\nwe do on on the cloud assembly -- we cannot know the properties that will be used a priori.",
"type": "object",
"additionalProperties": {},
"properties": {
"pluginName": {
"description": "The name of the plugin",
"type": "string"
}
},
"required": [
"pluginName"
]
},
"RuntimeInfo": {
"description": "Information about the application's runtime components.",
"type": "object",
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"version":"16.0.0"}
{"version":"17.0.0"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious - do we bump the major version of the schema with every change? Is this change a breaking change? (If this is a silly question, please explain why. 😄 I don't fully understand the contract here. Is the API only used by us?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do bump the MV of the schema with every change. Every change is indeed potentially breaking (at any point the framework could be requesting services from the CLI that the CLI may not know how to provide--i.e., CLI needs an update to match).

It's a bit onerous to do this though, and you're right that in this particular case it's probably not necessary. This was just forced upon me by the tools we're using. Will revert the version.

15 changes: 10 additions & 5 deletions packages/@aws-cdk/core/lib/context-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,13 @@ export interface GetContextKeyOptions {
* Provider-specific properties.
*/
readonly props?: { [key: string]: any };

/**
* Whether to include the stack's account and region automatically.
*
* @default true
*/
readonly includeEnvironment?: boolean;
}

/**
Expand Down Expand Up @@ -60,11 +67,9 @@ export class ContextProvider {
public static getKey(scope: Construct, options: GetContextKeyOptions): GetContextKeyResult {
const stack = Stack.of(scope);

const props = {
account: stack.account,
region: stack.region,
...options.props || {},
};
const props = options.includeEnvironment ?? true
? { account: stack.account, region: stack.region, ...options.props }
: (options.props ?? {});

if (Object.values(props).find(x => Token.isUnresolved(x))) {
throw new Error(
Expand Down
5 changes: 5 additions & 0 deletions packages/@aws-cdk/core/test/context.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,11 @@ describe('context', () => {


});

test('can skip account/region from attach to context', () => {
const stack = new Stack(undefined, 'TestStack', { env: { account: '12345', region: 'us-east-1' } });
expect(ContextProvider.getKey(stack, { provider: 'asdf', includeEnvironment: false }).key).toEqual('asdf:');
});
});

/**
Expand Down
18 changes: 17 additions & 1 deletion packages/aws-cdk/lib/context-providers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import { VpcNetworkContextProviderPlugin } from './vpcs';
export type ContextProviderFactory = ((sdk: SdkProvider) => ContextProviderPlugin);
export type ProviderMap = {[name: string]: ContextProviderFactory};

const PLUGIN_PROVIDER_PREFIX = 'plugin';

/**
* Iterate over the list of missing context values and invoke the appropriate providers from the map to retrieve them
*/
Expand All @@ -28,7 +30,12 @@ export async function provideContextValues(

for (const missingContext of missingValues) {
const key = missingContext.key;
const factory = availableContextProviders[missingContext.provider];

const providerName = missingContext.provider === cxschema.ContextProvider.PLUGIN
? `${PLUGIN_PROVIDER_PREFIX}:${(missingContext.props as cxschema.PluginContextQuery).pluginName}`
: missingContext.provider;

const factory = availableContextProviders[providerName];
if (!factory) {
// eslint-disable-next-line max-len
throw new Error(`Unrecognized context provider name: ${missingContext.provider}. You might need to update the toolkit to match the version of the construct library.`);
Expand Down Expand Up @@ -70,6 +77,15 @@ export function registerContextProvider(name: string, provider: ContextProviderP
availableContextProviders[name] = () => provider;
}

/**
* Register a plugin context provider
*
* A plugin provider cannot reuse the SDKs authentication mechanisms.
*/
export function registerPluginContextProvider(name: string, provider: ContextProviderPlugin) {
registerContextProvider(`${PLUGIN_PROVIDER_PREFIX}:${name}`, provider);
}

/**
* Register a context provider factory
*
Expand Down
20 changes: 17 additions & 3 deletions packages/aws-cdk/lib/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { inspect } from 'util';
import * as chalk from 'chalk';

import { CredentialProviderSource } from './api/aws-auth/credentials';
import { registerContextProvider } from './context-providers';
import { registerPluginContextProvider } from './context-providers';
import { ContextProviderPlugin, isContextProviderPlugin } from './context-providers/provider';
import { error } from './logging';

Expand Down Expand Up @@ -106,12 +106,26 @@ export class PluginHost {
* This feature is experimental, and only intended to be used internally at Amazon
* as a trial.
*
* After registering with 'my-plugin-name', the provider must be addressed as follows:
*
* ```ts
* const value = ContextProvider.getValue(this, {
* providerName: 'plugin',
* props: {
* pluginName: 'my-plugin-name',
* myParameter1: 'xyz',
* },
* includeEnvironment: true | false,
* dummyValue: 'what-to-return-on-the-first-pass',
* })
* ```
*
* @experimental
*/
public registerContextProviderAlpha(providerName: string, provider: ContextProviderPlugin) {
public registerContextProviderAlpha(pluginProviderName: string, provider: ContextProviderPlugin) {
if (!isContextProviderPlugin(provider)) {
throw new Error(`Object you gave me does not look like a ContextProviderPlugin: ${inspect(provider)}`);
}
registerContextProvider(providerName, provider);
registerPluginContextProvider(pluginProviderName, provider);
}
}
11 changes: 6 additions & 5 deletions packages/aws-cdk/test/context-providers/generic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { MockSdkProvider } from '../util/mock-sdk';
const mockSDK = new MockSdkProvider();

const TEST_PROVIDER: any = 'testprovider';
const PLUGIN_PROVIDER: any = 'plugin';

test('errors are reported into the context value', async () => {
// GIVEN
Expand Down Expand Up @@ -84,7 +85,7 @@ test('context provider can be registered using PluginHost', async () => {
let called = false;

// GIVEN
PluginHost.instance.registerContextProviderAlpha(TEST_PROVIDER, {
PluginHost.instance.registerContextProviderAlpha('prov', {
async getValue(_: {[key: string]: any}): Promise<any> {
called = true;
return '';
Expand All @@ -94,16 +95,16 @@ test('context provider can be registered using PluginHost', async () => {

// WHEN
await contextproviders.provideContextValues([
{ key: 'asdf', props: { account: '1234', region: 'us-east-1' }, provider: TEST_PROVIDER },
{ key: 'asdf', props: { account: '1234', region: 'us-east-1', pluginName: 'prov' }, provider: PLUGIN_PROVIDER },
], context, mockSDK);

// THEN - error is marked transient
expect(called).toEqual(true);
});

test('context provider can be called without account/region', async () => {
test('plugin context provider can be called without account/region', async () => {
// GIVEN
PluginHost.instance.registerContextProviderAlpha(TEST_PROVIDER, {
PluginHost.instance.registerContextProviderAlpha('prov', {
async getValue(_: {[key: string]: any}): Promise<any> {
return 'yay';
},
Expand All @@ -112,7 +113,7 @@ test('context provider can be called without account/region', async () => {

// WHEN
await contextproviders.provideContextValues([
{ key: 'asdf', props: { banana: 'yellow' } as any, provider: TEST_PROVIDER },
{ key: 'asdf', props: { banana: 'yellow', pluginName: 'prov' } as any, provider: PLUGIN_PROVIDER },
], context, mockSDK);

// THEN - error is marked transient
Expand Down