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

cli: add --http-addr flag #7475

Merged
merged 1 commit into from
Jul 9, 2016
Merged

cli: add --http-addr flag #7475

merged 1 commit into from
Jul 9, 2016

Conversation

glycerine
Copy link
Contributor

@glycerine glycerine commented Jun 26, 2016

For security, the --http-addr flags makes the Admin UI
bind an IP address that can be private or distinct from
the cluster (such as 127.0.0.1).

Fixes #7474.


This change is Reviewable

@glycerine
Copy link
Contributor Author

glycerine commented Jun 26, 2016

I don't grok the test failures from CI.
Update: oh I think I get it. The docker container acceptance tests depend on the UI being on the expected IP that is the same as the host/container. I kept the default the same to fix this.

@knz
Copy link
Contributor

knz commented Jun 26, 2016

@tamird @mberhault what's the best place to add a test for this flag in our acceptance tests?

@@ -33,6 +33,7 @@ const (
PasswordName = "password"
PortName = "port"
HTTPPortName = "http-port"
HTTPIPAddrName = "http-addr"
Copy link
Contributor

Choose a reason for hiding this comment

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

small nit: use HTTPAddrName, it doesn't have to be an IP address, dns names work fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, a bigger nit: let's stick to the same naming scheme as {host, port}, where we now have {http-host, http-port}.

@tamird
Copy link
Contributor

tamird commented Jun 27, 2016

Please hold off on merging this - it's going to conflict with multiple addresses / external address work, and the semantics aren't completely clear (i.e. we may want to move to specifying full addresses rather than addresses and ports).

@mberhault
Copy link
Contributor

A test would be nice, and the acceptance tests are probably the best place since we would ideally like some networking isolation.

@tamird: thoughts on the tests? Also how this may fit in with future post modifications?

@glycerine
Copy link
Contributor Author

Marc - I agree a test or two would be nice. I had the worst time trying to get the acceptance suite to run locally. I filed #7477 about that.

Tamir - okay, let me know when I might be of assistance.

@glycerine
Copy link
Contributor Author

@tamird Have the semantics been decided? I'd like to move forward on this.

Working at adding a test and fixing the nits above...

For security, the --http-addr flags makes the Admin UI
bind an IP address that can be private or distinct from
the cluster. If --host is given, we default to that
address if --http-addr is unspecified.

Fixes #7474.
@glycerine
Copy link
Contributor Author

glycerine commented Jul 8, 2016

I've added a surgically precise test/unit test. I think an acceptance test is beyond the scope of what I, with my limited resources, can attempt at this point.

Also, I've addressed the nits.

@tamird, @mberhault, @knz, would you be okay with proceeding? It's a very small change, and it fixes a very big security hole. This would unblock me from serious further testing.

If there's a big change later towards combined IPs and ports, this really doesn't add much more to change.

@knz knz self-assigned this Jul 9, 2016
@knz
Copy link
Contributor

knz commented Jul 9, 2016

:lgtm:


Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


Comments from Reviewable

@knz
Copy link
Contributor

knz commented Jul 9, 2016

@glycerine I'll take that one. The fact we want to do things perhaps in a different way in the future does not prevent us from accepting your patch now, for use in the interim.

@knz knz changed the title add --http-addr flag cli: add --http-addr flag Jul 9, 2016
@knz knz merged commit 21cf08f into cockroachdb:master Jul 9, 2016
@glycerine
Copy link
Contributor Author

Thanks @knz.

glycerine added a commit to glycerine/docs that referenced this pull request Jul 9, 2016
glycerine added a commit to glycerine/docs that referenced this pull request Jul 9, 2016
glycerine added a commit to glycerine/docs that referenced this pull request Jul 9, 2016
@glycerine glycerine deleted the jaten_httpip_7474 branch July 9, 2016 18:16
glycerine added a commit to glycerine/docs that referenced this pull request Jul 9, 2016
glycerine added a commit to glycerine/docs that referenced this pull request Jul 9, 2016
glycerine added a commit to glycerine/docs that referenced this pull request Jul 11, 2016
glycerine added a commit to glycerine/docs that referenced this pull request Jul 11, 2016
glycerine added a commit to glycerine/docs that referenced this pull request Jul 11, 2016
@glycerine
Copy link
Contributor Author

@jseldess Now that cockroachdb/docs#443 merged, I think the docs-todo label can be removed.

@jseldess
Copy link
Contributor

Thanks, @glycerine. Just tweaking a few minor things before publishing (probably early tomorrow).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants