Failover support #20

linglingye001 · 2024-03-11T00:27:26Z

No description provided.

juniwang · 2024-03-11T02:15:55Z

api/v1/azureappconfigurationprovider_types.go

-	Configuration             AzureAppConfigurationKeyValueOptions     `json:"configuration,omitempty"`
-	Secret                    *AzureKeyVaultReference                  `json:"secret,omitempty"`
-	FeatureFlag               *AzureAppConfigurationFeatureFlagOptions `json:"featureFlag,omitempty"`
+	Endpoint                  *string `json:"endpoint,omitempty"`


indentation?

juniwang · 2024-03-11T03:08:20Z

internal/loader/configuration_client_manager.go

+		if err != nil {
+			return nil, err
+		}
+		manager.secret, err = parseConnectionString(manager.ConnectionString, SecretSection)


why we keep both secret/id and connectionString? looks like one of them is enough.

juniwang · 2024-03-11T03:09:32Z

internal/loader/configuration_client_manager.go

+	return manager, nil
+}
+
+func SrvTargetHostQuery(host string) ([]string, error) {


put the verb in the beginning sounds more natural as a method name.

juniwang · 2024-03-11T03:13:43Z

internal/loader/configuration_client_manager.go

+		return nil, err
+	}
+
+	originHost := strings.TrimSuffix(originRecords[0].Target, ".")


just want to make sure, the originRecords will never be an empty array. right?

Each replica has _origin._tcp.* SRV record been created to indicate the origin store of the replica. If there's no replica of user's appconfig store, the originRecoreds will be empty.

juniwang · 2024-03-11T03:15:43Z

internal/loader/configuration_client_manager.go

+			}
+		}
+
+		altHost := strings.TrimSuffix(altRecords[0].Target, ".")


why only altRecords[0]?

juniwang · 2024-03-11T03:19:43Z

internal/loader/configuration_client_manager.go

