-
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 |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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 | ||
|
|
@@ -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 | ||
| } | ||
| } | ||
| } | ||
|
|
||
| secrets, err := azappcfg.loadKeyVaultSecrets(ctx, keyVaultRefs) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to load Key Vault secrets: %w", err) | ||
|
|
@@ -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 | ||
|
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 AI Configuration is used in the snapshot ref, do we need to adding it to the telemetry?
Member
Author
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, added |
||
| 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 { | ||
|
|
@@ -1019,3 +1110,20 @@ func isFailoverable(err error) bool { | |
|
|
||
| return false | ||
| } | ||
|
|
||
| // "{\"snapshot_name\":\"referenced-snapshot\"}" | ||
linglingye001 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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 | ||
| } | ||
| 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. |
||
| } | ||
| 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.