-
Notifications
You must be signed in to change notification settings - Fork 5
Snapshot reference support #57
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
base: release/v1.6.0
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,6 +50,9 @@ type eTagsClient interface { | |
| checkIfETagChanged(ctx context.Context) (bool, error) | ||
| } | ||
|
|
||
| // snapshotSettingsLoader is a function type that loads settings from a snapshot by name. | ||
| type snapshotSettingsLoader func(ctx context.Context, snapshotName string) ([]azappconfig.Setting, error) | ||
|
|
||
| type refreshClient struct { | ||
| loader settingsClient | ||
| monitor eTagsClient | ||
|
|
@@ -86,24 +89,11 @@ func (s *selectorSettingsClient) getSettings(ctx context.Context) (*settingsResp | |
|
|
||
| pageETags[filter.comparableKey()] = eTags | ||
| } else { | ||
| snapshot, err := s.client.GetSnapshot(ctx, filter.SnapshotName, nil) | ||
| snapshotSettings, err := loadSnapshotSettings(ctx, s.client, filter.SnapshotName) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if snapshot.CompositionType == nil || *snapshot.CompositionType != azappconfig.CompositionTypeKey { | ||
| return nil, fmt.Errorf("composition type for the selected snapshot '%s' must be 'key'", filter.SnapshotName) | ||
| } | ||
|
|
||
| pager := s.client.NewListSettingsForSnapshotPager(filter.SnapshotName, nil) | ||
| for pager.More() { | ||
| page, err := pager.NextPage(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } else if page.Settings != nil { | ||
| settings = append(settings, page.Settings...) | ||
| } | ||
| } | ||
| settings = append(settings, snapshotSettings...) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -211,3 +201,31 @@ func (c *pageETagsClient) checkIfETagChanged(ctx context.Context) (bool, error) | |
|
|
||
| return false, nil | ||
| } | ||
|
|
||
| func loadSnapshotSettings(ctx context.Context, client *azappconfig.Client, snapshotName string) ([]azappconfig.Setting, error) { | ||
| settings := make([]azappconfig.Setting, 0) | ||
| snapshot, err := client.GetSnapshot(ctx, snapshotName, nil) | ||
| if err != nil { | ||
| var respErr *azcore.ResponseError | ||
| if errors.As(err, &respErr) && respErr.StatusCode == 404 { | ||
linglingye001 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return settings, nil // treat non-existing snapshot as empty | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just realized that we introduced a breaking change when supporting snapshot reference for .net provider. This applies for all providers since they all use .net provider as blueprint. @jimmyca15 @avanigupta Is this behavior change intended?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @zhiyuanliang-ms I believe this was intentional. We discussed this in my python PR as originally, I didn't catch the error: Azure/azure-sdk-for-python#44116 (comment)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes.
While I agree there was a change, I'm not sure I would qualify it as breaking. A consumer would have had to depend on exceptions for missing snapshots.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there is an unresolvable key vault reference, we throw and fail the whole configuration loading. I'm wondering what our reasoning is.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Snapshots follow key-value semantics. If a key-value doesn’t exist, we don’t fail loading since it may be added later. That same “eventual availability” behavior applies to snapshots and snapshot references, so unresolved snapshot references are ignored.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't load a specific key-value. What we have are filters to key-values. So there is no such a scenario that a key-value doesn't exist. All we have is key-values that match the filtering conditions. However, when a key-value does match the condition, we want to make sure it's resolvable to ensure the configuration data integrity.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the original design for snapshots, configuration providers would throw if attempting to load a snapshot that didn't exist. This was the stance that I agreed with at the time as well. When snapshot references were being discussed, it was argued that this was not the right behavior for snapshots. The point of view here is that snapshots are a bit like a named select statement for key-values, and selecting key-values doesn't result in errors. It was also brought up that this was how the App Configuration API is designed, given that when listing /kv for a non-existent snapshot we return HTTP 200 instead of HTTP 404. That discussion was for loading snapshots in general, and the outcome was to no longer throw. This naturally carried through to snapshot references.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the sentinel refresh scenario, we load a specific key‑value and don’t throw if it’s missing. That behavior is probably where my understanding of key‑values being “eventually available” comes from. |
||
| } | ||
| return nil, err | ||
| } | ||
|
|
||
| if snapshot.CompositionType == nil || *snapshot.CompositionType != azappconfig.CompositionTypeKey { | ||
| return nil, fmt.Errorf("composition type for the selected snapshot '%s' must be 'key'", snapshotName) | ||
| } | ||
|
|
||
| pager := client.NewListSettingsForSnapshotPager(snapshotName, nil) | ||
| for pager.More() { | ||
| page, err := pager.NextPage(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } else if page.Settings != nil { | ||
| settings = append(settings, page.Settings...) | ||
| } | ||
| } | ||
|
|
||
| return settings, nil | ||
| } | ||
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.
When
settingsClientis not a*selectorSettingsClient,loadSnapshotremainsniland the snapshot references collected insnapshotRefsare silently dropped — no settings are loaded from them and no error or warning is emitted. While all current callers ofloadKeyValuesdo pass a*selectorSettingsClient, this silent no-op is fragile. If a newsettingsClientimplementation is added in the future, snapshot references would silently fail to load. Consider logging a warning when snapshot references are detected but cannot be resolved, or returning an error.Uh oh!
There was an error while loading. Please reload this page.
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.
@linglingye001 This is right, if we have to check
settingClientisselectorSettingsClient, we need throw if it's not. Or we don't have to check it.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.
loadKeyValuesaccepts asettingsClientinterface, It has no access to the underlying*azappconfig.Client. ButloadSnapshotSettingsneeds the raw*azappconfig.Clientto callclient.GetSnapshot()andclient.NewListSettingsForSnapshotPager(). So the type assertion here is to grabclient.clientthrough the interface.