Vault 2823 cc namespace #12393

akshya96 · 2021-08-20T23:35:31Z

https://hashicorp.atlassian.net/browse/VAULT-2823

(PR addresses comments on https://github.com/hashicorp/vault-enterprise/pull/2103)

will be creating a PR for enterprise updating the test for partialmonthlycount with entitites and tokens created using different namespaces

Issue: Currently, the partial client count API /sys/internal/counters - HTTP API | Vault by HashiCorp does not return the information subdivided by namespaces. The reason for that, is that we do not store entity counts broken down by namespace in memory at the moment. Ideally, we want customers to be able to observe the client counts broken down by namespace for the current month as well as prior months.

Change: Using a namespace->entity counter map (a.entityCountByNamespaceID) and incrementing it only if the entity is not in the map
Using non-entity token count (a.currentSegment.tokenCount.CountByNamespaceID) and distinct entity count (a.entityCountByNamespaceID) to divide the response by namespaces

Testing: (Open Source Vault)
{"request_id":"21dfb313-80b5-63df-25cf-9ced0cbe389d","lease_id":"","renewable":false,"lease_duration":0,"data":{"by_namespace":[{"namespace_id":"root","namespace_path":"","counts":{"distinct_entities":3,"non_entity_tokens":0,"clients":3}}],"clients":3,"distinct_entities":3,"non_entity_tokens":0},"wrap_info":null,"warnings":null,"auth":null}

(Enterprise : with namespaces)
{"request_id":"992b21de-0d7d-650d-5d6f-930b70bb2ecf","lease_id":"","renewable":false,"lease_duration":0,"data":{"by_namespace":[{"namespace_id":"root","namespace_path":"","counts":{"distinct_entities":3,"non_entity_tokens":0,"clients":3}},{"namespace_id":"N3MZO","namespace_path":"education/","counts":{"distinct_entities":1,"non_entity_tokens":0,"clients":1}},{"namespace_id":"kOiIk","namespace_path":"education/training/","counts":{"distinct_entities":1,"non_entity_tokens":0,"clients":1}}],"clients":5,"distinct_entities":5,"non_entity_tokens":0},"wrap_info":null,"warnings":null,"auth":null}

swayne275 · 2021-08-30T20:26:49Z

the failed test-go-remote-docker looks transient to me - you can probably just give that test a re-run

swayne275 · 2021-08-30T20:27:34Z

Also - big fan of the description with the PR!

swayne275

I think it's looking pretty good so far. I left a few comments/suggestions - let me know your thoughts

swayne275 · 2021-08-30T20:29:06Z

vault/activity_log_testing_util.go

@@ -36,6 +36,29 @@ func (c *Core) InjectActivityLogDataThisMonth(t *testing.T) (map[string]struct{}
 	return activeEntities, tokens
 }

+// InjectActivityLogDataThisMonth populates the in-memory client store
+// with some entities and tokens for root namespace , overriding what was already there


Suggested change

// with some entities and tokens for root namespace , overriding what was already there

// with some entities and tokens for root namespace, overriding what was already there

swayne275 · 2021-08-30T20:34:43Z

vault/activity_log.go

+
+}
+
+func (a *ActivityLog) AddEntity(e *activity.EntityRecord) {


i think this should be private - i didn't see any outside-package usage (or any reason to make it public)

swayne275 · 2021-08-30T20:40:31Z

vault/activity_log.go

-		tokenCount += int(countByNS)
+	queryNS, err := namespace.FromContext(ctx)
+	if err != nil {
+		return responseData, err


what are your thoughts on returning this empty responseData vs nil when there is an err? It seems to be a pretty common pattern to return nil with an error

same in other err != nil cases

swayne275 · 2021-08-30T20:40:49Z

vault/activity_log.go

-	responseData["non_entity_tokens"] = tokenCount
-	responseData["clients"] = clientCount
+	for nsID, clients := range clientCountTable {
+


Suggested change

swayne275 · 2021-08-30T20:45:11Z

vault/activity_log.go

+		clientCountTable[nsID].distinctEntities += count
+
+	}
+	//add non-entity token count


Suggested change

//add non-entity token count

swayne275 · 2021-08-30T20:45:23Z

vault/activity_log.go

+
+//createClientCountTable maps the entitycount and token count to the namespace id
+func createClientCountTable(entityMap map[string]uint64, tokenMap map[string]uint64) map[string]*clients {
+	//add distinct entity count


Suggested change

//add distinct entity count

swayne275 · 2021-08-30T20:45:38Z

vault/activity_log.go

+
+	}
+	return clientCountTable
+


Suggested change

swayne275 · 2021-08-30T20:47:28Z

vault/logical_system_activity.go

@@ -142,7 +142,12 @@ func (b *SystemBackend) handleMonthlyActivityCount(ctx context.Context, req *log
 		return logical.ErrorResponse("no activity log present"), nil
 	}

-	results := a.partialMonthClientCount(ctx)
+	results, err := a.partialMonthClientCount(ctx)


i made this comment earlier, but it doesn't look like results has any info in it when err != nil - it might be more clear to show that results has nothing in it and put an empty Data object in the response in this error case

did you have any thoughts on this?

The Go convention for funcs that return (x,error) is to populate only one of the two return values - I'd prefer if it were exactly one, but sometimes we do return (nil,nil), which tends to cause bugs because people are so used to the more common practice of always returning exactly one non-nil value.

This carries over to our handlers as well. If they return a non-nil error, they should be returning a nil response. The complication is that we can return errors in two ways. We can either return (nil, err) in which case the user will not an error message, they'll just get a 500 internal server error. Or we can return (logical.ErrorResponse("message"), nil) in which case we'll put "message" into the response's error field and return a 400 bad request. Or we can return (logical.ErrorReponse("message"), err), in which case they'll see "message" in the response's error field and the status code of the response will depend on err, see logical.RespondErrorCommon for specifics.

There's no general guidance about when to use these different approaches, though we should probably write some. One tip I can offer is that if the error is an "expected" error, e.g. the user provided bad input in their request, or asked to do something we don't want to allow, it's good to provide an explanation of the problem in the response's error message. If it's an internal failure that shouldn't have happened, that isn't predictable based on the inputs, then we usually tend more towards the (nil, err) return where they just a 500. This is because we don't know what appear in the error, and we might wind up leaking information that the user shouldn't see. Often in that case we'll log the error.

Anyway, in this case, when err != nil, I would simply return (nil, err).

swayne275 · 2021-08-30T20:50:54Z

vault/activity_log_test.go

+	}
+
+	var tokenRecords map[string]uint64
+	if includeTokens {


everything up to this point looks identical with setupActivityRecordsInStorage - it might be a good idea to break some of that out into a base/helper function, and then just add the namespace flavor on top for the namespace version of the function. this would result in more clear and maintainable code

swayne275 · 2021-08-30T20:54:13Z

vault/activity_log_test.go

-	results := a.partialMonthClientCount(ctx)
+	results, err := a.partialMonthClientCount(ctx)
+	if err != nil {
+		t.Fatal(err.Error())


no need to call .Error() here if you don't want to

swayne275 · 2021-09-03T18:43:21Z

vault/logical_system_activity.go

@@ -142,7 +142,12 @@ func (b *SystemBackend) handleMonthlyActivityCount(ctx context.Context, req *log
 		return logical.ErrorResponse("no activity log present"), nil
 	}

-	results := a.partialMonthClientCount(ctx)
+	results, err := a.partialMonthClientCount(ctx)


did you have any thoughts on this?

swayne275 · 2021-09-03T18:48:19Z

vault/activity_log_test.go

 				Timestamp:   time.Now().Unix(),
 			},
 			{
 				EntityID:    "33333333-2222-2222-2222-222222222222",
-				NamespaceID: "root",
+				NamespaceID: namespace.RootNamespaceID,


swayne275 · 2021-09-03T18:49:15Z

vault/activity_log_test.go

 				Timestamp:   time.Now().Unix(),
 			},
 		}
+		if constants.IsEnterprise {


it's probably better to put this in vault/activity_log_ent_test.go

it's an enterprise specific test involving enterprise features (namespaces). that said, it does mean less code duplication by having this as an optional add-on, rather than two separate tests

what do we generally do in this situation?

We went back and forth on some variations on this theme. Typically we have two variations on the same test, one oss, one ent, or we have the test only in ent. I think this approach is fine too, and has some advantages. We should probably expand the wiki "Writing Tests: Models to Follow" to talk about this topic.

ncabatoff · 2021-09-03T19:02:38Z

vault/activity_log.go

+	for nsID, clients := range clientCountTable {
+		ns, err := NamespaceByID(ctx, nsID, a.core)
+		if err != nil {
+			return responseData, err


I think this should be return nil, err.

* vault-2823 adding changes * VAULT-2823 adding alias * Vault-2823 addressing comments * Vault-2823 removing comments * Vault-2823 removing comments * vault-2823 removing q debug * adding changelog * Vault-2823 updating external test * adding approved changes * fixing returns * fixing returns

akshya96 added 8 commits August 19, 2021 10:22

vault-2823 adding changes

b88bb92

Merge branch 'main' into Vault-2823_CC_Namespace

be69519

VAULT-2823 adding alias

1d61470

Vault-2823 addressing comments

b601a56

Merge branch 'main' into Vault-2823_CC_Namespace

dc9746f

Vault-2823 removing comments

b2ccd9b

Vault-2823 removing comments

06736f5

vault-2823 removing q debug

2ed883c

akshya96 requested review from swayne275, briankassouf, ncabatoff, pmmukh, ccapurso, raskchanky and vishalnayak August 20, 2021 23:36

adding changelog

23b3e3f

vercel bot temporarily deployed to Preview – vault August 20, 2021 23:45 Inactive

vercel bot temporarily deployed to Preview – vault-storybook August 20, 2021 23:45 Inactive

akshya96 added 2 commits August 23, 2021 15:14

Vault-2823 updating external test

f226f53

Merge branch 'main' into Vault-2823_CC_Namespace

9e18d91

vercel bot deployed to Preview – vault August 23, 2021 22:15 View deployment

vercel bot temporarily deployed to Preview – vault-storybook August 23, 2021 22:15 Inactive

swayne275 reviewed Aug 30, 2021

View reviewed changes

akshya96 added 2 commits September 2, 2021 21:55

adding approved changes

9bc2ee5

Merge branch 'main' into Vault-2823_CC_Namespace

b16defc

vercel bot deployed to Preview – vault-storybook September 3, 2021 15:57 View deployment

vercel bot deployed to Preview – vault September 3, 2021 15:57 View deployment

fixing returns

22a1820

vercel bot temporarily deployed to Preview – vault-storybook September 3, 2021 18:28 Inactive

vercel bot temporarily deployed to Preview – vault September 3, 2021 18:28 Inactive

akshya96 requested a review from swayne275 September 3, 2021 18:28

swayne275 reviewed Sep 3, 2021

View reviewed changes

ncabatoff reviewed Sep 3, 2021

View reviewed changes

fixing returns

589fb78

vercel bot temporarily deployed to Preview – vault-storybook September 3, 2021 19:54 Inactive

vercel bot temporarily deployed to Preview – vault September 3, 2021 19:54 Inactive

Merge branch 'main' into Vault-2823_CC_Namespace

9587842

vercel bot temporarily deployed to Preview – vault September 3, 2021 20:00 Inactive

vercel bot deployed to Preview – vault-storybook September 3, 2021 20:00 View deployment

ncabatoff approved these changes Sep 7, 2021

View reviewed changes

akshya96 merged commit 650cf8a into main Sep 7, 2021

briankassouf deleted the Vault-2823_CC_Namespace branch January 5, 2022 01:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vault 2823 cc namespace #12393

Vault 2823 cc namespace #12393

akshya96 commented Aug 20, 2021

swayne275 commented Aug 30, 2021

swayne275 commented Aug 30, 2021

swayne275 left a comment

swayne275 Aug 30, 2021

swayne275 Aug 30, 2021

swayne275 Aug 30, 2021

swayne275 Aug 30, 2021

swayne275 Aug 30, 2021

swayne275 Aug 30, 2021

swayne275 Aug 30, 2021

swayne275 Aug 30, 2021

swayne275 Aug 30, 2021

swayne275 Sep 3, 2021

ncabatoff Sep 3, 2021

swayne275 Aug 30, 2021

swayne275 Aug 30, 2021

swayne275 Sep 3, 2021

swayne275 Sep 3, 2021

swayne275 Sep 3, 2021

ncabatoff Sep 3, 2021

swayne275 Sep 3, 2021

ncabatoff Sep 3, 2021

ncabatoff Sep 3, 2021

	// with some entities and tokens for root namespace , overriding what was already there
	// with some entities and tokens for root namespace, overriding what was already there


		}

		func (a ActivityLog) AddEntity(e activity.EntityRecord) {

Vault 2823 cc namespace #12393

Vault 2823 cc namespace #12393

Conversation

akshya96 commented Aug 20, 2021

swayne275 commented Aug 30, 2021

swayne275 commented Aug 30, 2021

swayne275 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment