From e42117a9cfa37a4f257f970cd7273e88f858ac6e Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Wed, 21 Jun 2023 16:37:37 -0400 Subject: [PATCH 01/27] create monitor for certificate expirations --- .../cluster/certificateexpirationstatuses.go | 66 +++++++++ .../certificateexpirationstatuses_test.go | 139 ++++++++++++++++++ pkg/monitor/cluster/cluster.go | 1 + 3 files changed, 206 insertions(+) create mode 100644 pkg/monitor/cluster/certificateexpirationstatuses.go create mode 100644 pkg/monitor/cluster/certificateexpirationstatuses_test.go diff --git a/pkg/monitor/cluster/certificateexpirationstatuses.go b/pkg/monitor/cluster/certificateexpirationstatuses.go new file mode 100644 index 00000000000..63dd4b331a0 --- /dev/null +++ b/pkg/monitor/cluster/certificateexpirationstatuses.go @@ -0,0 +1,66 @@ +package cluster + +import ( + "context" + "crypto/x509" + "encoding/pem" + "time" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/Azure/ARO-RP/pkg/operator" + "github.com/Azure/ARO-RP/pkg/operator/controllers/genevalogging" + "github.com/Azure/ARO-RP/pkg/util/dns" +) + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error { + // report NotAfter dates for Geneva (always), Ingress, and API (on managed domain) certificates + var certs []x509.Certificate + + mdsdCert, err := mon.getCertificate(ctx, operator.SecretName, operator.Namespace, genevalogging.GenevaCertName) + if err != nil { + return err + } + certs = append(certs, *mdsdCert[0]) + + if dns.IsManagedDomain(mon.oc.Properties.ClusterProfile.Domain) { + infraID := mon.oc.Properties.InfraID + ingressCertificateName := infraID + "-ingress" + apiCertificateName := infraID + "-apiserver" + + ingressCertificate, err := mon.getCertificate(ctx, ingressCertificateName, operator.Namespace, corev1.TLSCertKey) + if err != nil { + return err + } + + apiCertificate, err := mon.getCertificate(ctx, apiCertificateName, operator.Namespace, corev1.TLSCertKey) + if err != nil { + return err + } + + certs = append(certs, *ingressCertificate[0], *apiCertificate[0]) + } + + for _, cert := range certs { + mon.emitGauge("certificate.expirationdate", 1, map[string]string{ + "subject": cert.Subject.CommonName, + "expirationDate": cert.NotAfter.UTC().Format(time.RFC3339), + }) + } + mon.emitGauge("managedCertificates.count", int64(len(certs)), nil) + return nil +} + +func (mon *Monitor) getCertificate(ctx context.Context, secretName, secretNamespace, secretKey string) ([]*x509.Certificate, error) { + secret, err := mon.cli.CoreV1().Secrets(secretNamespace).Get(ctx, secretName, metav1.GetOptions{}) + if err != nil { + return nil, err + } + + certBlock, _ := pem.Decode(secret.Data[secretKey]) + return x509.ParseCertificates(certBlock.Bytes) +} diff --git a/pkg/monitor/cluster/certificateexpirationstatuses_test.go b/pkg/monitor/cluster/certificateexpirationstatuses_test.go new file mode 100644 index 00000000000..1d0c4dd3cde --- /dev/null +++ b/pkg/monitor/cluster/certificateexpirationstatuses_test.go @@ -0,0 +1,139 @@ +package cluster + +import ( + "context" + "crypto/x509" + "encoding/pem" + "testing" + "time" + + "github.com/golang/mock/gomock" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/fake" + + "github.com/Azure/ARO-RP/pkg/api" + mock_metrics "github.com/Azure/ARO-RP/pkg/util/mocks/metrics" + utiltls "github.com/Azure/ARO-RP/pkg/util/tls" +) + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +func TestEmitCertificateExpirationStatuses(t *testing.T) { + expiration := time.Now().Add(time.Hour * 24 * 5) + expirationString := expiration.UTC().Format(time.RFC3339) + for _, tt := range []struct { + name string + isManaged bool + wantExpirations []map[string]string + }{ + { + name: "unmanaged domain", + isManaged: false, + wantExpirations: []map[string]string{ + { + "subject": "geneva.certificate", + "expirationDate": expirationString, + }, + }, + }, + { + name: "managed domain", + isManaged: true, + wantExpirations: []map[string]string{ + { + "subject": "geneva.certificate", + "expirationDate": expirationString, + }, + { + "subject": "contoso.aroapp.io", + "expirationDate": expirationString, + }, + { + "subject": "api.contoso.aroapp.io", + "expirationDate": expirationString, + }, + }, + }, + } { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + + var secrets []runtime.Object + _, genevaCert, err := utiltls.GenerateTestKeyAndCertificate("geneva.certificate", nil, nil, false, false, func(template *x509.Certificate) { + template.NotAfter = expiration + }) + secrets = append(secrets, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + Namespace: "openshift-azure-operator", + }, + Data: map[string][]byte{ + "gcscert.pem": pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: genevaCert[0].Raw, + }), + }, + }) + + var domain string + if tt.isManaged { + domain = "contoso.aroapp.io" + for _, sec := range []struct{ secretName, certSubject string }{ + { + "foo12-ingress", + domain, + }, + { + "foo12-apiserver", + "api." + domain, + }, + } { + _, cert, _ := utiltls.GenerateTestKeyAndCertificate(sec.certSubject, nil, nil, false, false, func(template *x509.Certificate) { + template.NotAfter = expiration + }) + secrets = append(secrets, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: sec.secretName, + Namespace: "openshift-azure-operator", + }, + Data: map[string][]byte{ + "tls.crt": pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: cert[0].Raw, + }), + }, + }) + } + } else { + domain = "aro.contoso.com" + } + + m := mock_metrics.NewMockEmitter(gomock.NewController(t)) + for _, gauge := range tt.wantExpirations { + m.EXPECT().EmitGauge("certificate.expirationdate", int64(1), gauge) + } + m.EXPECT().EmitGauge("managedCertificates.count", int64(len(tt.wantExpirations)), map[string]string{}) + + mon := &Monitor{ + cli: fake.NewSimpleClientset(secrets...), + m: m, + oc: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ClusterProfile: api.ClusterProfile{ + Domain: domain, + }, + InfraID: "foo12", + }, + }, + } + + err = mon.emitCertificateExpirationStatuses(ctx) + if err != nil { + t.Errorf("got error %v", err) + } + }) + } +} diff --git a/pkg/monitor/cluster/cluster.go b/pkg/monitor/cluster/cluster.go index 209a550ead9..b0aae7a8178 100644 --- a/pkg/monitor/cluster/cluster.go +++ b/pkg/monitor/cluster/cluster.go @@ -175,6 +175,7 @@ func (mon *Monitor) Monitor(ctx context.Context) (errs []error) { mon.emitHiveRegistrationStatus, mon.emitOperatorFlagsAndSupportBanner, mon.emitPucmState, + mon.emitCertificateExpirationStatuses, mon.emitPrometheusAlerts, // at the end for now because it's the slowest/least reliable } { err = f(ctx) From 3add7e78a3d5fb3d41108c14c1c7acd6130a3d21 Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Wed, 5 Jul 2023 12:08:08 -0400 Subject: [PATCH 02/27] check for err in test setup --- pkg/monitor/cluster/certificateexpirationstatuses_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses_test.go b/pkg/monitor/cluster/certificateexpirationstatuses_test.go index 1d0c4dd3cde..1e09a9c1b09 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses_test.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses_test.go @@ -65,6 +65,9 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { _, genevaCert, err := utiltls.GenerateTestKeyAndCertificate("geneva.certificate", nil, nil, false, false, func(template *x509.Certificate) { template.NotAfter = expiration }) + if err != nil { + t.Fatal(err) + } secrets = append(secrets, &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "cluster", From 1554e6598347e5df2494eab07b5944fb0b448d86 Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Wed, 5 Jul 2023 13:18:41 -0400 Subject: [PATCH 03/27] only get first certificate in chain --- .../cluster/certificateexpirationstatuses.go | 34 +++++++++---------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses.go b/pkg/monitor/cluster/certificateexpirationstatuses.go index 63dd4b331a0..ee3f388c922 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses.go @@ -4,6 +4,7 @@ import ( "context" "crypto/x509" "encoding/pem" + "fmt" "time" corev1 "k8s.io/api/core/v1" @@ -19,30 +20,23 @@ import ( func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error { // report NotAfter dates for Geneva (always), Ingress, and API (on managed domain) certificates - var certs []x509.Certificate + var certs []*x509.Certificate mdsdCert, err := mon.getCertificate(ctx, operator.SecretName, operator.Namespace, genevalogging.GenevaCertName) if err != nil { return err } - certs = append(certs, *mdsdCert[0]) + certs = append(certs, mdsdCert) if dns.IsManagedDomain(mon.oc.Properties.ClusterProfile.Domain) { infraID := mon.oc.Properties.InfraID - ingressCertificateName := infraID + "-ingress" - apiCertificateName := infraID + "-apiserver" - - ingressCertificate, err := mon.getCertificate(ctx, ingressCertificateName, operator.Namespace, corev1.TLSCertKey) - if err != nil { - return err - } - - apiCertificate, err := mon.getCertificate(ctx, apiCertificateName, operator.Namespace, corev1.TLSCertKey) - if err != nil { - return err + for _, secretName := range []string{infraID + "-ingress", infraID + "-apiserver"} { + certificate, err := mon.getCertificate(ctx, secretName, operator.Namespace, corev1.TLSCertKey) + if err != nil { + return err + } + certs = append(certs, certificate) } - - certs = append(certs, *ingressCertificate[0], *apiCertificate[0]) } for _, cert := range certs { @@ -55,12 +49,16 @@ func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error return nil } -func (mon *Monitor) getCertificate(ctx context.Context, secretName, secretNamespace, secretKey string) ([]*x509.Certificate, error) { +func (mon *Monitor) getCertificate(ctx context.Context, secretName, secretNamespace, secretKey string) (*x509.Certificate, error) { secret, err := mon.cli.CoreV1().Secrets(secretNamespace).Get(ctx, secretName, metav1.GetOptions{}) if err != nil { - return nil, err + return &x509.Certificate{}, err } certBlock, _ := pem.Decode(secret.Data[secretKey]) - return x509.ParseCertificates(certBlock.Bytes) + if certBlock == nil { + return &x509.Certificate{}, fmt.Errorf("certificate data for %s not found", secretName) + } + // we only care about the first certificate in the block + return x509.ParseCertificate(certBlock.Bytes) } From 01c2d9557b2e79f0b5ecbe6f081e3fbe43b5dc3c Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Wed, 5 Jul 2023 14:17:42 -0400 Subject: [PATCH 04/27] remove monitor for certificate number --- pkg/monitor/cluster/certificateexpirationstatuses.go | 1 - pkg/monitor/cluster/certificateexpirationstatuses_test.go | 1 - 2 files changed, 2 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses.go b/pkg/monitor/cluster/certificateexpirationstatuses.go index ee3f388c922..ea1d41bfad7 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses.go @@ -45,7 +45,6 @@ func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error "expirationDate": cert.NotAfter.UTC().Format(time.RFC3339), }) } - mon.emitGauge("managedCertificates.count", int64(len(certs)), nil) return nil } diff --git a/pkg/monitor/cluster/certificateexpirationstatuses_test.go b/pkg/monitor/cluster/certificateexpirationstatuses_test.go index 1e09a9c1b09..04c7a6b8ba5 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses_test.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses_test.go @@ -118,7 +118,6 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { for _, gauge := range tt.wantExpirations { m.EXPECT().EmitGauge("certificate.expirationdate", int64(1), gauge) } - m.EXPECT().EmitGauge("managedCertificates.count", int64(len(tt.wantExpirations)), map[string]string{}) mon := &Monitor{ cli: fake.NewSimpleClientset(secrets...), From 38492d22b96943a18763fd97445ec5ee14f21f1b Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Mon, 10 Jul 2023 12:40:43 -0400 Subject: [PATCH 05/27] refactor tests for readability --- .../certificateexpirationstatuses_test.go | 89 +++++++++++-------- 1 file changed, 51 insertions(+), 38 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses_test.go b/pkg/monitor/cluster/certificateexpirationstatuses_test.go index 04c7a6b8ba5..cfdbaad3843 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses_test.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses_test.go @@ -20,6 +20,14 @@ import ( // Copyright (c) Microsoft Corporation. // Licensed under the Apache License 2.0. +type certInfo struct { + secretName, certSubject string +} + +const ( + managedDomainName = "contoso.aroapp.io" + unmanagedDomainName = "aro.contoso.com" +) func TestEmitCertificateExpirationStatuses(t *testing.T) { expiration := time.Now().Add(time.Hour * 24 * 5) @@ -27,10 +35,11 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { for _, tt := range []struct { name string isManaged bool + certsPresent []certInfo wantExpirations []map[string]string }{ { - name: "unmanaged domain", + name: "only emits MDSD status for unmanaged domain", isManaged: false, wantExpirations: []map[string]string{ { @@ -40,8 +49,12 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { }, }, { - name: "managed domain", + name: "includes ingress and API status for managed domain", isManaged: true, + certsPresent: []certInfo{ + {"foo12-ingress", managedDomainName}, + {"foo12-apiserver", "api."+managedDomainName}, + }, wantExpirations: []map[string]string{ { "subject": "geneva.certificate", @@ -62,13 +75,11 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { ctx := context.Background() var secrets []runtime.Object - _, genevaCert, err := utiltls.GenerateTestKeyAndCertificate("geneva.certificate", nil, nil, false, false, func(template *x509.Certificate) { - template.NotAfter = expiration - }) + _, genevaCert, err := utiltls.GenerateTestKeyAndCertificate("geneva.certificate", nil, nil, false, false, tweakTemplateFn(expiration)) if err != nil { t.Fatal(err) } - secrets = append(secrets, &corev1.Secret{ + s := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "cluster", Namespace: "openshift-azure-operator", @@ -79,40 +90,15 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { Bytes: genevaCert[0].Raw, }), }, - }) + } + secrets = append(secrets, s) + secretsFromCertInfo := getSecretsFromCertsInfo(tt.certsPresent, tweakTemplateFn(expiration)) + secrets = append(secrets, secretsFromCertInfo...) - var domain string + domain := unmanagedDomainName if tt.isManaged { - domain = "contoso.aroapp.io" - for _, sec := range []struct{ secretName, certSubject string }{ - { - "foo12-ingress", - domain, - }, - { - "foo12-apiserver", - "api." + domain, - }, - } { - _, cert, _ := utiltls.GenerateTestKeyAndCertificate(sec.certSubject, nil, nil, false, false, func(template *x509.Certificate) { - template.NotAfter = expiration - }) - secrets = append(secrets, &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: sec.secretName, - Namespace: "openshift-azure-operator", - }, - Data: map[string][]byte{ - "tls.crt": pem.EncodeToMemory(&pem.Block{ - Type: "CERTIFICATE", - Bytes: cert[0].Raw, - }), - }, - }) - } - } else { - domain = "aro.contoso.com" - } + domain = managedDomainName + } m := mock_metrics.NewMockEmitter(gomock.NewController(t)) for _, gauge := range tt.wantExpirations { @@ -139,3 +125,30 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { }) } } + +func tweakTemplateFn(expiration time.Time) func(*x509.Certificate) { + return func(template *x509.Certificate) { + template.NotAfter = expiration + } +} + +func getSecretsFromCertsInfo(certsInfo []certInfo, tweakTemplateFn func(*x509.Certificate)) []runtime.Object { + var secrets []runtime.Object + for _, sec := range certsInfo { + _, cert, _ := utiltls.GenerateTestKeyAndCertificate(sec.certSubject, nil, nil, false, false, tweakTemplateFn) + s := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: sec.secretName, + Namespace: "openshift-azure-operator", + }, + Data: map[string][]byte{ + "tls.crt": pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: cert[0].Raw, + }), + }, + } + secrets = append(secrets, s) + } + return secrets +} From fa71f5cbaaa40ad8a957601665979c99f9fb9584 Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Tue, 11 Jul 2023 11:25:11 -0400 Subject: [PATCH 06/27] add tests for error states --- .../cluster/certificateexpirationstatuses.go | 2 +- .../certificateexpirationstatuses_test.go | 140 +++++++++++++++--- 2 files changed, 121 insertions(+), 21 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses.go b/pkg/monitor/cluster/certificateexpirationstatuses.go index ea1d41bfad7..4bc80cbf264 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses.go @@ -56,7 +56,7 @@ func (mon *Monitor) getCertificate(ctx context.Context, secretName, secretNamesp certBlock, _ := pem.Decode(secret.Data[secretKey]) if certBlock == nil { - return &x509.Certificate{}, fmt.Errorf("certificate data for %s not found", secretName) + return &x509.Certificate{}, fmt.Errorf(`certificate "%s" not found on secret "%s"`, secretKey, secretName) } // we only care about the first certificate in the block return x509.ParseCertificate(certBlock.Bytes) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses_test.go b/pkg/monitor/cluster/certificateexpirationstatuses_test.go index cfdbaad3843..b55526bef2a 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses_test.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses_test.go @@ -53,7 +53,7 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { isManaged: true, certsPresent: []certInfo{ {"foo12-ingress", managedDomainName}, - {"foo12-apiserver", "api."+managedDomainName}, + {"foo12-apiserver", "api." + managedDomainName}, }, wantExpirations: []map[string]string{ { @@ -98,7 +98,7 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { domain := unmanagedDomainName if tt.isManaged { domain = managedDomainName - } + } m := mock_metrics.NewMockEmitter(gomock.NewController(t)) for _, gauge := range tt.wantExpirations { @@ -124,6 +124,106 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { } }) } + t.Run("returns error when cluster secret has been deleted", func(t *testing.T) { + wantErr := `secrets "cluster" not found` + + ctx := context.Background() + m := mock_metrics.NewMockEmitter(gomock.NewController(t)) + mon := &Monitor{ + cli: fake.NewSimpleClientset(), + m: m, + oc: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ClusterProfile: api.ClusterProfile{ + Domain: managedDomainName, + }, + InfraID: "foo12", + }, + }, + } + + err := mon.emitCertificateExpirationStatuses(ctx) + if err.Error() != wantErr { + t.Errorf("expected error `%v` but got `%v`", wantErr, err) + } + }) + + t.Run("returns error when managed domain secret has been deleted", func(t *testing.T) { + wantErr := `secrets "foo12-ingress" not found` + + var secrets []runtime.Object + _, genevaCert, err := utiltls.GenerateTestKeyAndCertificate("geneva.certificate", nil, nil, false, false, tweakTemplateFn(expiration)) + if err != nil { + t.Fatal(err) + } + s := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + Namespace: "openshift-azure-operator", + }, + Data: map[string][]byte{ + "gcscert.pem": pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: genevaCert[0].Raw, + }), + }, + } + secrets = append(secrets, s) + + ctx := context.Background() + m := mock_metrics.NewMockEmitter(gomock.NewController(t)) + mon := &Monitor{ + cli: fake.NewSimpleClientset(secrets...), + m: m, + oc: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ClusterProfile: api.ClusterProfile{ + Domain: managedDomainName, + }, + InfraID: "foo12", + }, + }, + } + + err = mon.emitCertificateExpirationStatuses(ctx) + if err.Error() != wantErr { + t.Errorf("expected error `%v` but got `%v`", wantErr, err) + } + }) + + t.Run("returns error when secret is present but certificate data has been deleted", func(t *testing.T) { + wantErr := `certificate "gcscert.pem" not found on secret "cluster"` + + var secrets []runtime.Object + s := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster", + Namespace: "openshift-azure-operator", + }, + Data: map[string][]byte{}, + } + secrets = append(secrets, s) + + ctx := context.Background() + m := mock_metrics.NewMockEmitter(gomock.NewController(t)) + mon := &Monitor{ + cli: fake.NewSimpleClientset(secrets...), + m: m, + oc: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ClusterProfile: api.ClusterProfile{ + Domain: managedDomainName, + }, + InfraID: "foo12", + }, + }, + } + + err := mon.emitCertificateExpirationStatuses(ctx) + if err.Error() != wantErr { + t.Errorf("expected error `%v` but got `%v`", wantErr, err) + } + }) } func tweakTemplateFn(expiration time.Time) func(*x509.Certificate) { @@ -133,22 +233,22 @@ func tweakTemplateFn(expiration time.Time) func(*x509.Certificate) { } func getSecretsFromCertsInfo(certsInfo []certInfo, tweakTemplateFn func(*x509.Certificate)) []runtime.Object { - var secrets []runtime.Object - for _, sec := range certsInfo { - _, cert, _ := utiltls.GenerateTestKeyAndCertificate(sec.certSubject, nil, nil, false, false, tweakTemplateFn) - s := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: sec.secretName, - Namespace: "openshift-azure-operator", - }, - Data: map[string][]byte{ - "tls.crt": pem.EncodeToMemory(&pem.Block{ - Type: "CERTIFICATE", - Bytes: cert[0].Raw, - }), - }, - } - secrets = append(secrets, s) - } - return secrets + var secrets []runtime.Object + for _, sec := range certsInfo { + _, cert, _ := utiltls.GenerateTestKeyAndCertificate(sec.certSubject, nil, nil, false, false, tweakTemplateFn) + s := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: sec.secretName, + Namespace: "openshift-azure-operator", + }, + Data: map[string][]byte{ + "tls.crt": pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: cert[0].Raw, + }), + }, + } + secrets = append(secrets, s) + } + return secrets } From 353944bfb9f6ef00f4f2a15740af436996b6eca4 Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Tue, 11 Jul 2023 12:04:37 -0400 Subject: [PATCH 07/27] condense error tests into testing struct --- .../certificateexpirationstatuses_test.go | 124 +++++------------- 1 file changed, 36 insertions(+), 88 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses_test.go b/pkg/monitor/cluster/certificateexpirationstatuses_test.go index b55526bef2a..be60e5e3456 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses_test.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses_test.go @@ -37,10 +37,12 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { isManaged bool certsPresent []certInfo wantExpirations []map[string]string + wantErr string }{ { - name: "only emits MDSD status for unmanaged domain", - isManaged: false, + name: "only emits MDSD status for unmanaged domain", + isManaged: false, + certsPresent: []certInfo{{"cluster", "geneva.certificate"}}, wantExpirations: []map[string]string{ { "subject": "geneva.certificate", @@ -52,6 +54,7 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { name: "includes ingress and API status for managed domain", isManaged: true, certsPresent: []certInfo{ + {"cluster", "geneva.certificate"}, {"foo12-ingress", managedDomainName}, {"foo12-apiserver", "api." + managedDomainName}, }, @@ -70,29 +73,29 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { }, }, }, + { + name: "returns error when cluster secret has been deleted", + isManaged: false, + wantErr: `secrets "cluster" not found`, + }, + { + name: "returns error when managed domain secret has been deleted", + isManaged: true, + certsPresent: []certInfo{ + {"cluster", "geneva.certificate"}, + {"foo12-ingress", managedDomainName}, + }, + wantErr: `secrets "foo12-apiserver" not found`, + }, } { t.Run(tt.name, func(t *testing.T) { ctx := context.Background() var secrets []runtime.Object - _, genevaCert, err := utiltls.GenerateTestKeyAndCertificate("geneva.certificate", nil, nil, false, false, tweakTemplateFn(expiration)) + secretsFromCertInfo, err := generateTestSecrets(tt.certsPresent, tweakTemplateFn(expiration)) if err != nil { t.Fatal(err) } - s := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cluster", - Namespace: "openshift-azure-operator", - }, - Data: map[string][]byte{ - "gcscert.pem": pem.EncodeToMemory(&pem.Block{ - Type: "CERTIFICATE", - Bytes: genevaCert[0].Raw, - }), - }, - } - secrets = append(secrets, s) - secretsFromCertInfo := getSecretsFromCertsInfo(tt.certsPresent, tweakTemplateFn(expiration)) secrets = append(secrets, secretsFromCertInfo...) domain := unmanagedDomainName @@ -119,77 +122,15 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { } err = mon.emitCertificateExpirationStatuses(ctx) - if err != nil { + if tt.wantErr != "" { + if tt.wantErr != err.Error() { + t.Errorf("expected error `%v` but got `%v`", tt.wantErr, err) + } + } else if err != nil { t.Errorf("got error %v", err) } }) } - t.Run("returns error when cluster secret has been deleted", func(t *testing.T) { - wantErr := `secrets "cluster" not found` - - ctx := context.Background() - m := mock_metrics.NewMockEmitter(gomock.NewController(t)) - mon := &Monitor{ - cli: fake.NewSimpleClientset(), - m: m, - oc: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ClusterProfile: api.ClusterProfile{ - Domain: managedDomainName, - }, - InfraID: "foo12", - }, - }, - } - - err := mon.emitCertificateExpirationStatuses(ctx) - if err.Error() != wantErr { - t.Errorf("expected error `%v` but got `%v`", wantErr, err) - } - }) - - t.Run("returns error when managed domain secret has been deleted", func(t *testing.T) { - wantErr := `secrets "foo12-ingress" not found` - - var secrets []runtime.Object - _, genevaCert, err := utiltls.GenerateTestKeyAndCertificate("geneva.certificate", nil, nil, false, false, tweakTemplateFn(expiration)) - if err != nil { - t.Fatal(err) - } - s := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cluster", - Namespace: "openshift-azure-operator", - }, - Data: map[string][]byte{ - "gcscert.pem": pem.EncodeToMemory(&pem.Block{ - Type: "CERTIFICATE", - Bytes: genevaCert[0].Raw, - }), - }, - } - secrets = append(secrets, s) - - ctx := context.Background() - m := mock_metrics.NewMockEmitter(gomock.NewController(t)) - mon := &Monitor{ - cli: fake.NewSimpleClientset(secrets...), - m: m, - oc: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ClusterProfile: api.ClusterProfile{ - Domain: managedDomainName, - }, - InfraID: "foo12", - }, - }, - } - - err = mon.emitCertificateExpirationStatuses(ctx) - if err.Error() != wantErr { - t.Errorf("expected error `%v` but got `%v`", wantErr, err) - } - }) t.Run("returns error when secret is present but certificate data has been deleted", func(t *testing.T) { wantErr := `certificate "gcscert.pem" not found on secret "cluster"` @@ -232,17 +173,24 @@ func tweakTemplateFn(expiration time.Time) func(*x509.Certificate) { } } -func getSecretsFromCertsInfo(certsInfo []certInfo, tweakTemplateFn func(*x509.Certificate)) []runtime.Object { +func generateTestSecrets(certsInfo []certInfo, tweakTemplateFn func(*x509.Certificate)) ([]runtime.Object, error) { var secrets []runtime.Object for _, sec := range certsInfo { - _, cert, _ := utiltls.GenerateTestKeyAndCertificate(sec.certSubject, nil, nil, false, false, tweakTemplateFn) + _, cert, err := utiltls.GenerateTestKeyAndCertificate(sec.certSubject, nil, nil, false, false, tweakTemplateFn) + if err != nil { + return nil, err + } + certKey := "tls.crt" + if sec.secretName == "cluster" { + certKey = "gcscert.pem" + } s := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: sec.secretName, Namespace: "openshift-azure-operator", }, Data: map[string][]byte{ - "tls.crt": pem.EncodeToMemory(&pem.Block{ + certKey: pem.EncodeToMemory(&pem.Block{ Type: "CERTIFICATE", Bytes: cert[0].Raw, }), @@ -250,5 +198,5 @@ func getSecretsFromCertsInfo(certsInfo []certInfo, tweakTemplateFn func(*x509.Ce } secrets = append(secrets, s) } - return secrets + return secrets, nil } From 091ac733184fc76c19df1a4a5257ca2869690dce Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Wed, 12 Jul 2023 10:09:08 -0400 Subject: [PATCH 08/27] remove bool from test struct --- .../certificateexpirationstatuses_test.go | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses_test.go b/pkg/monitor/cluster/certificateexpirationstatuses_test.go index be60e5e3456..483f41f788f 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses_test.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses_test.go @@ -34,14 +34,14 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { expirationString := expiration.UTC().Format(time.RFC3339) for _, tt := range []struct { name string - isManaged bool + domain string certsPresent []certInfo wantExpirations []map[string]string wantErr string }{ { name: "only emits MDSD status for unmanaged domain", - isManaged: false, + domain: unmanagedDomainName, certsPresent: []certInfo{{"cluster", "geneva.certificate"}}, wantExpirations: []map[string]string{ { @@ -52,7 +52,7 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { }, { name: "includes ingress and API status for managed domain", - isManaged: true, + domain: managedDomainName, certsPresent: []certInfo{ {"cluster", "geneva.certificate"}, {"foo12-ingress", managedDomainName}, @@ -75,12 +75,12 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { }, { name: "returns error when cluster secret has been deleted", - isManaged: false, + domain: unmanagedDomainName, wantErr: `secrets "cluster" not found`, }, { name: "returns error when managed domain secret has been deleted", - isManaged: true, + domain: managedDomainName, certsPresent: []certInfo{ {"cluster", "geneva.certificate"}, {"foo12-ingress", managedDomainName}, @@ -98,11 +98,6 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { } secrets = append(secrets, secretsFromCertInfo...) - domain := unmanagedDomainName - if tt.isManaged { - domain = managedDomainName - } - m := mock_metrics.NewMockEmitter(gomock.NewController(t)) for _, gauge := range tt.wantExpirations { m.EXPECT().EmitGauge("certificate.expirationdate", int64(1), gauge) @@ -114,7 +109,7 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { oc: &api.OpenShiftCluster{ Properties: api.OpenShiftClusterProperties{ ClusterProfile: api.ClusterProfile{ - Domain: domain, + Domain: tt.domain, }, InfraID: "foo12", }, From e81af7f26178eb39555937b7e973af6b15fec49a Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Wed, 12 Jul 2023 12:51:12 -0400 Subject: [PATCH 09/27] use error test util --- .../cluster/certificateexpirationstatuses_test.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses_test.go b/pkg/monitor/cluster/certificateexpirationstatuses_test.go index 483f41f788f..55e01e99d02 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses_test.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses_test.go @@ -16,6 +16,7 @@ import ( "github.com/Azure/ARO-RP/pkg/api" mock_metrics "github.com/Azure/ARO-RP/pkg/util/mocks/metrics" utiltls "github.com/Azure/ARO-RP/pkg/util/tls" + utilerror "github.com/Azure/ARO-RP/test/util/error" ) // Copyright (c) Microsoft Corporation. @@ -117,13 +118,8 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { } err = mon.emitCertificateExpirationStatuses(ctx) - if tt.wantErr != "" { - if tt.wantErr != err.Error() { - t.Errorf("expected error `%v` but got `%v`", tt.wantErr, err) - } - } else if err != nil { - t.Errorf("got error %v", err) - } + + utilerror.AssertErrorMessage(t, err, tt.wantErr) }) } From 89feb4cd0a155e6ff2b835e3a752898f51e6f17e Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Wed, 12 Jul 2023 13:03:24 -0400 Subject: [PATCH 10/27] extract test bootstrapping --- .../certificateexpirationstatuses_test.go | 100 ++++++++---------- 1 file changed, 46 insertions(+), 54 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses_test.go b/pkg/monitor/cluster/certificateexpirationstatuses_test.go index 55e01e99d02..8f43b0c68d2 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses_test.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses_test.go @@ -35,14 +35,14 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { expirationString := expiration.UTC().Format(time.RFC3339) for _, tt := range []struct { name string - domain string + domain string certsPresent []certInfo wantExpirations []map[string]string wantErr string }{ { name: "only emits MDSD status for unmanaged domain", - domain: unmanagedDomainName, + domain: unmanagedDomainName, certsPresent: []certInfo{{"cluster", "geneva.certificate"}}, wantExpirations: []map[string]string{ { @@ -52,7 +52,7 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { }, }, { - name: "includes ingress and API status for managed domain", + name: "includes ingress and API status for managed domain", domain: managedDomainName, certsPresent: []certInfo{ {"cluster", "geneva.certificate"}, @@ -75,12 +75,12 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { }, }, { - name: "returns error when cluster secret has been deleted", - domain: unmanagedDomainName, - wantErr: `secrets "cluster" not found`, + name: "returns error when cluster secret has been deleted", + domain: unmanagedDomainName, + wantErr: `secrets "cluster" not found`, }, { - name: "returns error when managed domain secret has been deleted", + name: "returns error when managed domain secret has been deleted", domain: managedDomainName, certsPresent: []certInfo{ {"cluster", "geneva.certificate"}, @@ -104,18 +104,7 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { m.EXPECT().EmitGauge("certificate.expirationdate", int64(1), gauge) } - mon := &Monitor{ - cli: fake.NewSimpleClientset(secrets...), - m: m, - oc: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ClusterProfile: api.ClusterProfile{ - Domain: tt.domain, - }, - InfraID: "foo12", - }, - }, - } + mon := buildMonitor(m, tt.domain, secrets...) err = mon.emitCertificateExpirationStatuses(ctx) @@ -124,37 +113,18 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { } t.Run("returns error when secret is present but certificate data has been deleted", func(t *testing.T) { - wantErr := `certificate "gcscert.pem" not found on secret "cluster"` - var secrets []runtime.Object - s := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cluster", - Namespace: "openshift-azure-operator", - }, - Data: map[string][]byte{}, - } + data := map[string][]byte{} + s := buildSecret("cluster", data) secrets = append(secrets, s) ctx := context.Background() m := mock_metrics.NewMockEmitter(gomock.NewController(t)) - mon := &Monitor{ - cli: fake.NewSimpleClientset(secrets...), - m: m, - oc: &api.OpenShiftCluster{ - Properties: api.OpenShiftClusterProperties{ - ClusterProfile: api.ClusterProfile{ - Domain: managedDomainName, - }, - InfraID: "foo12", - }, - }, - } + mon := buildMonitor(m, managedDomainName, secrets...) + wantErr := `certificate "gcscert.pem" not found on secret "cluster"` err := mon.emitCertificateExpirationStatuses(ctx) - if err.Error() != wantErr { - t.Errorf("expected error `%v` but got `%v`", wantErr, err) - } + utilerror.AssertErrorMessage(t, err, wantErr) }) } @@ -175,19 +145,41 @@ func generateTestSecrets(certsInfo []certInfo, tweakTemplateFn func(*x509.Certif if sec.secretName == "cluster" { certKey = "gcscert.pem" } - s := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: sec.secretName, - Namespace: "openshift-azure-operator", - }, - Data: map[string][]byte{ - certKey: pem.EncodeToMemory(&pem.Block{ - Type: "CERTIFICATE", - Bytes: cert[0].Raw, - }), - }, + data := map[string][]byte{ + certKey: pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: cert[0].Raw, + }), } + s := buildSecret(sec.secretName, data) secrets = append(secrets, s) } return secrets, nil } + +func buildSecret(secretName string, data map[string][]byte) *corev1.Secret { + s := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: "openshift-azure-operator", + }, + Data: data, + } + return s +} + +func buildMonitor(m *mock_metrics.MockEmitter, domain string, secrets ...runtime.Object) *Monitor { + mon := &Monitor{ + cli: fake.NewSimpleClientset(secrets...), + m: m, + oc: &api.OpenShiftCluster{ + Properties: api.OpenShiftClusterProperties{ + ClusterProfile: api.ClusterProfile{ + Domain: domain, + }, + InfraID: "foo12", + }, + }, + } + return mon +} From 5d47c655baa23be075cdc4f22c0eb4f937b87129 Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Wed, 26 Jul 2023 09:08:07 -0400 Subject: [PATCH 11/27] more graceful error handling --- .../cluster/certificateexpirationstatuses.go | 25 ++++++++++--- .../certificateexpirationstatuses_test.go | 36 +++++++++++++++---- 2 files changed, 49 insertions(+), 12 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses.go b/pkg/monitor/cluster/certificateexpirationstatuses.go index 4bc80cbf264..4ffdaad388d 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses.go @@ -8,6 +8,7 @@ import ( "time" corev1 "k8s.io/api/core/v1" + kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/Azure/ARO-RP/pkg/operator" @@ -17,30 +18,44 @@ import ( // Copyright (c) Microsoft Corporation. // Licensed under the Apache License 2.0. +const ( + certificateExpirationMetricName = "certificate.expirationdate" + secretMissingMetricName = "certificate.secretnotfound" +) func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error { // report NotAfter dates for Geneva (always), Ingress, and API (on managed domain) certificates var certs []*x509.Certificate mdsdCert, err := mon.getCertificate(ctx, operator.SecretName, operator.Namespace, genevalogging.GenevaCertName) - if err != nil { + if kerrors.IsNotFound(err) { + mon.emitGauge(secretMissingMetricName, int64(1), map[string]string{ + "secretMissing": operator.SecretName, + }) + } else if err != nil { return err + } else { + certs = append(certs, mdsdCert) } - certs = append(certs, mdsdCert) if dns.IsManagedDomain(mon.oc.Properties.ClusterProfile.Domain) { infraID := mon.oc.Properties.InfraID for _, secretName := range []string{infraID + "-ingress", infraID + "-apiserver"} { certificate, err := mon.getCertificate(ctx, secretName, operator.Namespace, corev1.TLSCertKey) - if err != nil { + if kerrors.IsNotFound(err) { + mon.emitGauge(secretMissingMetricName, int64(1), map[string]string{ + "secretMissing": secretName, + }) + } else if err != nil { return err + } else { + certs = append(certs, certificate) } - certs = append(certs, certificate) } } for _, cert := range certs { - mon.emitGauge("certificate.expirationdate", 1, map[string]string{ + mon.emitGauge(certificateExpirationMetricName, 1, map[string]string{ "subject": cert.Subject.CommonName, "expirationDate": cert.NotAfter.UTC().Format(time.RFC3339), }) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses_test.go b/pkg/monitor/cluster/certificateexpirationstatuses_test.go index 8f43b0c68d2..d11815e07b9 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses_test.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses_test.go @@ -38,6 +38,7 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { domain string certsPresent []certInfo wantExpirations []map[string]string + wantWarning []map[string]string wantErr string }{ { @@ -75,18 +76,36 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { }, }, { - name: "returns error when cluster secret has been deleted", - domain: unmanagedDomainName, - wantErr: `secrets "cluster" not found`, + name: "emits warning metric when cluster secret has been deleted", + domain: unmanagedDomainName, + wantWarning: []map[string]string{ + { + "secretMissing": "cluster", + }, + }, }, { - name: "returns error when managed domain secret has been deleted", + name: "emits warning metric when managed domain secret has been deleted", domain: managedDomainName, certsPresent: []certInfo{ {"cluster", "geneva.certificate"}, {"foo12-ingress", managedDomainName}, }, - wantErr: `secrets "foo12-apiserver" not found`, + wantExpirations: []map[string]string{ + { + "subject": "geneva.certificate", + "expirationDate": expirationString, + }, + { + "subject": "contoso.aroapp.io", + "expirationDate": expirationString, + }, + }, + wantWarning: []map[string]string{ + { + "secretMissing": "foo12-apiserver", + }, + }, }, } { t.Run(tt.name, func(t *testing.T) { @@ -100,8 +119,11 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { secrets = append(secrets, secretsFromCertInfo...) m := mock_metrics.NewMockEmitter(gomock.NewController(t)) - for _, gauge := range tt.wantExpirations { - m.EXPECT().EmitGauge("certificate.expirationdate", int64(1), gauge) + for _, w := range tt.wantWarning { + m.EXPECT().EmitGauge("certificate.secretnotfound", int64(1), w) + } + for _, g := range tt.wantExpirations { + m.EXPECT().EmitGauge(certificateExpirationMetricName, int64(1), g) } mon := buildMonitor(m, tt.domain, secrets...) From 6bbc532ade1f2eb3c514ec23da6dc1cfde72030f Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Wed, 26 Jul 2023 10:34:54 -0400 Subject: [PATCH 12/27] fix managed secret names --- .../cluster/certificateexpirationstatuses.go | 9 +++-- .../certificateexpirationstatuses_test.go | 34 ++++++++++++++----- pkg/monitor/cluster/cluster.go | 16 +++++---- 3 files changed, 41 insertions(+), 18 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses.go b/pkg/monitor/cluster/certificateexpirationstatuses.go index 4ffdaad388d..851ed96e359 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses.go @@ -5,6 +5,7 @@ import ( "crypto/x509" "encoding/pem" "fmt" + "strings" "time" corev1 "k8s.io/api/core/v1" @@ -39,8 +40,12 @@ func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error } if dns.IsManagedDomain(mon.oc.Properties.ClusterProfile.Domain) { - infraID := mon.oc.Properties.InfraID - for _, secretName := range []string{infraID + "-ingress", infraID + "-apiserver"} { + ingressController, err := mon.operatorcli.OperatorV1().IngressControllers("openshift-ingress-operator").Get(ctx, "default", metav1.GetOptions{}) + if err != nil { + return err + } + ingressSecretName := ingressController.Spec.DefaultCertificate.Name + for _, secretName := range []string{ingressSecretName, strings.Replace(ingressSecretName, "-ingress", "-apiserver", 1)} { certificate, err := mon.getCertificate(ctx, secretName, operator.Namespace, corev1.TLSCertKey) if kerrors.IsNotFound(err) { mon.emitGauge(secretMissingMetricName, int64(1), map[string]string{ diff --git a/pkg/monitor/cluster/certificateexpirationstatuses_test.go b/pkg/monitor/cluster/certificateexpirationstatuses_test.go index d11815e07b9..4db1a1c9529 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses_test.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses_test.go @@ -8,6 +8,8 @@ import ( "time" "github.com/golang/mock/gomock" + operatorv1 "github.com/openshift/api/operator/v1" + operatorfake "github.com/openshift/client-go/operator/clientset/versioned/fake" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -16,6 +18,7 @@ import ( "github.com/Azure/ARO-RP/pkg/api" mock_metrics "github.com/Azure/ARO-RP/pkg/util/mocks/metrics" utiltls "github.com/Azure/ARO-RP/pkg/util/tls" + "github.com/Azure/ARO-RP/pkg/util/uuid" utilerror "github.com/Azure/ARO-RP/test/util/error" ) @@ -33,6 +36,8 @@ const ( func TestEmitCertificateExpirationStatuses(t *testing.T) { expiration := time.Now().Add(time.Hour * 24 * 5) expirationString := expiration.UTC().Format(time.RFC3339) + clusterID := uuid.DefaultGenerator.Generate() + for _, tt := range []struct { name string domain string @@ -57,8 +62,8 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { domain: managedDomainName, certsPresent: []certInfo{ {"cluster", "geneva.certificate"}, - {"foo12-ingress", managedDomainName}, - {"foo12-apiserver", "api." + managedDomainName}, + {clusterID + "-ingress", managedDomainName}, + {clusterID + "-apiserver", "api." + managedDomainName}, }, wantExpirations: []map[string]string{ { @@ -89,7 +94,7 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { domain: managedDomainName, certsPresent: []certInfo{ {"cluster", "geneva.certificate"}, - {"foo12-ingress", managedDomainName}, + {clusterID + "-ingress", managedDomainName}, }, wantExpirations: []map[string]string{ { @@ -97,13 +102,13 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { "expirationDate": expirationString, }, { - "subject": "contoso.aroapp.io", + "subject": "contoso.aroapp.io", "expirationDate": expirationString, }, }, wantWarning: []map[string]string{ { - "secretMissing": "foo12-apiserver", + "secretMissing": clusterID + "-apiserver", }, }, }, @@ -126,7 +131,7 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { m.EXPECT().EmitGauge(certificateExpirationMetricName, int64(1), g) } - mon := buildMonitor(m, tt.domain, secrets...) + mon := buildMonitor(m, tt.domain, clusterID, secrets...) err = mon.emitCertificateExpirationStatuses(ctx) @@ -142,7 +147,7 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { ctx := context.Background() m := mock_metrics.NewMockEmitter(gomock.NewController(t)) - mon := buildMonitor(m, managedDomainName, secrets...) + mon := buildMonitor(m, managedDomainName, clusterID, secrets...) wantErr := `certificate "gcscert.pem" not found on secret "cluster"` err := mon.emitCertificateExpirationStatuses(ctx) @@ -190,7 +195,18 @@ func buildSecret(secretName string, data map[string][]byte) *corev1.Secret { return s } -func buildMonitor(m *mock_metrics.MockEmitter, domain string, secrets ...runtime.Object) *Monitor { +func buildMonitor(m *mock_metrics.MockEmitter, domain, id string, secrets ...runtime.Object) *Monitor { + ingressController := &operatorv1.IngressController{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Namespace: "openshift-ingress-operator", + }, + Spec: operatorv1.IngressControllerSpec{ + DefaultCertificate: &corev1.LocalObjectReference{ + Name: id + "-ingress", + }, + }, + } mon := &Monitor{ cli: fake.NewSimpleClientset(secrets...), m: m, @@ -199,9 +215,9 @@ func buildMonitor(m *mock_metrics.MockEmitter, domain string, secrets ...runtime ClusterProfile: api.ClusterProfile{ Domain: domain, }, - InfraID: "foo12", }, }, + operatorcli: operatorfake.NewSimpleClientset(ingressController), } return mon } diff --git a/pkg/monitor/cluster/cluster.go b/pkg/monitor/cluster/cluster.go index b0aae7a8178..9c05c873cce 100644 --- a/pkg/monitor/cluster/cluster.go +++ b/pkg/monitor/cluster/cluster.go @@ -11,6 +11,7 @@ import ( configv1 "github.com/openshift/api/config/v1" configclient "github.com/openshift/client-go/config/clientset/versioned" machineclient "github.com/openshift/client-go/machine/clientset/versioned" + operatorclient "github.com/openshift/client-go/operator/clientset/versioned" mcoclient "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned" "github.com/sirupsen/logrus" appsv1 "k8s.io/api/apps/v1" @@ -33,13 +34,14 @@ type Monitor struct { oc *api.OpenShiftCluster dims map[string]string - restconfig *rest.Config - cli kubernetes.Interface - configcli configclient.Interface - maocli machineclient.Interface - mcocli mcoclient.Interface - m metrics.Emitter - arocli aroclient.Interface + restconfig *rest.Config + cli kubernetes.Interface + configcli configclient.Interface + maocli machineclient.Interface + mcocli mcoclient.Interface + m metrics.Emitter + arocli aroclient.Interface + operatorcli operatorclient.Interface hiveclientset client.Client From 7d9f065fc5417d76fbbdb092f8d601fa323deec4 Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Wed, 26 Jul 2023 14:37:13 -0400 Subject: [PATCH 13/27] add explanation comment --- pkg/monitor/cluster/certificateexpirationstatuses.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses.go b/pkg/monitor/cluster/certificateexpirationstatuses.go index 851ed96e359..41f3a09d6e4 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses.go @@ -45,7 +45,7 @@ func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error return err } ingressSecretName := ingressController.Spec.DefaultCertificate.Name - for _, secretName := range []string{ingressSecretName, strings.Replace(ingressSecretName, "-ingress", "-apiserver", 1)} { + for _, secretName := range []string{ingressSecretName, strings.Replace(ingressSecretName, "-ingress", "-apiserver", 1)} { // certificate name is uuid + "-ingress" or "-apiserver" certificate, err := mon.getCertificate(ctx, secretName, operator.Namespace, corev1.TLSCertKey) if kerrors.IsNotFound(err) { mon.emitGauge(secretMissingMetricName, int64(1), map[string]string{ From f78b9f80d19dac319f6d77dce34daf9580071e30 Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Thu, 27 Jul 2023 06:38:11 -0600 Subject: [PATCH 14/27] Update pkg/monitor/cluster/certificateexpirationstatuses.go Co-authored-by: Ben Vesel <10840174+bennerv@users.noreply.github.com> --- pkg/monitor/cluster/certificateexpirationstatuses.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses.go b/pkg/monitor/cluster/certificateexpirationstatuses.go index 41f3a09d6e4..3d66585ee59 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses.go @@ -71,7 +71,7 @@ func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error func (mon *Monitor) getCertificate(ctx context.Context, secretName, secretNamespace, secretKey string) (*x509.Certificate, error) { secret, err := mon.cli.CoreV1().Secrets(secretNamespace).Get(ctx, secretName, metav1.GetOptions{}) if err != nil { - return &x509.Certificate{}, err + return nil, err } certBlock, _ := pem.Decode(secret.Data[secretKey]) From 8999e67d5cb2b07adab056138e5a5c29225534a6 Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Thu, 27 Jul 2023 06:38:32 -0600 Subject: [PATCH 15/27] Update pkg/monitor/cluster/certificateexpirationstatuses.go Co-authored-by: Ben Vesel <10840174+bennerv@users.noreply.github.com> --- pkg/monitor/cluster/certificateexpirationstatuses.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses.go b/pkg/monitor/cluster/certificateexpirationstatuses.go index 3d66585ee59..dbe5f532759 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses.go @@ -76,7 +76,7 @@ func (mon *Monitor) getCertificate(ctx context.Context, secretName, secretNamesp certBlock, _ := pem.Decode(secret.Data[secretKey]) if certBlock == nil { - return &x509.Certificate{}, fmt.Errorf(`certificate "%s" not found on secret "%s"`, secretKey, secretName) + return nil, fmt.Errorf(`certificate "%s" not found on secret "%s"`, secretKey, secretName) } // we only care about the first certificate in the block return x509.ParseCertificate(certBlock.Bytes) From 8a2548123b3cac300c55c192e7c8d1472811ba1a Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Thu, 27 Jul 2023 06:38:57 -0600 Subject: [PATCH 16/27] Update pkg/monitor/cluster/certificateexpirationstatuses.go Co-authored-by: Ben Vesel <10840174+bennerv@users.noreply.github.com> --- pkg/monitor/cluster/certificateexpirationstatuses.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses.go b/pkg/monitor/cluster/certificateexpirationstatuses.go index dbe5f532759..c40ef4b0983 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses.go @@ -29,12 +29,14 @@ func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error var certs []*x509.Certificate mdsdCert, err := mon.getCertificate(ctx, operator.SecretName, operator.Namespace, genevalogging.GenevaCertName) - if kerrors.IsNotFound(err) { - mon.emitGauge(secretMissingMetricName, int64(1), map[string]string{ - "secretMissing": operator.SecretName, - }) - } else if err != nil { + if err != nil { + if !kerrors.IsNotFound(err) { return err + } + mon.emitGauge(secretMissingMetricName, int64(1), map[string]string{ + "secretMissing": operator.SecretName, + }) + } } else { certs = append(certs, mdsdCert) } From 0b3882d493a46ec4c58909f0817aa08ed4f47bfc Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Thu, 27 Jul 2023 06:39:29 -0600 Subject: [PATCH 17/27] Update pkg/monitor/cluster/certificateexpirationstatuses.go Co-authored-by: Ben Vesel <10840174+bennerv@users.noreply.github.com> --- pkg/monitor/cluster/certificateexpirationstatuses.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses.go b/pkg/monitor/cluster/certificateexpirationstatuses.go index c40ef4b0983..c3be083e1b2 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses.go @@ -25,7 +25,7 @@ const ( ) func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error { - // report NotAfter dates for Geneva (always), Ingress, and API (on managed domain) certificates + // report NotAfter dates for Ingress and API (on managed domains), and Geneva (always) var certs []*x509.Certificate mdsdCert, err := mon.getCertificate(ctx, operator.SecretName, operator.Namespace, genevalogging.GenevaCertName) From 70c63751148f983d617722b87db7d69a5923e539 Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Thu, 27 Jul 2023 11:27:55 -0400 Subject: [PATCH 18/27] match k8s parameter order convention --- .../cluster/certificateexpirationstatuses.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses.go b/pkg/monitor/cluster/certificateexpirationstatuses.go index c3be083e1b2..78b6bbb7930 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses.go @@ -28,15 +28,14 @@ func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error // report NotAfter dates for Ingress and API (on managed domains), and Geneva (always) var certs []*x509.Certificate - mdsdCert, err := mon.getCertificate(ctx, operator.SecretName, operator.Namespace, genevalogging.GenevaCertName) + mdsdCert, err := mon.getCertificate(ctx, operator.Namespace, operator.SecretName, genevalogging.GenevaCertName) if err != nil { - if !kerrors.IsNotFound(err) { - return err - } - mon.emitGauge(secretMissingMetricName, int64(1), map[string]string{ + if !kerrors.IsNotFound(err) { + return err + } + mon.emitGauge(secretMissingMetricName, int64(1), map[string]string{ "secretMissing": operator.SecretName, - }) - } + }) } else { certs = append(certs, mdsdCert) } @@ -48,7 +47,7 @@ func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error } ingressSecretName := ingressController.Spec.DefaultCertificate.Name for _, secretName := range []string{ingressSecretName, strings.Replace(ingressSecretName, "-ingress", "-apiserver", 1)} { // certificate name is uuid + "-ingress" or "-apiserver" - certificate, err := mon.getCertificate(ctx, secretName, operator.Namespace, corev1.TLSCertKey) + certificate, err := mon.getCertificate(ctx, operator.Namespace, secretName, corev1.TLSCertKey) if kerrors.IsNotFound(err) { mon.emitGauge(secretMissingMetricName, int64(1), map[string]string{ "secretMissing": secretName, @@ -70,7 +69,7 @@ func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error return nil } -func (mon *Monitor) getCertificate(ctx context.Context, secretName, secretNamespace, secretKey string) (*x509.Certificate, error) { +func (mon *Monitor) getCertificate(ctx context.Context, secretNamespace, secretName, secretKey string) (*x509.Certificate, error) { secret, err := mon.cli.CoreV1().Secrets(secretNamespace).Get(ctx, secretName, metav1.GetOptions{}) if err != nil { return nil, err From 57efed9c171955cb5ba6c9ae189128a4b393d017 Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Thu, 27 Jul 2023 12:20:46 -0400 Subject: [PATCH 19/27] use generic client --- .../cluster/certificateexpirationstatuses.go | 21 +++++++++++---- .../certificateexpirationstatuses_test.go | 26 +++++++++++-------- pkg/monitor/cluster/cluster.go | 23 +++++++++------- 3 files changed, 45 insertions(+), 25 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses.go b/pkg/monitor/cluster/certificateexpirationstatuses.go index 78b6bbb7930..a675d7f5e28 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses.go @@ -8,9 +8,10 @@ import ( "strings" "time" + operatorv1 "github.com/openshift/api/operator/v1" corev1 "k8s.io/api/core/v1" kerrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" "github.com/Azure/ARO-RP/pkg/operator" "github.com/Azure/ARO-RP/pkg/operator/controllers/genevalogging" @@ -41,12 +42,18 @@ func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error } if dns.IsManagedDomain(mon.oc.Properties.ClusterProfile.Domain) { - ingressController, err := mon.operatorcli.OperatorV1().IngressControllers("openshift-ingress-operator").Get(ctx, "default", metav1.GetOptions{}) + ic := &operatorv1.IngressController{} + err := mon.clientset.Get(ctx, client.ObjectKey{ + Namespace: "openshift-ingress-operator", + Name: "default", + }, ic) if err != nil { return err } - ingressSecretName := ingressController.Spec.DefaultCertificate.Name - for _, secretName := range []string{ingressSecretName, strings.Replace(ingressSecretName, "-ingress", "-apiserver", 1)} { // certificate name is uuid + "-ingress" or "-apiserver" + ingressSecretName := ic.Spec.DefaultCertificate.Name + + // secret with managed certificates is uuid + "-ingress" or "-apiserver" + for _, secretName := range []string{ingressSecretName, strings.Replace(ingressSecretName, "-ingress", "-apiserver", 1)} { certificate, err := mon.getCertificate(ctx, operator.Namespace, secretName, corev1.TLSCertKey) if kerrors.IsNotFound(err) { mon.emitGauge(secretMissingMetricName, int64(1), map[string]string{ @@ -70,7 +77,11 @@ func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error } func (mon *Monitor) getCertificate(ctx context.Context, secretNamespace, secretName, secretKey string) (*x509.Certificate, error) { - secret, err := mon.cli.CoreV1().Secrets(secretNamespace).Get(ctx, secretName, metav1.GetOptions{}) + secret := &corev1.Secret{} + err := mon.clientset.Get(ctx, client.ObjectKey{ + Namespace: secretNamespace, + Name: secretName, + }, secret) if err != nil { return nil, err } diff --git a/pkg/monitor/cluster/certificateexpirationstatuses_test.go b/pkg/monitor/cluster/certificateexpirationstatuses_test.go index 4db1a1c9529..85a3384090c 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses_test.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses_test.go @@ -9,11 +9,10 @@ import ( "github.com/golang/mock/gomock" operatorv1 "github.com/openshift/api/operator/v1" - operatorfake "github.com/openshift/client-go/operator/clientset/versioned/fake" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/kubernetes/fake" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" "github.com/Azure/ARO-RP/pkg/api" mock_metrics "github.com/Azure/ARO-RP/pkg/util/mocks/metrics" @@ -116,7 +115,7 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { t.Run(tt.name, func(t *testing.T) { ctx := context.Background() - var secrets []runtime.Object + var secrets []client.Object secretsFromCertInfo, err := generateTestSecrets(tt.certsPresent, tweakTemplateFn(expiration)) if err != nil { t.Fatal(err) @@ -140,7 +139,7 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { } t.Run("returns error when secret is present but certificate data has been deleted", func(t *testing.T) { - var secrets []runtime.Object + var secrets []client.Object data := map[string][]byte{} s := buildSecret("cluster", data) secrets = append(secrets, s) @@ -161,8 +160,8 @@ func tweakTemplateFn(expiration time.Time) func(*x509.Certificate) { } } -func generateTestSecrets(certsInfo []certInfo, tweakTemplateFn func(*x509.Certificate)) ([]runtime.Object, error) { - var secrets []runtime.Object +func generateTestSecrets(certsInfo []certInfo, tweakTemplateFn func(*x509.Certificate)) ([]client.Object, error) { + var secrets []client.Object for _, sec := range certsInfo { _, cert, err := utiltls.GenerateTestKeyAndCertificate(sec.certSubject, nil, nil, false, false, tweakTemplateFn) if err != nil { @@ -195,7 +194,7 @@ func buildSecret(secretName string, data map[string][]byte) *corev1.Secret { return s } -func buildMonitor(m *mock_metrics.MockEmitter, domain, id string, secrets ...runtime.Object) *Monitor { +func buildMonitor(m *mock_metrics.MockEmitter, domain, id string, secrets ...client.Object) *Monitor { ingressController := &operatorv1.IngressController{ ObjectMeta: metav1.ObjectMeta{ Name: "default", @@ -207,9 +206,15 @@ func buildMonitor(m *mock_metrics.MockEmitter, domain, id string, secrets ...run }, }, } + + clientset := fake. + NewClientBuilder(). + WithObjects(ingressController). + WithObjects(secrets...). + Build() mon := &Monitor{ - cli: fake.NewSimpleClientset(secrets...), - m: m, + clientset: clientset, + m: m, oc: &api.OpenShiftCluster{ Properties: api.OpenShiftClusterProperties{ ClusterProfile: api.ClusterProfile{ @@ -217,7 +222,6 @@ func buildMonitor(m *mock_metrics.MockEmitter, domain, id string, secrets ...run }, }, }, - operatorcli: operatorfake.NewSimpleClientset(ingressController), } return mon } diff --git a/pkg/monitor/cluster/cluster.go b/pkg/monitor/cluster/cluster.go index 9c05c873cce..cfc7633952e 100644 --- a/pkg/monitor/cluster/cluster.go +++ b/pkg/monitor/cluster/cluster.go @@ -11,7 +11,6 @@ import ( configv1 "github.com/openshift/api/config/v1" configclient "github.com/openshift/client-go/config/clientset/versioned" machineclient "github.com/openshift/client-go/machine/clientset/versioned" - operatorclient "github.com/openshift/client-go/operator/clientset/versioned" mcoclient "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned" "github.com/sirupsen/logrus" appsv1 "k8s.io/api/apps/v1" @@ -34,15 +33,15 @@ type Monitor struct { oc *api.OpenShiftCluster dims map[string]string - restconfig *rest.Config - cli kubernetes.Interface - configcli configclient.Interface - maocli machineclient.Interface - mcocli mcoclient.Interface - m metrics.Emitter - arocli aroclient.Interface - operatorcli operatorclient.Interface + restconfig *rest.Config + cli kubernetes.Interface + configcli configclient.Interface + maocli machineclient.Interface + mcocli mcoclient.Interface + m metrics.Emitter + arocli aroclient.Interface + clientset client.Client hiveclientset client.Client // access below only via the helper functions in cache.go @@ -93,6 +92,11 @@ func NewMonitor(log *logrus.Entry, restConfig *rest.Config, oc *api.OpenShiftClu return nil, err } + clientset, err := client.New(restConfig, client.Options{}) + if err != nil { + return nil, err + } + hiveclientset, err := getHiveClientSet(hiveRestConfig) if err != nil { log.Error(err) @@ -112,6 +116,7 @@ func NewMonitor(log *logrus.Entry, restConfig *rest.Config, oc *api.OpenShiftClu mcocli: mcocli, arocli: arocli, m: m, + clientset: clientset, hiveclientset: hiveclientset, }, nil } From 78d7042c0149796f45b40bdf79f05b6847adde51 Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Thu, 27 Jul 2023 15:53:16 -0400 Subject: [PATCH 20/27] Revert "Update pkg/monitor/cluster/certificateexpirationstatuses.go" This reverts commit 8317ff63f95421b73d440ba9586ced7b776ad669. --- pkg/monitor/cluster/certificateexpirationstatuses.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses.go b/pkg/monitor/cluster/certificateexpirationstatuses.go index a675d7f5e28..cdb72cc95c1 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses.go @@ -30,13 +30,12 @@ func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error var certs []*x509.Certificate mdsdCert, err := mon.getCertificate(ctx, operator.Namespace, operator.SecretName, genevalogging.GenevaCertName) - if err != nil { - if !kerrors.IsNotFound(err) { - return err - } + if kerrors.IsNotFound(err) { mon.emitGauge(secretMissingMetricName, int64(1), map[string]string{ "secretMissing": operator.SecretName, }) + } else if err != nil { + return err } else { certs = append(certs, mdsdCert) } From c9694e8921096e08a78ec00bd4286a1cb0c3478c Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Fri, 28 Jul 2023 09:02:26 -0400 Subject: [PATCH 21/27] report better information for missing secrets --- .../cluster/certificateexpirationstatuses.go | 15 +++++++++------ .../cluster/certificateexpirationstatuses_test.go | 8 +++++--- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses.go b/pkg/monitor/cluster/certificateexpirationstatuses.go index cdb72cc95c1..b05a70a75fe 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses.go @@ -31,9 +31,7 @@ func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error mdsdCert, err := mon.getCertificate(ctx, operator.Namespace, operator.SecretName, genevalogging.GenevaCertName) if kerrors.IsNotFound(err) { - mon.emitGauge(secretMissingMetricName, int64(1), map[string]string{ - "secretMissing": operator.SecretName, - }) + mon.emitGauge(secretMissingMetricName, int64(1), secretMissingMetric(operator.Namespace, operator.SecretName)) } else if err != nil { return err } else { @@ -55,9 +53,7 @@ func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error for _, secretName := range []string{ingressSecretName, strings.Replace(ingressSecretName, "-ingress", "-apiserver", 1)} { certificate, err := mon.getCertificate(ctx, operator.Namespace, secretName, corev1.TLSCertKey) if kerrors.IsNotFound(err) { - mon.emitGauge(secretMissingMetricName, int64(1), map[string]string{ - "secretMissing": secretName, - }) + mon.emitGauge(secretMissingMetricName, int64(1), secretMissingMetric(operator.Namespace, secretName)) } else if err != nil { return err } else { @@ -92,3 +88,10 @@ func (mon *Monitor) getCertificate(ctx context.Context, secretNamespace, secretN // we only care about the first certificate in the block return x509.ParseCertificate(certBlock.Bytes) } + +func secretMissingMetric(namespace, name string) map[string]string { + return map[string]string{ + "namespace": namespace, + "name": name, + } +} diff --git a/pkg/monitor/cluster/certificateexpirationstatuses_test.go b/pkg/monitor/cluster/certificateexpirationstatuses_test.go index 85a3384090c..8ba2d87c410 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses_test.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses_test.go @@ -84,7 +84,8 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { domain: unmanagedDomainName, wantWarning: []map[string]string{ { - "secretMissing": "cluster", + "namespace": "openshift-azure-operator", + "name": "cluster", }, }, }, @@ -107,7 +108,8 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { }, wantWarning: []map[string]string{ { - "secretMissing": clusterID + "-apiserver", + "namespace": "openshift-azure-operator", + "name": clusterID + "-apiserver", }, }, }, @@ -124,7 +126,7 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { m := mock_metrics.NewMockEmitter(gomock.NewController(t)) for _, w := range tt.wantWarning { - m.EXPECT().EmitGauge("certificate.secretnotfound", int64(1), w) + m.EXPECT().EmitGauge(secretMissingMetricName, int64(1), w) } for _, g := range tt.wantExpirations { m.EXPECT().EmitGauge(certificateExpirationMetricName, int64(1), g) From bb255ec40097e2c9683d2af8d6a8f5f3361205bc Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Tue, 1 Aug 2023 11:15:36 -0400 Subject: [PATCH 22/27] include days until certificate expiration --- .../cluster/certificateexpirationstatuses.go | 7 +++-- .../certificateexpirationstatuses_test.go | 30 +++++++++++-------- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses.go b/pkg/monitor/cluster/certificateexpirationstatuses.go index b05a70a75fe..ae58c5f24db 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses.go @@ -5,6 +5,7 @@ import ( "crypto/x509" "encoding/pem" "fmt" + "math" "strings" "time" @@ -63,9 +64,11 @@ func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error } for _, cert := range certs { + daysUntilExpiration := int(math.Round(cert.NotAfter.UTC().Sub(time.Now().UTC()).Hours() / 24)) mon.emitGauge(certificateExpirationMetricName, 1, map[string]string{ - "subject": cert.Subject.CommonName, - "expirationDate": cert.NotAfter.UTC().Format(time.RFC3339), + "subject": cert.Subject.CommonName, + "expirationDate": cert.NotAfter.UTC().Format(time.RFC3339), + "daysUntilExpiration": fmt.Sprintf("%d", daysUntilExpiration), }) } return nil diff --git a/pkg/monitor/cluster/certificateexpirationstatuses_test.go b/pkg/monitor/cluster/certificateexpirationstatuses_test.go index 8ba2d87c410..c518fcf05e4 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses_test.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses_test.go @@ -51,8 +51,9 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { certsPresent: []certInfo{{"cluster", "geneva.certificate"}}, wantExpirations: []map[string]string{ { - "subject": "geneva.certificate", - "expirationDate": expirationString, + "subject": "geneva.certificate", + "expirationDate": expirationString, + "daysUntilExpiration": "5", }, }, }, @@ -66,16 +67,19 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { }, wantExpirations: []map[string]string{ { - "subject": "geneva.certificate", - "expirationDate": expirationString, + "subject": "geneva.certificate", + "expirationDate": expirationString, + "daysUntilExpiration": "5", }, { - "subject": "contoso.aroapp.io", - "expirationDate": expirationString, + "subject": "contoso.aroapp.io", + "expirationDate": expirationString, + "daysUntilExpiration": "5", }, { - "subject": "api.contoso.aroapp.io", - "expirationDate": expirationString, + "subject": "api.contoso.aroapp.io", + "expirationDate": expirationString, + "daysUntilExpiration": "5", }, }, }, @@ -98,12 +102,14 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { }, wantExpirations: []map[string]string{ { - "subject": "geneva.certificate", - "expirationDate": expirationString, + "subject": "geneva.certificate", + "expirationDate": expirationString, + "daysUntilExpiration": "5", }, { - "subject": "contoso.aroapp.io", - "expirationDate": expirationString, + "subject": "contoso.aroapp.io", + "expirationDate": expirationString, + "daysUntilExpiration": "5", }, }, wantWarning: []map[string]string{ From 93285b7468c470f202477103e6d9f3c87c3515c3 Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Wed, 30 Aug 2023 10:51:28 -0400 Subject: [PATCH 23/27] use built-in to calculate duration --- pkg/monitor/cluster/certificateexpirationstatuses.go | 3 +-- .../cluster/certificateexpirationstatuses_test.go | 12 ++++++------ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses.go b/pkg/monitor/cluster/certificateexpirationstatuses.go index ae58c5f24db..a3a5f6349f7 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses.go @@ -5,7 +5,6 @@ import ( "crypto/x509" "encoding/pem" "fmt" - "math" "strings" "time" @@ -64,7 +63,7 @@ func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error } for _, cert := range certs { - daysUntilExpiration := int(math.Round(cert.NotAfter.UTC().Sub(time.Now().UTC()).Hours() / 24)) + daysUntilExpiration := time.Until(cert.NotAfter) / (24 * time.Hour) mon.emitGauge(certificateExpirationMetricName, 1, map[string]string{ "subject": cert.Subject.CommonName, "expirationDate": cert.NotAfter.UTC().Format(time.RFC3339), diff --git a/pkg/monitor/cluster/certificateexpirationstatuses_test.go b/pkg/monitor/cluster/certificateexpirationstatuses_test.go index c518fcf05e4..0f0b300a784 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses_test.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses_test.go @@ -53,7 +53,7 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { { "subject": "geneva.certificate", "expirationDate": expirationString, - "daysUntilExpiration": "5", + "daysUntilExpiration": "4", }, }, }, @@ -69,17 +69,17 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { { "subject": "geneva.certificate", "expirationDate": expirationString, - "daysUntilExpiration": "5", + "daysUntilExpiration": "4", }, { "subject": "contoso.aroapp.io", "expirationDate": expirationString, - "daysUntilExpiration": "5", + "daysUntilExpiration": "4", }, { "subject": "api.contoso.aroapp.io", "expirationDate": expirationString, - "daysUntilExpiration": "5", + "daysUntilExpiration": "4", }, }, }, @@ -104,12 +104,12 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { { "subject": "geneva.certificate", "expirationDate": expirationString, - "daysUntilExpiration": "5", + "daysUntilExpiration": "4", }, { "subject": "contoso.aroapp.io", "expirationDate": expirationString, - "daysUntilExpiration": "5", + "daysUntilExpiration": "4", }, }, wantWarning: []map[string]string{ From a79f7fc9eaa98cb8a813b79144e9bd50ebe4265e Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Fri, 15 Sep 2023 11:11:23 -0400 Subject: [PATCH 24/27] use constants for ingress name and namespace --- pkg/monitor/cluster/certificateexpirationstatuses.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses.go b/pkg/monitor/cluster/certificateexpirationstatuses.go index a3a5f6349f7..163d4847684 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses.go @@ -23,6 +23,8 @@ import ( const ( certificateExpirationMetricName = "certificate.expirationdate" secretMissingMetricName = "certificate.secretnotfound" + ingressNamespace = "openshift-ingress-operator" + ingressName = "default" ) func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error { @@ -41,8 +43,8 @@ func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error if dns.IsManagedDomain(mon.oc.Properties.ClusterProfile.Domain) { ic := &operatorv1.IngressController{} err := mon.clientset.Get(ctx, client.ObjectKey{ - Namespace: "openshift-ingress-operator", - Name: "default", + Namespace: ingressNamespace, + Name: ingressName, }, ic) if err != nil { return err From e7bc84b999204f3575271138334feb2ba2ed2019 Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Fri, 15 Sep 2023 11:13:19 -0400 Subject: [PATCH 25/27] rename struct member --- pkg/monitor/cluster/certificateexpirationstatuses.go | 4 ++-- pkg/monitor/cluster/certificateexpirationstatuses_test.go | 6 +++--- pkg/monitor/cluster/cluster.go | 6 +++--- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses.go b/pkg/monitor/cluster/certificateexpirationstatuses.go index 163d4847684..057016aac0f 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses.go @@ -42,7 +42,7 @@ func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error if dns.IsManagedDomain(mon.oc.Properties.ClusterProfile.Domain) { ic := &operatorv1.IngressController{} - err := mon.clientset.Get(ctx, client.ObjectKey{ + err := mon.ocpclientset.Get(ctx, client.ObjectKey{ Namespace: ingressNamespace, Name: ingressName, }, ic) @@ -77,7 +77,7 @@ func (mon *Monitor) emitCertificateExpirationStatuses(ctx context.Context) error func (mon *Monitor) getCertificate(ctx context.Context, secretNamespace, secretName, secretKey string) (*x509.Certificate, error) { secret := &corev1.Secret{} - err := mon.clientset.Get(ctx, client.ObjectKey{ + err := mon.ocpclientset.Get(ctx, client.ObjectKey{ Namespace: secretNamespace, Name: secretName, }, secret) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses_test.go b/pkg/monitor/cluster/certificateexpirationstatuses_test.go index 0f0b300a784..9c6f9d11f44 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses_test.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses_test.go @@ -215,14 +215,14 @@ func buildMonitor(m *mock_metrics.MockEmitter, domain, id string, secrets ...cli }, } - clientset := fake. + ocpclientset := fake. NewClientBuilder(). WithObjects(ingressController). WithObjects(secrets...). Build() mon := &Monitor{ - clientset: clientset, - m: m, + ocpclientset: ocpclientset, + m: m, oc: &api.OpenShiftCluster{ Properties: api.OpenShiftClusterProperties{ ClusterProfile: api.ClusterProfile{ diff --git a/pkg/monitor/cluster/cluster.go b/pkg/monitor/cluster/cluster.go index cfc7633952e..89a320ebd9f 100644 --- a/pkg/monitor/cluster/cluster.go +++ b/pkg/monitor/cluster/cluster.go @@ -41,7 +41,7 @@ type Monitor struct { m metrics.Emitter arocli aroclient.Interface - clientset client.Client + ocpclientset client.Client hiveclientset client.Client // access below only via the helper functions in cache.go @@ -92,7 +92,7 @@ func NewMonitor(log *logrus.Entry, restConfig *rest.Config, oc *api.OpenShiftClu return nil, err } - clientset, err := client.New(restConfig, client.Options{}) + ocpclientset, err := client.New(restConfig, client.Options{}) if err != nil { return nil, err } @@ -116,7 +116,7 @@ func NewMonitor(log *logrus.Entry, restConfig *rest.Config, oc *api.OpenShiftClu mcocli: mcocli, arocli: arocli, m: m, - clientset: clientset, + ocpclientset: ocpclientset, hiveclientset: hiveclientset, }, nil } From 00e0728d3c4c0802cfda04208f4e55093681a48f Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Fri, 15 Sep 2023 11:31:54 -0400 Subject: [PATCH 26/27] use util package --- pkg/monitor/cluster/certificateexpirationstatuses.go | 9 ++------- .../cluster/certificateexpirationstatuses_test.go | 2 +- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses.go b/pkg/monitor/cluster/certificateexpirationstatuses.go index 057016aac0f..120710f835c 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses.go @@ -3,7 +3,6 @@ package cluster import ( "context" "crypto/x509" - "encoding/pem" "fmt" "strings" "time" @@ -16,6 +15,7 @@ import ( "github.com/Azure/ARO-RP/pkg/operator" "github.com/Azure/ARO-RP/pkg/operator/controllers/genevalogging" "github.com/Azure/ARO-RP/pkg/util/dns" + "github.com/Azure/ARO-RP/pkg/util/pem" ) // Copyright (c) Microsoft Corporation. @@ -85,12 +85,7 @@ func (mon *Monitor) getCertificate(ctx context.Context, secretNamespace, secretN return nil, err } - certBlock, _ := pem.Decode(secret.Data[secretKey]) - if certBlock == nil { - return nil, fmt.Errorf(`certificate "%s" not found on secret "%s"`, secretKey, secretName) - } - // we only care about the first certificate in the block - return x509.ParseCertificate(certBlock.Bytes) + return pem.ParseFirstCertificate(secret.Data[secretKey]) } func secretMissingMetric(namespace, name string) map[string]string { diff --git a/pkg/monitor/cluster/certificateexpirationstatuses_test.go b/pkg/monitor/cluster/certificateexpirationstatuses_test.go index 9c6f9d11f44..4b20aaa2433 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses_test.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses_test.go @@ -156,7 +156,7 @@ func TestEmitCertificateExpirationStatuses(t *testing.T) { m := mock_metrics.NewMockEmitter(gomock.NewController(t)) mon := buildMonitor(m, managedDomainName, clusterID, secrets...) - wantErr := `certificate "gcscert.pem" not found on secret "cluster"` + wantErr := "unable to find certificate" err := mon.emitCertificateExpirationStatuses(ctx) utilerror.AssertErrorMessage(t, err, wantErr) }) From b7ee4adcf5db4921747f591209332ed307036a48 Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Fri, 15 Sep 2023 11:32:56 -0400 Subject: [PATCH 27/27] return struct literals --- pkg/monitor/cluster/certificateexpirationstatuses_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/monitor/cluster/certificateexpirationstatuses_test.go b/pkg/monitor/cluster/certificateexpirationstatuses_test.go index 4b20aaa2433..850e3df9aae 100644 --- a/pkg/monitor/cluster/certificateexpirationstatuses_test.go +++ b/pkg/monitor/cluster/certificateexpirationstatuses_test.go @@ -192,14 +192,13 @@ func generateTestSecrets(certsInfo []certInfo, tweakTemplateFn func(*x509.Certif } func buildSecret(secretName string, data map[string][]byte) *corev1.Secret { - s := &corev1.Secret{ + return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: secretName, Namespace: "openshift-azure-operator", }, Data: data, } - return s } func buildMonitor(m *mock_metrics.MockEmitter, domain, id string, secrets ...client.Object) *Monitor { @@ -220,7 +219,7 @@ func buildMonitor(m *mock_metrics.MockEmitter, domain, id string, secrets ...cli WithObjects(ingressController). WithObjects(secrets...). Build() - mon := &Monitor{ + return &Monitor{ ocpclientset: ocpclientset, m: m, oc: &api.OpenShiftCluster{ @@ -231,5 +230,4 @@ func buildMonitor(m *mock_metrics.MockEmitter, domain, id string, secrets ...cli }, }, } - return mon }