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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basics around deployment #5

Merged
merged 12 commits into from
Oct 6, 2023
Merged

Basics around deployment #5

merged 12 commits into from
Oct 6, 2023

Conversation

aschmahmann
Copy link
Contributor

This has mostly been copied from bifrost-gateway. Main changes are:

  • add Kubo RPC URL env vars and /api/v0 support via
    delegation/forwarding
  • add DNS resolution caching layer
  • add tracing and metrics
  • add debugging and metrics endpoints
  • add Dockerfile
  • switched cli flags to only expose the port and always run on localhost

@BigLep
Copy link
Contributor

BigLep commented Oct 4, 2023

I'm forgetting why we need /api/v0 support. Any chance we can not propagate that forward?

@aschmahmann
Copy link
Contributor Author

I'm forgetting why we need /api/v0 support. Any chance we can not propagate that forward?

They're only really needed if someone wanted to deploy this to ipfs.io. The last time we looked people were still using them. If that's not true anymore then dropping support everywhere (kubo, bifrost-gateway, here) sounds good.

Here's the issue where this was tracked for bifrost-gateway with links to the analysis ipfs-inactive/bifrost-gateway#13. To be fair time has passed though and this might not continue to be valid.

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is fine to keep newKuboRPCHandler. We've sunk time on this earlier this year and it provides backward-compatibility + metrics that show its use.

(We can remove it later, when we also remove it from gateway port in Kubo, or we have no other use for Kubo RPC in rainbow).

Small nits inline, but fine to proceed I think.

docker/get-docker-tags.sh Outdated Show resolved Hide resolved
docker/get-docker-tags.sh Outdated Show resolved Hide resolved
docs/environment-variables.md Outdated Show resolved Hide resolved
docs/environment-variables.md Outdated Show resolved Hide resolved
Value: ":8080",
&cli.IntFlag{
Name: "gateway-port",
Value: 8080,
Copy link
Member

@lidel lidel Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 maybe we should pick a different default port than Kubo and bifrost-gateway, for the sake of local dev's sanity, as I know I will run all three of them this quarter many times at the same time.

Suggested change
Value: 8080,
Value: 8090,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm for the sake of drop-in I wouldn't...

Value: "127.0.0.1:8081",
&cli.IntFlag{
Name: "api-port",
Value: 8081,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, avoid clash with metrics port of bifrost-gateway.

Suggested change
Value: 8081,
Value: 8091,

main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
refresher *time.Ticker
}

func newCachedDNS(refreshInterval time.Duration) *cachedDNS {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally we would install a local resolver with its own caching in place but it wouldn't be needed in any case with this, iirc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, if we want to drop this and just have the OS take care of it that's also totally fine. Leaving it for now though

aschmahmann and others added 3 commits October 5, 2023 11:45
* add Kubo RPC URL env vars and /api/v0 support via
  delegation/forwarding
* add DNS resolution caching layer
* add tracing and metrics
* add debugging and metrics endpoints
* add Dockerfile
* switched cli flags to only expose the port and always run on localhost
@hsanjuan
Copy link
Contributor

hsanjuan commented Oct 6, 2023

Let's merge and pick up from here with smaller things?

@aschmahmann
Copy link
Contributor Author

Yep, I'm fine to merge as is and we can discuss some of the other changes later if there's still some contention (e.g. default ports, if we want to test other DHT configurations, etc.). This seems like it should be fine though.

Note: it'd be nice to add some more CI testing here 😅, but can do that in a follow up.

Someone can hit merge once CI is green (still running for me)

@aschmahmann aschmahmann merged commit dad528b into main Oct 6, 2023
9 checks passed
@aschmahmann aschmahmann deleted the feat/deployment-basics branch October 6, 2023 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants