Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 108 additions & 0 deletions azureappconfiguration/azureappconfiguration.go
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@ func (azappcfg *AzureAppConfiguration) loadKeyValues(ctx context.Context, settin
var useAIConfiguration, useAIChatCompletionConfiguration bool
kvSettings := make(map[string]any, len(settingsResponse.settings))
keyVaultRefs := make(map[string]string)
snapshotRefs := make(map[string]string)
for trimmedKey, setting := range rawSettings {
if setting.ContentType == nil || setting.Value == nil {
kvSettings[trimmedKey] = setting.Value
Expand All @@ -396,6 +397,9 @@ func (azappcfg *AzureAppConfiguration) loadKeyValues(ctx context.Context, settin
continue // ignore feature flag while getting key value settings
case secretReferenceContentType:
keyVaultRefs[trimmedKey] = *setting.Value
case snapshotReferenceContentType:
snapshotRefs[trimmedKey] = *setting.Value
azappcfg.tracingOptions.UseSnapshotReference = true
default:
if isJsonContentType(setting.ContentType) {
var v any
Expand Down Expand Up @@ -424,6 +428,21 @@ func (azappcfg *AzureAppConfiguration) loadKeyValues(ctx context.Context, settin
azappcfg.tracingOptions.UseAIConfiguration = useAIConfiguration
azappcfg.tracingOptions.UseAIChatCompletionConfiguration = useAIChatCompletionConfiguration

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
}
}
Comment on lines +431 to +443
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Copy link
Member

@RichardChen820 RichardChen820 Mar 17, 2026

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 settingClient is selectorSettingsClient, we need throw if it's not. Or we don't have to check it.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

}

secrets, err := azappcfg.loadKeyVaultSecrets(ctx, keyVaultRefs)
if err != nil {
return fmt.Errorf("failed to load Key Vault secrets: %w", err)
Expand All @@ -437,6 +456,78 @@ func (azappcfg *AzureAppConfiguration) loadKeyValues(ctx context.Context, settin
return nil
}

func (azappcfg *AzureAppConfiguration) loadSettingsFromSnapshotRefs(ctx context.Context, loadSnapshot snapshotSettingsLoader, snapshotRefs map[string]string, kvSettings map[string]any, keyVaultRefs map[string]string) error {
var useAIConfiguration, useAIChatCompletionConfiguration bool
for key, snapshotRef := range snapshotRefs {
// Parse the snapshot reference
snapshotName, err := parseSnapshotReference(snapshotRef)
if err != nil {
return fmt.Errorf("invalid format for Snapshot reference setting %s: %w", key, err)
}

// Load the snapshot settings
settingsFromSnapshot, err := loadSnapshot(ctx, snapshotName)
if err != nil {
return fmt.Errorf("failed to load snapshot settings: key=%s, error=%w", key, err)
}

for _, setting := range settingsFromSnapshot {
if setting.Key == nil {
continue
}

trimmedKey := azappcfg.trimPrefix(*setting.Key)
if len(trimmedKey) == 0 {
log.Printf("Key of the setting '%s' is trimmed to the empty string, just ignore it", *setting.Key)
continue
}

if setting.ContentType == nil || setting.Value == nil {
kvSettings[trimmedKey] = setting.Value
continue
}

contentType := strings.TrimSpace(strings.ToLower(*setting.ContentType))
if contentType == featureFlagContentType {
continue
}

if contentType == secretReferenceContentType {
keyVaultRefs[trimmedKey] = *setting.Value
continue
}

// Handle JSON content types (similar to regular key-value loading)
if isJsonContentType(setting.ContentType) {
var v any
if err := json.Unmarshal([]byte(*setting.Value), &v); err != nil {
// If the value is not valid JSON, try to remove comments and parse again
if err := json.Unmarshal(jsonc.StripComments([]byte(*setting.Value)), &v); err != nil {
// If still invalid, log the error and treat it as a plain string
log.Printf("Failed to unmarshal JSON value from snapshot: key=%s, error=%s", *setting.Key, err.Error())
kvSettings[trimmedKey] = setting.Value
continue
}
}
kvSettings[trimmedKey] = v
if isAIConfigurationContentType(setting.ContentType) {
useAIConfiguration = true
}
if isAIChatCompletionContentType(setting.ContentType) {
useAIChatCompletionConfiguration = true
}
} else {
kvSettings[trimmedKey] = setting.Value
}
}
}

azappcfg.tracingOptions.UseAIConfiguration = useAIConfiguration
azappcfg.tracingOptions.UseAIChatCompletionConfiguration = useAIChatCompletionConfiguration

return nil
}

func (azappcfg *AzureAppConfiguration) loadKeyVaultSecrets(ctx context.Context, keyVaultRefs map[string]string) (map[string]any, error) {
secrets := make(map[string]any)
if len(keyVaultRefs) == 0 {
Expand Down Expand Up @@ -1019,3 +1110,20 @@ func isFailoverable(err error) bool {

return false
}

// "{\"snapshot_name\":\"referenced-snapshot\"}"
func parseSnapshotReference(ref string) (string, error) {
var snapshotRef struct {
SnapshotName string `json:"snapshot_name"`
}

if err := json.Unmarshal([]byte(ref), &snapshotRef); err != nil {
return "", fmt.Errorf("failed to parse snapshot reference: %w", err)
}

if snapshotRef.SnapshotName == "" {
return "", fmt.Errorf("snapshot_name is empty in snapshot reference")
}

return snapshotRef.SnapshotName, nil
}
17 changes: 9 additions & 8 deletions azureappconfiguration/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,15 @@ const (

// General configuration constants
const (
defaultLabel = "\x00"
wildCard = "*"
defaultSeparator = "."
secretReferenceContentType string = "application/vnd.microsoft.appconfig.keyvaultref+json;charset=utf-8"
featureFlagContentType string = "application/vnd.microsoft.appconfig.ff+json;charset=utf-8"
featureFlagKeyPrefix string = ".appconfig.featureflag/"
featureManagementSectionKey string = "feature_management"
featureFlagSectionKey string = "feature_flags"
defaultLabel = "\x00"
wildCard = "*"
defaultSeparator = "."
secretReferenceContentType string = "application/vnd.microsoft.appconfig.keyvaultref+json;charset=utf-8"
snapshotReferenceContentType string = "application/json; profile=\"https://azconfig.io/mime-profiles/snapshot-ref\"; charset=utf-8"
featureFlagContentType string = "application/vnd.microsoft.appconfig.ff+json;charset=utf-8"
featureFlagKeyPrefix string = ".appconfig.featureflag/"
featureManagementSectionKey string = "feature_management"
featureFlagSectionKey string = "feature_flags"
)

// Feature flag constants
Expand Down
6 changes: 6 additions & 0 deletions azureappconfiguration/internal/tracing/tracing.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ const (
FailoverRequestTag = "Failover"
ReplicaCountKey = "ReplicaCount"
LoadBalancingEnabledTag = "LB"
SnapshotReferenceTag = "SnapshotRef"

// Feature flag usage tracing
FMGoVerEnv = "MS_FEATURE_MANAGEMENT_GO_VERSION"
Expand Down Expand Up @@ -74,6 +75,7 @@ type Options struct {
KeyVaultRefreshConfigured bool
UseAIConfiguration bool
UseAIChatCompletionConfiguration bool
UseSnapshotReference bool
IsFailoverRequest bool
ReplicaCount int
IsLoadBalancingEnabled bool
Expand Down Expand Up @@ -143,6 +145,10 @@ func CreateCorrelationContextHeader(ctx context.Context, options Options) http.H
features = append(features, LoadBalancingEnabledTag)
}

if options.UseSnapshotReference {
features = append(features, SnapshotReferenceTag)
}

if len(features) > 0 {
featureStr := FeaturesKey + "=" + strings.Join(features, DelimiterPlus)
output = append(output, featureStr)
Expand Down
61 changes: 60 additions & 1 deletion azureappconfiguration/internal/tracing/tracing_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,12 +145,70 @@ func TestCreateCorrelationContextHeader(t *testing.T) {
assert.Contains(t, features, AIChatCompletionConfigurationTag)
})

t.Run("with snapshot reference", func(t *testing.T) {
ctx := context.Background()
options := Options{
UseSnapshotReference: true,
}

header := CreateCorrelationContextHeader(ctx, options)

corrContext := header.Get(CorrelationContextHeader)
assert.Contains(t, corrContext, FeaturesKey+"="+SnapshotReferenceTag)
})

t.Run("with snapshot reference not set", func(t *testing.T) {
ctx := context.Background()
options := Options{
UseSnapshotReference: false,
}

header := CreateCorrelationContextHeader(ctx, options)

corrContext := header.Get(CorrelationContextHeader)
assert.NotContains(t, corrContext, SnapshotReferenceTag)
})

t.Run("with snapshot reference and other features", func(t *testing.T) {
ctx := context.Background()
options := Options{
UseAIConfiguration: true,
UseSnapshotReference: true,
}

header := CreateCorrelationContextHeader(ctx, options)

corrContext := header.Get(CorrelationContextHeader)
assert.Contains(t, corrContext, FeaturesKey+"=")

// Extract the Features part
parts := strings.Split(corrContext, DelimiterComma)
var featuresPart string
for _, part := range parts {
if strings.HasPrefix(part, FeaturesKey+"=") {
featuresPart = part
break
}
}

// Check both tags are in the features part
assert.Contains(t, featuresPart, AIConfigurationTag)
assert.Contains(t, featuresPart, SnapshotReferenceTag)

// Check the delimiter is correct
features := strings.Split(strings.TrimPrefix(featuresPart, FeaturesKey+"="), DelimiterPlus)
assert.Len(t, features, 2)
assert.Contains(t, features, AIConfigurationTag)
assert.Contains(t, features, SnapshotReferenceTag)
})

t.Run("with all options", func(t *testing.T) {
options := Options{
Host: HostTypeAzureFunction,
KeyVaultConfigured: true,
UseAIConfiguration: true,
UseAIChatCompletionConfiguration: true,
UseSnapshotReference: true,
}

header := CreateCorrelationContextHeader(context.Background(), options)
Expand All @@ -172,9 +230,10 @@ func TestCreateCorrelationContextHeader(t *testing.T) {
}
}

// Check both AI tags are in the features part
// Check all feature tags are in the features part
assert.Contains(t, featuresPart, AIConfigurationTag)
assert.Contains(t, featuresPart, AIChatCompletionConfigurationTag)
assert.Contains(t, featuresPart, SnapshotReferenceTag)

// Verify the header format
assert.Equal(t, 4, strings.Count(corrContext, DelimiterComma)+1, "Should have 4 parts")
Expand Down
2 changes: 1 addition & 1 deletion azureappconfiguration/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ type Selector struct {
// comparableKey returns a comparable representation of the Selector that can be used as a map key.
// This method creates a deterministic string representation by sorting the TagFilter slice.
func (s Selector) comparableKey() comparableSelector {
cs := comparableSelector{
cs := comparableSelector{
KeyFilter: s.KeyFilter,
LabelFilter: s.LabelFilter,
SnapshotName: s.SnapshotName,
Expand Down
48 changes: 33 additions & 15 deletions azureappconfiguration/settings_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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...)
}
}

Expand Down Expand Up @@ -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 {
return settings, nil // treat non-existing snapshot as empty
Copy link
Member

Choose a reason for hiding this comment

The 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.
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?

Copy link
Member

Choose a reason for hiding this comment

The 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)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

The 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.
If there is an unresolvable snapshot reference, we swallow and ignore the error.

I'm wondering what our reasoning is.

Copy link
Member

Choose a reason for hiding this comment

The 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.
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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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
}
Loading
Loading