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

feat: add dry-run flag for config profile apply command #5455

Merged
merged 2 commits into from
Oct 24, 2018
Merged

feat: add dry-run flag for config profile apply command #5455

merged 2 commits into from
Oct 24, 2018

Conversation

qiwaa
Copy link
Contributor

@qiwaa qiwaa commented Sep 13, 2018

Issue: #4914

License: MIT
Signed-off-by: chenminjian [email protected]

@qiwaa qiwaa requested a review from Kubuxu as a code owner September 13, 2018 11:38
@ghost ghost assigned Stebalien Sep 13, 2018
@ghost ghost added the status/in-progress In progress label Sep 13, 2018
@Stebalien
Copy link
Member

  • Our CI needs to be able to fetch gx packages so some reachable IPFS node has to have them pinned locally (done).
  • Gx packages need to specify a dvcsImport path. Otherwise, path rewriting won't work. I've fixed up the gx packages so this should be fine now.

(basically, gx has a lot of rough edges we're working on ironing out).

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Looking good but we'll definitely need some sharness tests. You'll probably want to add them to test/sharness/t0021-config.sh.

core/commands/config.go Show resolved Hide resolved
core/commands/config.go Outdated Show resolved Hide resolved
@Stebalien Stebalien added the need/author-input Needs input from the original author label Sep 13, 2018
@qiwaa
Copy link
Contributor Author

qiwaa commented Sep 14, 2018

@Stebalien Thanks for your guidance. I find that /elgris/jsondiff doesn't support unmarshal. Should I write a new jsondiff lib to support this feat?

@Stebalien Stebalien removed the need/author-input Needs input from the original author label Sep 14, 2018
@Stebalien
Copy link
Member

Hm. I see. Well, we have two options:

  1. Send a []jsondiff.Item array (by calling diff.Items()) and then reconstruct the diff using diff.Add on the client. I'm not a fan of this approach...
  2. Send back the old config and the new config and do the diff on the client. Personally, I'd rather do this.

That is, send back ConfigUpdateOutput { Old: OldConfig, New: NewConfig } and call jsondiff on the client. Does that sound reasonable? That's probably the most flexible solution.

@qiwaa
Copy link
Contributor Author

qiwaa commented Sep 17, 2018

@Stebalien Thx. The second method looks great and I use it. Could you help me review my code again?

@Stebalien
Copy link
Member

@schomatis in an effort to "spread the love", I'm assigning you to review 😄.

Copy link
Contributor

@schomatis schomatis left a comment

Choose a reason for hiding this comment

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

Feeling the love 😬

core/commands/config.go Show resolved Hide resolved
core/commands/config.go Outdated Show resolved Hide resolved
@qiwaa
Copy link
Contributor Author

qiwaa commented Sep 20, 2018

hey, @schomatis . I have completed modifications according to your advice. Could you help me review my code again? 😊

schomatis
schomatis previously approved these changes Sep 21, 2018
Copy link
Contributor

@schomatis schomatis left a comment

Choose a reason for hiding this comment

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

Niiiiiicee, this is definitely much cleaner, thanks for taking the time to make those changes.

Copy link
Contributor

@schomatis schomatis left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@qiwaa
Copy link
Contributor Author

qiwaa commented Oct 9, 2018

@Stebalien Is there any other work that needs to be done? If so, please tell me.

Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

Hard blocking the PR.

We changed other config commands to stop exposing Identity.PrivKey over HTTP API for security reasons. This command will also need to do that.

@schomatis
Copy link
Contributor

@Kubuxu Could you expand some more on that please? How is this PR impacting security?

@Kubuxu Kubuxu removed the RFM label Oct 9, 2018
@Kubuxu
Copy link
Member

Kubuxu commented Oct 9, 2018

In the past we hardened config endpoints not to return Identity.PrivKey because it is possible to break CORS protection and access API endpoint in some browsers.

This would reintroduce this risk.

@schomatis schomatis added the topic/security Topic security label Oct 10, 2018
@schomatis
Copy link
Contributor

Oh, I see, the newly added diff output could expose sensitive information, makes sense, thanks.

@qiwaa
Copy link
Contributor Author

qiwaa commented Oct 10, 2018

This is a good suggestion. Thx a lot. I have updated the pr to stop exposing Identity.PrivKey over HTTP API.

@Kubuxu
Copy link
Member

Kubuxu commented Oct 10, 2018

Thanks for resolving it quickly.

@Kubuxu Kubuxu added RFM and removed topic/repo Topic repo labels Oct 10, 2018
@Kubuxu Kubuxu requested a review from Stebalien October 10, 2018 14:44
Stebalien added a commit that referenced this pull request Oct 23, 2018
This fixes the data-race in the config. Depends
on: ipfs/go-ipfs-config#16.

This does not fix #4942 as there's still a
logical race: parallel config updates clobber each other.

However, we'll still need the Clone function for the new `--dry-run` flag
introduced in #5455.

License: MIT
Signed-off-by: Steven Allen <[email protected]>
@Stebalien Stebalien mentioned this pull request Oct 23, 2018
@Stebalien
Copy link
Member

Blocked on #5455, unfortunately.

@Stebalien Stebalien added the status/blocked Unable to be worked further until needs are met label Oct 23, 2018
@Stebalien
Copy link
Member

(that is, the dereference isn't sufficient to actually clone the config).

@qiwaa
Copy link
Contributor Author

qiwaa commented Oct 24, 2018

Maybe dereference is sufficient. Because config.Transformer, which is transformConfig' parameter , just updates map' point, not map' content. This does not affect the global config. Does that make sense?

@Stebalien
Copy link
Member

I see. Yeah, this should work... (well, shouldn't be any worse than it already is).

@Stebalien Stebalien removed the status/blocked Unable to be worked further until needs are met label Oct 24, 2018
chenminjian and others added 2 commits October 24, 2018 05:46
@ghost ghost added the status/in-progress In progress label Oct 24, 2018
@Stebalien Stebalien merged commit d3bba25 into ipfs:master Oct 24, 2018
@ghost ghost removed the status/in-progress In progress label Oct 24, 2018
@qiwaa
Copy link
Contributor Author

qiwaa commented Oct 24, 2018

Hey, @Stebalien . Thx for your review. If you need a better jsondiff lib in the future, ping me please. I am sure I will do this well.

@qiwaa qiwaa deleted the feat/flags/profile-apply branch October 27, 2018 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFM topic/security Topic security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants