-
Notifications
You must be signed in to change notification settings - Fork 29
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
Gateway spins up Ngrok CRDs #343
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @Abdiramen and the rest of your teammates on |
ce63e7b
to
0433734
Compare
47f2cf9
to
64e0b2c
Compare
d3d394d
to
810522a
Compare
2c502b4
to
0ecef97
Compare
Agreed, we'll have to get to that after the first release! |
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.
my bits lgtm
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.
a few comments 🤔
ea4f07d
to
06b158b
Compare
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.
more comments 👍
if filter.Path == nil { | ||
from := ".*" //"^https?://[^/]+(:[0-9]*)?(/[^\\?]*)?(\\?.*)?$" | ||
to := fmt.Sprintf("%s://%s%s$uri", scheme, hostname, port) | ||
err := d.createUrlRedirectConfig(from, to, requestHeaders, filter.StatusCode, actions) |
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 like you can just do
return d.createUrlRedirectConfig(from, to, requestHeaders, filter.StatusCode, actions)
here 🤷
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.
same further down 🤷
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'll cut a new ticket to clean up the rest
d.log.Error(err, "cannot convert request redirect filter to json", "HTTPRequestRedirectFilter", urlRedirectAction) | ||
return err | ||
} | ||
actions.endpointActions = append( |
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.
be great if we hide this behind Add
🤷
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'll cut a new ticket to clean up the rest
case gatewayv1.HTTPRouteFilterExtensionRef: | ||
return nil, errors.NewErrorNotFound(fmt.Sprintf("Unsupported filter HTTPRouteFilterType %v found", filter.Type)) | ||
default: | ||
return nil, errors.NewErrorNotFound(fmt.Sprintf("Unknown filter HTTPRouteFilterType %v found", filter.Type)) |
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.
errors.NewErrorNotFound
doesn't seem quite appropriate here, wdyt?
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'll cut a new ticket to add new errors
} | ||
|
||
if len(match.QueryParams) > 0 { | ||
d.log.Error(fmt.Errorf("Unsupported match type"), "Unsupported match type", "HTTPQueryParamMatch", match.QueryParams) |
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.
should there return errors here? (and above)
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.
yes, agreed, it should throw an error here.
|
||
for _, gtw := range gateways { | ||
for _, listener := range gtw.Spec.Listeners { | ||
if listener.Hostname == nil { |
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.
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.
good catch, i'll add all of these to a follow up ticket
What
Domains
CRDs spun up byGateway
HTTPSEdges
spun up byHTTPRoute
attached to aGateway
:HTTPSEdgeRoute
created on anHTTPSEdge
corresponding to aHTTPRouteRule
How
Gateway
into cachestore, store, and driverHTTPRoute
into cachestore, store, and driverBreaking Changes
Nope, but the gateway currently just throws an error, and is prone to change!