search

stackabletech / opa-operator

A kubernetes operator for the Open Policy Agent
Other
15 stars 3 forks source link

Make OPA telemetry opt-in #489

Open sbernauer opened 8 months ago

sbernauer commented 8 months ago

We want to have an opt-in flag at our OpaClusters that controls --disable-telemetry disables anonymous information reporting (see: https://www.openpolicyagent.org/docs/latest/privacy)

NickLarsenNZ commented 8 months ago

Should that be an opt-in thing? I can imagine most want to disable telemetry, but perhaps some want to enable it?

sbernauer commented 7 months ago

Yeah, it should be an opt-in thing. We would need to add a CRD field and read that I guess. Changing the Issue to reflect that

lfrancke commented 6 months ago

I think we should just disable by default and have people use a config override for that.

Is that possible in this case?

sbernauer commented 6 months ago

That's exactly what this Ticket wants. It's currently not possible to activate it, as we hard-code the start command

lfrancke commented 6 months ago

Sorry, I will clarify.

We would need to add a CRD field and read that I guess.

I think we should not add a CRD field for this if it is possible to set this using a config override. Default: Telemetry disabled, can be enabled using config override (not a CRD field)

sbernauer commented 6 months ago

Ah I see. https://www.openpolicyagent.org/docs/latest/privacy/ only mentions the CLI flag, but there might also be a config setting.

NickLarsenNZ commented 6 months ago

Yeah, I dug through code and it doesn't look like there is a companion ENV var for it.

So if we explicitly disable it, and if someone really wants to enable, would they have to override the whole command? Or would we make an env var they can set to enable it and then adjust the command line based on it?

sbernauer commented 6 months ago

https://github.com/stackabletech/opa-operator/blob/d3ad0da6ad4178b2ab79f449091a9fa2cbb000ab/rust/operator-binary/src/controller.rs#L864C15-L864C15 adds the parameter unconditional to the start command. You could (in theory) overwrite it with podOverrides but that's pain in the ***. IMHO we should add a field such as spec.clusterConfig.telemetryEnabled which defaults to false and add --disable-telemetry conditionally.

lfrancke commented 5 months ago

Ah! So do I understand this correctly that telemetry is already disabled now?

The PR says "Closes" this one but this one is not closed.

I assume this one is now about making it possible to enable telemetry again?

sbernauer commented 5 months ago

Ah! So do I understand this correctly that telemetry is already disabled now?

Yes, you are right. Originally this issues was "Disable telemetry", PR did so and closed this Issue.

Than @NickLarsenNZ had the request, that users should be able to opt in, and we re-opened the ticket changed it to reflect the new requirements

lfrancke commented 5 months ago

Thank you!