-
Notifications
You must be signed in to change notification settings - Fork 4k
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
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. |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
,FirewallRuleGroup
andFirewallRuleGroupAssociation
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license