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

Does Nacos provide an alternative JSON library, rather than FastJSON, as a new option #2842

Closed
wu-sheng opened this issue May 21, 2020 · 16 comments
Assignees
Projects
Milestone

Comments

@wu-sheng
Copy link
Contributor

Issue Description

JSON library dependency discussion

Describe what happened (or what feature you want)

The context comes from the discussion mail list, https://lists.apache.org/thread.html/rd9cf6b3d8c410a667f1e26b9e8b73b495732ff245ed4a768887061a2%40%3Cdev.skywalking.apache.org%3E
With the continuously reported FastJson security issues, the SkyWalking community is being making the decision whether to remove the Nacos as a configuration center and cluster coordinator option from our official release. Because the security reports make the SkyWalking binary tar can't pass the security scan, and it keeps happening.

Describe what you expected to happen

The consensus of the mail list discussion is very clear, people are preferring to remove the dependency in order to keep the security stability. Before the official discussion, I want to confirm with the Nacos team, does Nacos provide an alternative JSON library? As a new option

@chuntaojun
Copy link
Collaborator

We will gradually unify the selection of Json shelf packages, which should eventually be selected from gson or Jackson

@wu-sheng
Copy link
Contributor Author

As SkyWalking has been keeping received the reports, we have to make the decision in the short term. As you can see from the mail list, nearly all prefer to remove the dependency for now.

Is there any chance, Nacos would remove it in the short term? If not, I am afraid the decision would be, remove now, and discuss adding it back later.

@KomachiSion
Copy link
Collaborator

I can try to fix it.

@wu-sheng
Copy link
Contributor Author

SkyWalkimg will wait until next weekend, if we can't have luck, I will initial the deletion action for next release.

@yanlinly yanlinly added this to the 1.3.0 milestone May 21, 2020
@chuntaojun chuntaojun added this to To do in Nacos via automation May 21, 2020
@chuntaojun
Copy link
Collaborator

#2015

@KomachiSion
Copy link
Collaborator

@wu-sheng How about jackson? We can replace all fastjson to jackson before next weekend.

@wu-sheng
Copy link
Contributor Author

I think Jackson should be more stable, if you don't like GSON. SkyWalking uses JSON as the major JSON repo. But Jackson is imported because of ElasticSearch somehow already.

yanlinly added a commit that referenced this issue May 25, 2020
[ISSUE #2842] Replace Fastjson with Jackson for naming instance and health checker.
yanlinly added a commit that referenced this issue May 26, 2020
[ISSUE #2842]Replace Fastjson with Jackson for nacos-api and nacos-client
yanlinly added a commit that referenced this issue May 27, 2020
[ISSUE #2842] Replace Fastjson with Jackson except nacos-test module
yanlinly added a commit that referenced this issue May 28, 2020
TsingLiang added a commit that referenced this issue May 29, 2020
[ISSUE #2842] Fix serverStatus api jackson error
@wu-sheng
Copy link
Contributor Author

Hi, I noticed the Fastjson replacement has been done and had the pre-release. Do we have a scheduled date for the official release?

@KomachiSion
Copy link
Collaborator

@wu-sheng, We are doing stability and compatibility test. If there are no more issues, the official version may be released next week.

@wu-sheng
Copy link
Contributor Author

wu-sheng commented Jun 3, 2020

I noticed new PR is still linked to this, when should I expect the release date? SkyWalking is going to run 8.0 release test next week.

@KomachiSion
Copy link
Collaborator

@wu-sheng, The new PR is remove fastjson the inner module nacos-test, which will not as release module.

After the new PR merged, fastjson will be removed completely from Nacos. If the new PR not merged, there are no affect for releasing and skywalking.

For the release, We has finished the stability and compatibility test, there are no block issues, so we plan release at Friday, this week.

@wu-sheng
Copy link
Contributor Author

wu-sheng commented Jun 3, 2020

Great!! I could follow that.

@wu-sheng
Copy link
Contributor Author

wu-sheng commented Jun 3, 2020

@songzhendong Could you finish the SkyWalking main dependency upgrade?

yanlinly added a commit that referenced this issue Jun 3, 2020
[ISSUE #2842] Replace all fastjson with jackson
@chuntaojun chuntaojun moved this from To do to In progress in Nacos Jun 4, 2020
@chuntaojun chuntaojun moved this from In progress to Done in Nacos Jun 4, 2020
@yanlinly yanlinly mentioned this issue Jun 5, 2020
5 tasks
@songzhendong
Copy link

A few days after Nacos released the 1.3.0 (waiting for stabilization,the beta version is not suitable for skywalking 8 release,temporarily remove Nacos from skywalking main dependency), I'm going to re integrate it into skywalking.

@wu-sheng
Copy link
Contributor Author

wu-sheng commented Jun 6, 2020

@songzhendong Thanks for following this. And I noticed you have gotten removing PR merged in SkyWalking main repo.

To Nacos team, thanks for providing this. After the SkyWalking 8.0 release, we will evaluate the latest Nacos release again.

@KomachiSion
Copy link
Collaborator

Well, Nacos has already removed fastjson in 1.3.0. We look forward to working with Skywalking again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Nacos
  
Done
Development

No branches or pull requests

5 participants