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

Document the --http-addr flag. #443

Merged
merged 1 commit into from
Jul 11, 2016

Conversation

glycerine
Copy link
Contributor

@glycerine glycerine commented Jul 9, 2016

Accompanies the merged fix cockroachdb/cockroach#7475


This change is Reviewable

@glycerine
Copy link
Contributor Author

@jseldess could you take a look?

Also CircleCI says s3 sync failed, not sure what that means. Let me know if I need to revise something.

@glycerine glycerine force-pushed the jaten_docs_7475 branch 2 times, most recently from 73a5cc0 to 8bb749d Compare July 9, 2016 18:56
@uptimeDBA
Copy link
Contributor

I don't think the CircleCI fail is your fault @glycerine, I hit the same problem in PR #417.
From the comments: "circleci doesn't push aws credentials for fork PRs do avoid leaking our credentials (which makes sense). I'm not sure there's a good way around that, other than a completely public AWS bucket which isn't a great solution."
The short answer is the S3 sync part of the CircleCI process doesn't work for external contributors that are working from a fork of docs.
@jseldess is going to update the Contributing to CockroachDB Docs to reflect this.

@jseldess
Copy link
Contributor

Thanks for the update, @glycerine! And sorry for my delay.

See comments below, but my overall sense is that we should demonstrate this flag not on start-a-local-cluster.md or secure-a-cluster.md, both of which focus on local clusters listening on localhost, but rather manual-deployment.md, which focuses on an insecure and secure distributed cluster.

Paul's right about the CircleCI failure; it's out of your control. It's actually already mentioned in the docs' contributing.md file, but it's pretty buried. I think we'll need to find a better solution soonish.


Reviewed 3 of 3 files at r1.
Review status: 1 of 3 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


secure-a-cluster.md, line 49 [r1] (raw file):

    ~~~ shell
    $ cockroach start --ca-cert=certs/ca.cert --cert=certs/node.cert --key=certs/node.key --background --http-addr=127.0.0.1

@glycerine, this page and start-a-local-cluster.md assume you're running a local cluster on localhost only, so I don't think this is the best place to feature the --http-addr flag. Instead, would you mind working it into the manual-deployment.md page? I'm also happy to take this on, if you prefer.


secure-a-cluster.md, line 68 [r1] (raw file):

    ~~~ shell
    $ cockroach start --store=node2 --port=26258 --http-port=8081 --http-addr=127.0.0.1 --join=localhost:26257 --ca-cert=certs/ca.cert --cert=certs/node.cert --key=certs/node.key --background

Same as above. This page probably isn't the best place for this flag.


secure-a-cluster.md, line 69 [r1] (raw file):

    ~~~ shell
    $ cockroach start --store=node2 --port=26258 --http-port=8081 --http-addr=127.0.0.1 --join=localhost:26257 --ca-cert=certs/ca.cert --cert=certs/node.cert --key=certs/node.key --background
    $ cockroach start --store=node3 --port=26259 --http-port=8082 --http-addr=127.0.0.1 --join=localhost:26257 --ca-cert=certs/ca.cert --cert=certs/node.cert --key=certs/node.key --background

Same as above.


secure-a-cluster.md, line 122 [r1] (raw file):

    When you're done using the SQL shell, press **CTRL + D** to exit.

