chore: Refactor Enterprise code to layer on top of AGPL #4034

kylecarbs · 2022-09-13T02:44:25Z

This is an experiment to invert the import order of the Enterprise code to layer on top of AGPL.

This change was motivated by a desire to have AGPL Coder be more of a root in the codebase that we can layer on top of. I'm not attached to any patterns here, and I mostly took precedent from testing in coderd. These reflect integration tests much more closely than unit tests, but I don't believe this pattern excludes us from unit testing in the future.

Colin also needed to call coderd.API.createUser, but he couldn't since Enterprise didn't have an instantiated API for the handlers. Ideally, our solution is to expose that function rather than abstract it further, which this now enables.

This is an experiment to invert the import order of the Enterprise code to layer on top of AGPL.

f0ssel

This organization is clearer to me and I had similar ideas when browsing this code for the first time. Overall improvement

coderd/coderd.go

enterprise/coderd/coderd.go

f0ssel · 2022-09-13T15:28:52Z

enterprise/coderd/coderd.go

+	api.AGPL.APIHandler.Group(func(r chi.Router) {
+		r.Get("/entitlements", api.entitlements)
+		r.Route("/licenses", func(r chi.Router) {
+			r.Use(apiKeyMiddleware)
+			r.Post("/", api.postLicense)
+			r.Get("/", api.licenses)
+			r.Delete("/{id}", api.deleteLicense)
+		})
+	})


I like this pattern a lot

coderd/coderd.go

enterprise/coderd/coderd.go

spikecurtis · 2022-09-13T17:37:25Z

coderd/coderd_test.go

-	skipRoutes, assertRoute := coderdtest.AGPLRoutes(a)
-	a.Test(ctx, assertRoute, skipRoutes)
-}
-


Without this there are no AGPL tests of authorization on endpoints, which is a loss for AGPL code.

Good point. I'll fix this up.

spikecurtis · 2022-09-13T17:51:44Z

enterprise/coderd/coderd.go

+	api.mutex.RLock()
+	hasLicense := api.hasLicense
+	activeUsers := api.activeUsers
+	auditLogs := api.auditLogs


You've replaced something that will scale to a large number of features (entitlements struct) with something that will not.

I don't understand how wrapping this in a struct enables scale. If we had 10 of these options, I'd agree we should make a struct... but this seems like an easy to solve problem once we approach it.

It's not just the struct itself, but the way the code used to work was that there was a function that computed the struct, which completely specifies and encapsulates the relevant license state. A change in license state resulted in a new struct. Here it's spread across three fields that are mutated in place, so it's conceptually less clear that these are the only things that change when license state changes.

Hmm, fair point. I can encapsulate these inside a struct so the mental model is simpler to understand.

This is still pending @kylecarbs

Oops, forgot about this... fixed!

spikecurtis · 2022-09-13T17:53:41Z

enterprise/coderd/coderd.go

