-
Notifications
You must be signed in to change notification settings - Fork 465
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
Allow cross region PrivateLink connections #30897
Allow cross region PrivateLink connections #30897
Conversation
if !connection_az | ||
.chars() | ||
.all(|c| c.is_ascii_alphanumeric() || c == '-') | ||
|| !connection_az.contains("-az") |
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.
seems like we could tighten this up to be more precise. https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/using-regions-availability-zones.html#az-ids has more details on the structure
the most legit solution would be to automatically pull every AZ ID from the partition we're operating in during our Pulumi run and then plumbing it 20 layers deep to here... in practice I think just ensuring we're following the structure in the doc above is sufficient
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.
(and if we pull out some logic to validate the ids, we should add a quick unit test)
a5f7d10
to
a1bf87b
Compare
fn is_valid_az_format(az: &str) -> bool { | ||
let az_pattern = Regex::new(r"^[a-z]{3,4}\d-az[1-6]$").unwrap(); | ||
az_pattern.is_match(az) | ||
} |
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.
It worries me a bit injecting a pattern when we don't know what rules AWS uses internally for naming these, but this is probably as good as we can do without injecting the complete list.
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.
The code itself looks good to me, not super familiar though with cross region PrivateLink so would defer to Cloud folks on that
Looking into this further I think that this PR might not even be necessary! Still validating this but will keep the threat updated. |
Closing this PR in favor ofhttps://github.com/MaterializeInc/cloud/pull/10617 No changes will be needed on the database side nor the AZ id validation on the controller side. AWS handles all this on their end. All that we would need to do to support this is to just pass the service_region to the create request. We are extracting that information from the service name itself as it contains the region already.
I will open just a follow-up docs PR for this clarification. |
Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$T
mapping (possibly in a backwards-incompatible way), then it is tagged with aT-proto
label.