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.

Sign up for GitHub

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

Jump to bottom

storage/cloud: Make it possible to disable external http storage #44900

Merged
merged 1 commit into from
Feb 21, 2020
Merged

storage/cloud: Make it possible to disable external http storage #44900

merged 1 commit into from
Feb 21, 2020

Conversation

miretskiy
Copy link
Contributor

@miretskiy miretskiy commented Feb 9, 2020 •
edited
Loading

Informs #44320

Make it possible to disable external http storage.

This change enables the operators to disable access
to external http servers as well as custom http endpoints
overrides for cloud storage implementations.

Release note (security update): Make it possible for operators
to disable external http access when performing certain
operations (BACKUP, IMPORT, etc).

The external http access, as well as custom http endpoints, are
disabled by providing an --external-io-disable-http flag.

This flag provides a light weight option to disable http external
access in the environments where running a full fledged proxy
server may not be feasible. If running a proxy service is
acceptible, operators may choose to start cockroach binary
specifying HTTP(s)_PROXY environment setting.

@miretskiy miretskiy requested a review from dt February 9, 2020 02:32
@miretskiy miretskiy requested a review from a team as a code owner February 9, 2020 02:32
@cockroach-teamcity
Copy link
Member

This change is Reviewable

dt
dt reviewed Feb 11, 2020
View reviewed changes
pkg/cli/flags.go Outdated
@@ -360,6 +403,14 @@ func init() {
// We share the default with the ClientInsecure flag.
BoolFlag(f, &startCtx.serverInsecure, cliflags.ServerInsecure, startCtx.serverInsecure)

// Enable/disable various external storage endpoints.
// TODO(yevgeniy): currently we enable all schemas by default, but we should probably
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this point about defaults -- I think we want s3/gcs/etc to "just work" out of the box, and I think the default is probably to have most of them on. Indeed, I'm sort of inclined to invert the struct fields to be DisableFoo form so the zero-value is everything on, since flipping one off, like HTTP, is expected to be the exception, not the rule?

I donno, maybe @mwang1026 has thoughts

miretskiy
miretskiy commented Feb 11, 2020
View reviewed changes
Copy link
Contributor Author

@miretskiy miretskiy left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @mwang1026)

pkg/cli/flags.go, line 407 at r1 (raw file):

Previously, dt (David Taylor) wrote…

I'm not sure about this point about defaults -- I think we want s3/gcs/etc to "just work" out of the box, and I think the default is probably to have most of them on. Indeed, I'm sort of inclined to invert the struct fields to be DisableFoo form so the zero-value is everything on, since flipping one off, like HTTP, is expected to be the exception, not the rule?

I donno, maybe @mwang1026 has thoughts

I started off with just that... Disable http.
However, I kinda feel like if you want to be paranoid, you might want to disable most of them.
Or at least want to be able to disable any of them...

@mwang1026
Copy link

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @mwang1026)

pkg/cli/flags.go, line 407 at r1 (raw file):

Previously, dt (David Taylor) wrote…

I'm not sure about this point about defaults -- I think we want s3/gcs/etc to "just work" out of the box, and I think the default is probably to have most of them on. Indeed, I'm sort of inclined to invert the struct fields to be DisableFoo form so the zero-value is everything on, since flipping one off, like HTTP, is expected to be the exception, not the rule?
I donno, maybe @mwang1026 has thoughts

I started off with just that... Disable http.
However, I kinda feel like if you want to be paranoid, you might want to disable most of them.
Or at least want to be able to disable any of them...

If the action is to disable that makes sense to me for it to be DisableFoo. If you are paranoid you're probably willing to do the work to disable.

@miretskiy miretskiy changed the title storage/cloud: Make it possible to disable external storage storage/cloud: Make it possible to disable external http storage Feb 18, 2020
dt
dt approved these changes Feb 19, 2020
View reviewed changes
pkg/storage/cloud/s3_storage.go Outdated
) (ExternalStorage, error) {
if conf == nil {
return nil, errors.Errorf("s3 upload requested but info missing")
}
region := conf.Region
config := conf.Keys()
if conf.Endpoint != "" {
if ioConf.DisableHTTP {
return nil, errors.New("Custom endpoints disallowed for s3")
Copy link
Member

Choose a reason for hiding this comment

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

nit: imo this error would be a bit more helpful if it explained why -- that the http disallow flag was the reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

additionally, error messages do not start with a capital - they are not sentences and can be embedded in larger sentences

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@knz
Copy link
Contributor

knz commented Feb 19, 2020

The commit message is a bit messed up:

  • the first line should be the commit message title
  • the "Informs" line should be at the beginning, otherwise it becomes part of the release note.

Check this: https://wiki.crdb.io/wiki/spaces/CRDB/pages/73072807/Git+Commit+Messages

This change enables the operators to disable access
6bb33e6 
to external http servers as well as custom http endpoints
overrides for cloud storage implementations.

Informs cockroachdb#44320

Release note (security update): Make it possible for operators
to disable external http access when performing certain
operations (BACKUP, IMPORT, etc).

The external http access, as well as custom http endpoints, are
disabled by providing an --external-io-disable-http flag.

This flag provides a light weight option to disable http external
access in the environments where running a full fledged proxy
server may not be feasible.  If running a proxy service is
acceptible, operators may choose to start cockroach binary
specifying HTTP(s)_PROXY environment setting.
@miretskiy miretskiy requested a review from knz February 20, 2020 13:26
@miretskiy
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 21, 2020

Build succeeded

@craig craig bot merged commit 9f79943 into cockroachdb:master Feb 21, 2020
spaskob pushed a commit to spaskob/cockroach that referenced this pull request Feb 24, 2020
@spaskob
Merge cockroachdb#44900
8a56c27 
44900: storage/cloud: Make it possible to disable external http storage r=miretskiy a=miretskiy

Informs cockroachdb#44320

Make it possible to disable external http storage.

This change enables the operators to disable access
 to external http servers as well as custom http endpoints
 overrides for cloud storage implementations.

Release note (security update): Make it possible for operators
to disable external http access when performing certain
operations (BACKUP, IMPORT, etc).

The external http access, as well as custom http endpoints, are
disabled by providing an --external-io-disable-http flag.

This flag provides a light weight option to disable http external
access in the environments where running a full fledged proxy
server may not be feasible.  If running a proxy service is
acceptible, operators may choose to start cockroach binary
specifying HTTP(s)_PROXY environment setting.

Co-authored-by: Yevgeniy Miretskiy <yevgeniy@cockroachlabs.com>
@jseldess jseldess mentioned this pull request Mar 2, 2020
Closed
@miretskiy miretskiy deleted the disable_external branch April 27, 2020 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dt dt dt approved these changes

@knz knz Awaiting requested review from knz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@miretskiy @cockroach-teamcity @mwang1026 @knz @dt