feat(route53resolver): DNS Firewall#15031
Conversation
Add L2s for `FirewallDomainList`, `FirewallRuleGroup` and `FirewallRuleGroupAssociation`.
|
|
…into route53resolver-firewall
|
hey @njlynch, i see you removed your assignment here, is someone picking this up? thx |
|
Hi @jogold !
Not immediately. Apologies, I should have left a comment along with my unassignment to explain. We're taking efforts to triage and prioritize issues and PRs more uniformly, and then use a combination of inputs (e.g., 👍s, priority, comments) to decide when to work on both. In tandem, we're also trying to signal and track better what we're actively working on (thus removing my assignment). I can't give a fixed ETA for when I'll be able to pick this up, but I'll try to carve out time in the next couple weeks. In the meantime, getting folks in the community to 👍 this will help prioritize to show there's a strong interest for these L2s. |
njlynch
left a comment
There was a problem hiding this comment.
Looks great! One error message to update and two minor nitpicks. Otherwise is good to go.
Thanks!
| if (!/^[\w-.]{1,128}$/.test(domain)) { | ||
| throw new Error(`Invalid domain: ${domain}. The name must have 1-128 characters. Valid characters: A-Z, a-z, 0-9, _, -, .`); |
There was a problem hiding this comment.
No underscores are allowed in domain names, IIRC, and your regex isn't checking for them.
| if (!/^[\w-.]{1,128}$/.test(domain)) { | |
| throw new Error(`Invalid domain: ${domain}. The name must have 1-128 characters. Valid characters: A-Z, a-z, 0-9, _, -, .`); | |
| if (!/^[\w-.]{1,128}$/.test(domain)) { | |
| throw new Error(`Invalid domain: ${domain}. The name must have 1-128 characters. Valid characters: A-Z, a-z, 0-9, -, .`); |
There was a problem hiding this comment.
Underscores are unusual but allowed in (sub)domains names and \w includes underscores (\w = [a-zA-Z0-9_])
https://stackoverflow.com/questions/2180465/can-domain-name-subdomains-have-an-underscore-in-it
There was a problem hiding this comment.
Removed the {1,128} part which was incorrect and actually applies to the domain list name.
packages/@aws-cdk/aws-route53resolver/lib/firewall-rule-group.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Nick Lynch <nlynch@amazon.com>
Co-authored-by: Nick Lynch <nlynch@amazon.com>
|
@njlynch can you check this PR again? thx! |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Add L2s for `FirewallDomainList`, `FirewallRuleGroup` and `FirewallRuleGroupAssociation`. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Add L2s for `FirewallDomainList`, `FirewallRuleGroup` and `FirewallRuleGroupAssociation`. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Add L2s for
FirewallDomainList,FirewallRuleGroupandFirewallRuleGroupAssociation.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license