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

demo: add enterprise license that automatically applies to cockroach demo #40222

Closed
jordanlewis opened this issue Aug 26, 2019 · 24 comments · Fixed by #40273
Closed

demo: add enterprise license that automatically applies to cockroach demo #40222

jordanlewis opened this issue Aug 26, 2019 · 24 comments · Fixed by #40273
Labels
A-demo C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@jordanlewis
Copy link
Member

Users shouldn't have to actually have a license to be able to use the enterprise features in cockroach demo.

@jordanlewis jordanlewis added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Aug 26, 2019
@rohany
Copy link
Contributor

rohany commented Aug 26, 2019

We have to make sure that users can't apply this license to other clusters -- its easy to see the current cluster license using

root@127.0.0.1:62883/movr> show cluster setting enterprise.license;

rohany added a commit to rohany/cockroach that referenced this issue Aug 28, 2019
Fixes cockroachdb#40222.

Release note (cli change): cockroach demo tries to get a temporary
enterprise license upon startup.
rohany added a commit to rohany/cockroach that referenced this issue Aug 28, 2019
Fixes cockroachdb#40222.

Release note (cli change): cockroach demo tries to get a temporary
enterprise license upon startup.
rohany added a commit to rohany/cockroach that referenced this issue Aug 28, 2019
Fixes cockroachdb#40222.

Release note (cli change): cockroach demo tries to get a temporary
enterprise license upon startup.
@knz
Copy link
Contributor

knz commented Aug 28, 2019

We have to make sure that users can't apply this license to other clusters -- its easy to see the current cluster license using

This is security by obfuscation. It's trivial to modify the cockroach program to print out the license anyway.

@jordanlewis
Copy link
Member Author

@knz we are having a separate conversation about this with @dt, thanks for weighing in! The registration server will get involved with handing out demo licenses. If the network is unavailable, the demo will not have a license.

@knz
Copy link
Contributor

knz commented Aug 28, 2019

@jordanlewis thank you for having a separate conversation, given that I am also having a separate conversation of my own about this with @dt .

It would help all the parties involved to ensure their separate conversations are put in writing and recorded for review in the issue description. (I will do my own part of this homework shortly).

@knz
Copy link
Contributor

knz commented Aug 28, 2019

Here's my separate conversation:

  1. @rolandcrosby and @dt are brainstorming license self-serves in 20.1, based on an opt-in interactive choice. This new feature would be:
  • opt-in
  • accompanied by a clear text to advise the user that their data would be sent online
  • accompanied by the required legal links to GDPR data usage policy
  1. until the 20.1 work moves forward, @jordanlewis and @dt and @rohany have been tasked with enabling enterprise features in the demo shell

  2. emphasis on "enabling enterprise features" and not "enabling telemetry" (as I had initially mis-understood)

  3. as to the technical ways to do this here the spectrum of known solutions:

    1. a special boolean for demo that would create an alternate license validation path
    2. a pre-defined constant license string
    3. a locally auto-generated license string
    4. pinging a server online
  4. pros and cons:

    @dt does not like solution 1 because "it makes the code harder to reason about" (I agree).

    @dt has no objection to the other 3 solutions.

    I (@knz) do not like any solution which makes cockroach demo mis-behave or incomplete if used behind a firewall or offline (eg in a plane)

  5. for legal reasons, any solution that requires a roundtrip to our server needs to serve the user with a clear message that informs them this is taking place and, if we store this data, legal links to data retention policies

  6. for consistency with the social contract we've established with our user base when we designed telemetry originally, any solution that requires an internet roundtrip needs a clear way to opt out, documented upfront in the UI presented by the tool -- in the demo case, that would be the terminal UI: flag --help + a warning/info string printed to the terminal when starting the program.

@awoods187
Copy link
Contributor

I do think we want telemetry from Cockroach Demo--I think it will be invaluable information

@knz
Copy link
Contributor

knz commented Aug 28, 2019

I do think we want telemetry from Cockroach Demo--I think it will be invaluable information

