-
-
Notifications
You must be signed in to change notification settings - Fork 380
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:support of pass throught hosts #1397
Conversation
2a1c7a4
to
8f35c38
Compare
e78ff66
to
eeab5a0
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.
Please address the comments
withCoverage: false | ||
coverageReportPath: "" | ||
` | ||
# Example on using tests |
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 should be added in config guide. You can see how is the config guide attached to final output.
cmd/record.go
Outdated
if err != nil { | ||
if err == errFileNotFound { | ||
r.logger.Info("continuing without configuration file because file not found") | ||
r.logger.Info("Keploy config not found, using default config") |
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.
This log is wrong if keploy config is present we will not use default config. We consider only flags and continue with flags.
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.
Please provide an update on this
eeab5a0
to
a6afe99
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.
Please address the comments
pkg/hooks/loader.go
Outdated
@@ -141,6 +142,16 @@ func (h *Hook) GetProxyPort() uint32 { | |||
return h.proxyPort | |||
} | |||
|
|||
func (h *Hook) GetProxyHost() models.Stubs { |
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.
Rename the function. Current name provide wrong info
pkg/hooks/loader.go
Outdated
return h.passThroughHosts | ||
} | ||
|
||
func (h *Hook) SetProxyHosts(passThroughHosts []models.Filters) { |
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.
Rename the function
pkg/platform/yaml/yaml.go
Outdated
urlMatched = true | ||
} | ||
|
||
if len(urlMethods) != 0 { |
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 this logic has flaw lets say user has given path - x and port -y . even if there is a request with path - z and port - y it will be matched and considered as passthrough.
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 this selected code block deals with some other logic.
@@ -31,17 +32,17 @@ type HttpParser struct { | |||
} | |||
|
|||
// ProcessOutgoing implements proxy.DepInterface. | |||
func (http *HttpParser) ProcessOutgoing(request []byte, clientConn, destConn net.Conn, ctx context.Context) { | |||
func (http *HttpParser) ProcessOutgoing(request []byte, clientConn, destConn net.Conn, ctx context.Context, sourcePort int) { |
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.
Can't you extract port from clientConn or destConn..?
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.
will increase computation time, better to use a precalculated value
70ef6a4
to
77cb6cd
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.
please address the comments, rest looks fine.
cmd/record.go
Outdated
if err != nil { | ||
if err == errFileNotFound { | ||
r.logger.Info("continuing without configuration file because file not found") | ||
r.logger.Info("Keploy config not found, ontinuing without configuration") |
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.
please correct the spelling error.
cmd/test.go
Outdated
if err != nil { | ||
if err == errFileNotFound { | ||
t.logger.Info("continuing without configuration file because file not found") | ||
t.logger.Info("Keploy config not found, using default config") |
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.
Please provide an update on this.
pkg/platform/yaml/yaml.go
Outdated
return err, false | ||
} | ||
headerValueMatch := regex.MatchString(headerNameValue) | ||
if headerNameMatch || headerValueMatch { |
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.
Shouldn't this be a and condition..?
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.
earlier I thought of using either header name or value to be independent, will update it so that the combination is unique(i.e, the and condition)
|
||
} | ||
for _, filter := range portPassThrough { | ||
regex, err := regexp.Compile(filter.Path) |
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.
Is it not possible in config to provide path without port. Why is path and port are combined?
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 separate only port and combination of port +host in service layer. then in this layer we separate port+ host and only host. thus there are two loops too.
} | ||
matches := regex.MatchString(req.URL.String()) | ||
if matches && host != "" || req.Host == host { | ||
passthroughHost = true |
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.
Shouldn't we have a break statement 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.
done so
if req.Host == host { | ||
passThrough := models.PassThroughHosts | ||
portPassThrough := []models.Filters{} | ||
for _, filters := range h.GetPassThroughHosts().Filters { |
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.
similarly here too.
ab553d6
to
0c7bff4
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.
Ship it!
Related Issue
Closes: #1382
Describe the changes you've made
Genrated Yaml
Type of change
Checklist: