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 options to hide args and env vars #2306

Merged
merged 4 commits into from Mar 7, 2017

Conversation

mikebryant
Copy link
Contributor

To allow for use of weave-scope in an unauthenticated environment,
add options to the probe to hide comand line arguments and
environment variables, which might contain secret data.

Fixes #2222

To allow for use of weave-scope in an unauthenticated environment,
add options to the probe to hide comand line arguments and
environment variables, which might contain secret data.

Fixes #2222
@2opremio 2opremio self-assigned this Mar 6, 2017
@2opremio
Copy link
Contributor

2opremio commented Mar 6, 2017

Thanks a lot for this! I will try to review it before of the day (Judging from a preliminary, quick look it seems to be good. You even included tests!).

@@ -94,7 +96,7 @@ func newDockerClient(endpoint string) (Client, error) {
}

// NewRegistry returns a usable Registry. Don't forget to Stop it.
func NewRegistry(interval time.Duration, pipes controls.PipeClient, collectStats bool, hostID string, handlerRegistry *controls.HandlerRegistry, dockerEndpoint string) (Registry, error) {
func NewRegistry(interval time.Duration, pipes controls.PipeClient, collectStats bool, hostID string, handlerRegistry *controls.HandlerRegistry, dockerEndpoint string, noCommandLineArguments bool, noEnvironmentVariables bool) (Registry, error) {

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@2opremio
Copy link
Contributor

2opremio commented Mar 6, 2017

LGTM (modulo minor comment)

registry, _ := docker.NewRegistry(10*time.Second, nil, false, "", hr, "")
registry, _ := docker.NewRegistry(docker.RegistryOptions{
Interval: 10 * time.Second,
Pipes: nil,

This comment was marked as abuse.

registry, _ := docker.NewRegistry(10*time.Second, nil, true, "", hr, "")
registry, _ := docker.NewRegistry(docker.RegistryOptions{
Interval: 10 * time.Second,
Pipes: nil,

This comment was marked as abuse.

prog/probe.go Outdated
@@ -195,7 +195,16 @@ func probeMain(flags probeFlags, targets []appclient.Target) {
log.Errorf("Docker: problem with bridge %s: %v", flags.dockerBridge, err)
}
}
if registry, err := docker.NewRegistry(flags.dockerInterval, clients, true, hostID, handlerRegistry, dockerEndpoint); err == nil {
if registry, err := docker.NewRegistry(docker.RegistryOptions{

This comment was marked as abuse.

This comment was marked as abuse.

@2opremio 2opremio merged commit 764afb6 into weaveworks:master Mar 7, 2017
2opremio pushed a commit that referenced this pull request Mar 7, 2017
@mikebryant mikebryant deleted the wip-2222 branch March 7, 2017 16:52
rade added a commit that referenced this pull request Mar 8, 2017
Revert "Add options to hide args and env vars (#2306)"
@rade
Copy link
Member

rade commented Mar 8, 2017

This has been reverted on master by #2310 because it fails the lint check.

2opremio pushed a commit that referenced this pull request Mar 8, 2017
* Revert "Revert "Add options to hide args and env vars (#2306)""

* Make linter happy
@2opremio
Copy link
Contributor

2opremio commented Mar 9, 2017

I merged it again at #2311 after fixing the linting problems ( linting didn't run in this PR due to #2192 )

@mikebryant
Copy link
Contributor Author

Thanks. I'll remember to run those manually next time, sorry about that

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.

None yet

3 participants