-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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: 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 <[email protected]>
if !writableOptionFound { | ||
writable = cfg.Gateway.Writable | ||
if writableOptionFound { | ||
cfg.Gateway.Writable = writable |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :) |
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 [email protected]