-
Notifications
You must be signed in to change notification settings - Fork 853
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
Expose IConfigChangeListener callback service #1734
Conversation
@Tratcher Thanks for the feedback! I'll come back to you shortly. |
0dcd1ff
to
5e0e600
Compare
Done. I've rebased the PR and applied your recommendations. |
5e0e600
to
1aa8af6
Compare
Note to self: Reword commit message and cleanup history. |
@samsp-msft PTAL |
@nulltoken sorry I'm slow reviewing this, I wanted to show the design to the team. I'll get back to it later next week. |
How would you want to report the config 'version' when there are multiple config's loaded/merged? A version for each source, or a combined count of how many times the config had successfully changed? For the former I think we'd need to add an Id field to IProxyConfig and then invoke a callback whenever that config changed and loaded successfully, passing that instance (and the source?). For the latter, we could have a callback that was invoked every time config changed and you could keep your own count. |
1aa8af6
to
e76e100
Compare
e76e100
to
42ba409
Compare
42ba409
to
9525529
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patience @nulltoken. This looks pretty good. I'll show it to the team and get their thoughts.
@samsp-msft what do you think about this approach to making config updates more trackable? Give each instance an ID and fire a service callback when it's been applied/failed.
/// <summary> | ||
/// Optional configuration version identifier | ||
/// </summary> | ||
string? Id { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to do this in a API non-breaking way, like using C#'s new Default Interface Methods. I'd also suggest making it non-nullable and assigning it a new random/guid value by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Tratcher I may have misunderstood your proposal and need some guidance here.
If we assign a new random guid to the Id
property through the default interface methods, and this property isn't explicitly implemented in derived types (existing code for instance), we will end up with a non stable behavior when retrieving the content of the Id
.
Which may make this harder to test and reason about.
Below a repro code putting under the light the potential issue.
Is this what you had in mind or did I misunderstand the intended implementation?
public interface IProxyConfig
{
string Id { get { return Guid.NewGuid().ToString(); } }
IReadOnlyList<RouteConfig> Routes { get; }
IReadOnlyList<ClusterConfig> Clusters { get; }
IChangeToken ChangeToken { get; }
}
private class SomeInMemoryConfig : IProxyConfig
{
private readonly CancellationTokenSource _cts = new CancellationTokenSource();
public SomeInMemoryConfig(IReadOnlyList<RouteConfig> routes, IReadOnlyList<ClusterConfig> clusters)
{
Routes = routes;
Clusters = clusters;
ChangeToken = new CancellationChangeToken(_cts.Token);
}
public IReadOnlyList<RouteConfig> Routes { get; }
public IReadOnlyList<ClusterConfig> Clusters { get; }
public IChangeToken ChangeToken { get; }
internal void SignalChange()
{
_cts.Cancel();
}
}
[Fact]
public void test()
{
var old = new SomeInMemoryConfig(new List<RouteConfig>(), new List<ClusterConfig>());
var proxyConfig = old as IProxyConfig;
output.WriteLine(proxyConfig.Id); // => 9e934246-cab8-48b0-9002-7f228fbd459d
output.WriteLine(proxyConfig.Id); // => 710fb8a8-daa4-434a-8ecb-08cec511846c
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try: string Id { get } = Guid.NewGuid().ToString();
That way it's initialized once when the object is created and then remains stable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Tratcher I may be missing something...
The compiler is complaining with "CS8053 Instance properties in interfaces cannot have initializers."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Tratcher any feedback regarding my previous comment about not being able to initialize the property once when targeting the interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use a trick like this:
private static readonly ConditionalWeakTable<IProxyConfig, string> _revisionIdsTable = new();
string? RevisionId => _revisionIdsTable.GetValue(this, static _ => Guid.NewGuid().ToString());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MihaZupan Thanks for the (really great!) hint. That did the trick.
RevisionId
is now non nullable.
@nulltoken Thanks for submitting this. We're generally happy with the approach, and we intend to take this PR. We'll do some more review and suggest a few minor changes (naming, API design, etc.). |
src/Kubernetes.Controller/ConfigProvider/KubernetesConfigProvider.cs
Outdated
Show resolved
Hide resolved
src/ReverseProxy/Configuration/ConfigProvider/ConfigurationSnapshot.cs
Outdated
Show resolved
Hide resolved
9525529
to
f64fe64
Compare
@MihaZupan @adityamandaleeka Thanks for the comments. Unfortunately, I won't be able to work on this for the next two weeks. I'll ping you when I'm done. |
1861b59
to
9195ea4
Compare
@MihaZupan @adityamandaleeka 👋 I believe all the comments have been dealt with. |
9195ea4
to
5939f24
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nulltoken
3f44dcd
to
aecee74
Compare
@Tratcher @MihaZupan Comments have been dealt with. Ready for another pass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work here nulltoken and for the patience through reviews.
I think the changes here look good, my only concern is with the interface name.
I'm not a fan of *Notifier
given that the interface is being notified, not doing the notifying. Could be something like Listener
/Subscriber
/Observer
/Consumer
instead?
Prior art: IClusterChangeListener
I'll bring it up to the team and we should have a decision today either way.
src/Kubernetes.Controller/ConfigProvider/KubernetesConfigProvider.cs
Outdated
Show resolved
Hide resolved
@MihaZupan No problem. Let me know what's the team's decision regarding the name and I'll update it. |
89613e2
to
7791617
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nulltoken
Fix #1619
From the original issue:
"When dynamically applying loaded configuration (cf. https://microsoft.github.io/reverse-proxy/articles/config-providers.html) after the initial startup, any validation failure will be logged, then silenced.
We would need a way to get notified of both successful configuration changes and invalid configuration rejection."
This is an attempt to propagate whether or not the loaded configuration has been applied or not.