We can do so by enabling telemetry the regular way (and this does not require an enterprise license, so it's a separate conversation). We can easily recognize telemetry from a demo session once enabled because the app name cockroach demo does not get scrubbed.

@dt
Copy link
Member

dt commented Aug 28, 2019

My preference is for approach 4, as implemented in this patch, but I think the concerns @knz raises about respecting users' wishes w.r.t. their software talking to things are important to address if we do go with this approach.

As for why I like the approach: I like it being a real license that is installed and acts just like any other. Now it seems like what is left to decide is where it comes from: a constant, local generation or requested from cockroach labs. I have a slight preference to the latter: I see a license as something one gets from Cockroach Labs, either in the case of enterprise by signing a contract or in the case of trails and evaluation by just asking for one (via the website, API, email, etc).

By asking via a network request each time, we can a) change our mind later about if/what we return there (changing the type/term/etc) and b) maintain the property that licenses are things that Cockroach Labs makes and distributes, which means we know how many licenses we've given out / how many licensed clusters there are. So yes, there is sort of a telemetry component to doing it this way, but of all the telemetry things, this seems pretty reasonable to me: A license is an relationship with Cockroach Labs, so having it come from Cockroach Labs, and having Cockroach Labs know about it, doesn't seem all that unreasonable.

HOWEVER, that said, like any "phone home" thing, it is critical that we a) document when/what we do and then b) give users control over what we do.

In 20.1 the plan is to have license acquisition wrapped in some form of interactive flow that can be explicit about what it is doing. For now though, the more limited goal is just to make demo able to demo enterprise features.

Wha I think is is needed to make the approach in this change work is a) documenting that it will do this and b) how to disable it. We already have precedent for this: our normal anonymous telemetry collection works this way. IMO, checking the same env var that disables that telemetry collection to skip this process would address most of this concern, as it'd mean that this new feature does not materially change the existing behavior w.r.t when we make network requests.

@knz
Copy link
Contributor

knz commented Aug 28, 2019

I'm sensitive to the "a license is a contract so it's important CRL remains actual party to contracts established in its name by a tool automatically" so I don't have fundamental objections to the idea to use a lic server.

However the opt-out and clear documentation about data retention is still a pre.

@knz
Copy link
Contributor

knz commented Aug 28, 2019

There's an alternative as well: a license is a contract but the license key is not the only way to establish such a contract.

CRL could establish this contract upfront (eg via the web site) with a special-purpoose license saying "enterprise features are also granted implicitly to every user of the cockroach demo tool as long as clusters created in this way are not used for production purposes or made persistent". That's called a "blanket" license and could work a similar way (albeit with different terms from) the APL.

(and then automatically enable enterprise features, under the terms of that blanket license, via a boolean triggered by cockroach demo, technical opinions from @dt and I notwithstanding)

If we operate this way:

  • we still have visibility over demo clusters in telemetry data, by enabling telemetry the regular way.
  • the things works fine even when ran offline or behind a firewall.
  • users will know how to disable "phone home" in the same way as usual, and we already have clear docs for that.

@dt
Copy link
Member

dt commented Aug 28, 2019

@knz Telemetry reporting is already on-by-default and disabled, if desired, via an env var. As long as the approach taken in this patch is extended to respect that same env var, I don't see this as significantly changing the expectations and behavior w.r.t. if we make network requests to Cockroach Labs.

@knz
Copy link
Contributor

knz commented Aug 28, 2019

Telemetry reporting is already on-by-default

Are you sure? IIRC cockroach demo uses TestServer internally and that disables telemetry. I haven't looked at the code recently so I don't recall if there's an override.

@dt
Copy link
Member

dt commented Aug 28, 2019

If that is the case, that sounds like an oversight, not feature? demo was meat to replace the two-shell ./cockroach start and ./cockroach sql, and in that case the ./cockroach start would have the standard (i.e. env-controlled) telemetry behavior.

@knz
Copy link
Contributor

knz commented Aug 28, 2019

I agree that'd be an oversight. Let me check quickly.

@knz
Copy link
Contributor

knz commented Aug 28, 2019

Ok so I was right (and you're right it's an oversight), the update and reg loop is enabled in cli/start.go upon cockroach start[-single-node] only:

      // Start up the update check loop.
      // We don't do this in (*server.Server).Start() because we don't want it
      // in tests.
      if !envutil.EnvOrDefaultBool("COCKROACH_SKIP_UPDATE_CHECK", false) {
        s.PeriodicallyCheckForUpdates(ctx)
      }

I gladly propose including this code in cockroach demo as well (via a suitably named shared function that also includes the env var check).

@dt
Copy link
Member

dt commented Aug 28, 2019

👍 thanks for confirming. We can follow up on that separately, but I think what is important is that assuming that demo has the standard telemetry behavior w.r.t when it is enabled and how to disable it, if the automatic license acquisition code respects the same knob, then I don't think it materially changes the "phone-home" situation: we document what it does by default and we point to how to change it.

@knz
Copy link
Contributor

knz commented Aug 28, 2019

I agree that, assuming we choose a license server, the right thing to do will be to place that activation behind the same logic as the telemetry.

I'm still a bit annoyed at the idea to make cockroach demo ineffective when used offline, given that alternatives choices exist (see above).

@dt
Copy link
Member

dt commented Aug 28, 2019

cockroach demo ineffective when used offline.

Eh, I think that is a bit of an exaggeration: it is just as effective as it is today. And indeed, the demo cluster is still just as effective as real cockroach cluster would be here -- if you acquire a license though some other channel, i.e. go to the website and ask for one yourself for your air-gapped cluster -- you can still install it yourself just the same via SET.

@awoods187
Copy link
Contributor

awoods187 commented Aug 28, 2019

I think we want cockroach demo to be usable offline with enterprise license--the whole point is to make this easy for developers to see and try features out? What is easy about these proposed solutions?

@knz
Copy link
Contributor

knz commented Aug 28, 2019

if you acquire a license though some other channel, i.e. go to the website and ask for one yourself for your air-gapped cluster -- you can still install it yourself just the same via SET.

oh I had not envisioned this alternative. That's actually pretty clever. Maybe it could be possible to include this idea into the warning messaage printed out if the HTTP conn to the lic server fails?

@knz
Copy link
Contributor

knz commented Aug 28, 2019

(I would even suggest printing this message even when the check succeeds, to inform the user "they can make their enterprise feature also available offline by requesting a trial key")

@knz
Copy link
Contributor

knz commented Aug 28, 2019

Responding to @awoods187

I think we want cockroach demo to be usable offline with enterprise license

As suggested by David this is possible by requesting a trial license separately and installing it manually in the demo cluster using the SET CLUSTER SETTING statement.

the whole point is to make this easy for developers to see and try features out? What is easy about these proposed solutions?

The proposed design (license server) would make enterprise features available automatically for users who use the command online, and available manually for offline users by requesting a trial license separately.

I think it may be worth prototyping this design and then evaluating its UX with some users (and some PM acceptance testing) to verify that it "works" and is easy enough.

p.s. Of course the acceptance testing would also verify that we provide the opt-out flag, links to data policy etc.

@dt
Copy link
Member

dt commented Aug 28, 2019

I feel like we're a bit in the weeds here.

We plan to do something more ambitious in 20.1 -- current ideas include an interactive flow, and are not limited to demo -- but goal here is to more limited: let's make the common case of ./cockroach demo able to show off enterprise features too.

This change as-is now, with the addition of a check to obey the stated preferences w.r.t. phoning home, gets us basically there. There are edge cases, like offline users, that it doesn't apply to, but I think it is important to note that they are no worse off than today. If you don't get an automatic license, you have have the same options you have today: you'll get an error about needing a license, the error already points you to the website, and you can install a license by hand just the same. IMO that is fine -- adding more messages / warning / etc should all be pushed to the 20.1 scoped project.

EDIT: I missed that this moved off the PR, so to clarify, here/above, when I mentioned "this patch" or "this change" I meant #40273.

@knz
Copy link
Contributor

knz commented Aug 28, 2019

No more comment on rationale from me.

Just a request that the more salient arguments here (and the things deliberately left out of scope) be mentioned on the eventual commit message.

rohany added a commit to rohany/cockroach that referenced this issue Aug 28, 2019
There is a wider discussion/design going on to serve licenses in
a more general way in the future, so this commit is not aiming
for a future-proof design, but instead an MVP to allow users to
demo enterprise features within `cockroach demo`.

We are not concerned about offline usage of enterprise features
as users can obtain a license and enable features manually using SET.

Fixes cockroachdb#40222.

