Add Correlation Context header for extension #47

RichardChen820 · 2024-06-05T07:15:34Z

Add Correlation Context header for extension

RichardChen820 · 2024-06-06T09:53:05Z

internal/controller/appconfigurationprovider_controller.go

@@ -311,7 +311,6 @@ func (reconciler *AzureAppConfigurationProviderReconciler) verifyTargetObjectExi
 }

 func (reconciler *AzureAppConfigurationProviderReconciler) logAndSetFailStatus(
-	ctx context.Context,


Just remove the unused parameter here.

linglingye001 · 2024-06-13T06:32:40Z

internal/loader/configuration_setting_loader.go

+	header := http.Header{}
+	output := make([]string, 0)
+
+	if provider.Spec.Configuration.Refresh != nil {


Spec.Configuration can be nil

Refresh.Enabled can be false, need to check it when Configuration.Refresh section has been set.

linglingye001 · 2024-06-13T06:41:32Z

internal/loader/configuration_setting_loader.go

+		enabled := true
+		enabled, _ = strconv.ParseBool(value)
+
+		if enabled {


It can be simplified to if enabled, _ := strconv.ParseBool(value); enabled {

linglingye001 · 2024-06-13T06:46:58Z

deploy/templates/deployment.yaml

@@ -55,6 +55,15 @@ spec:
          value: "{{ .Values.workloadIdentity.enabled }}"
        - name: WORKLOAD_IDENTITY_DISABLE_GLOBAL_SERVICE_ACCOUNT
          value: "{{ .Values.workloadIdentity.disableGlobalServiceAccount }}"
+        {{- if and (hasKey .Values "global") 
+            (hasKey .Values.global "azure")


Not sure, will these values like global, azure, extension and name be added during the extension installation?

Yes， this is what will be passed by the extension

zhenlan · 2024-06-19T01:58:37Z

internal/loader/configuration_setting_loader.go

+
+		if provider.Spec.FeatureFlag.Refresh != nil &&
+			provider.Spec.FeatureFlag.Refresh.Enabled {
+			output = append(output, "RefreshesFeatureFlag")


Feature flag by nature should be refreshed. This is not useful to collect.

The purpose of RefreshesKeyVault is to understand how many stores use appconfig for automatic secret rotation.

zhenlan · 2024-06-19T02:02:03Z

internal/loader/configuration_setting_loader.go

@@ -743,3 +759,51 @@ func MergeSecret(secret map[string]corev1.Secret, newSecret map[string]corev1.Se

 	return nil
 }
+
+func createCorrelationContextHeader(provider acpv1.AzureAppConfigurationProvider, clientManager ClientManager) http.Header {


Can we also report RequestType, Host, and Filter?

https://github.com/Azure/AppConfiguration-DotnetProvider/blob/main/src/Microsoft.Extensions.Configuration.AzureAppConfiguration/Constants/RequestTracingConstants.cs#L23

@zhenlan Added the RequestType and Host. In terms of the Filter, it's not cheap to report it from k8s provider, not trivial in K8s provider, two main reasons:

We currently don't parse the feature flags in K8s provider, we just get them and put into configMap, if we need to report the filters, we need to parse the feature flag values to know what filters are set.

K8s provider manages all yaml resouces, we need to let the controller can remember/manage all filter usage information for each resource.

How do you think about it? We add the Filter for now, or add in the future on demand?

If we are going to do dedup, we may parse it already. cc: @linglingye001

We don't parse feature flag now, the implementation of feature flag deduplication is based on feature flag key.

I think Zhenlan's view is that we need to dedup by id/name, then drives us having to parse it. But I have different idea about how to dedup(composition) featureFlag in provider. I will reply your email thread

linglingye001 · 2024-06-19T09:31:12Z

Can we update README for Data collection? https://github.com/Azure/AppConfiguration-DotnetProvider?tab=readme-ov-file#data-collection

zhenlan · 2024-06-20T02:21:46Z

internal/loader/request_tracing.go

+
+	if provider.Spec.Configuration.Refresh != nil &&
+		provider.Spec.Configuration.Refresh.Enabled {
+		output = append(output, "RefreshesKeyValue")


Not needed. We don't have this in .NET either.

I'm wondering why there is RefreshesKeyVault in dotnet provider but not RefreshesKeyValue?

We know customers like dynamic config. We don't need to collect telemetry to know it.

I explained the reason for key vault refresh here #47 (comment).

zhenlan · 2024-06-20T02:23:39Z

internal/loader/request_tracing.go

+	}
+
+	if provider.Spec.FeatureFlag != nil {
+		output = append(output, "UsesFeatureFlag")


Not needed. We can tell from requests.

RichardChen820 · 2024-06-20T02:26:09Z

Can we update README for Data collection? https://github.com/Azure/AppConfiguration-DotnetProvider?tab=readme-ov-file#data-collection

Yes, I will update it another PR

juniwang · 2024-06-20T13:58:03Z

internal/controller/appconfigurationprovider_controller.go

@@ -224,6 +224,10 @@ func (reconciler *AzureAppConfigurationProviderReconciler) Reconcile(ctx context
 		retriever = reconciler.Retriever
 	}

+	ctx = context.WithValue(ctx, loader.RequestTracingKey, loader.RequestTracing{
+		IsStartUp: reconciler.ProvidersReconcileState[req.NamespacedName].ConfigMapResourceVersion != nil,


Is ConfigMapResourceVersion nil when starting up?

My bad.. updated

zhenlan · 2024-06-21T16:54:34Z

internal/loader/configuration_setting_loader.go

@@ -88,6 +91,7 @@ const (
 	CertTypePfx                           string = "application/x-pkcs12"
 	TlsKey                                string = "tls.key"
 	TlsCrt                                string = "tls.crt"
+	RequestTracingEnabled                 string = "REQUEST_TRACING_ENABLED"


How does a customer disable the tracing? What about the AKS extension customer?

They can set requestTracing.enabled=false while installling the provider. Same to the extension customer.

You mean in the yaml file?

@zhenlan I mean here https://github.com/Azure/AppConfiguration-KubernetesProvider/pull/47/files#r1652128093

This is the parameter a user pass to the helm install? If so, how can an extension user pass the parameter?

Extension essentially use helm to do the installation, it supports all the helm values. You can consider there are no difference

I understand. My question is how an extension user can customize the helm install command and pass the extra parameter.

LIke how replicaCount can be set:
https://learn.microsoft.com/en-us/azure/aks/azure-app-configuration-settings#configure-the-replica-count

User can customize using this command while install the extension.

az k8s-extension create --cluster-type managedClusters --cluster-name myAKSCluster --resource-group myResourceGroup --name appconfigurationkubernetesprovider --extension-type Microsoft.AppConfiguration --configuration-settings "requestTracing.enabled=false"

Okay, so it's a different command. I am curious how you plan to explain this to customers.

RichardChen820 · 2024-06-25T07:15:47Z

deploy/parameter/helm-values.yaml

@@ -15,6 +15,9 @@ imagePullSecrets: []
 nameOverride: "appconfig-provider"
 fullnameOverride: "az-appconfig-k8s-provider"

+requestTracing:


Set requestTracing.enabled at installation time

* Not running on windows node (#30) * Bump up version to 2.0.0-preview (#42) * Workload identity support namespaced service account (#39) * Workload identity support namespaced service account * feedback * Feedback * Upgrade package (#44) * fix vulnerability * fix go version * update * update * update * update * update * update controller runtime * upgrade azsecrets package * update ci * update patched version * update * revert version * upgrade packages * update ci * revert * deduplicate feature flags (#50) * Treat get settings from one client failure as warning (#48) * Add node affinity and toleration configuration (#46) * K8s provider conformance test plugin (#43) * k8s provider conformance test plugin * rename * remove docker hub dependency * replace curl with wget * add version file * Setup golangci lint action (#51) * setup golangci lint action * fix linting error * update ci * update * add lint in makefile * Add Correlation Context header for extension (#47) * Add Correlation Context header for extension * Add more context * Add Host and RequestType in correlation context * Remove the kv refresh * update extension test plugin conformance file (#52) * added timeout parameter in golintCI (#54) * Revise the error message for selector object verification (#56) * Bump up version to 2.0.0 (#58) * Add data collection section in readme (#57) * Require to opt-in for the global service account (#60) * Require to opt in the global service account * Rename * Fix vulnerability (#68) * fix vulnerability * specify go version in golang lint ci --------- Co-authored-by: Richard chen <[email protected]>

RichardChen820 force-pushed the user/junbchen/correlationContext branch 6 times, most recently from 2b34d8a to 6b98edc Compare June 6, 2024 09:51

RichardChen820 commented Jun 6, 2024

View reviewed changes

RichardChen820 force-pushed the user/junbchen/correlationContext branch from 6b98edc to 3c98201 Compare June 6, 2024 09:55

Add Correlation Context header for extension

8e812f7

RichardChen820 force-pushed the user/junbchen/correlationContext branch from 3c98201 to 8e812f7 Compare June 6, 2024 09:58

RichardChen820 requested a review from linglingye001 June 11, 2024 02:04

linglingye001 reviewed Jun 13, 2024

View reviewed changes

Add more context

5b4b626

RichardChen820 force-pushed the user/junbchen/correlationContext branch from 015056e to 5b4b626 Compare June 18, 2024 03:19

zhenlan reviewed Jun 19, 2024

View reviewed changes

zhenlan reviewed Jun 20, 2024

View reviewed changes

juniwang reviewed Jun 20, 2024

View reviewed changes

Add Host and RequestType in correlation context

e9dfdeb

RichardChen820 force-pushed the user/junbchen/correlationContext branch from 9c6c60d to e9dfdeb Compare June 21, 2024 05:26

Remove the kv refresh

645b9ff

zhenlan reviewed Jun 21, 2024

View reviewed changes

RichardChen820 commented Jun 25, 2024

View reviewed changes

juniwang approved these changes Jul 4, 2024

View reviewed changes

RichardChen820 merged commit 6875962 into release/v2/preview Jul 4, 2024
1 check passed

RichardChen820 deleted the user/junbchen/correlationContext branch July 4, 2024 03:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Correlation Context header for extension #47

Add Correlation Context header for extension #47

RichardChen820 commented Jun 5, 2024

RichardChen820 Jun 6, 2024

linglingye001 Jun 13, 2024

linglingye001 Jun 13, 2024

linglingye001 Jun 13, 2024

linglingye001 Jun 13, 2024

RichardChen820 Jun 19, 2024

zhenlan Jun 19, 2024

zhenlan Jun 19, 2024

RichardChen820 Jun 19, 2024

zhenlan Jun 20, 2024

linglingye001 Jun 20, 2024

RichardChen820 Jun 20, 2024 •

edited

Loading

linglingye001 commented Jun 19, 2024

zhenlan Jun 20, 2024

juniwang Jun 20, 2024

zhenlan Jun 20, 2024

zhenlan Jun 20, 2024

RichardChen820 commented Jun 20, 2024

juniwang Jun 20, 2024

RichardChen820 Jun 21, 2024

zhenlan Jun 21, 2024

RichardChen820 Jun 24, 2024 •

edited

Loading

zhenlan Jun 25, 2024

RichardChen820 Jun 25, 2024

zhenlan Jun 25, 2024

RichardChen820 Jun 25, 2024

zhenlan Jun 25, 2024

RichardChen820 Jun 25, 2024 •

edited

Loading

zhenlan Jun 26, 2024

RichardChen820 Jun 25, 2024

Add Correlation Context header for extension #47

Add Correlation Context header for extension #47

Conversation

RichardChen820 commented Jun 5, 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

RichardChen820 Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

linglingye001 commented Jun 19, 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

RichardChen820 commented Jun 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Jun 24, 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

RichardChen820 Jun 25, 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 Jun 20, 2024 •

edited

Loading

RichardChen820 Jun 24, 2024 •

edited

Loading

RichardChen820 Jun 25, 2024 •

edited

Loading