gateway: clean up its surface, and remove BlockList #2874
Conversation
This patch is in preparation for the gateway's extraction. It's interesting to trace technical debt back to its origin, understanding the circumstances in which it was introduced and built up, and then cutting it back at exactly the right places. - Clean up the gateway's surface The option builder GatewayOption() now takes only arguments which are relevant for HTTP handler muxing, i.e. the paths where the gateway should be mounted. All other configuration happens through the GatewayConfig object. - Remove BlockList I know why this was introduced in the first place, but it never ended up fulfilling that purpose. Somehow it was only ever used by the API server, not the gateway, which really doesn't make sense. It was also never wired up with CLI nor fs-repo. Eventually @krl started punching holes into it to make the Web UI accessible. - Remove --unrestricted-api This was holes being punched into BlockList too, for accessing /ipfs and /ipn on the API server. With BlockList removed and /ipfs and /ipns freely accessible, putting this option out of action is safe. With the next major release, the option can be removed for good. License: MIT Signed-off-by: Lars Gierth <larsg@systemli.org>
| if !writableOptionFound { | ||
| writable = cfg.Gateway.Writable | ||
| if writableOptionFound { | ||
| cfg.Gateway.Writable = writable |
There was a problem hiding this comment.
As we are using defaults, the if block can be removed and just assign to cfg.Gateway.Writable.
There was a problem hiding this comment.
Mh okay, we shouldn't be using Default(false) with this one. From what I can tell, --writable if present is supposed to override cfg.Gateway.Writable.
There was a problem hiding this comment.
So... the second return value (writableOptionFound) is separate from defaults -- it tells specifically whether the option was passed on the CLI.
So this assumption is still true and everything's good as-is:
From what I can tell, --writable if present is supposed to override cfg.Gateway.Writable.
In other words, the default value for --writable is never used -- only if it has actually been passed.
|
overall this LGTM, its cleaner. |
Gonna investigate this before merging, hold off |
ghost
mentioned this pull request
All good - RFM! |
| test_expect_success "GET IPFS path on API forbidden" ' | ||
| test_curl_resp_http_code "http://127.0.0.1:$apiport/ipfs/$HASH" "HTTP/1.1 403 Forbidden" | ||
| ' | ||
|
|
There was a problem hiding this comment.
I thought this entire PR was reverted. I guess it was not because this test is not in master.
cc @lgierth we need this back, i think.
There was a problem hiding this comment.
test has been moved here https://github.com/ipfs/go-ipfs/blob/master/test/sharness/t0400-api-security.sh (because it's not a gateway test)
|
Were there other changes to gateway code in this release? If not, I think i would feel safer if all gateway changes were rebased out and placed at the end together so we can analyze them very clearly. (i guess we can diff the whole release in these files, but making sure this is safe may be worth rewriting history over...) |
|
Right but my question is: how do we know the behaviors are all back to the
|
But, but... I removed lots of code and the blocking logic is literally 7 lines of code now. [1] It wires I think it's is a net win, making this thing much more obvious. Its obscurity was the reason why I was able to be pretty sure it was dead code, and introduce KeyThief. [1] daemon.go |
well I not actually introduced it, but at least opened the gates. Anyway that's a different topic :) |
ghost
mentioned this pull request
This patch is in preparation for the gateway's extraction.
It's interesting to trace technical debt back to its
origin, understanding the circumstances in which it
was introduced and built up, and then cutting it back
at exactly the right places.
The option builder GatewayOption() now takes only
arguments which are relevant for HTTP handler muxing,
i.e. the paths where the gateway should be mounted.
All other configuration happens through the
GatewayConfig object.
I know why this was introduced in the first place,
but it never ended up fulfilling that purpose.
Somehow it was only ever used by the API server,
not the gateway, which really doesn't make sense.
It was also never wired up with CLI nor fs-repo.
Eventually @krl started punching holes into it
to make the Web UI accessible.
This was holes being punched into BlockList too,
for accessing /ipfs and /ipn on the API server.
With BlockList removed and /ipfs and /ipns freely
accessible, putting this option out of action
is safe. With the next major release,
the option can be removed for good.
License: MIT
Signed-off-by: Lars Gierth larsg@systemli.org