components-contrib
components-contrib copied to clipboard
feat: file based naming resolver
Description
Provide a file based naming resolver to replace mDNS so that dapr can address each other either on the same host or via shared file system, which would be more stable and friendly for development.
The default directory to keep naming informations is: $HOME/.dapr/naming, and it can be customized in Dapr's config.yaml:
apiVersion: dapr.io/v1alpha1
kind: Configuration
metadata:
name: appconfig
spec:
nameResolution:
component: "file"
configuration:
dir: "/var/tmp/dapr-naming"
Pro and con of this solution:
- Pro: Since the name resolving is file based, it means that it can not only work in slim mode, but also in standalone mode, since the directory to keep the naming informations can be anywhere as long as it's accessible to Dapr's instances.
- Con: Since there has no lifecycle mechanism for components, this solution doesn't have chance to remove (unregister) its naming info from the file when Dapr instance goes away. It may not a big deal in development stage, but in the worst case user needs to know where the directory locates and delete the files manually.
Issue reference
This PR will close: dapr/dapr#3256
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list:
- [x] Code compiles correctly
- [x] Created/updated tests
- [ ] Extended the documentation / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]
@daixiang0 can you please review this?
@yaron2 sure.
ping @beiwei30
ping @beiwei30
sorry for late response, will follow up the review comments today.
@daixiang0 @artursouza I have updated this PR, pls. take a look again.
@beiwei30 Please resolve the conflicts.
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
keepalive
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
@yaron2 @daixiang0 would you mind to review it again? I resolved the conflict today.
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
@beiwei30 can you please run go mod tidy and push the changes? Thanks!
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!
Codecov Report
Merging #1380 (a12728c) into master (a69b9b9) will decrease coverage by
1.04%. The diff coverage is59.16%.
:exclamation: Current head a12728c differs from pull request most recent head e50d2b6. Consider uploading reports for the commit e50d2b6 to get more accurate results
@@ Coverage Diff @@
## master #1380 +/- ##
==========================================
- Coverage 37.62% 36.57% -1.05%
==========================================
Files 192 167 -25
Lines 23982 15640 -8342
==========================================
- Hits 9023 5721 -3302
+ Misses 14193 9272 -4921
+ Partials 766 647 -119
| Impacted Files | Coverage Δ | |
|---|---|---|
| nameresolution/file/file.go | 59.16% <59.16%> (ø) |
|
| metadata/duration.go | 0.00% <0.00%> (-59.62%) |
:arrow_down: |
| bindings/zeebe/command/deploy_process.go | 12.50% <0.00%> (-11.31%) |
:arrow_down: |
| bindings/alicloud/rocketmq/settings.go | 22.22% <0.00%> (-9.78%) |
:arrow_down: |
| bindings/azure/cosmosdb/cosmosdb.go | 18.00% <0.00%> (-7.69%) |
:arrow_down: |
| bindings/alicloud/nacos/settings.go | 33.33% <0.00%> (-6.67%) |
:arrow_down: |
| bindings/alicloud/dingtalk/webhook/settings.go | 33.33% <0.00%> (-6.67%) |
:arrow_down: |
| bindings/mqtt/mqtt.go | 25.00% <0.00%> (-5.64%) |
:arrow_down: |
| bindings/azure/blobstorage/blobstorage.go | 15.09% <0.00%> (-3.81%) |
:arrow_down: |
| lock/redis/standalone.go | 49.33% <0.00%> (-3.66%) |
:arrow_down: |
| ... and 191 more |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
@beiwei30 - When the app becomes unhealthy, there is lot of manual activity involved -
- User should manually delete the name resolution file and lock file.
- All the app instances should be restarted to create a new file with updated name-resolution information for all app instances.[Since we have a common name resolution information for all apps with same id]
Also having 777 file permissions allows anyone to write to the nameresolution file which is a security issue, since the consuming app can be misguided to connect to any endpoint.
To complete this solution, there should be a cleanup of the nameresolution information. when file is chosen as name resolution - dapr cli can periodically check if all the apps as running and clean up the nameresolution information to be accurate - thoughts? @mukundansundar @berndverst
@beiwei30 - When the app becomes unhealthy, there is lot of manual activity involved -
- User should manually delete the name resolution file and lock file.
- All the app instances should be restarted to create a new file with updated name-resolution information for all app instances.[Since we have a common name resolution information for all apps with same id]
Also having 777 file permissions allows anyone to write to the nameresolution file which is a security issue, since the consuming app can be misguided to connect to any endpoint.
To complete this solution, there should be a cleanup of the nameresolution information. when file is chosen as name resolution - dapr cli can periodically check if all the apps as running and clean up the nameresolution information to be accurate - thoughts? @mukundansundar @berndverst
I think we need someone to create a design document for the overall design / experience including the CLI, and the various edge cases before we should proceed here.
This feature is useful, but there seem to be plenty of gaps.
@berndverst and I synced yesterday, and I agree a file-based nameresolver should happen in 1.13, but IMHO this should be based on SQLIte.
Using a single SQLite database will already solve problems such as concurrent access by multiple sidecars (locking, etc). A single file is also easier to handle and to garbage-collect. Lastly, SQLite is also generally faster than the filesystem for these kinds of things.