Support generating cert based TLS type secret #10

RichardChen820 · 2024-01-30T09:45:55Z

Support generating cert-based TLS type secret

internal/controller/appconfigurationprovider_controller.go

linglingye001 · 2024-02-02T08:39:24Z

internal/controller/appconfigurationprovider_controller.go

 	// Reset the resource version if the configmap or secret was unexpected deleted
 	if existingConfigMap.Name == "" {
 		reconciler.ProvidersReconcileState[req.NamespacedName].ConfigMapResourceVersion = nil
 	}
+
 	if provider.Spec.Secret == nil || existingSecret.Name == "" {


when spec.secret != nil, it seems impossible that existingSecret.Name == "" because of the line 145 . And what if there're multiple k8s secrets existing in cluster and some of them are deleted?

Good catch! It should be updated accordingly

internal/controller/appconfigurationprovider_controller.go

internal/loader/configuration_setting_loader.go

linglingye001 · 2024-02-02T13:00:58Z

internal/loader/configuration_setting_loader.go

+					var secretType corev1.SecretType = corev1.SecretTypeOpaque
+					var err error
+					if secretTypeTag, ok := setting.Tags[PreservedSecretTypeTag]; ok {
+						secretType, err = parseSecretType(secretTypeTag)


parseSecretType looks like only supporting tls type? Can user set opaque type through tag?

No, current I don't want to support it, if someone want an opaque secret, just don't set any tag

linglingye001 · 2024-02-02T13:10:48Z

internal/loader/configuration_setting_loader.go

+						if err != nil {
+							return nil, err
+						}
+					} else if csl.Spec.Secret == nil {


Does the else if statement mean Spec.Secret can be nil when secret type is tls? If so, it may miss credentials when create SecretReferenceResolver.

Good catch, updated

juniwang · 2024-02-06T08:16:36Z

api/v1/azureappconfigurationprovider_types.go

@@ -141,8 +141,8 @@ type ManagedIdentityReferenceParameters struct {
 	Key string `json:"key"`
 }

-// AzureKeyVaultReference defines the authentication type used to Azure KeyVault resolve KeyVaultReference
-type AzureKeyVaultReference struct {
+// SecretReference defines the authentication type used to Azure KeyVault resolve KeyVaultReference


The description can be updated to match the latest usage. It contains more supported properties than auth.

juniwang · 2024-02-06T09:11:57Z

internal/controller/appconfigurationprovider_controller.go

+			return true
+		}
+	}
+


Is it possible that the loop of updating secret in createOrUpdateSecrets breaks early? ending up with a state that len(reconciler.ProvidersReconcileState[namespacedName].ExistingSecretReferences) == 0 is false and ExistingSecretReferences[name].SecretResourceVersion == secret.ResourceVersion is true for all existing secrets, but the ir lengths don't match the length of settings.SecretSettings hence needs reconcile.

Updated. And moved this shouldReconcile() function to processor to make the detailed logic transparency to the controller.

juniwang · 2024-02-06T09:24:39Z

internal/loader/configuration_setting_loader.go

+
+	if parsedType, ok := secretTypeMap[secretType]; ok {
+		if parsedType != corev1.SecretTypeTLS {
+			return "", fmt.Errorf("secret type %q is not supported", secretType)


So we don't allow customers to specify secretType opaque via the preserved tag?

The default Secret being generated is opaque. There's no reasonable circumstance to specify opaque as a preserved tag.

linglingye001 · 2024-02-20T05:39:15Z

internal/loader/configuration_setting_loader.go

+			secret[k] = v
+		} else if secret[k].Type != v.Type {
+			return fmt.Errorf("secret type mismatch for key %q", k)
+


nit: extra empty line

linglingye001 · 2024-02-20T07:14:33Z

internal/controller/processor.go

 	RefreshOptions         *RefreshOptions
-	ResolveSecretReference loader.ResolveSecretReference
+	ResolveSecretReference loader.SecretReferenceResolver


ResolveSecretReference can be renamed to SecretReferenceResolver

linglingye001 · 2024-02-20T07:25:56Z

internal/controller/processor.go

+		return false
+	}
+
+	if len(processor.ReconciliationState.ExistingSecretReferences) == 0 ||


We can generate ConfigMap with empty data, Secret as well. When will len(processor.ReconciliationState.ExistingSecretReferences) == 0 and need to reconcile?

Consider this scenario:
In beginning, no secret-related setting being specified in the yaml. Then user updates the yaml to add Secret. In this case, len(processor.ReconciliationState.ExistingSecretReferences) == 0 and need to reconcile

If user updates the yaml, it can be returned early in line245. Does this scenario happen when user updates the yaml after seeing error log? Also there's a scenario when user specified secret section in yaml but no key vault references in store, which may also make len(ExistingSecretReferences)==0

Also there's a scenario when user specified secret section in yaml but no key vault references in store, which may also make len(ExistingSecretReferences)==0

No, as long as user specify the secret section, at least one secret will be generated even it's empty.

I see, thanks

If user updates the yaml, it can be returned early in line245.

Right, but len(processor.ReconciliationState.ExistingSecretReferences) == 0 is an unexpected scenario, once it happens means something was wrong(may be just because the reconciliation never succeeded)

linglingye001 · 2024-02-20T07:56:36Z

internal/loader/configuration_setting_loader.go

+					default:
+						err = fmt.Errorf("failed to get certificate, unknown content type '%s'", *resolvedSecret.ContentType)
+					}
+				} else {


if the else statement deals with the situation when key vault reference is not backing certificate but with tls tag?

I was looking into this part as well, no need to have the if..else.. block, just need to check the contentType and take relevant action.

* Support generating cert based TLS type secret * Resolve comments * Resolve comments * Rename to SecretReferenceResolver * Add more test cases

RichardChen820 requested a review from linglingye001 January 30, 2024 09:45

RichardChen820 force-pushed the user/junbchen/secretTypeSupport branch 2 times, most recently from f9f7474 to 8532b05 Compare January 30, 2024 12:34

RichardChen820 changed the base branch from main to release/v1.1 January 31, 2024 07:55

RichardChen820 changed the base branch from release/v1.1 to main January 31, 2024 07:56

RichardChen820 force-pushed the user/junbchen/secretTypeSupport branch 5 times, most recently from 4c001fb to 0915f11 Compare February 1, 2024 08:22

RichardChen820 marked this pull request as ready for review February 1, 2024 08:22

RichardChen820 force-pushed the user/junbchen/secretTypeSupport branch 4 times, most recently from ae025e4 to f8d590d Compare February 2, 2024 07:45

linglingye001 reviewed Feb 2, 2024

View reviewed changes

juniwang reviewed Feb 6, 2024

View reviewed changes

linglingye001 reviewed Feb 20, 2024

View reviewed changes

RichardChen820 force-pushed the user/junbchen/secretTypeSupport branch 3 times, most recently from 339d54c to 75317f8 Compare February 21, 2024 03:04

linglingye001 approved these changes Mar 1, 2024

View reviewed changes

RichardChen820 changed the base branch from main to release/v1 March 1, 2024 16:07

RichardChen820 changed the base branch from release/v1 to main March 1, 2024 16:08

RichardChen820 added 4 commits March 4, 2024 10:06

Support generating cert based TLS type secret

0e607a7

Resolve comments

3bb6a97

Resolve comments

20884f3

Rename to SecretReferenceResolver

f07faa3

Add more test cases

ef366bf

RichardChen820 force-pushed the user/junbchen/secretTypeSupport branch from a5b98c2 to ef366bf Compare March 4, 2024 02:08

RichardChen820 changed the base branch from main to release/v1 March 4, 2024 02:10

RichardChen820 merged commit b3bc3b1 into release/v1 Mar 4, 2024
4 checks passed

RichardChen820 deleted the user/junbchen/secretTypeSupport branch March 4, 2024 02:29

RichardChen820 added a commit that referenced this pull request Apr 17, 2024

Support generating cert based TLS type secret (#10)

eb259b2

* Support generating cert based TLS type secret * Resolve comments * Resolve comments * Rename to SecretReferenceResolver * Add more test cases

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support generating cert based TLS type secret #10

Support generating cert based TLS type secret #10

RichardChen820 commented Jan 30, 2024

linglingye001 Feb 2, 2024

RichardChen820 Feb 4, 2024

linglingye001 Feb 2, 2024

RichardChen820 Feb 2, 2024 •

edited

Loading

linglingye001 Feb 2, 2024

RichardChen820 Feb 19, 2024

juniwang Feb 6, 2024

RichardChen820 Feb 19, 2024

juniwang Feb 6, 2024 •

edited

Loading

RichardChen820 Feb 19, 2024

juniwang Feb 6, 2024

RichardChen820 Feb 19, 2024 •

edited

Loading

linglingye001 Feb 20, 2024

linglingye001 Feb 20, 2024

RichardChen820 Feb 20, 2024

linglingye001 Feb 20, 2024

RichardChen820 Feb 20, 2024

linglingye001 Feb 20, 2024 •

edited

Loading

RichardChen820 Feb 20, 2024 •

edited

Loading

linglingye001 Feb 20, 2024

RichardChen820 Feb 20, 2024 •

edited

Loading

linglingye001 Feb 20, 2024

RichardChen820 Feb 20, 2024

Support generating cert based TLS type secret #10

Support generating cert based TLS type secret #10

Conversation

RichardChen820 commented Jan 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Feb 2, 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

juniwang Feb 6, 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 Feb 19, 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

linglingye001 Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

RichardChen820 Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Feb 20, 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 Feb 2, 2024 •

edited

Loading

juniwang Feb 6, 2024 •

edited

Loading

RichardChen820 Feb 19, 2024 •

edited

Loading

linglingye001 Feb 20, 2024 •

edited

Loading

RichardChen820 Feb 20, 2024 •

edited

Loading

RichardChen820 Feb 20, 2024 •

edited

Loading