-
Notifications
You must be signed in to change notification settings - Fork 269
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
fix(connlib): exclude sentinel dns range for resources ips #4200
fix(connlib): exclude sentinel dns range for resources ips #4200
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Terraform Cloud Plan Output
|
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.
LGTM, deferring to @thomaseizinger
Performance Test ResultsTCP
UDP
|
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 you are always passing constants, why don't you just make two constructors that use those constants internally?
Seems to be easier to understand to me.
Also, can IpProvider
just move into firezone_tunnel
? Why does it sit in shared?
To be honest I still prefer passing the ranges in the constructor, seems more explicit to me. |
f05f208
to
61aafc7
Compare
It is just a level of indirection though. Unless we use it with dynamic ranges, it seems to make more sense to just make two constructors: impl IpProvider {
fn client() -> Self { }
fn gateway() -> Self { }
} |
You can always dispatch to a more generic constructor from there. |
Oh I misunderstand how this is used. Nvm the |
In the future we will want to refactor this to a builder pattern to prevent the number of parameters from growing and have them clearer but this works simply for now.
Found while discussing #4174