Adopt Kubernetes style TLS Secret by somtochiama · Pull Request #597 · fluxcd/notification-controller

somtochiama · 2023-08-09T12:45:05Z

Deprecate the usage of caFile for the .spec.certSecretRef secret in Provider. Both caFile and ca.crt keys are supported for Provider with the latter taking precedence.

Part of: fluxcd/flux2#4137

darkowlzz · 2023-08-16T15:46:28Z

Would be good to include a makefile that generates this and the other files in this directory.
Similar to https://github.com/fluxcd/source-controller/blob/aa370f284db70e09d8acb49d5bed4e42908b8cb3/internal/controller/testdata/certs/Makefile#L17-L20 .

makkes · 2023-08-16T15:51:35Z

 	if err := r.Get(ctx, providerName, provider); err != nil {
 		// log not found errors since they get filtered out
-		ctrl.LoggerFrom(ctx).Error(err, "failed to get provider %s", providerName.String())
+		ctrl.LoggerFrom(ctx).Error(err, fmt.Sprintf("failed to get provider '%s'", providerName.String()))


In favor of structured logging I suggest:

Suggested change

ctrl.LoggerFrom(ctx).Error(err, fmt.Sprintf("failed to get provider '%s'", providerName.String()))

ctrl.LoggerFrom(ctx).Error(err, "failed to get provider", "provider", providerName.String())

makkes · 2023-08-16T15:52:50Z

-			return fmt.Errorf("no caFile found in secret %s", provider.Spec.CertSecretRef.Name)
+			caFile, ok = secret.Data["caFile"]
+			if !ok {
+				return fmt.Errorf("no `ca.crt` key found in secret '%s'", provider.Spec.CertSecretRef.Name)


Suggested change

return fmt.Errorf("no `ca.crt` key found in secret '%s'", provider.Spec.CertSecretRef.Name)

return fmt.Errorf("no 'ca.crt' nor 'caFile' key found in secret", "secret", provider.Spec.CertSecretRef.Name)

I am not sure if we want to add the deprecated key caFile to the log so as not to encourage users to use it.

Yeah, had the same thought. Up to you.

makkes · 2023-08-16T15:53:23Z

+			if !ok {
+				return fmt.Errorf("no `ca.crt` key found in secret '%s'", provider.Spec.CertSecretRef.Name)
+			}
+			log.Info("warning: specifying CA cert via `caFile` is deprecated, please use `ca.crt` instead")


Suggested change

log.Info("warning: specifying CA cert via `caFile` is deprecated, please use `ca.crt` instead")

log.Info("warning: specifying CA cert via 'caFile' is deprecated, please use 'ca.crt' instead")

makkes · 2023-08-16T15:55:08Z

-					continue
+					caFile, ok = secret.Data["caFile"]
+					if !ok {
+						alertLogger.Error(nil, "no `ca.crt` key found in secret '%s'", provider.Spec.CertSecretRef.Name)


Suggested change

alertLogger.Error(nil, "no `ca.crt` key found in secret '%s'", provider.Spec.CertSecretRef.Name)

alertLogger.Error(nil, "no 'ca.crt' nor 'caFile' key found in secret", "secret", provider.Spec.CertSecretRef.Name)

aryan9600 · 2023-08-18T05:52:30Z


 To enable notification-controller to communicate with a provider API over HTTPS
-using a self-signed TLS certificate, set the `caFile` like so:
+using a self-signed TLS certificate, set the `ca.crt` like so:


we also need to add a deprecation notice for the caFile key like the one here: https://github.com/fluxcd/source-controller/blob/main/docs/spec/v1beta2/helmrepositories.md#secret-reference

my bad, skipped it somehow but the same goes for the API docs

darkowlzz · 2023-08-23T11:29:15Z

+**Note:** Support for the `caFile` key have been
+deprecated. If you have any Secrets using these keys,


Suggested change

**Note:** Support for the `caFile` key have been

deprecated. If you have any Secrets using these keys,

**Warning:** Support for the `caFile` key has been

deprecated. If you have any Secrets using this key,

Also, maybe mention the types of Secret that's allowed in the above section, not part of this warning.

aryan9600

lgtm!

Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>

stefanprodan

LGTM

Thanks @somtochiama 🏅

somtochiama requested a review from aryan9600 August 9, 2023 12:45

aryan9600 mentioned this pull request Aug 9, 2023

Support Kubernetes style TLS Secrets fluxcd/flux2#4137

Closed

4 tasks

somtochiama marked this pull request as ready for review August 14, 2023 13:51

darkowlzz mentioned this pull request Jun 15, 2023

Refactor events and introduce v1beta3 API for Alert and Provider #540

Merged

darkowlzz reviewed Aug 16, 2023

View reviewed changes

makkes reviewed Aug 16, 2023

View reviewed changes

aryan9600 requested changes Aug 18, 2023

View reviewed changes

somtochiama force-pushed the kube-tls-secret branch from 015ebc4 to 0ac4494 Compare August 23, 2023 10:36

somtochiama requested a review from makkes August 23, 2023 10:38

darkowlzz reviewed Aug 23, 2023

View reviewed changes

aryan9600 approved these changes Aug 23, 2023

View reviewed changes

use kubernetes tls style secret in provider

23e733b

Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>

somtochiama force-pushed the kube-tls-secret branch from 794d100 to 23e733b Compare August 23, 2023 13:06

stefanprodan added the area/alerting Alerting related issues and PRs label Aug 23, 2023

stefanprodan approved these changes Aug 23, 2023

View reviewed changes

stefanprodan merged commit c04ad84 into main Aug 23, 2023

stefanprodan deleted the kube-tls-secret branch August 23, 2023 13:38

	ctrl.LoggerFrom(ctx).Error(err, fmt.Sprintf("failed to get provider '%s'", providerName.String()))
	ctrl.LoggerFrom(ctx).Error(err, "failed to get provider", "provider", providerName.String())

	return fmt.Errorf("no `ca.crt` key found in secret '%s'", provider.Spec.CertSecretRef.Name)
	return fmt.Errorf("no 'ca.crt' nor 'caFile' key found in secret", "secret", provider.Spec.CertSecretRef.Name)

	log.Info("warning: specifying CA cert via `caFile` is deprecated, please use `ca.crt` instead")
	log.Info("warning: specifying CA cert via 'caFile' is deprecated, please use 'ca.crt' instead")

	alertLogger.Error(nil, "no `ca.crt` key found in secret '%s'", provider.Spec.CertSecretRef.Name)
	alertLogger.Error(nil, "no 'ca.crt' nor 'caFile' key found in secret", "secret", provider.Spec.CertSecretRef.Name)

		Note: Support for the `caFile` key have been
		deprecated. If you have any Secrets using these keys,

Conversation

somtochiama commented Aug 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

somtochiama Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aryan9600 left a comment

Choose a reason for hiding this comment

Uh oh!

stefanprodan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

somtochiama Aug 16, 2023 •

edited

Loading