7.  Reopen the [Admin UI](explore-the-admin-ui.html) by establishing an SSH tunnel `ssh -L 8080:127.0.0.1:8080 ROACHs-MBP` (substitute your first node's address for ROACHs-MBP). Then point your browser at `https://127.0.0.1:8080`. You can also find the address in the `admin` field in the standard output of any node on startup. 

Same as above.


start-a-local-cluster.md, line 48 [r1] (raw file):

- Communication is insecure, with the server listening only on `localhost` on port `26257` for internal and client communication and on port `8080` for HTTP requests from the Admin UI. 
   - To bind to different ports, set `--port=<port>` and `--http-port=<port>`. 
   - To bind the Admin web UI to a private IP address or host, set `--http-addr=<private-addr>`.

If you're willing to demonstrate this flag's usage in manual-deployment.md, how about we add this, like in the bullet below: "For a demonstration, see Manual Deployment."


start-a-node.md, line 37 [r1] (raw file):

`--host` | The address to listen on for internal and client communication. The node also advertises itself to other nodes using this address. Therefore, if it is a hostname, it must be resolvable from all nodes, and if it is an IP address, it must be routable from all nodes.<br><br>When running an insecure local cluster (without `--insecure` and without cert flags), this defaults to `localhost` and cannot be changed. When running an insecure distributed cluster (with `--insecure` but without cert flags) or a secure local or distributed cluster (without `--insecure` but with cert flags), this can be an external address.
`--http-port` | The port to listen on for HTTP requests from the Admin UI. <br><br>**Default:** 8080
`--http-addr` | The IP address or hostname to listen on for HTTP requests for the Admin UI. <br><br>**Default:** same as --host

nits: change "for" to "from"


Comments from Reviewable

@jseldess
Copy link
Contributor

Review status: 1 of 3 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


start-a-node.md, line 93 [r1] (raw file):

Start a distributed cluster

@glycerine, should we add http-addr to this example as well? Since the cluster would be listening on external address, you might want to use a private address for the admin UI.


Comments from Reviewable

@jseldess
Copy link
Contributor

Review status: 1 of 3 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.


secure-a-cluster.md, line 49 [r1] (raw file):

Previously, jseldess wrote…

@glycerine, this page and start-a-local-cluster.md assume you're running a local cluster on localhost only, so I don't think this is the best place to feature the --http-addr flag. Instead, would you mind working it into the manual-deployment.md page? I'm also happy to take this on, if you prefer.

@glycerine, my apologies. I was confused here. Since we're starting a node with certs, it listens on all interfaces by default, so it does make sense to demonstrate using `http-addr` to have the admin ui listen on a specific address.

That said, all good here. One nit: Please move the flag before --background, similar to the start commands below.


secure-a-cluster.md, line 61 [r1] (raw file):

    <div id="details-secure3" class="collapse" markdown="1">

    This command restarts your first node with its existing data, but securely. The command is the same as before but now uses the additional `--ca-cert`, `--cert`, and `--key` flags to point to the CA certificate and the node certificate and key created in step 2.

Could you add a sentence about the http-addr flag here?


secure-a-cluster.md, line 75 [r1] (raw file):

    <div id="details-secure4" class="collapse" markdown="1">

    These commands restart additional nodes with their existing data, but securely. The commands are the same as before but now uses the additional `--ca-cert`, `--cert`, and `--key` flags to point to the CA certificate and the node certificate and key created in step 2.

Could you add a sentence about http-addr here as well?


Comments from Reviewable

@glycerine glycerine force-pushed the jaten_docs_7475 branch 2 times, most recently from 9fd1687 to c27403e Compare July 11, 2016 20:19
@glycerine
Copy link
Contributor Author

Review status: 1 of 4 files reviewed at latest revision, 9 unresolved discussions.


secure-a-cluster.md, line 49 [r1] (raw file):

Previously, jseldess wrote…

@glycerine, my apologies. I was confused here. Since we're starting a node with certs, it listens on all interfaces by default, so it does make sense to demonstrate using http-addr to have the admin ui listen on a specific address.

That said, all good here. One nit: Please move the flag before --background, similar to the start commands below.

done.

secure-a-cluster.md, line 61 [r1] (raw file):

Previously, jseldess wrote…

Could you add a sentence about the http-addr flag here?

Done.

secure-a-cluster.md, line 68 [r1] (raw file):

Previously, jseldess wrote…

Same as above. This page probably isn't the best place for this flag.

per above, we'll do both.

secure-a-cluster.md, line 69 [r1] (raw file):

Previously, jseldess wrote…

Same as above.

per above, we'll do both.

secure-a-cluster.md, line 75 [r1] (raw file):

Previously, jseldess wrote…

Could you add a sentence about http-addr here as well?

Done.

secure-a-cluster.md, line 122 [r1] (raw file):

Previously, jseldess wrote…

Same as above.

Done.

start-a-local-cluster.md, line 48 [r1] (raw file):

Previously, jseldess wrote…

If you're willing to demonstrate this flag's usage in manual-deployment.md, how about we add this, like in the bullet below: "For a demonstration, see Manual Deployment."

Done.

start-a-node.md, line 37 [r1] (raw file):

Previously, jseldess wrote…

nits: change "for" to "from"

The 'cockroach start' process is the one serving on the bound address, so that 'for' makes way more sense than 'from'. I edited the --http-port command as well to remove that same confusion.

start-a-node.md, line 93 [r1] (raw file):

Previously, jseldess wrote…

@glycerine, should we add http-addr to this example as well? Since the cluster would be listening on external address, you might want to use a private address for the admin UI.

Done.

Comments from Reviewable

@glycerine
Copy link
Contributor Author

glycerine commented Jul 11, 2016

Hi @jseldess,

I think I've addressed everything.

The 'for' vs. 'from' comment seemed to indicate a misunderstanding of when the cockroach process is serving (under 'start') and when it is acting as client (under 'sql'). Since we're talking about the server, I tried to clarify this language on both the port and the addr summary lines.

With that said, I'd be happy to just turn any other editing you'd like over to you to proceed as you feel best.

  • Jason

@jseldess
Copy link
Contributor

:lgtm: Thanks again, Jason.


Review status: 1 of 4 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.


start-a-node.md, line 37 [r1] (raw file):

Previously, glycerine (Jason E. Aten, Ph.D.) wrote…

The 'cockroach start' process is the one serving on the bound address, so that 'for' makes way more sense than 'from'. I edited the --http-port command as well to remove that same confusion.

Thanks, Jason. This is better. I might end up using the language in the CLI help text, but I'll figure it out in a separate pr.

Comments from Reviewable

@jseldess
Copy link
Contributor

Reviewed 2 of 3 files at r2.
Review status: 3 of 4 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.


Comments from Reviewable

@jseldess jseldess merged commit b81f5f7 into cockroachdb:gh-pages Jul 11, 2016
@@ -142,17 +142,17 @@ Store the CA key somewhere safe and keep a backup; if you lose it, you will not
Copy the `cockroach` binary, CA certificate, and node 1 certificate and key to the first machine and then start the node:

~~~ shell
$ cockroach start --ca-cert=ca.cert --cert=node1.cert --key=node1.key --host=<node1-hostname>
$ cockroach start --http-addr=127.0.0.1 --ca-cert=ca.cert --cert=node1.cert --key=node1.key --host=<node1-hostname>
Copy link
Contributor

Choose a reason for hiding this comment

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

throughout: we should avoid "127.0.0.1" and use "localhost" instead for ipv4/ipv6 agnosticism.

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed by #447

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