Conversation
linglingye001
force-pushed
the
linglingye/snapshot-ref
branch
from
d07e3d1 to
a047276
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds "Snapshot Reference" support to the Azure App Configuration Go provider. Snapshot references allow a key-value setting stored in App Configuration to point to a named snapshot — when loaded, the settings from that snapshot are merged into the configuration. The implementation refactors the existing snapshot loading into a shared helper function, adds content-type detection for snapshot references, and emits a tracing tag when they are used.
Changes:
- Extracts snapshot loading into a reusable
loadSnapshotSettingsfunction and addssnapshotSettingsLoaderfunction type to support snapshot reference resolution with dependency injection. - Adds detection of
snapshotReferenceContentTypeinloadKeyValues, collects snapshot references, and resolves them via the newloadSettingsFromSnapshotRefsmethod. - Adds
UseSnapshotReferencetracing option andSnapshotReferenceTagcorrelation context tag.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
azureappconfiguration/azureappconfiguration.go |
Core implementation: detection of snapshot reference content type, loadSettingsFromSnapshotRefs, and parseSnapshotReference |
azureappconfiguration/settings_client.go |
Extracts snapshot loading into loadSnapshotSettings helper; adds snapshotSettingsLoader type |
azureappconfiguration/internal/tracing/tracing.go |
Adds SnapshotReferenceTag and UseSnapshotReference option to correlation context header |
azureappconfiguration/constants.go |
Adds snapshotReferenceContentType constant |
azureappconfiguration/options.go |
Minor whitespace fix |
azureappconfiguration/snapshot_test.go |
Adds unit tests for parseSnapshotReference and loadSettingsFromSnapshotRefs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(snapshotRefs) > 0 { | ||
| var loadSnapshot snapshotSettingsLoader | ||
| if client, ok := settingsClient.(*selectorSettingsClient); ok { | ||
| loadSnapshot = func(ctx context.Context, snapshotName string) ([]azappconfig.Setting, error) { | ||
| return loadSnapshotSettings(ctx, client.client, snapshotName) | ||
| } | ||
| } | ||
|
|
||
| if loadSnapshot != nil { | ||
| if err := azappcfg.loadSettingsFromSnapshotRefs(ctx, loadSnapshot, snapshotRefs, kvSettings, keyVaultRefs); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
When settingsClient is not a *selectorSettingsClient, loadSnapshot remains nil and the snapshot references collected in snapshotRefs are silently dropped — no settings are loaded from them and no error or warning is emitted. While all current callers of loadKeyValues do pass a *selectorSettingsClient, this silent no-op is fragile. If a new settingsClient implementation 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.
There was a problem hiding this comment.
@linglingye001 This is right, if we have to check settingClient is selectorSettingsClient, we need throw if it's not. Or we don't have to check it.
There was a problem hiding this comment.
loadKeyValues accepts a settingsClient interface, It has no access to the underlying *azappconfig.Client. But loadSnapshotSettings needs the raw *azappconfig.Client to call client.GetSnapshot() and client.NewListSettingsForSnapshotPager(). So the type assertion here is to grab client.client through the interface.
| if err != nil { | ||
| var respErr *azcore.ResponseError | ||
| if errors.As(err, &respErr) && respErr.StatusCode == 404 { | ||
| return settings, nil // treat non-existing snapshot as empty |
There was a problem hiding this comment.
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.
In PR, we added a helper method to load configuration settings from snapshot and we will swallow the 404 if snapshot is not found. There are two code paths now: 1. select/load a snapshot (hard-coded in the code) 2. use a snapshot reference
Previously, if user selected a snapshot and that snapshot is not found, we will throw. Now this behavior change.
@jimmyca15 @avanigupta Is this behavior change intended?
There was a problem hiding this comment.
@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)
There was a problem hiding this comment.
Is this behavior change intended?
Yes.
I just realized that we introduced a breaking change
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.
There was a problem hiding this comment.
If there is an unresolvable key vault reference, we throw and fail the whole configuration loading.
If there is an unresolvable snapshot reference, we swallow and ignore the error.
I'm wondering what our reasoning is.
There was a problem hiding this comment.
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.
We have always treated Key Vault errors as fatal to avoid potential security risks. I think it’s less about reference-to-reference comparison and more about which behavior aligns with which type.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ref