search

weaveworks / weave-gitops

Weave GitOps provides insights into your application deployments, and makes continuous delivery with GitOps easier to adopt and scale across your teams.
https://docs.gitops.weave.works/
Apache License 2.0
930 stars 153 forks source link

Feature flags implementation is problematic #3578

Open jpellizzari opened 1 year ago

jpellizzari commented 1 year ago

With telemetry turned on and no flags.ACCOUNT_ID present, the UI explodes.

This line can resolve to undefined or some other value that does not have a .match method:

https://github.com/weaveworks/weave-gitops/blob/661f3f27f1e97ccf0aa94b8c8658dea7a2f5e883/ui/components/Pendo.tsx#L89

In general, we should not be passing data as a feature flag. These flags should be true/false to enable or disable parts of the UI. Our PENDO_ACCOUNT_ID should be some sort of env var or secret.

In addition, it appears we are relying on some magic strings on the backend to generate the token:

https://github.com/weaveworks/weave-gitops/blob/661f3f27f1e97ccf0aa94b8c8658dea7a2f5e883/pkg/telemetry/telemetry.go#L28

There appears to be some sort of encryption/hashing happening here, but then that value would be decrypted and sent in plain text to the Pendo API. I would have to consult the Pendo docs to understand if this is a suggested approach or not.

It is also a bit odd that we are using the Namespace.UID as an input to the hasing.

opudrovs commented 1 year ago

Here is the initial Pendo integration PR which might provide more context: https://github.com/weaveworks/weave-gitops/pull/2652

jpellizzari commented 1 year ago

This completely abuses the feature flag system

I'm glad the author and I agree :). Some tech debt we need to pay down.

lasomethingsomething commented 1 year ago

@jpellizzari Can you review this as part of "common UI" topics?