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

IOptionsMonitor always triggering OnChange #27

Closed
Reintjuu opened this issue Jul 5, 2022 · 2 comments
Closed

IOptionsMonitor always triggering OnChange #27

Reintjuu opened this issue Jul 5, 2022 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@Reintjuu
Copy link

Reintjuu commented Jul 5, 2022

Describe the bug

When using IOptionsMonitor in combination with this library, its OnChange always seems to trigger, even if there are no changes to the configuration.

  1. I'm not sure, but I think it's related to the OnReload always being triggered (https://github.com/MrZoidberg/VaultSharp.Extensions.Configuration/blob/master/Source/VaultSharp.Extensions.Configuration/VaultConfigurationProvider.cs#L83).
  2. I think it's also related to https://github.com/MrZoidberg/VaultSharp.Extensions.Configuration/blob/master/Source/VaultSharp.Extensions.Configuration/VaultConfigurationProvider.cs#L106. The logger is always logging that the key is outdated, even if it's just the "current" key. Shouldn't there be 3 states instead: new, current, outdated?

And I know it's not entirely related to this issue, but the README mentions that sometimes values don't reload and show old/empty values (dotnet/runtime#37860). I was not able to reproduce that issue. Could it be related to the RunAsync not being awaited and the configuration simply not being there yet (https://github.com/MrZoidberg/VaultSharp.Extensions.Configuration/blob/master/Source/VaultSharp.Extensions.Configuration/VaultConfigurationProvider.cs#L80)?

To Reproduce

I've reproduced the issue in this repo: https://github.com/Reintjuu/hashicorp-vault-dotnet-example.

Expected behaviour

OnChange should only trigger when there is an actual change to the configuration.

@Reintjuu Reintjuu added the bug Something isn't working label Jul 5, 2022
@MrZoidberg MrZoidberg self-assigned this Jul 6, 2022
MrZoidberg added a commit that referenced this issue Jul 7, 2022
@MrZoidberg
Copy link
Owner

Thanks for the great issue description.

Indeed, OnReload() is always reloaded in the VaultChangeWatcher, so it should trigger OnChange() in IOptionsMonitor. I've fixed this in the upcoming release. Please note that it checks the version of data in Vault, not the actual values.

RunAsync should not be awaited, it has a Join method that blocks the calling thread.

I hope my fix resolves your issue. I'm planning to do a release asap.

@Reintjuu
Copy link
Author

Reintjuu commented Jul 8, 2022

Thanks, it seems to work now!

Regarding my 2nd point, I think it's more of a semantic issue, it's always logging that the key is outdated because secretData.SecretData.Metadata.Version == currentVersion; is false for secretData.SecretData.Metadata.Version > currentVersion;. That's why I suggested implementing 3 states, instead of 2, for cases when a (current version) cached key is found.

I'll close the issue when your PR is merged.

@Reintjuu Reintjuu closed this as completed Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants