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

Add monitor http endpoint #2511

Merged
merged 3 commits into from
Nov 28, 2016
Merged

Add monitor http endpoint #2511

merged 3 commits into from
Nov 28, 2016

Conversation

kyhavlov
Copy link
Contributor

Add the monitor api as part of #2502

Copy link
Contributor

@slackpad slackpad left a comment

Choose a reason for hiding this comment

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

Looks awesome and is nice to use!

default:
// We can't log synchronously, since we are already being invoked
// from the logWriter, and a log will need to invoke Write() which
// already holds the lock. We must therefor do the log async, so
Copy link
Contributor

Choose a reason for hiding this comment

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

sp. "therefor"

@@ -471,6 +471,7 @@ func (c *Command) setupAgent(config *Config, logOutput io.Writer, logWriter *log
c.Ui.Error(fmt.Sprintf("Error starting agent: %s", err))
return err
}
agent.logWriter = logWriter
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd plumb this into Create() - seems weird to set it here.

// from the logWriter, and a log will need to invoke Write() which
// already holds the lock. We must therefor do the log async, so
// as to not deadlock
go h.logger.Printf("[WARN] Dropping logs to monitor http endpoint")
Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of worry this could make a ton of goroutines if things got into a weird state. I'd just count dropped lines in here, and then kick out a warning up in the agent handler when you are closing out if this count > 0. That way the operator can see that this is going on, but it's super cheap and ok if we are essentially dropping all of the logs.

defer srv.agent.Shutdown()

// Begin streaming logs from the monitor endpoint
req, _ := http.NewRequest("GET", "/v1/agent/monitor?loglevel=debug", nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to do a little deeper test of the level filter. You could just make sure you get an error for an invalid level which proves it gets plumbed down.

resp.WriteHeader(405)
return nil, nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We are going to add an ACL here, but I feel like this is a bit of a security vuln until we do that. Just so we don't expose anything weird while that work is going on in master, lets take a token here and call the Raft endpoint:

https://github.com/hashicorp/consul/blob/master/command/agent/operator_endpoint.go#L23-L26

This'll vet that they have operator read privs if it doesn't return an error (just throw away the response), which protects this with something while we develop ACLs.

@slackpad
Copy link
Contributor

LGTM

@kyhavlov kyhavlov merged commit 8079c49 into master Nov 28, 2016
@kyhavlov kyhavlov deleted the f-monitor-api branch November 28, 2016 23:36
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.

2 participants