Release note (cli change): cockroach demo attempts to contact a license
server to obtain a temporary license. cockroach demo now enables
telemetry for the demo cluster. This feature can be opted out of by
setting the `COCKROACH_SKIP_UPDATE_CHECK` environment variable
(https://www.cockroachlabs.com/docs/stable/diagnostics-reporting.html).
rohany added a commit to rohany/cockroach that referenced this issue Aug 29, 2019
There is a wider discussion/design going on to serve licenses in
a more general way in the future, so this commit is not aiming
for a future-proof design, but instead an MVP to allow users to
demo enterprise features within `cockroach demo`.

We are not concerned about offline usage of enterprise features
as users can obtain a license and enable features manually using SET.

Fixes cockroachdb#40222.

Release note (cli change): cockroach demo attempts to contact a license
server to obtain a temporary license. cockroach demo now enables
telemetry for the demo cluster. This feature can be opted out of by
setting the `COCKROACH_SKIP_ENABLING_DIAGNOSTIC_REPORTING` environment variable
(https://www.cockroachlabs.com/docs/stable/diagnostics-reporting.html).
rohany added a commit to rohany/cockroach that referenced this issue Aug 30, 2019
There is a wider discussion/design going on to serve licenses in
a more general way in the future, so this commit is not aiming
for a future-proof design, but instead an MVP to allow users to
demo enterprise features within `cockroach demo`.

We are not concerned about offline usage of enterprise features
as users can obtain a license and enable features manually using SET.

Fixes cockroachdb#40222.

Release note (cli change): cockroach demo attempts to contact a license
server to obtain a temporary license. cockroach demo now enables
telemetry for the demo cluster. This feature can be opted out of by
setting the `COCKROACH_SKIP_ENABLING_DIAGNOSTIC_REPORTING` environment variable
(https://www.cockroachlabs.com/docs/stable/diagnostics-reporting.html).
craig bot pushed a commit that referenced this issue Aug 30, 2019
40273: demo: cockroach demo attempts to obtain a temporary license upon startup and enables telemetry r=rohany a=rohany

There is a wider discussion/design going on to serve licenses in
a more general way in the future, so this commit is not aiming
for a future-proof design, but instead an MVP to allow users to
demo enterprise features within `cockroach demo`.

We are not concerned about offline usage of enterprise features
as users can obtain a license and enable features manually using SET.

Fixes #40222.

Release note (cli change): cockroach demo attempts to contact a license
server to obtain a temporary license. cockroach demo now enables
telemetry for the demo cluster. This feature can be opted out of by
setting the `COCKROACH_SKIP_ENABLING_DIAGNOSTIC_REPORTING` environment variable
(https://www.cockroachlabs.com/docs/stable/diagnostics-reporting.html).

Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
@craig craig bot closed this as completed in db7b0b4 Aug 30, 2019
rohany added a commit to rohany/cockroach that referenced this issue Sep 1, 2019
There is a wider discussion/design going on to serve licenses in
a more general way in the future, so this commit is not aiming
for a future-proof design, but instead an MVP to allow users to
demo enterprise features within `cockroach demo`.

We are not concerned about offline usage of enterprise features
as users can obtain a license and enable features manually using SET.

Fixes cockroachdb#40222.

Release note (cli change): cockroach demo attempts to contact a license
server to obtain a temporary license. cockroach demo now enables
telemetry for the demo cluster. This feature can be opted out of by
setting the `COCKROACH_SKIP_ENABLING_DIAGNOSTIC_REPORTING` environment variable
(https://www.cockroachlabs.com/docs/stable/diagnostics-reporting.html).
rohany added a commit to rohany/cockroach that referenced this issue Sep 10, 2019
There is a wider discussion/design going on to serve licenses in
a more general way in the future, so this commit is not aiming
for a future-proof design, but instead an MVP to allow users to
demo enterprise features within `cockroach demo`.

We are not concerned about offline usage of enterprise features
as users can obtain a license and enable features manually using SET.

Fixes cockroachdb#40222.

Release note (cli change): cockroach demo attempts to contact a license
server to obtain a temporary license. cockroach demo now enables
telemetry for the demo cluster. This feature can be opted out of by
setting the `COCKROACH_SKIP_ENABLING_DIAGNOSTIC_REPORTING` environment variable
(https://www.cockroachlabs.com/docs/stable/diagnostics-reporting.html).
ajwerner pushed a commit to ajwerner/cockroach that referenced this issue Sep 13, 2019
There is a wider discussion/design going on to serve licenses in
a more general way in the future, so this commit is not aiming
for a future-proof design, but instead an MVP to allow users to
demo enterprise features within `cockroach demo`.

We are not concerned about offline usage of enterprise features
as users can obtain a license and enable features manually using SET.

Fixes cockroachdb#40222.

Release note (cli change): cockroach demo attempts to contact a license
server to obtain a temporary license. cockroach demo now enables
telemetry for the demo cluster. This feature can be opted out of by
setting the `COCKROACH_SKIP_ENABLING_DIAGNOSTIC_REPORTING` environment variable
(https://www.cockroachlabs.com/docs/stable/diagnostics-reporting.html).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-demo C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
5 participants