Add websocket support to access envelope

[stephen-soltesz]

This change adds websocket support to the access envelope to simplify client development.

This change also adds an address.NullManager to disable iptables management when -envelop.tokens-required=false. Without this addition, the default address.Manager blocks all network access.

This change introduces a new package with chanio.ReadOnce that supports reading from an io.Reader via a returned channel. This allows callers to use a select loop with other channels.

---

This change is 

[coveralls]

Pull Request Test Coverage Report for Build 104

• 107 of 107 (100.0%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.05%) to 99.656%

---

Totals
Change from base Build 90:	0.05%
Covered Lines:	579
Relevant Lines:	581

---

💛 - Coveralls

[stephen-soltesz]

I don't understand why coverage fell. There is a test case that go coverage visits locally and in travis, but evidently goveralls doesn't pick it up. I'm not sure why.

[gfr10598]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @gfr10598 and @stephen-soltesz)

---

chanio/reader.go, line 10 at r2 (raw file):

	go func() {
		b := make([]byte, 1)
		// Block on read. Will return on EOF or when client sends data (which we don't want).

... (which is discarded). ?

---

cmd/envelope/main.go, line 89 at r2 (raw file):

func (env *envelopeHandler) AllowRequest(rw http.ResponseWriter, req *http.Request) {
	// AllowRequest is a state-changing POST method.
	if req.Method != http.MethodGet {

I'm confused - is it supposed to be a Get or a Post? See comment.

---

cmd/envelope/main.go, line 111 at r2 (raw file):

	}

	allow := net.ParseIP(host)

"allow" seems an odd name here

---

cmd/envelope/main.go, line 128 at r2 (raw file):

		logx.Debug.Println("setup websocket conn failed")
		rw.WriteHeader(http.StatusInternalServerError)
		rtx.PanicOnError(env.Revoke(allow), "Failed to remove rule for "+allow.String())

Should this also have a TODO panic... ?

---

cmd/envelope/main.go, line 134 at r2 (raw file):

	// At this point, we want a few things to happen:
	// * return from the handler so the client http request can return.
	// * wait asynchronously on the hijacked conn so the client can

I'm missing something. What is asynchronous?

---

cmd/envelope/main.go, line 139 at r2 (raw file):

	// TODO: handle panic.
	rtx.PanicOnError(env.Revoke(allow), "Failed to remove rule for "+allow.String())

I feel like I've asked before - the plan is to do something less extreme than panic?

---

cmd/envelope/main.go, line 187 at r2 (raw file):

func (env *envelopeHandler) wait(ctx context.Context, c *websocket.Conn, dl time.Time) {
	// NOTE: we are explicitly ignoring the error value from SetDeadline to

FYI: You could get the error and process it after defer c.Close()

[stephen-soltesz]

PTAL?

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @gfr10598)

---

chanio/reader.go, line 10 at r2 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

... (which is discarded). ?

Done.

---

cmd/envelope/main.go, line 89 at r2 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

I'm confused - is it supposed to be a Get or a Post? See comment.

Arg. it used to be POST b/c it was state changing. But, websockets require the GET method. Fixed the comment.

---

cmd/envelope/main.go, line 111 at r2 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

"allow" seems an odd name here

changed to "remote".

---

cmd/envelope/main.go, line 128 at r2 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

Should this also have a TODO panic... ?

Done.

---

cmd/envelope/main.go, line 134 at r2 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

I'm missing something. What is asynchronous?

Another out of date comment. Fixed.

---

cmd/envelope/main.go, line 139 at r2 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

I feel like I've asked before - the plan is to do something less extreme than panic?

The plan is to catch the panic to prevent the process from exiting. At that point we would need to either fail-stop accepting new connections (and take the node out of rotation) or reset the environment state to a known-good configuration (without dangling iptables rules for any client IPs).

---

cmd/envelope/main.go, line 187 at r2 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

FYI: You could get the error and process it after defer c.Close()

I believe all error cases here would be covered by the chanio.ReadOnce logic below. Is there an advantage I'm not seeing?

[gfr10598]

Reviewable status: 1 change requests, 0 of 1 approvals obtained

---

cmd/envelope/main.go, line 187 at r2 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

I believe all error cases here would be covered by the chanio.ReadOnce logic below. Is there an advantage I'm not seeing?

Only that the comment suggests you couldn't process the error.

[gfr10598]

Reviewed 2 of 2 files at r3.
Reviewable status: complete! 1 of 1 approvals obtained

[stephen-soltesz]

Reviewable status: complete! 1 of 1 approvals obtained

---

cmd/envelope/main.go, line 187 at r2 (raw file):

Previously, gfr10598 (Gregory Russell) wrote…

Only that the comment suggests you couldn't process the error.

I think you're right about the implication of the comment. I think that's another example of a comment that was vestigial. I've rephrased the comment. I don't believe there's much benefit from checking for any setdeadline error early. But, this won't be the last change to this repo, so we can revisit if needed.