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.

Sign up for GitHub

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

Jump to bottom

add a rate limiter to config auto-reload #12490

Merged
merged 108 commits into from
Apr 4, 2022
Merged

add a rate limiter to config auto-reload #12490

merged 108 commits into from
Apr 4, 2022

Conversation

dhiaayachi
Copy link
Contributor

@dhiaayachi dhiaayachi commented Mar 2, 2022 •
edited
Loading

This add a coalesce timer to auto config reload to reduce the risk that we reload incomplete configuration or DDOS Consul by modifying the config file.
The coalesce timer is implemented as a Watcher interface that wrap the original file watcher.

dhiaayachi and others added 30 commits February 21, 2022 11:47
@dhiaayachi
add config watcher to the config package
63d0a12
@dhiaayachi
add logging to watcher
9a40118
@dhiaayachi
add test and refactor to add WatcherEvent.
d8d34a8
@dhiaayachi
add all API calls and fix a bug with recreated files
14675c1
@dhiaayachi
add tests for watcher
fa069e7
@dhiaayachi
remove the unnecessary use of context
2a4efc4
@dhiaayachi
Add debug log and a test for file rename
5d6f328
@dhiaayachi
use inode to detect if the file is recreated/replaced and only listen…
5fbf587 
… to create events.
@thisisnotashwin @dhiaayachi
tidy ups (#1535)
060937a 
* tidy ups

* Add tests for inode reconcile
@dhiaayachi
fix linux vs windows syscall
d219ccb
@dhiaayachi
fix linux vs windows syscall
35135f7
@dhiaayachi
fix windows compile error
0598fc5
@dhiaayachi
increase timeout
e619140
@dhiaayachi
use ctime ID
6617bff
@dhiaayachi
remove remove/creation test as it's a use case that fail in linux
834f173
@dhiaayachi
fix linux/windows to use Ino/CreationTime
c29dcfa
@dhiaayachi
fix the watcher to only overwrite current file id
32cf0d6
@dhiaayachi
fix linter error
8bee625
@dhiaayachi
fix remove/create test
c7d4afa
@dhiaayachi
set reconcile loop to 200 Milliseconds
4471980
@dhiaayachi
fix watcher to not trigger event on remove, add more tests
f2d710a
@dhiaayachi
on a remove event try to add the file back to the watcher and trigger…
5919390 
… the handler if success
@dhiaayachi
fix race condition
3414acd
@dhiaayachi
fix flaky test
ed7f308
@dhiaayachi
fix race conditions
66611a6
@dhiaayachi
set level to info
b7fce5b
@dhiaayachi
fix when file is removed and get an event for it after
64e0a7d
@dhiaayachi
fix to trigger handler when we get a remove but re-add fail
d8c8444
@dhiaayachi
fix error message
5e61b7a
@dhiaayachi
add tests for directory watch and fixes
0aa983f
@vercel vercel bot temporarily deployed to Preview – consul April 1, 2022 17:07 Inactive
@vercel vercel bot temporarily deployed to Preview – consul April 1, 2022 17:14 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging April 1, 2022 17:14 Inactive
rboyer
rboyer reviewed Apr 1, 2022
View reviewed changes
agent/agent.go Outdated Show resolved Hide resolved
@dhiaayachi @rboyer
Apply suggestions from code review
cb7b1da 
Co-authored-by: R.B. Boyer <4903+rboyer@users.noreply.github.com>
@vercel vercel bot temporarily deployed to Preview – consul April 2, 2022 01:02 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging April 2, 2022 01:02 Inactive
@vercel vercel bot temporarily deployed to Preview – consul April 2, 2022 01:05 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging April 2, 2022 01:05 Inactive
@dhiaayachi dhiaayachi requested a review from rboyer April 2, 2022 01:06
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging April 4, 2022 13:10 Inactive
@vercel vercel bot temporarily deployed to Preview – consul April 4, 2022 13:10 Inactive
kisunji
kisunji approved these changes Apr 4, 2022
View reviewed changes
Copy link
Contributor

@kisunji kisunji left a comment

Choose a reason for hiding this comment

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

LGTM! Small nits

agent/config/ratelimited_file_watcher.go Outdated Show resolved Hide resolved
agent/config/ratelimited_file_watcher_test.go Outdated Show resolved Hide resolved
@dhiaayachi
Apply suggestions from code review
d5d68c4 
Co-authored-by: Chris S. Kim <ckim@hashicorp.com>
@vercel vercel bot temporarily deployed to Preview – consul April 4, 2022 14:14 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging April 4, 2022 14:14 Inactive
@dhiaayachi dhiaayachi dismissed rboyer’s stale review April 4, 2022 15:30

This need to go in for the release, I addressed all @rboyer comments and had @kisunji take a look

@dhiaayachi dhiaayachi merged commit 83720e5 into main Apr 4, 2022
@dhiaayachi dhiaayachi deleted the config-autoreload/rate-limiter branch April 4, 2022 15:31
@hc-github-team-consul-core
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/622677.

@kevinschoonover kevinschoonover mentioned this pull request Sep 4, 2022
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boxofrad boxofrad boxofrad left review comments

@acpana acpana acpana left review comments

@kisunji kisunji kisunji approved these changes

@rboyer rboyer Awaiting requested review from rboyer

Assignees
No one assigned
Labels
pr/no-changelog PR does not need a corresponding .changelog entry pr/no-metrics-test
Projects
None yet
Milestone
1.12.0-beta1
Development

Successfully merging this pull request may close these issues.
8 participants
@dhiaayachi @hashicorp-cla @hc-github-team-consul-core @rboyer @boxofrad @acpana @kisunji @thisisnotashwin