-
Notifications
You must be signed in to change notification settings - Fork 582
API for Default Gateways #3887
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
API for Default Gateways #3887
Conversation
/cc |
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.
Looking good. I have a few comments.
The main thing that I need to think about is semantically how we seem to be pushing for "Default Gateway" meaning "one Gateway, which is the default", whereas I wonder if we should be looking at it more like "by default, this Gateway will accept unbound routes given a scope".
Do we really want DefaultGateway
or do we actually want AcceptRoutesWithNoParentRefsWithinScope<Scope>: true
?
I made some comments about this, and I'm interested in hearing your perspective 🤔
github won't let me comment on the pre-existing parts of the GEP, but:
|
Signed-off-by: Flynn <[email protected]>
Signed-off-by: Flynn <[email protected]>
Signed-off-by: Flynn <[email protected]>
Signed-off-by: Flynn <[email protected]>
Signed-off-by: Flynn <[email protected]>
Signed-off-by: Flynn <[email protected]>
I believe that the correct text addresses all the feedback, so I've marked this Implementable. I suspect that folks may feel strongly about my choices for |
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.
I kind of liked the previous approach, but this one is more explicit, easier to define behavior for, and easier to write validation policies to disallow if Ian wants to.
/approve
/hold for further review.
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.
In the spirit of recent discussions about unclear feedback I want to be explicit: I don't think we should do this.
I don't feel the benefits outweigh the complexity and risk. I don't think its an issue with specific choices the implementation has made, I just think the idea is not one we should do.
It seems I am in the minority, and this is not harmful to my implementation or anything (we can easily implement it just like anyone else can), so not blocking. Just wanted to share my 2c.
Signed-off-by: Flynn <[email protected]>
I mean, I did too, but I can't argue myself out of needing to treat it as a breaking change. 🙁 Good candidate for v2. 😉 |
I appreciate both the candor and the willingness to disagree and commit. 🙂 |
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.
Thanks @kflynn, this is really well written!
@howardjohn I definitely see the complexity here, can you add more detail about what risks this could introduce for us? |
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.
I think this is very well thought out, and is close to ready to start implementing and experimenting with it. I have a couple of blocking comments regarding type choices which I would like us to discuss through before any merge.
Thank you @kflynn 🖖
Signed-off-by: Flynn <[email protected]>
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.
We came to some consensus on the types. There are still some changes to make, but I want to approve because I want to unblock, and think we're heading in a decent direction to get this into an Experimental
state and start letting people try it out and provide feedback.
So once all the comments are resolved:
/approve
Thanks for all the hard work and conversations @kflynn 🖖
…All. Update isDefault to defaultScope; switch both it and useDefaultGateway to scopes instead of bools. Signed-off-by: Flynn <[email protected]>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kflynn, shaneutt, youngnick The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Flynn <[email protected]>
Thanks @kflynn! /lgtm |
I think we've got enough approvals here. /hold cancel |
Add API description to GEP-3793. I'm sure this'll be totally noncontroversial. 😇
/kind gep