+	currentTime := metav1.Now()
+	if currentTime.After(manager.lastFallbackClientsAttempt.Time.Add(MinimalClientRefreshInterval)) &&
+		(manager.DynamicClients == nil ||
+			currentTime.After(manager.lastFallbackRefresh.Time.Add(FallbackClientRefreshExpireInterval))) {


what is lastFallbackClientsAttempt time and lastFallbackRefresh time?

lastFallbackRefresh -> lastFallbackClientRefresh
lastFallbackClientsRefreshAttempt -> lastFallbackClientRefreshAttempt

lastFallbackClientsAttempt time is the moment provider try to query srv hosts. It may fail during query process, if the query succeed, lastFallbackRefresh time will be updated.

We use them to ensure two things:

Not perform the lookup too frequently, if we see the last attempt is within a certain time period (30s), we will not perform a new lookup

To ensure discovered clients could be refreshed periodically (every 1 hour), if the last success time exceeds 1 hour, we perform a new lookup

internal/loader/configuration_client_manager.go

RichardChen820 · 2024-03-11T09:29:12Z

internal/loader/configuration_client_manager.go

+}
+
+func (manager *ConfigurationClientManager) GetClients() []*ConfigurationClientWrapper {
+	if manager.ReplicaDiscoveryEnabled != true {


!manager.ReplicaDiscoveryEnabled

RichardChen820 · 2024-03-14T02:52:22Z

api/v1/azureappconfigurationprovider_types.go

+	Endpoint                  *string `json:"endpoint,omitempty"`
+	ConnectionStringReference *string `json:"connectionStringReference,omitempty"`
+	// +kubebuilder:default=true
+	ReplicaDiscovery bool                                     `json:"replicaDiscovery,omitempty"`


Put it right after the Endpoint field

RichardChen820 · 2024-03-14T02:56:05Z

internal/controller/appconfigurationprovider_controller.go

@@ -198,18 +199,28 @@ func (reconciler *AzureAppConfigurationProviderReconciler) Reconcile(ctx context
 		}
 	}

+	if reconciler.ProvidersReconcileState[req.NamespacedName].ClientManager == nil ||
+		reconciler.ProvidersReconcileState[req.NamespacedName].Generation != provider.Generation {
+		manager, err := loader.NewConfigurationClientManager(ctx, *provider)


manager -> clientManager

RichardChen820 · 2024-03-14T02:59:59Z

internal/controller/appconfigurationprovider_controller.go

 	/* Create ConfigurationSettingLoader to get the key-value settings from Azure AppConfiguration. */
-	configProvider, err := loader.NewConfigurationSettingLoader(ctx, *provider, nil)
+	manager := reconciler.ProvidersReconcileState[req.NamespacedName].ClientManager


Put this line before line 202

In line 202, just judge whether manager == nil

RichardChen820 · 2024-03-14T03:09:29Z

internal/controller/appconfigurationprovider_controller.go

-	if reconciler.Retriever == nil {
-		retriever = configProvider
-	} else {
+	if reconciler.Retriever != nil {


Unnecessary change

RichardChen820 · 2024-03-14T03:10:25Z

internal/controller/appconfigurationprovider_controller_test.go

@@ -180,6 +181,7 @@ var _ = Describe("AppConfiguationProvider controller", func() {
 				},
 			}

+			// mockConfigClientManager.EXPECT().ExecuteFailoverPolicy(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil)


RichardChen820 · 2024-03-14T03:12:19Z

internal/loader/configuraiton_setting_loader_test.go

+	mockResolveSecretReference    *MockResolveSecretReference
+	mockCtrl                      *gomock.Controller
+	mockCongiurationClientManager *MockClientManager
+	EndpointName                  string = "https://fake-endpoint"


All variables are using camel-style, please update it to be consistent

RichardChen820 · 2024-03-14T03:23:57Z

internal/loader/configuration_setting_loader.go

@@ -39,7 +35,7 @@ import (
 type ConfigurationSettingLoader struct {
 	acpv1.AzureAppConfigurationProvider
 	getSettingsFunc GetSettingsFunc


Seems getSettingsFunc need to be Pascal style, since all struct fields are Pascal Style

RichardChen820 · 2024-03-14T03:34:30Z

internal/loader/configuration_client_manager.go

+
+type ConfigurationClientManager struct {
+	ReplicaDiscoveryEnabled   bool
+	SpecifiedClient           *azappconfig.Client


How about call it StaticClientWrappers and make it to be an array []*ConfigurationClientWrapper ? To potentially support customer can pass more than one static clients.

RichardChen820 · 2024-03-14T03:38:15Z

internal/loader/configuration_client_manager.go

+	DynamicClients            []*ConfigurationClientWrapper
+	ValidDomain               string
+	Endpoint                  string
+	ClientsFallbackStatus     map[string]*ConfigurationClientWrapper


Why we need this? Could use DynamicClients and StaticClients directly

Do you mean we don't need to update dynamicClients fallback status when autofailover enabled?

I'm not saying we don't need to update fallback of dynamicClients, I mean why we don't update it in DynamicClientwrappers directly, not need to use another object which is only for managing the fallbacks

RichardChen820 · 2024-03-14T03:38:41Z

internal/loader/configuration_client_manager.go

+type ConfigurationClientManager struct {
+	ReplicaDiscoveryEnabled   bool
+	SpecifiedClient           *azappconfig.Client
+	DynamicClients            []*ConfigurationClientWrapper


DynamicClientWrappers

RichardChen820 · 2024-03-14T04:22:22Z

internal/loader/configuration_client_manager.go

+	GetClients() []*ConfigurationClientWrapper
+	RefreshClients()
+	UpdateClientBackoffStatus(endpoint string, successful bool)
+	ExecuteFailoverPolicy(ctx context.Context, filters []acpv1.Selector, getSettingsFunc GetSettingsFunc) ([]azappconfig.Setting, error)


I don't expect the clientManager has the method ExecuteFailoverPolicy(), the clientManger just take the responsibility for managing the clients.

Agree, will move it to configLoader.

RichardChen820 · 2024-03-26T04:45:48Z

internal/loader/configuration_client_manager.go

+
+func parseConnectionString(connectionString string, token string) (string, error) {
+	if connectionString == "" {
+		return "", NewArgumentError("connectionString", fmt.Errorf("connectionString cannot be empty"))


Should not return as ArugmentError in this method, should handle all the errors as runtime error.

RichardChen820 · 2024-03-26T05:00:57Z

internal/loader/configuration_client_manager.go

+
+func Jitter(duration time.Duration) time.Duration {
+	if JitterRatio < 0 || JitterRatio > 1 {
+		panic("jitterRatio must be between 0 and 1")


why need to use panic here?

RichardChen820 · 2024-03-26T05:11:54Z

internal/loader/configuration_client_manager.go

+		currentTime.After(manager.lastFallbackClientAttempt.Time.Add(MinimalClientRefreshInterval)) {
+		manager.lastFallbackClientAttempt = currentTime
+		url, _ := url.Parse(manager.endpoint)
+		_ = manager.DiscoverFallbackClients(url.Host)


Should not ignore the error here.

Do we need to log the error?

DiscoverFallbackClients may return error in line 188, we should not ignore this error

Logging warning is fine in line 174

RichardChen820 · 2024-03-26T06:08:52Z

internal/loader/configuration_client_manager.go

+			if _, ok := err.(*net.DNSError); ok {
+				break
+			} else {
+				return nil, err


Should not return nil here, must be an empty array

RichardChen820 · 2024-03-26T06:11:54Z

internal/loader/configuration_client_manager.go

+		_, altRecords, err := net.LookupSRV(currentAlt, TCP, originHost)
+		if err != nil {
+			// If the host does not have SRV records => no more replicas
+			if _, ok := err.(*net.DNSError); ok {


In what situation the error is not *net.DNSError ? We should differentiate the not existing error from other errors

DNSError has IsNotFound field, we should use that, when IsNotFound is true, we should not treat it as error

It may return net error or something else. When there's no more alt records it will return dns error

If the error is net.DNSError and IsNotFound is true, just break the loop. Throw for any other errors.

When there's no alt recoreds, IsNotFound filed in net.DNSError is false.

RichardChen820 · 2024-03-26T06:16:28Z

internal/loader/configuration_client_manager.go

+	results := make([]string, 0)
+
+	_, originRecords, err := net.LookupSRV(Origin, TCP, host)
+	if err != nil {


error handling should same as line 231

Do you mean check the error's type?

Yes , judge type net.DNSError and IsNotFound

RichardChen820 · 2024-03-26T06:37:48Z

internal/loader/configuration_client_manager.go

+		return results, err
+	}
+
+	originHost := strings.TrimSuffix(originRecords[0].Target, ".")


The originHost should also be added to the return results.

RichardChen820 · 2024-03-26T06:39:25Z

internal/loader/configuration_client_manager.go

+
+	newDynamicClients := make([]*ConfigurationClientWrapper, 0)
+	for _, host := range srvTargetHosts {
+		if isValidEndpoint(host, manager.validDomain) {


Should judge whether the host is already in the staticClients, should not add it to dynamicClients if it is already a static client

RichardChen820 · 2024-03-26T06:57:51Z

internal/loader/configuration_setting_loader.go

+
+	// Failed to execute failover policy
+	csl.ClientManager.RefreshClients()
+	return nil, fmt.Errorf("failed to execute failover policy: all clients failed")


all app configuration clients failed to get settings

RichardChen820 · 2024-03-26T07:31:32Z

api/v1/azureappconfigurationprovider_types.go

-	Endpoint                  *string                                  `json:"endpoint,omitempty"`
+	Endpoint *string `json:"endpoint,omitempty"`
+	// +kubebuilder:default=true
+	ReplicaDiscovery          bool                                     `json:"replicaDiscovery,omitempty"`


Rename to ReplicaDiscoveryEnabled to keep consistent with other provdivers

RichardChen820 · 2024-03-27T06:15:18Z

internal/loader/configuration_client_manager.go

+		}
+	}
+
+	originHost := strings.TrimSuffix(originRecords[0].Target, ".")


We can't assume the originRecords has at least one item in it. we should judge its length, if length is 0, return empty result

RichardChen820 · 2024-03-27T10:40:18Z

internal/loader/configuration_client_manager.go

+func QuerySrvTargetHost(host string) ([]string, error) {
+	results := make([]string, 0)
+
+	_, originRecords, err := net.LookupSRV(Origin, TCP, host)


We need to use this method LookupSRV(ctx context.Context, service, proto, name string) (string, []*SRV, error) passing the context, which means all the calling chain should add context in the parameter

RichardChen820

Reset to request changes

juniwang · 2024-03-28T02:54:47Z

api/v1/azureappconfigurationprovider_types.go

@@ -30,7 +30,9 @@ import (
 type AzureAppConfigurationProviderSpec struct {
 	// The endpoint url of AppConfiguration which should sync the configuration key-values from.
 	// +kubebuilder:validation:Format=uri
-	Endpoint                  *string                                  `json:"endpoint,omitempty"`
+	Endpoint *string `json:"endpoint,omitempty"`


nit: indentation

There's kubebuilder notation between lines which influences go fmt result and the indention here.

juniwang · 2024-03-28T02:56:51Z

config/crd/bases/azconfig.io_azureappconfigurationproviders.yaml

@@ -37,6 +37,9 @@ spec:
            description: AzureAppConfigurationProviderSpec defines the desired state
              of AzureAppConfigurationProvider
            properties:
+              ReplicaDiscoveryEnabled:


is this customer-facting? should be camel case

RichardChen820 · 2024-03-28T02:58:39Z

api/v1/azureappconfigurationprovider_types.go

-	Endpoint                  *string                                  `json:"endpoint,omitempty"`
+	Endpoint *string `json:"endpoint,omitempty"`
+	// +kubebuilder:default=true
+	ReplicaDiscoveryEnabled   bool                                     `json:"ReplicaDiscoveryEnabled,omitempty"`


json:"replicaDiscoveryEnabled,omitempty", it should be camel casing

juniwang · 2024-03-28T03:00:29Z

internal/loader/configuration_client_manager.go

+//go:generate mockgen -destination=mocks/mock_configuration_client_manager.go -package mocks . ClientManager
+
+type ConfigurationClientManager struct {
+	ReplicaDiscoveryEnabledEnabled bool


why two Enabled in name

juniwang · 2024-03-28T03:01:19Z

internal/loader/configuration_client_manager.go

+	ReplicaDiscoveryEnabledEnabled bool
+	StaticClientWrappers           []*ConfigurationClientWrapper
+	DynamicClientWrappers          []*ConfigurationClientWrapper
+	validDomain                    string


what's the guidance of naming? inconsistent here: some fields are camel case, others are pascal case.

The type starting with an uppercase letter are exported to other packages. Those starting with a lowercase letter can be used only inside the package.

juniwang · 2024-03-28T03:18:33Z

internal/loader/configuration_client_manager.go

+	srvTargetHosts, err := QuerySrvTargetHost(ctx, host)
+	if err != nil {
+		klog.Warningf("Fail to build fall back clients %s", err.Error())
+	}


why not return or throw in case of error? It continues to execute below codes when something wrong

It should return, updated.

RichardChen820 · 2024-03-28T06:27:06Z

internal/loader/configuration_client_manager.go

+	return connectionString[startIndex+len(parseToken) : endIndex], nil
+}
+
+func verfityEndpointFromConnectionString(endpoint string) error {


verfity -> verify

This method seems not being used.

* failover support * modify client manager * resolve conflicts * add net error to failoverable error * resolve comments * resove comments * fix nil pointer * resolve comments * add failover test * update getclients func * resolve comments * resolve comments * upgrage go version * resolve comments * resolve comments * fix typo * use context with timeout

juniwang reviewed Mar 11, 2024

View reviewed changes

internal/loader/configuration_client_manager.go Show resolved Hide resolved

RichardChen820 reviewed Mar 11, 2024

View reviewed changes

linglingye001 force-pushed the user/linglingye/replicaDiscovery branch from dd527b4 to 93775e6 Compare March 13, 2024 04:32

RichardChen820 reviewed Mar 14, 2024

View reviewed changes

linglingye001 added 6 commits March 15, 2024 10:42

failover support

98aa3a6

modify client manager

116202a

resolve conflicts

826cc13

add net error to failoverable error

394b8e7

resolve comments

96da2e9

resove comments

da91ced

linglingye001 force-pushed the user/linglingye/replicaDiscovery branch from 5b94884 to da91ced Compare March 15, 2024 05:42

linglingye001 added 3 commits March 18, 2024 16:30

fix nil pointer

ceeb562

resolve comments

54cf5c3

add failover test

db479ad

RichardChen820 reviewed Mar 26, 2024

View reviewed changes

linglingye001 and others added 3 commits March 26, 2024 16:30

resolve comments

96f9cdc

upgrage go version

edc2e4b

Merge branch 'release/v1' into user/linglingye/replicaDiscovery

c5f19dc

RichardChen820 approved these changes Mar 27, 2024

View reviewed changes

RichardChen820 requested changes Mar 28, 2024

View reviewed changes

juniwang reviewed Mar 28, 2024

View reviewed changes

RichardChen820 reviewed Mar 28, 2024

View reviewed changes

juniwang reviewed Mar 28, 2024

View reviewed changes

resolve comments

c39479f

juniwang reviewed Mar 28, 2024

View reviewed changes

resolve comments

3e27c67

RichardChen820 reviewed Mar 28, 2024

View reviewed changes

fix typo

57ee5b4

RichardChen820 approved these changes Mar 28, 2024

View reviewed changes

use context with timeout

c9e3021

linglingye001 merged commit e9f2050 into release/v1 Apr 1, 2024
2 checks passed

linglingye001 deleted the user/linglingye/replicaDiscovery branch April 1, 2024 03:18

Failover support #20

Failover support #20

Conversation

linglingye001 commented Mar 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

RichardChen820 Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Mar 14, 2024 •

edited

Loading

RichardChen820 Mar 14, 2024 •

edited

Loading

RichardChen820 Mar 14, 2024 •

edited

Loading

RichardChen820 Mar 26, 2024 •

edited

Loading

RichardChen820 Mar 26, 2024 •

edited

Loading

RichardChen820 Mar 26, 2024 •

edited

Loading

RichardChen820 Mar 26, 2024 •

edited

Loading

RichardChen820 left a comment •

edited

Loading

RichardChen820 Mar 28, 2024 •

edited

Loading