-
Notifications
You must be signed in to change notification settings - Fork 78
Kvs updates #45
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
Kvs updates #45
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned a few minor changes, but overall this looks good.
kvs-jwt-verify/README.md
Outdated
@@ -2,39 +2,48 @@ | |||
|
|||
**CloudFront Functions event type: viewer request** | |||
|
|||
**Note:** We recommend using the [kvs-jwt-verify example](https://github.com/aws-samples/amazon-cloudfront-functions/tree/main/kvs-jwt-verify) instead. It uses Amazon CloudFront KeyValueStore to store the secret key, and is compatible with JavaScript 2.0 runtime. | |||
This function validates a JSON Web Token (JWT) in the query string of the incoming request. It is compatible with JavaScript 2.0 runtime. Using your CloudFront KeyValueStore ID is optional. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function validates a JSON Web Token (JWT) in the query string of the incoming request. It is compatible with the CloudFront Functions JavaScript 2.0 runtime and uses CloudFront KeyValueStore to store the secret. Using your CloudFront KeyValueStore ID is optional.
kvs-jwt-verify/verify-jwt.js
Outdated
|
||
//Response when JWT is not valid. | ||
const response401 = { | ||
statusCode: 401, | ||
statusDescription: 'Unauthorized' | ||
}; | ||
|
||
// Replace the KVS_ID with your KVS ID | ||
// (Optional) Replace the KVS_ID with your KVS ID | ||
const kvsId = "KVS_ID"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is not needed, we should just take this out. Making it optional is confusing (to me at least). Maybe just putting a comment in here about remembering to attach the KVS to your function before calling the const kvsKey = 'jwt.secret'
kvs-conditional-read/README.md
Outdated
|
||
**CloudFront Functions event type: viewer request** | ||
|
||
This example provides a dynamic request URI rewriting mechanism, allowing for A/B testing or gradual rollout of application versions, while also maintaining user stickiness to ensure a consistent experience. It rewrites the request URI based on a configuration stored in CloudFront KeyValueStore. Using your KeyValueStore ID in the code is optional. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove the last sentence about storing the KVS ID in the code. I think we should instead just say that you have to create a KVS and put the secret in there and then attach the KVS to the function. I don't quite know the level of detail that we need to go into how to setup KVS for this readme, but if the KVS ID is optional then we should not even mention it since its much easier without it in the code.
kvs-key-value-pairs/README.md
Outdated
|
||
**CloudFront Functions event type: viewer request** | ||
|
||
The example uses key-value pairs from an Amazon CloudFront [key value store](https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/kvs-with-functions.html) in a CloudFront function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use the formal name of KeyValueStore
kvs-key-value-pairs/README.md
Outdated
|
||
The example uses key-value pairs from an Amazon CloudFront [key value store](https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/kvs-with-functions.html) in a CloudFront function. | ||
|
||
The example shows a function that uses the content of the URL in the HTTP request to look up a custom path in the key value store. CloudFront then uses that custom path to make the request. This function helps manage the multiple paths that are part of a website. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add an example here. For example, To update the version of a blog platform on a website. The old blog has origin path /blog-v1
while the new blog has origin path /blog-v2
. This function can lookup the URL path of the incoming request and rewrite the URL path (/blog-v1
) to the new version of the blog (/blog-v2
).
kvs-key-value-pairs/README.md
Outdated
|
||
The example shows a function that uses the content of the URL in the HTTP request to look up a custom path in the key value store. CloudFront then uses that custom path to make the request. This function helps manage the multiple paths that are part of a website. | ||
|
||
The example works with JavaScript runtime 2.0. The CloudFront KeyValueStore ID is optional. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I would take out the KVS ID is optional and replace it with some commentary on attaching a KVS to the function. Also do we want to link to the 2.0 runtime in our public docs?
@@ -0,0 +1,27 @@ | |||
import cf from 'cloudfront'; | |||
| |||
// (Optional) Declare the ID of the key value store that you have associated with this function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove this and replace it with a comment about remembering to attach the KVS to your function before referencing KVS in your code.
| ||
// (Optional) Declare the ID of the key value store that you have associated with this function | ||
|
||
const kvsId = "a1b2c3d4-5678-90ab-cdef-EXAMPLE11111"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we put in a comment about referencing/attaching KVS to your function then we can remove this line.
|
||
const kvsId = "a1b2c3d4-5678-90ab-cdef-EXAMPLE11111"; | ||
| ||
const kvsHandle = cf.kvs(kvsId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed if we aren't defining the KVS ID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy text looks good but I see a couple of issues with the code samples that need to be fixed.
try { | ||
// Replace the first path of the pathname with the value of the key | ||
// For example http(s)://domain/<value>/something/else | ||
pathSegments[1] = await kvsHandle.get(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're not defining kvsHandle
anywhere so this function would get an error. You need something like this:
const kvsHandle = cf.kvs();
outside the function handler.
|
||
//Response when JWT is not valid. | ||
const response401 = { | ||
statusCode: 401, | ||
statusDescription: 'Unauthorized' | ||
}; | ||
|
||
// Replace the KVS_ID with your KVS ID | ||
const kvsId = "KVS_ID"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You remove the KVS_ID from the code, but its still referenced in live 131. You will also need to remove it from line 131.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looks good. Approved.
Issue #, if available: Need to update KVS examples and add examples from CloudFront Developer Guide for parity.
https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/example-function-normalize-query-string.html
https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/example-function-key-value-pairs.html
Description of changes: Deleted verify-jwt directory in favor of kvs-jwt-verify. For kvs-jwt-verify, updated README and test output example, and migrated test-objects directory to kvs-jwt-verify. Added missing README for kvs-conditional-read. Added two new examples with READMEs: kvs-key-value-pairs and normalize-query-string-parameters.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.