V2-telemetry: Simplify sensitive metadata allowlist to accept feature only by akalia25 · Pull Request #63325 · sourcegraph/sourcegraph-public-snapshot

akalia25 · 2024-06-18T22:20:47Z

This PR adds another option to the sensitive metadata allowlist by simplifying the input requirements. to accept feature and the allowlisted privateMetadata keys for that feature.

This change is particularly beneficial when a feature has multiple associated action(s), and the privateMetadata key needs to be allowed for all events related to that feature.

Building upon this initial PR:

https://github.com/sourcegraph/sourcegraph/pull/62830/files

Test plan

CI and unit tests

Changelog

…list

bobheadxi · 2024-06-18T22:42:41Z

@akalia25 ah, I misunderstood - I was hoping we could make action optional, not always omitted. Could we do that instead here?

In env var maybe the action segment could accept "-" to omit it, eg feature::-::..., and in code we don't check the action if it's empty

…pt-feature-only

bobheadxi · 2024-06-24T18:47:10Z

 func TestEventTypesRedact(t *testing.T) {
 	allowedTypes := eventTypes(EventType{
 		Feature:                    "example",
-		Action:                     "exampleAction",


Removing this should cause a failure right? Empty value is unsupported, only explicit wildcard is supported

Either way, these tests should remain unchanged to ensure the existing behaviour works. What should happen instead is you add a few new test cases that demonstrate the wildcard behaviour

this is the test I added for the wildcard behavior, should I add more?

That one tests "is allowed", this test is TestEventTypesRedact which tests redaction. If redaction behaviour is unchanged, then the tests should remain unchanged as well

(i.e. just remove all your changes to this test :) )

ahhh whoops 🙈 sorry, got the tests mixed up. No change to the redaction logic. I'll remove this. Thank you!

Removed the changes I had for TestEventTypesRedact and added a new test allowlisted, with wildcard(*) action match and the expected redaction it should do. Let me know if it's unnecessary and I can remove that . Thanks again!

I think it's a good test to have, thanks! Can you add an assertion on the resulting data?

the function directly mutates the value you give it

bobheadxi · 2024-06-24T18:49:42Z

-			name: "disallowed event with additional allowed event type",
+			name: "disallowed event with additional allowed event type (feature only)",
 			event: &v1.Event{
 				Feature: "cody.completion",
-				Action:  "accepted",
+				Action:  "*",
+			},
+			expectAllowed: true,


This might be a carry-over from what was here before, but the test case says "disallowed event" but the result is expectAllowed: true, it sounds like the test name is incorrect?

yeah, I think the test name is confusing. I tried keeping the same convention, and had to interpret it like this event has privateMetadata ("disallowed event") and there is an allowlist of keys ("with additional allowed event type"). I'll update to make more clear?

I think the test case should say, "event with private metadata, that has fields it is allwoed to export"

updated with new naming :)

bobheadxi · 2024-06-24T18:50:15Z

+			name: "disallowed event with additional allowed event type",
+			event: &v1.Event{
+				Feature: "cody.hoverCommands",
+				Action:  "visible",
 			},
 			expectAllowed: true,


same here, the test case says "disallowed event" but the result is expectAllowed: true

updated with new naming

…card match

bobheadxi · 2024-06-24T19:40:33Z

+					Feature:                    "baz.bar",
+					AllowedPrivateMetadataKeys: []string{"field"},


Shouldn't the expected result here have Action: "*"?

bobheadxi · 2024-06-24T19:41:02Z

 type EventTypes struct {
 	types []EventType
-	// index of '{feature}.{action}:{allowedfields}' for checking
+	// index of '{feature}:{action}:{allowedfields}' for checking


Suggested change

// index of '{feature}:{action}:{allowedfields}' for checking

// index of '{feature}/{action}:{allowedfields}' for checking

bobheadxi · 2024-06-24T19:41:53Z

+		// add condition that if Action is "*"
+		if parts[1] == "*" {
+			types = append(types, EventType{
+				Feature:                    parts[0],
+				AllowedPrivateMetadataKeys: parts[2:],
+			})
+		} else {
+			types = append(types, EventType{
+				Feature:                    parts[0],
+				Action:                     parts[1],
+				AllowedPrivateMetadataKeys: parts[2:],
+			})
+		}


I don't think this is needed because we should have Action: "*" explicitly right?

Correct! this is not needed, and this was also affecting this, because we would not get back action: * on EventType. Updated this. Thank you!

…pes`

…pt-feature-only

…list

…card match

…pes`

…only' of https://github.com/sourcegraph/sourcegraph into ak/update-sensitivemetadataallowlist-to-accept-feature-only

…pt-feature-only

bobheadxi · 2024-06-27T17:39:50Z

 			event: &v1.Event{
 				Feature: "cody.completion",
-				Action:  "accepted",
+				Action:  "*",


This is the event that goes into the "is allowed" check, I think there shouldn't be a wildcard here? We can note that this is defined as a wildcard in the "is allowed" check's list of known events

Suggested change

Action: "*",

Action: "accepted",

bobheadxi · 2024-06-27T18:24:28Z

 			event: &v1.Event{
 				Feature: "cody.completion",
-				Action:  "accepted",
+				Action:  "Accepted",


per naming convention:

Suggested change

Action: "Accepted",

Action: "accepted",

ahh 🤦 thank you, sorry about that!

bobheadxi · 2024-06-27T18:24:59Z

+			mode := allowedTypes.Redact(ev)
+
+			assert.Equal(t, redactMarketingAndUnallowedPrivateMetadataKeys, mode)


Sorry for another spacing nit 😆

Suggested change

mode := allowedTypes.Redact(ev)

assert.Equal(t, redactMarketingAndUnallowedPrivateMetadataKeys, mode)

mode := allowedTypes.Redact(ev)

assert.Equal(t, redactMarketingAndUnallowedPrivateMetadataKeys, mode)

oh you're good! Do you have a linter you use, I'm surprised its not auto-corrected sometimes

No, this is just eyeing it and see if it looks good to read :)

bobheadxi

Nice work!

update to only require feature when creating sensitive metadata allow…

10385b1

…list

cla-bot Bot added the cla-signed label Jun 18, 2024

Update sensitivemetadataallowlist.go

bbf7c87

akalia25 changed the title ~~update to only require feature when creating sensitive metadata allow…~~ V2-telemetry: Simplify sensitive metadata allowlist to accept feature only Jun 18, 2024

akalia25 requested review from bobheadxi and jac June 18, 2024 22:40

akalia25 marked this pull request as ready for review June 18, 2024 22:41

akalia25 marked this pull request as draft June 18, 2024 22:58

akalia25 added 3 commits June 21, 2024 10:45

update to make action optional when allowlisting privateMetadata keys

944eaf5

Merge branch 'main' into ak/update-sensitivemetadataallowlist-to-acce…

74240ae

…pt-feature-only

fix linter issue

6343e49

akalia25 marked this pull request as ready for review June 21, 2024 15:22

allow languageId from blob.codeintel

14e7272

bobheadxi reviewed Jun 21, 2024

View reviewed changes

Comment thread internal/telemetry/sensitivemetadataallowlist/sensitivemetadataallowlist.go Outdated

bobheadxi reviewed Jun 21, 2024

View reviewed changes

Comment thread internal/telemetry/sensitivemetadataallowlist/sensitivemetadataallowlist.go Outdated

bobheadxi reviewed Jun 21, 2024

View reviewed changes

Comment thread internal/telemetry/sensitivemetadataallowlist/sensitivemetadataallowlist.go Outdated

bobheadxi reviewed Jun 21, 2024

View reviewed changes

Comment thread internal/telemetry/sensitivemetadataallowlist/sensitivemetadataallowlist.go Outdated

bobheadxi reviewed Jun 21, 2024

View reviewed changes

Comment thread internal/telemetry/sensitivemetadataallowlist/sensitivemetadataallowlist.go Outdated

update based on feedback

87982dc

bobheadxi reviewed Jun 24, 2024

View reviewed changes

update tests with better naming and add test for redaction on on wild…

edcb17e

…card match

bobheadxi reviewed Jun 24, 2024

View reviewed changes

update assertion on tests and simplify `parseAdditionalAllowedEventTy…

2968bb3

…pes`

bobheadxi reviewed Jun 25, 2024

View reviewed changes

Comment thread internal/telemetry/sensitivemetadataallowlist/sensitivemetadataallowlist_test.go

Update sensitivemetadataallowlist_test.go

8ceb495

akalia25 added 10 commits June 26, 2024 11:24

Merge branch 'main' into ak/update-sensitivemetadataallowlist-to-acce…

083cfe6

…pt-feature-only

update to only require feature when creating sensitive metadata allow…

e25f068

…list

Update sensitivemetadataallowlist.go

1fa2ec0

update to make action optional when allowlisting privateMetadata keys

897f400

fix linter issue

b27d8b6

allow languageId from blob.codeintel

9b41dbe

update based on feedback

54d34c4

update tests with better naming and add test for redaction on on wild…

00c4c02

…card match

update assertion on tests and simplify `parseAdditionalAllowedEventTy…

1dc5580

…pes`

Update sensitivemetadataallowlist_test.go

50e60f1

bobheadxi reviewed Jun 26, 2024

View reviewed changes

Comment thread internal/telemetry/sensitivemetadataallowlist/sensitivemetadataallowlist.go Outdated

bobheadxi reviewed Jun 26, 2024

View reviewed changes

Comment thread internal/telemetry/sensitivemetadataallowlist/sensitivemetadataallowlist_test.go Outdated

bobheadxi reviewed Jun 26, 2024

View reviewed changes

Comment thread internal/telemetry/sensitivemetadataallowlist/sensitivemetadataallowlist_test.go Outdated

akalia25 added 5 commits June 26, 2024 12:49

Merge branch 'ak/update-sensitivemetadataallowlist-to-accept-feature-…

8b25eb2

…only' of https://github.com/sourcegraph/sourcegraph into ak/update-sensitivemetadataallowlist-to-accept-feature-only

update test and remove unneccessary else

f6c0340

Update sensitivemetadataallowlist_test.go

a02e71b

fix test

288662c

Merge branch 'main' into ak/update-sensitivemetadataallowlist-to-acce…

8f2fb7b

…pt-feature-only

bobheadxi reviewed Jun 27, 2024

View reviewed changes

Comment thread internal/telemetry/sensitivemetadataallowlist/sensitivemetadataallowlist_test.go Outdated

rm extra line

2444500

bobheadxi reviewed Jun 27, 2024

View reviewed changes

Update sensitivemetadataallowlist_test.go

f6331ab

bobheadxi reviewed Jun 27, 2024

View reviewed changes

Update sensitivemetadataallowlist_test.go

a9e5922

bobheadxi approved these changes Jun 27, 2024

View reviewed changes

akalia25 merged commit 0777ced into main Jun 27, 2024

akalia25 deleted the ak/update-sensitivemetadataallowlist-to-accept-feature-only branch June 27, 2024 19:22

		Feature: "baz.bar",
		AllowedPrivateMetadataKeys: []string{"field"},

	// index of '{feature}:{action}:{allowedfields}' for checking
	// index of '{feature}/{action}:{allowedfields}' for checking

		mode := allowedTypes.Redact(ev)

		assert.Equal(t, redactMarketingAndUnallowedPrivateMetadataKeys, mode)

Conversation

akalia25 commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Changelog

Uh oh!

bobheadxi commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

bobheadxi Jun 27, 2024 • 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!

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!

bobheadxi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

akalia25 commented Jun 18, 2024 •

edited

Loading

bobheadxi commented Jun 18, 2024 •

edited

Loading

bobheadxi Jun 27, 2024 •

edited

Loading