+		return nil, xerrors.Errorf("update entitlements: %w", err)
+	}
+	api.closeLicenseSubscribe, err = api.Pubsub.Subscribe(pubSubEventLicenses, func(ctx context.Context, message []byte) {
+		_ = api.updateEntitlements(ctx)


This construction drops errors without logging them, and doesn't retry if we fail.

Good catch. Will fix.

spikecurtis · 2022-09-13T17:57:13Z

enterprise/coderd/coderd.go

+		select {
+		case <-ctx.Done():
+			return
+		case <-ticker.C:


While this time-only loop is simpler, it means we lose retries on error. So, a momentary hiccup in DB connectivity means we don't update for 10 minutes. This kind of thing makes software a pain to operate, and I don't see the value of stripping out something of higher sophistication considering it had good unit tests.

Yup, good point. I'll revert this.

enterprise/coderd/features_internal_test.go

enterprise/coderd/coderd.go

coderd/features/features.go

coderd/templates.go

coadler · 2022-09-13T21:50:47Z

I think the layering here is easier to conceptualize for me. I like the changes 👍

kylecarbs · 2022-09-13T22:40:25Z

@spikecurtis and I made some good progress discussing the current pitfalls of the approach:

The /entitlements route has an ambiguous implementation point. Contributors could easily look at our AGPL coderd and be misled by the implementation without it being clear where the real implementation is.
The entitlements HTTP handler and feature replacement code should be in a single, well-tested unit. Right now it's baked into the Enterprise coderd implementation which might be fine but should be at high test coverage like before.
We should use a context.Context to get a feature interface like Auditor. It should be stored for the lifecycle of the context too, so that future retrievals return the same pointer. This prevents an implementation swap mid-way through a request, which would cause oddities.
Calling Store() shouldn't be permitted inside the AGPL code. It's a privileged operation and could easily be misused.

Spike feel free to leave additional improvements if any come to mind!

sreya

So I like the the composability of Spike's implementation however I'm not a big fan of "injecting" enterprise logic into the core (aka free) API.

I think we should strive for a consistent developer experience for core and enterprise features, and it seems that developing enterprise features today deviates from the way we've developed features in the past. To that end I also think it's important that we try to avoid leaking enterprise-specific logic (specifically routes) into core Coder as much as possible (some leakage may be inevitable).

The aspect of Kyle's PR that I really like is that it wraps the coderd API and injects the enterprise routes on top. I know @spikecurtis you have some reservations about injecting routes into the handler in enterprise/coderd so I'd love to hear more of your thoughts on that because to me it feels like a cleaner separation of concerns and mirrors the way routes are added in coderd.

However, I do agree with Spike that there's some improvements that can be made to how we're currently getting features in this PR.The limited surface area of Spike's featureService implementation is really clean but adding features in the features.go.setImplementation is somewhat intimidating. It's possible the complexity is simply a result of ensuring correctness but I think it's worth trying to simplify the devex in this regard which is why I lean more towards something akin to the atomic pointer.

sreya · 2022-09-14T03:56:23Z

coderd/coderd.go

+	RootHandler         chi.Router
+	APIHandler          chi.Router


I understand why this needs to occur but it looks really sketchy and deserves a comment.

spikecurtis · 2022-09-14T16:40:24Z

I know @spikecurtis you have some reservations about injecting routes into the handler in enterprise/coderd so I'd love to hear more of your thoughts on that because to me it feels like a cleaner separation of concerns and mirrors the way routes are added in coderd.

I think it's fine to inject new routes into the handler, but I don't like rewriting existing routes, since this makes it hard for someone reading AGPL code to realize that there are multiple implementations of some routes.

This uses a context to ensure the same value persists through multiple executions to `Load()`.

coderd/templates.go

coderd/pointer/pointer.go

coderd/pointer/pointer_test.go

enterprise/coderd/coderd_test.go

enterprise/coderd/licenses.go

enterprise/coderd/coderd.go

kylecarbs · 2022-09-20T00:32:34Z

🥳 @spikecurtis if we find anything after merge that's questionable, I'm always happy to fix!

kylecarbs force-pushed the entfactor branch 3 times, most recently from 17b9e3f to db6eed8 Compare September 13, 2022 03:04

chore: Refactor Enterprise code to layer on top of AGPL

fd05de9

This is an experiment to invert the import order of the Enterprise code to layer on top of AGPL.

kylecarbs force-pushed the entfactor branch from db6eed8 to fd05de9 Compare September 13, 2022 03:05

Merge branch 'main' into entfactor

04c5d1d

kylecarbs force-pushed the entfactor branch from 24cf747 to 04c5d1d Compare September 13, 2022 03:21

kylecarbs self-assigned this Sep 13, 2022

f0ssel approved these changes Sep 13, 2022

View reviewed changes

kylecarbs requested a review from spikecurtis September 13, 2022 15:37

Fix Garrett's comments

aad7be4

spikecurtis requested changes Sep 13, 2022

View reviewed changes

deansheather self-requested a review September 13, 2022 18:11

spikecurtis reviewed Sep 13, 2022

View reviewed changes

enterprise/coderd/coderd.go Outdated Show resolved Hide resolved

spikecurtis reviewed Sep 13, 2022

View reviewed changes

enterprise/coderd/coderd.go Outdated Show resolved Hide resolved

coadler reviewed Sep 13, 2022

View reviewed changes

coderd/features/features.go Show resolved Hide resolved

coderd/templates.go Outdated Show resolved Hide resolved

sreya reviewed Sep 14, 2022

View reviewed changes

kylecarbs added 9 commits September 15, 2022 01:24

Merge branch 'main' into entfactor

d0526ed

Add pointer.Handle to atomically obtain references

27f53aa

This uses a context to ensure the same value persists through multiple executions to `Load()`.

Remove entitlements API from AGPL coderd

ed5b96d

Remove AGPL Coder entitlements endpoint test

be96fff

Fix warnings output

b4cbd6c

Merge branch 'main' into entfactor

ad3cb79

Add command-line flag to toggle audit logging

1964a64

Fix hasLicense being set

0e88f51

Remove features interface

586b3e8

kylecarbs added 4 commits September 15, 2022 05:34

Fix audit logging default

1bb6d0f

Add bash as a dependency

8c603f6

Add comment

2dcf1f8

Merge branch 'main' into entfactor

2cfcfac

spikecurtis reviewed Sep 19, 2022

View reviewed changes

coderd/templates.go Outdated Show resolved Hide resolved

coderd/pointer/pointer.go Outdated Show resolved Hide resolved

coderd/pointer/pointer_test.go Outdated Show resolved Hide resolved

kylecarbs added 3 commits September 19, 2022 18:21

Merge branch 'main' into entfactor

5215fe9

Add tests for resync and pubsub, and add back previous exp backoff retry

1955ae3

Separate authz code again

e01a6ce

kylecarbs marked this pull request as ready for review September 19, 2022 19:25

kylecarbs requested a review from a team as a code owner September 19, 2022 19:25

kylecarbs requested review from Kira-Pilot and removed request for a team September 19, 2022 19:25

Add pointer loading example from comment

b0a3c95

spikecurtis requested changes Sep 19, 2022

View reviewed changes

kylecarbs added 3 commits September 19, 2022 20:37

Fix duplicate test, remove pointer.Handle

2b87ae1

Fix expired license

7fd0903

Add entitlements struct

c67605b

spikecurtis approved these changes Sep 20, 2022

View reviewed changes

Merge branch 'main' into entfactor

b8776cb

kylecarbs force-pushed the entfactor branch 3 times, most recently from 7b6fe85 to fb35e01 Compare September 20, 2022 03:22

Fix context passing

7f9ed39

kylecarbs force-pushed the entfactor branch from fb35e01 to 7f9ed39 Compare September 20, 2022 03:25

kylecarbs merged commit db0ba85 into main Sep 20, 2022

kylecarbs deleted the entfactor branch September 20, 2022 04:11

chore: Refactor Enterprise code to layer on top of AGPL #4034

chore: Refactor Enterprise code to layer on top of AGPL #4034

Uh oh!

Conversation

kylecarbs commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

f0ssel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coadler commented Sep 13, 2022

Uh oh!

kylecarbs commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sreya left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spikecurtis commented Sep 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kylecarbs commented Sep 20, 2022

Uh oh!

Uh oh!

kylecarbs commented Sep 13, 2022 •

edited

Loading

kylecarbs commented Sep 13, 2022 •

edited

Loading

sreya left a comment •

edited

Loading