fix(cloudfront-origins): readTimeout and keepaliveTimeout reject valid values - #38432
fix(cloudfront-origins): readTimeout and keepaliveTimeout reject valid values#38432mekza wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
The pull request linter fails with the following errors:
❌ Fixes must contain a change to an integration test file and the resulting snapshot.
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.
✅ A exemption request has been requested. Please wait for a maintainer's review.
|
Exemption Request An integration test cannot meaningfully cover this change. The PR removes a client-side bound so that values permitted by an account-specific, adjustable service quota can be expressed. Exercising the new behaviour requires Using a still-deployable value like 180 would satisfy the linter but test nothing, since 180 was already accepted before this change. A second account-dependent constraint makes a portable test impractical: CloudFront bounds the product of Snapshots are unaffected regardless: this removes a synth-time guard rather than changing synthesized output, so there is no template diff to capture. Covered instead by:
Happy to add an integ test at a deployable value if maintainers prefer one for form, but it would not exercise the code path in question. |
…d values
Both props are validated against a hardcoded upper bound of 180 seconds, which
matches no current CloudFront quota:
- Response timeout per origin is documented as 1-120 seconds, adjustable via
a limit increase. The 180 bound is a stale snapshot (it was 60, aws#18628
raised it to 180, and the documented default has since moved to 120), so
values a raised quota permits are rejected at synth time with no way to
express them through the L2.
- Keep-alive timeout per origin is documented as 1-300 seconds. Here the
bound is simply wrong today: values between 181 and 300 need no quota
increase at all, yet synth refuses them.
The existing doc comment already states that values above the default require
an approved limit increase and otherwise fail at deploy time, so the construct
already defers enforcement of these quotas to the service. Hardcoding a ceiling
contradicts that, and re-hardcoding a higher number would only recur at the next
quota change.
Remove the upper bound for both props in HttpOrigin, VpcOrigin,
FunctionUrlOrigin and RestApiOrigin. The 1-second minimum and the
whole-number-of-seconds validation are unchanged, and
validateSecondsInRangeOrUndefined now accepts an optional max so other callers
keep their bounds. Doc comments now describe each prop's own quota rather than
citing the response timeout quota for both.
c0ee7c8 to
8feedcb
Compare
Issue # (if applicable)
Closes #38433.
Reason for this change
readTimeoutandkeepaliveTimeoutare both validated against a hardcoded upper bound of 180 seconds, which corresponds to no current CloudFront quota. Per the CloudFront quotas page:1-120 seconds, Request a higher quota1-300 seconds, Request a higher quotaTwo distinct problems:
readTimeout: the 180 bound is a stale snapshot. It was 60, (cloudfront-origins): allow to extendreadTimeoutover 60 seconds inHttpOriginProps#18628 raised it to 180, and the documented default has since moved to 120. Once a limit increase is approved, values the service accepts are rejected at synth time with no way to express them through the L2.keepaliveTimeout: the bound is simply wrong today. Values between 181 and 300 are within the default quota and need no limit increase at all, yet synth refuses them.The existing doc comment already states that values above the default require an approved limit increase and will otherwise produce an error at deploy time, so the construct already defers enforcement of these quotas to the service. A hardcoded ceiling contradicts that, and re-hardcoding a higher number would only recur at the next quota change.
Description of changes
Removed the upper bound for
readTimeoutandkeepaliveTimeoutinHttpOrigin,VpcOrigin,FunctionUrlOriginandRestApiOrigin. The 1-second minimum and the whole-number-of-seconds validation are unchanged, so values above the account's quota now surface as a deploy-time service error, consistent with the documented behaviour for values above the default.validateSecondsInRangeOrUndefinednow acceptsmax: number | undefined, so every other caller keeps its existing bounds.Doc comments now describe each prop's own quota. Previously both cited the response timeout quota and a 60-second threshold, which was inaccurate for
keepaliveTimeout.Describe any new or updated permissions being added
None.
Description of how you validated changes
Updated unit tests for
HttpOriginandVpcOrigin: values below the minimum still throw, values above 180 no longer throw, and the whole-number-of-seconds validation is unchanged.Verified against a live CloudFront distribution configured with a
readTimeoutof 300 seconds after an approved quota increase, confirming the service accepts values the construct currently rejects.Worth noting for anyone hitting this: CloudFront also bounds the product
connectionAttempts x (connectionTimeout + readTimeout). At the default 3 attempts, a 300-secondreadTimeoutis rejected with "When your OriginReadTimeout value exceeds the maximum permitted value, ConnectionAttempts must not be set too high"; withconnectionAttempts: 1it is accepted. That constraint is enforced by the service and is unaffected by this change.No integration test is included, see the exemption request in the comments.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license