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.

Sign up for GitHub

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

Jump to bottom

Allow disabling controls in probes #1627

Merged
merged 6 commits into from
Jul 7, 2016
Merged

Allow disabling controls in probes #1627

merged 6 commits into from
Jul 7, 2016

Conversation

2opremio
Copy link
Contributor

@2opremio 2opremio commented Jul 1, 2016 •
edited
Loading

Fixes #1605

tomwilkie
tomwilkie reviewed Jul 1, 2016
View reviewed changes
probe/appclient/app_client.go
@@ -231,6 +233,9 @@ func (c *appClient) controlConnection() (bool, error) {
}

func (c *appClient) ControlConnection() {
if c.noControls {
return
}

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

tomwilkie
tomwilkie reviewed Jul 4, 2016
View reviewed changes
probe/appclient/multi_client.go
@@ -100,9 +102,7 @@ func (c *multiClient) Set(hostname string, endpoints []string) {
hostIDs := report.MakeIDList()
for tuple := range clients {
hostIDs = hostIDs.Add(tuple.ID)

_, ok := c.clients[tuple.ID]
if !ok {

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@rade
Copy link
Member

rade commented Jul 6, 2016

excuse my ignorance, but does 'no controls' mean 'no logs'? Or just 'no terminals', 'no start/stop', ...?

@tomwilkie
Copy link
Contributor

No logs, terminals, start / stop etc.

On Wednesday, 6 July 2016, Matthias Radestock notifications@github.com
wrote:

excuse my ignorance, but does 'no controls' mean 'no logs'? Or just 'no
terminals', 'no start/stop', ...?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1627 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAbGhQSEFDYIirgLatyK2VYZ4gZZ95d5ks5qS-AjgaJpZM4JDOm7
.

@rade
Copy link
Member

rade commented Jul 6, 2016

Hmm. 'no logs' is a bit annoying.

@tomwilkie
Copy link
Contributor

To do this would require us to leave all the pipe and control logic
running, and would make the surface area to 'secure' much larger and make
it much easier to mess up. This way has a high degree of certainty of
working and not rotting.

If we did allow some controls / pipes, we'd need to specify and enforce
which ones are read only, and also build a concept of a one way pipe. Not
impossible, but worth the extra few days? can always be done in the
future.

On Wednesday, 6 July 2016, Matthias Radestock notifications@github.com
wrote:

Hmm. 'no logs' is a bit annoying.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1627 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAbGhZq2uegzvqow_SfEEQtJux7TCjTHks5qS-XNgaJpZM4JDOm7
.

@rade
Copy link
Member

rade commented Jul 6, 2016

Agreed.

@rade
Copy link
Member

rade commented Jul 6, 2016

PS: would be really good to get this merged asap, so I can drop it from my priority list :)

@2opremio
Copy link
Contributor Author

2opremio commented Jul 6, 2016

PS: would be really good to get this merged asap, so I can drop it from my priority list :)

It's just pending a LGTM :)

@rade
Copy link
Member

rade commented Jul 6, 2016

It's just pending a LGTM :)

I know. I am not qualified to provide that.

tomwilkie
tomwilkie reviewed Jul 7, 2016
View reviewed changes
prog/main.go
@@ -145,6 +146,7 @@ func main() {
flag.DurationVar(&flags.probe.publishInterval, "probe.publish.interval", 3*time.Second, "publish (output) interval")
flag.DurationVar(&flags.probe.spyInterval, "probe.spy.interval", time.Second, "spy (scan) interval")
flag.StringVar(&flags.probe.pluginsRoot, "probe.plugins.root", "/var/run/scope/plugins", "Root directory to search for plugins")
flag.BoolVar(&flags.probe.noControls, "probe.no-controls", false, "Disable controls (read-only mode)")

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@tomwilkie
Copy link
Contributor

LGTM with one minor question about flag naming, but I don't particularly mind what its called...

@tomwilkie tomwilkie removed their assignment Jul 7, 2016
@2opremio 2opremio merged commit 6d52782 into master Jul 7, 2016
@2opremio 2opremio deleted the 1605-read-only branch July 7, 2016 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@2opremio @rade @tomwilkie