Check SDK versions via version check call #2365

bergundy · 2022-01-12T01:40:43Z

What changed?

Version check call now includes SDK versions

Why?

To be able to alert when they use an outdated or vulnerable SDK version

How did you test it?

Added unit tests

Potential risks

Is hotfix candidate?

No

Closes #2333

yiminc · 2022-01-12T03:04:30Z

service/frontend/workflowHandler.go

@@ -247,7 +257,7 @@ func (wh *WorkflowHandler) RegisterNamespace(ctx context.Context, request *workf
 		return nil, errShuttingDown
 	}

-	if err := wh.versionChecker.ClientSupported(ctx, wh.config.EnableClientVersionCheck()); err != nil {
+	if err := wh.RecordClientVersionAndCheckIfSupported(ctx); err != nil {


why not do this in interceptor?

I was just following the pattern that we already had with the version checker, I'll look into moving it into an interceptor

yiminc · 2022-01-12T03:05:13Z

service/frontend/versionChecker.go

+		// Store if wasn't added racy
+		valIface, _ = vc.sdkInfoCounter.LoadOrStore(info, &sdkCount{})
+	}
+	atomic.AddInt64(&valIface.(*sdkCount).count, 1)


the counter will essentially be request count make by this type of sdk?

I think we have decided not to collect any stats.

yes, it is reset before we send the request

I can change this to be a set instead of counting

common/rpc/interceptor/sdk_version.go

yiminc · 2022-01-12T17:08:11Z

common/rpc/interceptor/sdk_version.go

+}
+
+func (vi *SDKVersionInterceptor) GetAndResetSDKInfo() []check.SDKInfo {
+	sdkInfo := make([]check.SDKInfo, 0)


cap init to the same size as vi.sdkInfoCounter

I was going to suggest this, but since it's not thread safe I figured meh.

Technically if you set the cap as 1000 and a then len changed in another thread making it 1001 before you ranged, once you attempted append on the 1001st item, it'd create a new backing array of size 2002 ((cap + 1) * 2). Granted that's not as likely in an expected-new-key-to-be-rare map like this.

But negligible either way I figured.

I think it's negligible but don't mind adding it

common/rpc/interceptor/sdk_version.go

cretz · 2022-01-13T14:08:00Z

common/rpc/interceptor/sdk_version.go

+	return &SDKVersionInterceptor{
+		sdkInfoSet:           make(map[sdkNameVersion]bool),
+		lock:                 sync.RWMutex{},
+		infoSetSizeCapGetter: infoSetSizeCapGetter,


What is the purpose of this getter? Can't you just have a fixed size? If the size doesn't change at runtime, then just accept a normal int here.

I saw that I only had dynamic config available so that's what I used.
The getter here is to avoid using the frontend Config struct directly because that causes an import cycle.

Can you just pass in the result of the getter during construction instead of storing the getter?

But then it wouldn't make sense to call this dynamic config

Not sure such a config needs to be dynamic (such an "extreme max" value really is better as just a const IMO so as not to add noise to the pieces that actually deserve to be configurable). But 🤷 it matters little.

common/rpc/interceptor/sdk_version.go

bergundy

I'll address the comments soon

common/rpc/interceptor/sdk_version.go

bergundy · 2022-01-13T16:25:07Z

common/rpc/interceptor/sdk_version.go

+	return &SDKVersionInterceptor{
+		sdkInfoSet:           make(map[sdkNameVersion]bool),
+		lock:                 sync.RWMutex{},
+		infoSetSizeCapGetter: infoSetSizeCapGetter,


I saw that I only had dynamic config available so that's what I used.
The getter here is to avoid using the frontend Config struct directly because that causes an import cycle.

common/rpc/interceptor/sdk_version.go

yiminc · 2022-01-15T05:10:18Z

common/rpc/interceptor/sdk_version.go

+}
+
+type SDKVersionInterceptor struct {
+	sdkInfoSet map[sdkNameVersion]bool


nit, can use map[sdkNameVersion]struct{}

Yup brought up at #2365 (comment) but figured it was inconsequential

alexshtin · 2022-01-25T23:37:23Z

common/rpc/interceptor/sdk_version.go

+
+type SDKVersionInterceptor struct {
+	sdkInfoSet map[sdkNameVersion]struct{}
+	lock       sync.RWMutex


lock usually goes as embedded struct.

Suggested change

lock sync.RWMutex

sync.RWMutex

Oh nice, I can do that.

alexshtin · 2022-01-25T23:40:59Z

common/rpc/interceptor/sdk_version.go

+type sdkNameVersion struct {
+	name    string
+	version string
+}
+
+type SDKVersionInterceptor struct {
+	sdkInfoSet map[sdkNameVersion]struct{}
+	lock       sync.RWMutex
+	maxSetSize int
+}


If there is more than one type declaration they both should go inside single type decalration:

Suggested change

type sdkNameVersion struct {

name string

version string

}

type SDKVersionInterceptor struct {

sdkInfoSet map[sdkNameVersion]struct{}

lock sync.RWMutex

maxSetSize int

}

type (

sdkNameVersion struct {

name string

version string

}

SDKVersionInterceptor struct {

sdkInfoSet map[sdkNameVersion]struct{}

lock sync.RWMutex

maxSetSize int

}

)

Sure, can we get a lint rule that enforces this style?

alexshtin · 2022-01-25T23:43:27Z

common/rpc/interceptor/sdk_version.go

+
+// RecordSDKInfo records name and version tuple in memory
+func (vi *SDKVersionInterceptor) RecordSDKInfo(name, version string) {
+	info := sdkNameVersion{name, version}


I would avoid nameless struct initialization. It is not possible to find references with it.

I thought it'd be okay in this simple case, I don't mind fixing

alexshtin · 2022-01-25T23:45:13Z

common/rpc/interceptor/sdk_version.go

+	vi.lock.RLock()
+	overCap := len(vi.sdkInfoSet) >= vi.maxSetSize
+	_, found := vi.sdkInfoSet[info]
+	vi.lock.RUnlock()


Unlock call usually is called with deffer and it goes right after Lock. I understand the here it would require 2 more funcs and probably doesn't worth it though.

Yeah, it's not worth it here

alexshtin · 2022-01-25T23:46:53Z

common/rpc/interceptor/sdk_version.go

+
+	sdkInfo := make([]check.SDKInfo, 0, len(currSet))
+	for k := range currSet {
+		sdkInfo = append(sdkInfo, check.SDKInfo{Name: k.name, Version: k.version})


k is the struct, right? You can use it directly, it will be copied by value:

Suggested change

sdkInfo = append(sdkInfo, check.SDKInfo{Name: k.name, Version: k.version})

sdkInfo = append(sdkInfo, k)

I could have probably just used check.SDKInfo here, in the first iteration of this PR this struct had a times_seen field which we decided to drop.

bergundy

Thanks @alexshtin

bergundy · 2022-01-26T00:07:26Z

common/rpc/interceptor/sdk_version.go

+
+	sdkInfo := make([]check.SDKInfo, 0, len(currSet))
+	for k := range currSet {
+		sdkInfo = append(sdkInfo, check.SDKInfo{Name: k.name, Version: k.version})


I could have probably just used check.SDKInfo here, in the first iteration of this PR this struct had a times_seen field which we decided to drop.

bergundy · 2022-01-26T00:07:48Z

common/rpc/interceptor/sdk_version.go

+	vi.lock.RLock()
+	overCap := len(vi.sdkInfoSet) >= vi.maxSetSize
+	_, found := vi.sdkInfoSet[info]
+	vi.lock.RUnlock()


Yeah, it's not worth it here

bergundy · 2022-01-26T00:08:23Z

common/rpc/interceptor/sdk_version.go

+
+// RecordSDKInfo records name and version tuple in memory
+func (vi *SDKVersionInterceptor) RecordSDKInfo(name, version string) {
+	info := sdkNameVersion{name, version}


I thought it'd be okay in this simple case, I don't mind fixing

bergundy · 2022-01-26T00:08:52Z

common/rpc/interceptor/sdk_version.go

+type sdkNameVersion struct {
+	name    string
+	version string
+}
+
+type SDKVersionInterceptor struct {
+	sdkInfoSet map[sdkNameVersion]struct{}
+	lock       sync.RWMutex
+	maxSetSize int
+}


Sure, can we get a lint rule that enforces this style?

bergundy · 2022-01-26T00:09:28Z

common/rpc/interceptor/sdk_version.go

+
+type SDKVersionInterceptor struct {
+	sdkInfoSet map[sdkNameVersion]struct{}
+	lock       sync.RWMutex


Oh nice, I can do that.

bergundy requested review from cretz, alexshtin, yiminc and a team January 12, 2022 01:40

bergundy self-assigned this Jan 12, 2022

bergundy force-pushed the sdk-version-check branch from 5f4d0f3 to 9c761a0 Compare January 12, 2022 01:42

yiminc reviewed Jan 12, 2022

View reviewed changes

bergundy force-pushed the sdk-version-check branch 2 times, most recently from 8da885f to fde9b67 Compare January 12, 2022 08:43

bergundy requested a review from yiminc January 12, 2022 09:12

cretz reviewed Jan 12, 2022

View reviewed changes

yiminc reviewed Jan 12, 2022

View reviewed changes

bergundy force-pushed the sdk-version-check branch from de385c7 to 63eb27a Compare January 13, 2022 05:40

cretz reviewed Jan 13, 2022

View reviewed changes

bergundy commented Jan 13, 2022

View reviewed changes

cretz approved these changes Jan 13, 2022

View reviewed changes

bergundy force-pushed the sdk-version-check branch 2 times, most recently from 31c6e67 to 429cda1 Compare January 14, 2022 06:54

yiminc approved these changes Jan 15, 2022

View reviewed changes

bergundy force-pushed the sdk-version-check branch from 429cda1 to aea47c0 Compare January 18, 2022 17:17

bergundy added 6 commits January 21, 2022 13:28

Check SDK versions via version check call

daf5114

Use interceptor to record sdk versions

947dd37

Don't keep counts of SDK info, cap and monitor SDK info set size

c57858e

Address review comments

0a9038e

Simplify version interceptor, remove dynamicconfig and metric

c1eb1fb

Use struct{} for sdkInfoSet

b01a99f

bergundy force-pushed the sdk-version-check branch from aea47c0 to b01a99f Compare January 21, 2022 21:28

bergundy enabled auto-merge (squash) January 21, 2022 21:29

bergundy merged commit 65e4958 into master Jan 21, 2022

bergundy deleted the sdk-version-check branch January 21, 2022 21:56

alexshtin reviewed Jan 25, 2022

View reviewed changes

bergundy commented Jan 26, 2022

View reviewed changes

bergundy added a commit that referenced this pull request Feb 1, 2022

Address late review comments from #2365

4a2dcfe

bergundy added a commit that referenced this pull request Feb 2, 2022

Address late review comments from #2365 (#2441)

9fc6b63

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check SDK versions via version check call #2365

Check SDK versions via version check call #2365

bergundy commented Jan 12, 2022

yiminc Jan 12, 2022

bergundy Jan 12, 2022

yiminc Jan 12, 2022

yiminc Jan 12, 2022

bergundy Jan 12, 2022

bergundy Jan 12, 2022

yiminc Jan 12, 2022

cretz Jan 12, 2022 •

edited

Loading

bergundy Jan 12, 2022

cretz Jan 13, 2022

bergundy Jan 13, 2022

cretz Jan 13, 2022

bergundy Jan 13, 2022

cretz Jan 13, 2022

bergundy left a comment

bergundy Jan 13, 2022

yiminc Jan 15, 2022

cretz Jan 18, 2022

alexshtin Jan 25, 2022

bergundy Jan 26, 2022

alexshtin Jan 25, 2022

bergundy Jan 26, 2022

alexshtin Jan 25, 2022

bergundy Jan 26, 2022

alexshtin Jan 25, 2022

bergundy Jan 26, 2022

alexshtin Jan 25, 2022

bergundy Jan 26, 2022

bergundy left a comment

bergundy Jan 26, 2022

bergundy Jan 26, 2022

bergundy Jan 26, 2022

bergundy Jan 26, 2022

bergundy Jan 26, 2022

	sdkInfo = append(sdkInfo, check.SDKInfo{Name: k.name, Version: k.version})
	sdkInfo = append(sdkInfo, k)

Check SDK versions via version check call #2365

Check SDK versions via version check call #2365

Conversation

bergundy commented Jan 12, 2022

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

cretz Jan 12, 2022 • edited Loading

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

bergundy 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

bergundy 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

cretz Jan 12, 2022 •

edited

Loading