Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix listener receiving encrypted content when kms is open. #549

Merged
merged 5 commits into from
Dec 26, 2022

Conversation

a3d21
Copy link
Contributor

@a3d21 a3d21 commented Dec 9, 2022

fixes:#507

@@ -481,7 +481,14 @@ func (client *ConfigClient) refreshContentAndCheck(cacheData *cacheData, notify
}
cacheData.md5 = util.Md5(cacheData.content)
if cacheData.md5 != cacheData.cacheDataListener.lastMd5 {
go cacheData.cacheDataListener.listener(cacheData.tenant, cacheData.group, cacheData.dataId, cacheData.content)
var decryptedContent string
decryptedContent, err = client.decrypt(cacheData.dataId, cacheData.content)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

decryptedContent, err := client.decrypt(cacheData.dataId, cacheData.content) 使用:=简洁一些

cacheData.group, cacheData.tenant)
return
}
go cacheData.cacheDataListener.listener(cacheData.tenant, cacheData.group, cacheData.dataId, decryptedContent)
cacheData.cacheDataListener.lastMd5 = cacheData.md5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

将设置md5以及更新cacheMap的逻辑 放到decrypt和回调listenr前,当解密出现err时 也可以正常的更新md5。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cache逻辑必须放在decrypt()之后,不然decrypt() 异常不会重新触发listener。

Copy link
Member

@binbin0325 binbin0325 Dec 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

目的就是异常后不重新出发listenr,打印日志就可以了。 因为一旦发生异常大概率会造成重复触发listener.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

更新md5也放在回调listener前

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我认为cacheDataListener.lastMd5的更新应该放在decrypt()之后,用来判断是否触发listener,你怎么看?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

和上面的逻辑是一样的,如果不更新md5会重复触发listener。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

按我理解,只有用户开启加密且以下场景之一,才会导致解密失败:

  1. 网络导致
  2. 数据错误

对于 1,应该重复触发listener;对于2,不能重复触发。
我们无法区分1、2,所以统一不重复触发listener。

我理解对吗?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

是的,相当于降级处理。某一次的decrypt失败,不会使sdk一直循环在notify的逻辑,否则不仅仅是sdk的服务CPU上升,还会增大nacos server的压力。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@codecov-commenter
Copy link

Codecov Report

Base: 29.99% // Head: 31.26% // Increases project coverage by +1.27% 🎉

Coverage data is based on head (dfd6212) compared to base (a13f6f2).
Patch coverage: 50.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #549      +/-   ##
==========================================
+ Coverage   29.99%   31.26%   +1.27%     
==========================================
  Files          40       40              
  Lines        2967     2987      +20     
==========================================
+ Hits          890      934      +44     
+ Misses       2013     1988      -25     
- Partials       64       65       +1     
Impacted Files Coverage Δ
clients/config_client/config_client.go 38.75% <0.00%> (-0.41%) ⬇️
...nts/naming_client/naming_http/naming_http_proxy.go 0.00% <0.00%> (ø)
common/nacos_server/nacos_server.go 16.98% <88.23%> (+15.37%) ⬆️
clients/naming_client/naming_http/beat_reactor.go 75.00% <100.00%> (ø)
util/md5.go 100.00% <0.00%> (ø)
.../naming_client/naming_cache/service_info_holder.go 27.36% <0.00%> (+2.10%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@binbin0325 binbin0325 merged commit 8fb78d1 into nacos-group:master Dec 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants