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(server): Added validation for duplicated link config names #516

Merged
merged 3 commits into from
Dec 19, 2024

Conversation

ffuerste
Copy link

Feature or Problem

If an application manifest is applied in which several link configurations have the same name, this leads to a dead loop.

Related Issues

closes #478

Release Information

This PR fixes a problem without making any breaking changes. It can therefore be safely included in the next version.

Consumer Impact

This PR introduces an additional validation logic that prevents users to apply invalid manifests. If the manifest is valid, nothing changes for the users.

Additionally, it is planned to modify the BackoffWrapper and introduce an exponentially increasing delay for a retry in case of an error, which introduces longer feedback loops for (debugging) users.

Testing

Unit Test(s)

Acceptance or Integration

An additional test case has been added for the new validation logic.

Note: Further tests may be added as part of the “BackoffWrapper” modification.

@ffuerste
Copy link
Author

@brooksmtownsend I created a draft PR for the first part of your suggestion. So that you can already review it.

Meanwhile I will look into the BackoffWrapper idea and update the PR accordingly.

Many thanks.

Copy link
Member

@brooksmtownsend brooksmtownsend left a comment

Choose a reason for hiding this comment

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

Great work @ffuerste ! I think we could get this one ready for review and merge as-is as the individual feature if you're interested, rather than adding on the BackoffWrapper bit here.

I left a really small nit that you can address if you need to push up any additional fixes, but it's fine to just leave it as-is as well. Feel free to mark it as resolved once you feel it's fixed or that it is not necessary and we can merge.

crates/wadm-types/src/validation.rs Outdated Show resolved Hide resolved
@ffuerste ffuerste force-pushed the fix/validate-config-names branch from 74e7364 to 6f717b6 Compare December 18, 2024 19:00
@ffuerste ffuerste force-pushed the fix/validate-config-names branch from 6f717b6 to b92055b Compare December 18, 2024 19:44
@ffuerste
Copy link
Author

@brooksmtownsend Many thanks for the review!

I agree, I think it is better to merge the validation logic as an individual feature for now. Figuring the BackoffWrapper part out is a little bit tricky for me at the moment. I suggest, that we create maybe another issue for this part?

Therefore, I remove the draft status and this PR can be reviewed from my side.

@ffuerste ffuerste marked this pull request as ready for review December 18, 2024 19:52
@ffuerste ffuerste requested a review from a team as a code owner December 18, 2024 19:52
@brooksmtownsend brooksmtownsend merged commit b2a1082 into wasmCloud:main Dec 19, 2024
5 checks passed
@ffuerste ffuerste deleted the fix/validate-config-names branch December 19, 2024 20:35
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.

[BUG] Deadloop for duplicated configuration
2 participants