[usage] Validate spending limits in UpdateCostCenter #13798

easyCZ · 2022-10-12T11:27:49Z

Description

Enforces usage limit values for users & teams.

Adds a new piece of config for the default value of UsersOnStripe. When this rolls out, it will be 0 (due to missing config) but because we set it on the server side, this is okay and the check here will simply check if new limit is > 0. We'll add the config to ensure it does not prevent setting lower in subsequent PR.

Related Issue(s)

Part of #13389

How to test

Release Notes

NONE

Documentation

Werft options:

/werft with-local-preview
If enabled this will build install/preview
/werft with-preview
/werft with-payment
/werft with-integration-tests=all
Valid options are all, workspace, webapp, ide

easyCZ · 2022-10-12T11:42:33Z

components/usage/pkg/db/cost_center.go

@@ -102,57 +105,112 @@ func getCostCenter(ctx context.Context, conn *gorm.DB, attributionId Attribution
 	return costCenter, nil
 }

-func (c *CostCenterManager) UpdateCostCenter(ctx context.Context, costCenter CostCenter) (CostCenter, error) {
+func (c *CostCenterManager) UpdateCostCenter(ctx context.Context, newCC CostCenter) (CostCenter, error) {


The multi-level branching here could be simplified to shorten the code, but it's deliberately done this way for readability (at the cost of "duplicating" the handling of Other -> Stripe. I feel this is acceptable, as we win more points on understanding the flows.

werft-gitpod-dev-com · 2022-10-12T12:31:51Z

started the job as gitpod-build-mp-usage-cost-center-validations.5 because the annotations in the pull request description changed
(with .werft/ from main)

werft-gitpod-dev-com · 2022-10-13T07:59:18Z

started the job as gitpod-build-mp-usage-cost-center-validations.7 because the annotations in the pull request description changed
(with .werft/ from main)

geropl · 2022-10-13T08:31:07Z

Starting to review...

jankeromnes

Thanks! The code looks good to me, modulo a few optional nits.

I will also test this code by rebasing my PR on top of these changes (thus making the PRs dependent) to verify that the entire test flow I described now passes. 🤞

jankeromnes · 2022-10-13T08:22:19Z

components/usage/pkg/apiv1/usage_test.go

+		ForUsers:                  500,
+		MinForIndividualsOnStripe: 1000,


Nit: Slight cognitive dissonance between "Users" above and "Individuals" below (they're actually the same thing). Maybe the new field should be called MinForUsersOnStripe for consistency?

Suggested change

ForUsers: 500,

MinForIndividualsOnStripe: 1000,

ForUsers: 500,

MinForUsersOnStripe: 1000,

components/usage/pkg/db/cost_center.go

components/usage/pkg/db/cost_center_test.go

geropl

The code itself LGTM and tests run.

/hold Did not test, though, as Jan is doing that in combination with the UI branch here.

jankeromnes · 2022-10-13T09:06:40Z

components/usage/pkg/db/cost_center.go

+
+		// Downgrading from stripe
+		if existingCC.BillingStrategy == CostCenter_Stripe && newCC.BillingStrategy == CostCenter_Other {
+			newCC.SpendingLimit = c.cfg.ForUsers


Hmm, this flow still doesn't seem to work properly 🤔

Create a team called "Gitpod [Something]"

Go to /billing (you have 500 credits 👍)

(view DB state)

mysql> select * from d_b_cost_center where id = 'user:f0f6f6f5-f8e4-47e4-80f9-420f8cbcbfb7'; +-------------------------------------------+---------------+---------+----------------------------+--------------------------+-----------------+--------------------------+ | id | spendingLimit | deleted | _lastModified | creationTime | billingStrategy | nextBillingTime | +-------------------------------------------+---------------+---------+----------------------------+--------------------------+-----------------+--------------------------+ | user:f0f6f6f5-f8e4-47e4-80f9-420f8cbcbfb7 | 500 | 0 | 2022-10-13 08:59:20.491162 | 2022-10-13T08:59:20.488Z | other | 2022-11-13T08:59:20.489Z | +-------------------------------------------+---------------+---------+----------------------------+--------------------------+-----------------+--------------------------+ 1 row in set (0.01 sec)

~~Upgrade by adding a payment method, then refresh (limit is 100 ? 😳 Should be 1000)~~ EDIT: Actually, I'm not sure where the 100 comes from, but in any case the upgrade should fail if the provided limit is < 1000 right?

(view DB state)

mysql> select * from d_b_cost_center where id = 'user:f0f6f6f5-f8e4-47e4-80f9-420f8cbcbfb7'; +-------------------------------------------+---------------+---------+----------------------------+--------------------------+-----------------+--------------------------+ | id | spendingLimit | deleted | _lastModified | creationTime | billingStrategy | nextBillingTime | +-------------------------------------------+---------------+---------+----------------------------+--------------------------+-----------------+--------------------------+ | user:f0f6f6f5-f8e4-47e4-80f9-420f8cbcbfb7 | 500 | 0 | 2022-10-13 08:59:20.491162 | 2022-10-13T08:59:20.488Z | other | 2022-11-13T08:59:20.489Z | | user:f0f6f6f5-f8e4-47e4-80f9-420f8cbcbfb7 | 100 | 0 | 2022-10-13 09:01:33.580476 | 2022-10-13T09:01:33.576Z | stripe | | +-------------------------------------------+---------------+---------+----------------------------+--------------------------+-----------------+--------------------------+ 2 rows in set (0.02 sec)

Try to set the limit to 150 -- it should fail because that's < 1000, but actually it works 😳

(view DB state)

mysql> select * from d_b_cost_center where id = 'user:f0f6f6f5-f8e4-47e4-80f9-420f8cbcbfb7'; +-------------------------------------------+---------------+---------+----------------------------+--------------------------+-----------------+--------------------------+ | id | spendingLimit | deleted | _lastModified | creationTime | billingStrategy | nextBillingTime | +-------------------------------------------+---------------+---------+----------------------------+--------------------------+-----------------+--------------------------+ | user:f0f6f6f5-f8e4-47e4-80f9-420f8cbcbfb7 | 500 | 0 | 2022-10-13 08:59:20.491162 | 2022-10-13T08:59:20.488Z | other | 2022-11-13T08:59:20.489Z | | user:f0f6f6f5-f8e4-47e4-80f9-420f8cbcbfb7 | 100 | 0 | 2022-10-13 09:01:33.580476 | 2022-10-13T09:01:33.576Z | stripe | | | user:f0f6f6f5-f8e4-47e4-80f9-420f8cbcbfb7 | 100 | 0 | 2022-10-13 09:03:49.189822 | 2022-10-13T09:03:49.186Z | stripe | | | user:f0f6f6f5-f8e4-47e4-80f9-420f8cbcbfb7 | 150 | 0 | 2022-10-13 09:03:53.190384 | 2022-10-13T09:03:53.187Z | stripe | | +-------------------------------------------+---------------+---------+----------------------------+--------------------------+-----------------+--------------------------+ 4 rows in set (0.02 sec)

Manage plan, then cancel your Stripe subscription -- the limit should reset to 500, but instead it remains at its previous value 🐛

(view DB state)

mysql> select * from d_b_cost_center where id = 'user:f0f6f6f5-f8e4-47e4-80f9-420f8cbcbfb7'; +-------------------------------------------+---------------+---------+----------------------------+--------------------------+-----------------+--------------------------+ | id | spendingLimit | deleted | _lastModified | creationTime | billingStrategy | nextBillingTime | +-------------------------------------------+---------------+---------+----------------------------+--------------------------+-----------------+--------------------------+ | user:f0f6f6f5-f8e4-47e4-80f9-420f8cbcbfb7 | 500 | 0 | 2022-10-13 08:59:20.491162 | 2022-10-13T08:59:20.488Z | other | 2022-11-13T08:59:20.489Z | | user:f0f6f6f5-f8e4-47e4-80f9-420f8cbcbfb7 | 100 | 0 | 2022-10-13 09:01:33.580476 | 2022-10-13T09:01:33.576Z | stripe | | | user:f0f6f6f5-f8e4-47e4-80f9-420f8cbcbfb7 | 100 | 0 | 2022-10-13 09:03:49.189822 | 2022-10-13T09:03:49.186Z | stripe | | | user:f0f6f6f5-f8e4-47e4-80f9-420f8cbcbfb7 | 150 | 0 | 2022-10-13 09:03:53.190384 | 2022-10-13T09:03:53.187Z | stripe | | +-------------------------------------------+---------------+---------+----------------------------+--------------------------+-----------------+--------------------------+ 4 rows in set (0.01 sec)

When this rolls out, it will be 0 (due to missing config) but because we set it on the server side, this is okay and the check here will simply check if new limit is > 0. We'll add the config to ensure it does not prevent setting lower in subsequent PR.

Aha, so it's even 0 in preview environments 👌 thanks for clarifying. That explains the allowed < 1000 custom limit.

Still, this doesn't explain why, upon cancellation, the user cost center does not go back to other and doesn't reset its usage limit to 500.

Manage plan, then cancel your Stripe subscription -- the limit should reset to 500, but instead it remains at its previous value 🐛

That's because the request to the usage component didn't set the billing strategy to Other

user:f0f6f6f5-f8e4-47e4-80f9-420f8cbcbfb7 | 150 | 0 | 2022-10-13 09:03:53.190384 | 2022-10-13T09:03:53.187Z | stripe

But shouldn't that happen automatically when Stripe calls Gitpod to tell it that a subscription was cancelled?

Setting the config (now done in preview also) addresses 3. not failing.

The default 100 comes from protected defaultSpendingLimit = 100; defined in gitpod-server-impl.ts

jankeromnes

Code also looks good to me and works as advertised.

Also, the error of cost centers not resetting seems somewhat unrelated to this PR (i.e. it's probably a misconfigured webhook in preview environments), so maybe let's investigate/fix this in a follow-up issue.

Please take a look at my comments when you get a chance, and decide whether to address or ignore them. 🌴

easyCZ · 2022-10-13T10:57:17Z

Applied suggested changes. I've also added a default config into preview which sets the MinForStripeUsers to 1k

easyCZ · 2022-10-13T11:59:48Z

/unhold

roboquat added release-note-none do-not-merge/work-in-progress size/XL labels Oct 12, 2022

easyCZ force-pushed the mp/usage-cost-center-validations branch from 47784ab to 5beb613 Compare October 12, 2022 11:28

roboquat added size/L and removed size/XL labels Oct 12, 2022

easyCZ force-pushed the mp/usage-cost-center-validations branch from 5beb613 to cc16570 Compare October 12, 2022 11:33

roboquat added size/XL and removed size/L labels Oct 12, 2022

easyCZ force-pushed the mp/usage-cost-center-validations branch from cc16570 to f253dc9 Compare October 12, 2022 11:39

easyCZ marked this pull request as ready for review October 12, 2022 11:41

easyCZ requested a review from a team October 12, 2022 11:41

roboquat removed the do-not-merge/work-in-progress label Oct 12, 2022

github-actions bot added the team: webapp Issue belongs to the WebApp team label Oct 12, 2022

easyCZ commented Oct 12, 2022

View reviewed changes

easyCZ mentioned this pull request Oct 12, 2022

[Usage-based] Adjust Stripe usage limits to spec #13746

Merged

4 tasks

easyCZ force-pushed the mp/usage-cost-center-validations branch from f253dc9 to 27b7dfe Compare October 12, 2022 12:28

easyCZ force-pushed the mp/usage-cost-center-validations branch from 27b7dfe to 1221e08 Compare October 12, 2022 12:44

jankeromnes reviewed Oct 13, 2022

View reviewed changes

geropl approved these changes Oct 13, 2022

View reviewed changes

roboquat added the do-not-merge/hold label Oct 13, 2022

jankeromnes reviewed Oct 13, 2022

View reviewed changes

jankeromnes approved these changes Oct 13, 2022

View reviewed changes

easyCZ force-pushed the mp/usage-cost-center-validations branch from daba166 to 4ebbe89 Compare October 13, 2022 10:55

[usage] Validate spending limits in UpdateCostCenter

304c8dc

easyCZ force-pushed the mp/usage-cost-center-validations branch from 4ebbe89 to 304c8dc Compare October 13, 2022 10:56

roboquat removed the do-not-merge/hold label Oct 13, 2022

roboquat merged commit f626371 into main Oct 13, 2022

roboquat deleted the mp/usage-cost-center-validations branch October 13, 2022 12:00

jankeromnes mentioned this pull request Oct 13, 2022

Adjust cost centers and usage limits depending to spec #13389

Closed

Furisto mentioned this pull request Oct 13, 2022

ws-manager: NODE_EXTRA_CA_CERTS is the value of the gitpod internals #13821

Merged

3 tasks

roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Oct 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[usage] Validate spending limits in UpdateCostCenter #13798

[usage] Validate spending limits in UpdateCostCenter #13798

easyCZ commented Oct 12, 2022 •

edited by jankeromnes

Loading

easyCZ Oct 12, 2022

werft-gitpod-dev-com bot commented Oct 12, 2022

werft-gitpod-dev-com bot commented Oct 13, 2022

geropl commented Oct 13, 2022

jankeromnes left a comment

jankeromnes Oct 13, 2022

geropl left a comment

jankeromnes Oct 13, 2022 •

edited

Loading

easyCZ Oct 13, 2022

jankeromnes Oct 13, 2022 •

edited

Loading

easyCZ Oct 13, 2022

jankeromnes Oct 13, 2022

easyCZ Oct 13, 2022

jankeromnes left a comment

easyCZ commented Oct 13, 2022

easyCZ commented Oct 13, 2022

[usage] Validate spending limits in UpdateCostCenter #13798

[usage] Validate spending limits in UpdateCostCenter #13798

Conversation

easyCZ commented Oct 12, 2022 • edited by jankeromnes Loading

Description

Related Issue(s)

How to test

Release Notes

Documentation

Werft options:

easyCZ Oct 12, 2022

Choose a reason for hiding this comment

werft-gitpod-dev-com bot commented Oct 12, 2022

werft-gitpod-dev-com bot commented Oct 13, 2022

geropl commented Oct 13, 2022

jankeromnes left a comment

Choose a reason for hiding this comment

jankeromnes Oct 13, 2022

Choose a reason for hiding this comment

geropl left a comment

Choose a reason for hiding this comment

jankeromnes Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

easyCZ Oct 13, 2022

Choose a reason for hiding this comment

jankeromnes Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

easyCZ Oct 13, 2022

Choose a reason for hiding this comment

jankeromnes Oct 13, 2022

Choose a reason for hiding this comment

easyCZ Oct 13, 2022

Choose a reason for hiding this comment

jankeromnes left a comment

Choose a reason for hiding this comment

easyCZ commented Oct 13, 2022

easyCZ commented Oct 13, 2022

easyCZ commented Oct 12, 2022 •

edited by jankeromnes

Loading

jankeromnes Oct 13, 2022 •

edited

Loading

jankeromnes Oct 13, 2022 •

edited

Loading