Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

TEM-226: subdomain-based proxying #22

Merged
merged 14 commits into from Mar 10, 2019

Conversation

@bobheadxi
Copy link
Contributor

bobheadxi commented Mar 3, 2019

馃懛 Purpose

allow map proxying based only on host, without /network/feature paths

馃殌 Changes

  • new config option, Domain, to set domain
  • subdomain handling:
    • {network_name}.{feature}.{domain}, for example: my-network.api.nexus.temporal.cloud

鈿狅笍 Breaking Changes

all network names with . in it will break. I can make the parsing more intelligent, or we can ban . in names. 馃

todo

  • make sure it works in practice
  • test with swarm
@bobheadxi bobheadxi requested a review from bonedaddy Mar 3, 2019
@bobheadxi bobheadxi added the pr: wip label Mar 3, 2019
@bonedaddy

This comment has been minimized.

Copy link
Member

bonedaddy commented Mar 3, 2019

I think names with a . in it should be banned. - seems like a reasonable separator to have since it keeps it all on the same subdomain.

@bobheadxi

This comment has been minimized.

Copy link
Contributor Author

bobheadxi commented Mar 4, 2019

Sounds good! That will have to be enforced elsewhere, probably in RTradeLtd/Temporal

bobheadxi added 2 commits Mar 4, 2019
This reverts commit d9cd5eb.
@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 4, 2019

Codecov Report

Merging #22 into master will increase coverage by 0.27%.
The diff coverage is 89.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #22      +/-   ##
==========================================
+ Coverage   83.16%   83.43%   +0.27%     
==========================================
  Files          27       26       -1     
  Lines        1223     1291      +68     
==========================================
+ Hits         1017     1077      +60     
- Misses        144      153       +9     
+ Partials       62       61       -1
Impacted Files Coverage 螖
config/config.go 100% <100%> (酶) 猬嗭笍
delegator/proxy.go 100% <100%> (酶) 猬嗭笍
delegator/engine.go 85.23% <85.94%> (-0.58%) 猬囷笍
delegator/context.go 0% <0%> (-100%) 猬囷笍
network/ports.go 100% <0%> (+6.9%) 猬嗭笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 1a4636e...0fb5baa. Read the comment docs.

@bonedaddy

This comment has been minimized.

Copy link
Member

bonedaddy commented Mar 4, 2019

There might be an issue with the dependencies in go.sum:

solidity@dark:~/go/src/github.com/RTradeLtd/Nexus$ go mod tidy
verifying github.com/grpc-ecosystem/go-grpc-middleware@v1.0.0: checksum mismatch
        downloaded: h1:BWIsLfhgKhV5g/oF34aRjniBHLTZe5DNekSjbAjIS6c=
        go.sum:     h1:Iju5GlWwrvL6UBg4zJJt3btmonfrMlCDdsejg4CZE7c=
r.HandleFunc("/status", e.NetworkStatus)
r.Route(fmt.Sprintf("/{%s}", keyFeature), func(r chi.Router) {
r.HandleFunc("/*", e.Redirect)
})
})

// handle subdomain-based routing
if e.domain != "" {

This comment has been minimized.

Copy link
@bonedaddy

bonedaddy Mar 4, 2019

Member

This part I'm a little confused about, shouldn't we always want to spin this up? Or is this instantiated individually for each IPFS network that gets registered?

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Mar 4, 2019

Author Contributor

Well if you don鈥檛 provide a domain it鈥檒l match for 鈥*.api.鈥 Etc Which is not useful, and potentially have all sorts of terrible side effects

This comment has been minimized.

Copy link
@bonedaddy

bonedaddy Mar 4, 2019

Member

Oh okay I guess I was confused then, this would be the domain that nexus is running behind? ie nexus.temporal.cloud so this would then match *.nexus.temporal.cloud yea?

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Mar 4, 2019

Author Contributor

The next 10 or so lines of the code show what it matches, but in config you would put nexus.temporal.cloud

Copy link
Member

bonedaddy left a comment

Looks good 馃憣

@bonedaddy

This comment has been minimized.

@bobheadxi

This comment has been minimized.

Copy link
Contributor Author

bobheadxi commented Mar 6, 2019

Ah I know the issue - will fix asap

@bobheadxi

This comment has been minimized.

Copy link
Contributor Author

bobheadxi commented Mar 6, 2019

@postables can you try again with 05035ce ? I've pushed up a change that disables the custom path manipulation that was needed to get the old proxy to work, so now it either uses the subdomain routing or the path-based routing (but not both at the same time)

If this works, I'm thinking of probably deprecating the latter, since this way is much better and more consistent with multiaddr

let me know if it works!

@bonedaddy

This comment has been minimized.

Copy link
Member

bonedaddy commented Mar 9, 2019

@bobheadxi sorry for the delay, going to try and debug the issues here

@bonedaddy bonedaddy merged commit d28e7f0 into master Mar 10, 2019
4 checks passed
4 checks passed
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
codecov/patch 89.89% of diff hit (target 50%)
Details
codecov/project 83.43% (+0.27%) compared to 1a4636e
Details
@bonedaddy bonedaddy deleted the delegator/subdomains branch Mar 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.