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

appsync - apiKeyConfig expires outputting incorrect format #8698

Closed
m0rph13 opened this issue Jun 23, 2020 · 2 comments · Fixed by #9122 or vrr-21/aws-cdk#2
Closed

appsync - apiKeyConfig expires outputting incorrect format #8698

m0rph13 opened this issue Jun 23, 2020 · 2 comments · Fixed by #9122 or vrr-21/aws-cdk#2
Assignees
Labels
@aws-cdk/aws-appsync Related to AWS AppSync bug This issue is a bug. effort/small Small work item – less than a day of effort in-progress This issue is being actively worked on. p2

Comments

@m0rph13
Copy link

m0rph13 commented Jun 23, 2020

When specifying the attribute "expires" for apiKeyConfig, the rendered cloudformation gets a value of null.

Reproduction Steps

const api = new GraphQLApi(this, 'api', {
            name: getResourceName('graphql'),
            authorizationConfig: {
                defaultAuthorization: {
                    authorizationType: AuthorizationType.API_KEY,
                    apiKeyConfig: {
                        name: "api-key",
                        description: "api-key",
                        expires: "1614164998"
                    }
                },    
            schemaDefinitionFile: 'schema.graphql',
        });

Error Log

Synth gives:

 apiapikeyApiKeyXXXXXX:
    Type: AWS::AppSync::ApiKey
    Properties:
      ApiId:
        Fn::GetAtt:
          - apiCXXXXXX
          - ApiId
      Description: api-key
      Expires: null

Environment

  • CLI Version : 1.46.0
  • Framework Version: 1.46.0
  • Node.js Version: v12.16.1
  • OS : OSX
  • Language (Version): 3.8.3

Other


This is 🐛 Bug Report

@m0rph13 m0rph13 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 23, 2020
@civilizeddev
Copy link
Contributor

According to the definition:

export interface ApiKeyConfig {
  ...

  /**
   * The time from creation time after which the API key expires, using RFC3339 representation.
   * It must be a minimum of 1 day and a maximum of 365 days from date of creation.
   * Rounded down to the nearest hour.
   * @default - 7 days from creation time
   */
  readonly expires?: string;
}

Workaround

You should set expires like 2020-12-31T23:30:00Z

Use new Date(...).toISOString() or any other date/time library.

Proposed solution

But in fact, it is also prone to errors and needs to be changed in a different way.

export interface ApiKeyConfig {
  ...

  /**
   * The duration from creation time after which the API key expires.
   * It must be a minimum of 1 day and a maximum of 365 days from date of creation.
   * Rounded down to the nearest hour.
   * @default - cdk.Duration.days(7)
   */
  readonly expiresAfter?: cdk.Duration;
}

Any good idea?

@SomayaB SomayaB added the @aws-cdk/aws-appsync Related to AWS AppSync label Jun 23, 2020
@SomayaB SomayaB changed the title @aws-cdk/aws-appsync: (@aws-cdk/aws-appsync): Jun 23, 2020
@MrArnoldPalmer MrArnoldPalmer changed the title (@aws-cdk/aws-appsync): appsync - apiKeyConfig expires outputting incorrect format Jun 24, 2020
@MrArnoldPalmer MrArnoldPalmer added p2 and removed needs-triage This issue or PR still needs to be triaged. labels Jun 24, 2020
@civilizeddev
Copy link
Contributor

civilizeddev commented Jul 3, 2020

What about using kind of Expires?

AS-IS

{
  ...,
  expires: '2020-12-31T23:30:00Z'
}

TO-BE

  • expires: Expires.atDate(new Date('2020-12-31T23:30:00Z'))
  • expires: Expires.after(cdk.Duration.months(6))
  • expires: Expires.fromTimestamp(1614164998000)
  • expires: Expires.fromString('2020-12-31T23:30:00Z')

References

I hope the things below would be promoted to @aws-cdk/core, as well as Duration

@nija-at @jogold

Expires

/**
* Used for HTTP expires header, which influences downstream caches. Does NOT influence deletion of the object.
* @see https://docs.aws.amazon.com/AmazonS3/latest/dev/UsingMetadata.html#SysMetadata
*/
export class Expires {
/**
* Expire at the specified date
* @param d date to expire at
*/
public static atDate(d: Date) { return new Expires(d.toUTCString()); }
/**
* Expire at the specified timestamp
* @param t timestamp in unix milliseconds
*/
public static atTimestamp(t: number) { return Expires.atDate(new Date(t)); }
/**
* Expire once the specified duration has passed since deployment time
* @param t the duration to wait before expiring
*/
public static after(t: cdk.Duration) { return Expires.atDate(new Date(now + t.toMilliseconds())); }
public static fromString(s: string) { return new Expires(s); }
private constructor(public readonly value: any) {}
}

Schedule

/**
* Schedule for scheduled scaling actions
*/
export abstract class Schedule {
/**
* Construct a schedule from a literal schedule expression
*
* @param expression The expression to use. Must be in a format that Application AutoScaling will recognize
*/
public static expression(expression: string): Schedule {
return new LiteralSchedule(expression);
}
/**
* Construct a schedule from an interval and a time unit
*/
public static rate(duration: Duration): Schedule {
if (duration.toSeconds() === 0) {
throw new Error('Duration cannot be 0');
}
let rate = maybeRate(duration.toDays({ integral: false }), 'day');
if (rate === undefined) { rate = maybeRate(duration.toHours({ integral: false }), 'hour'); }
if (rate === undefined) { rate = makeRate(duration.toMinutes({ integral: true }), 'minute'); }
return new LiteralSchedule(rate);
}
/**
* Construct a Schedule from a moment in time
*/
public static at(moment: Date): Schedule {
return new LiteralSchedule(`at(${formatISO(moment)})`);
}
/**
* Create a schedule from a set of cron fields
*/
public static cron(options: CronOptions): Schedule {
if (options.weekDay !== undefined && options.day !== undefined) {
throw new Error('Cannot supply both \'day\' and \'weekDay\', use at most one');
}
const minute = fallback(options.minute, '*');
const hour = fallback(options.hour, '*');
const month = fallback(options.month, '*');
const year = fallback(options.year, '*');
// Weekday defaults to '?' if not supplied. If it is supplied, day must become '?'
const day = fallback(options.day, options.weekDay !== undefined ? '?' : '*');
const weekDay = fallback(options.weekDay, '?');
return new LiteralSchedule(`cron(${minute} ${hour} ${day} ${month} ${weekDay} ${year})`);
}
/**
* Retrieve the expression for this schedule
*/
public abstract readonly expressionString: string;
protected constructor() {
}
}

@BryanPan342 BryanPan342 self-assigned this Jul 16, 2020
@BryanPan342 BryanPan342 added the in-progress This issue is being actively worked on. label Jul 17, 2020
@MrArnoldPalmer MrArnoldPalmer added the effort/small Small work item – less than a day of effort label Aug 17, 2020
@mergify mergify bot closed this as completed in #9122 Sep 9, 2020
mergify bot pushed a commit that referenced this issue Sep 9, 2020
**[ISSUE]**
`apiKeyConfig` has prop `expires` that has unclear documentation/not strongly typed and is prone to user errors. 

**[APPROACH]**
Force `expires` to take `Expiration` class from `core` and will be able to output api key configurations easily through `Expiration` static functions: `after(...)`, `fromString(...)`, ` atDate(...)`, `atTimeStamp(...)`.

Fixes #8698 

BREAKING CHANGE:  force `apiKeyConfig` require a Expiration class instead of string
- **appsync**: Parameter `apiKeyConfig` takes `Expiration` class instead of `string`

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-appsync Related to AWS AppSync bug This issue is a bug. effort/small Small work item – less than a day of effort in-progress This issue is being actively worked on. p2
Projects
None yet
5 participants