From 59ca825fbd942a1e9bd2bb866acfbb3a3c33632b Mon Sep 17 00:00:00 2001 From: Vadim Rutkovsky Date: Tue, 22 Apr 2025 10:17:21 +0200 Subject: [PATCH 1/4] Fix order of elements in "Updating annotation" log message It should be -, not the other way around --- pkg/operator/certrotation/annotations.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/operator/certrotation/annotations.go b/pkg/operator/certrotation/annotations.go index fa9709ec06..7fcd4a9aa9 100644 --- a/pkg/operator/certrotation/annotations.go +++ b/pkg/operator/certrotation/annotations.go @@ -28,19 +28,19 @@ func (a AdditionalAnnotations) EnsureTLSMetadataUpdate(meta *metav1.ObjectMeta) } if len(a.JiraComponent) > 0 && meta.Annotations[annotations.OpenShiftComponent] != a.JiraComponent { diff := cmp.Diff(meta.Annotations[annotations.OpenShiftComponent], a.JiraComponent) - klog.V(2).Infof("Updating %q annotation for %s/%s, diff: %s", annotations.OpenShiftComponent, meta.Name, meta.Namespace, diff) + klog.V(2).Infof("Updating %q annotation for %s/%s, diff: %s", annotations.OpenShiftComponent, meta.Namespace, meta.Name, diff) meta.Annotations[annotations.OpenShiftComponent] = a.JiraComponent modified = true } if len(a.Description) > 0 && meta.Annotations[annotations.OpenShiftDescription] != a.Description { diff := cmp.Diff(meta.Annotations[annotations.OpenShiftDescription], a.Description) - klog.V(2).Infof("Updating %q annotation for %s/%s, diff: %s", annotations.OpenShiftDescription, meta.Name, meta.Namespace, diff) + klog.V(2).Infof("Updating %q annotation for %s/%s, diff: %s", annotations.OpenShiftDescription, meta.Namespace, meta.Name, diff) meta.Annotations[annotations.OpenShiftDescription] = a.Description modified = true } if len(a.AutoRegenerateAfterOfflineExpiry) > 0 && meta.Annotations[AutoRegenerateAfterOfflineExpiryAnnotation] != a.AutoRegenerateAfterOfflineExpiry { diff := cmp.Diff(meta.Annotations[AutoRegenerateAfterOfflineExpiryAnnotation], a.AutoRegenerateAfterOfflineExpiry) - klog.V(2).Infof("Updating %q annotation for %s/%s, diff: %s", AutoRegenerateAfterOfflineExpiryAnnotation, meta.Name, meta.Namespace, diff) + klog.V(2).Infof("Updating %q annotation for %s/%s, diff: %s", AutoRegenerateAfterOfflineExpiryAnnotation, meta.Namespace, meta.Name, diff) meta.Annotations[AutoRegenerateAfterOfflineExpiryAnnotation] = a.AutoRegenerateAfterOfflineExpiry modified = true } From 50550b965b01b18b7f5c944d4bcd986c6810e20f Mon Sep 17 00:00:00 2001 From: Vadim Rutkovsky Date: Thu, 24 Apr 2025 10:23:41 +0200 Subject: [PATCH 2/4] crypto: error message should use duration instead of years --- pkg/crypto/crypto.go | 6 +++--- pkg/crypto/crypto_test.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/crypto/crypto.go b/pkg/crypto/crypto.go index 80f5efc2c0..33a09ae16e 100644 --- a/pkg/crypto/crypto.go +++ b/pkg/crypto/crypto.go @@ -629,7 +629,7 @@ func MakeSelfSignedCAConfig(name string, lifetime time.Duration) (*TLSCertificat func MakeSelfSignedCAConfigForSubject(subject pkix.Name, lifetime time.Duration) (*TLSCertificateConfig, error) { if lifetime <= 0 { lifetime = DefaultCACertificateLifetimeDuration - fmt.Fprintf(os.Stderr, "Validity period of the certificate for %q is unset, resetting to %d years!\n", subject.CommonName, lifetime) + fmt.Fprintf(os.Stderr, "Validity period of the certificate for %q is unset, resetting to %s!\n", subject.CommonName, lifetime.String()) } if lifetime > DefaultCACertificateLifetimeDuration { @@ -1018,7 +1018,7 @@ func newSigningCertificateTemplateForDuration(subject pkix.Name, caLifetime time func newServerCertificateTemplate(subject pkix.Name, hosts []string, lifetime time.Duration, currentTime func() time.Time, authorityKeyId, subjectKeyId []byte) *x509.Certificate { if lifetime <= 0 { lifetime = DefaultCertificateLifetimeDuration - fmt.Fprintf(os.Stderr, "Validity period of the certificate for %q is unset, resetting to %d years!\n", subject.CommonName, lifetime) + fmt.Fprintf(os.Stderr, "Validity period of the certificate for %q is unset, resetting to %s!\n", subject.CommonName, lifetime.String()) } if lifetime > DefaultCertificateLifetimeDuration { @@ -1105,7 +1105,7 @@ func CertsFromPEM(pemCerts []byte) ([]*x509.Certificate, error) { func NewClientCertificateTemplate(subject pkix.Name, lifetime time.Duration, currentTime func() time.Time) *x509.Certificate { if lifetime <= 0 { lifetime = DefaultCertificateLifetimeDuration - fmt.Fprintf(os.Stderr, "Validity period of the certificate for %q is unset, resetting to %d years!\n", subject.CommonName, lifetime) + fmt.Fprintf(os.Stderr, "Validity period of the certificate for %q is unset, resetting to %s!\n", subject.CommonName, lifetime.String()) } if lifetime > DefaultCertificateLifetimeDuration { diff --git a/pkg/crypto/crypto_test.go b/pkg/crypto/crypto_test.go index d269783c17..f0158efa5f 100644 --- a/pkg/crypto/crypto_test.go +++ b/pkg/crypto/crypto_test.go @@ -142,7 +142,7 @@ func TestCrypto(t *testing.T) { func newSigningCertificateTemplate(subject pkix.Name, lifetime time.Duration, currentTime func() time.Time) *x509.Certificate { if lifetime <= 0 { lifetime = DefaultCACertificateLifetimeDuration - fmt.Fprintf(os.Stderr, "Validity period of the certificate for %q is unset, resetting to %d years!\n", subject.CommonName, lifetime) + fmt.Fprintf(os.Stderr, "Validity period of the certificate for %q is unset, resetting to %s!\n", subject.CommonName, lifetime.String()) } if lifetime > DefaultCACertificateLifetimeDuration { From 6f70d12d6d2a2b10e7eab338829bcb0e59df9c5e Mon Sep 17 00:00:00 2001 From: Vadim Rutkovsky Date: Wed, 19 Feb 2025 08:33:40 +0100 Subject: [PATCH 3/4] CombineCABundleConfigMaps: use optimistic create/update Instead of re-creating configmap from scratch every time this function should attempt to use existing configmap and replace the contents only. This would prevent extra configmap updates when metadata changes --- pkg/operator/resourcesynccontroller/core.go | 61 +++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/pkg/operator/resourcesynccontroller/core.go b/pkg/operator/resourcesynccontroller/core.go index 9711348b33..bbd2ab58fb 100644 --- a/pkg/operator/resourcesynccontroller/core.go +++ b/pkg/operator/resourcesynccontroller/core.go @@ -70,3 +70,64 @@ func CombineCABundleConfigMaps(destinationConfigMap ResourceLocation, lister cor } return cm, nil } + +func CombineCABundleConfigMapsOptimistically(destinationConfigMap *corev1.ConfigMap, lister corev1listers.ConfigMapLister, additionalAnnotations certrotation.AdditionalAnnotations, inputConfigMaps ...ResourceLocation) (*corev1.ConfigMap, bool, error) { + var cm *corev1.ConfigMap + if destinationConfigMap == nil { + cm = &corev1.ConfigMap{} + } else { + cm = destinationConfigMap.DeepCopy() + } + certificates := []*x509.Certificate{} + for _, input := range inputConfigMaps { + inputConfigMap, err := lister.ConfigMaps(input.Namespace).Get(input.Name) + if apierrors.IsNotFound(err) { + continue + } + if err != nil { + return nil, false, err + } + + // configmaps must conform to this + inputContent := inputConfigMap.Data["ca-bundle.crt"] + if len(inputContent) == 0 { + continue + } + inputCerts, err := cert.ParseCertsPEM([]byte(inputContent)) + if err != nil { + return nil, false, fmt.Errorf("configmap/%s in %q is malformed: %v", input.Name, input.Namespace, err) + } + certificates = append(certificates, inputCerts...) + } + + certificates = crypto.FilterExpiredCerts(certificates...) + finalCertificates := []*x509.Certificate{} + // now check for duplicates. n^2, but super simple + for i := range certificates { + found := false + for j := range finalCertificates { + if reflect.DeepEqual(certificates[i].Raw, finalCertificates[j].Raw) { + found = true + break + } + } + if !found { + finalCertificates = append(finalCertificates, certificates[i]) + } + } + + caBytes, err := crypto.EncodeCertificates(finalCertificates...) + if err != nil { + return nil, false, err + } + + modified := additionalAnnotations.EnsureTLSMetadataUpdate(&cm.ObjectMeta) + newCMData := map[string]string{ + "ca-bundle.crt": string(caBytes), + } + if !reflect.DeepEqual(cm.Data, newCMData) { + cm.Data = newCMData + modified = true + } + return cm, modified, nil +} From 65142f98d55203087c75cbed29d27c635d07996c Mon Sep 17 00:00:00 2001 From: Vadim Rutkovsky Date: Thu, 24 Apr 2025 10:23:13 +0200 Subject: [PATCH 4/4] Add unit test for TestCombineCABundleConfigMaps --- .../resourcesynccontroller/core_test.go | 297 ++++++++++++++++++ 1 file changed, 297 insertions(+) create mode 100644 pkg/operator/resourcesynccontroller/core_test.go diff --git a/pkg/operator/resourcesynccontroller/core_test.go b/pkg/operator/resourcesynccontroller/core_test.go new file mode 100644 index 0000000000..7c51d00db8 --- /dev/null +++ b/pkg/operator/resourcesynccontroller/core_test.go @@ -0,0 +1,297 @@ +package resourcesynccontroller + +import ( + "fmt" + "testing" + "time" + + "github.com/google/go-cmp/cmp" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + labels "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime/schema" + corev1listers "k8s.io/client-go/listers/core/v1" + + "github.com/openshift/api/annotations" + "github.com/openshift/library-go/pkg/crypto" + "github.com/openshift/library-go/pkg/operator/certrotation" +) + +// mockConfigMapLister is a mock implementation of the ConfigMapLister interface for testing +type mockConfigMapLister struct { + configMaps map[string]map[string]*corev1.ConfigMap +} + +func (m *mockConfigMapLister) List(selector labels.Selector) ([]*corev1.ConfigMap, error) { + panic("not implemented") +} + +func (m *mockConfigMapLister) ConfigMaps(namespace string) corev1listers.ConfigMapNamespaceLister { + return &mockConfigMapNamespaceLister{ + namespace: namespace, + configMaps: m.configMaps, + } +} + +type mockConfigMapNamespaceLister struct { + namespace string + configMaps map[string]map[string]*corev1.ConfigMap +} + +func (m *mockConfigMapNamespaceLister) List(selector labels.Selector) ([]*corev1.ConfigMap, error) { + panic("not implemented") +} + +func (m *mockConfigMapNamespaceLister) Get(name string) (*corev1.ConfigMap, error) { + if m.configMaps == nil { + return nil, apierrors.NewNotFound(schema.GroupResource{Resource: "configmaps"}, name) + } + if namespace, ok := m.configMaps[m.namespace]; ok { + if cm, ok := namespace[name]; ok { + return cm, nil + } + } + return nil, apierrors.NewNotFound(schema.GroupResource{Resource: "configmaps"}, name) +} + +func TestCombineCABundleConfigMapsOptimistically(t *testing.T) { + // Generate test certificates + validCert, err := crypto.MakeSelfSignedCAConfig("Test CA", time.Hour) + if err != nil { + t.Fatalf("Failed to create test certificate: %v", err) + } + + validCertPEM, err := crypto.EncodeCertificates(validCert.Certs[0]) + if err != nil { + t.Fatalf("Failed to encode certificate: %v", err) + } + + validCert2, err := crypto.MakeSelfSignedCAConfig("Test CA", time.Hour) + if err != nil { + t.Fatalf("Failed to create test certificate: %v", err) + } + + validCertPEM2, err := crypto.EncodeCertificates(validCert2.Certs[0]) + if err != nil { + t.Fatalf("Failed to encode certificate: %v", err) + } + + expiredCert, err := crypto.MakeSelfSignedCAConfig("Expired CA", -1*time.Hour) + if err != nil { + t.Fatalf("Failed to create expired certificate: %v", err) + } + + expiredCertPEM, err := crypto.EncodeCertificates(expiredCert.Certs[0]) + if err != nil { + t.Fatalf("Failed to encode expired certificate: %v", err) + } + + jiraComponent := "foobar" + destinationConfigMap := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cm1", + Namespace: "ns1", + Annotations: map[string]string{ + annotations.OpenShiftComponent: jiraComponent, + }, + }, + Data: map[string]string{ + "ca-bundle.crt": string(validCertPEM), + }, + } + tests := []struct { + name string + destinationConfigMap *corev1.ConfigMap + mockConfigMaps map[string]map[string]*corev1.ConfigMap + inputLocations []ResourceLocation + additionalAnnotations certrotation.AdditionalAnnotations + expectModified bool + expectedCABundle *corev1.ConfigMap + }{ + { + name: "combine valid certificates", + mockConfigMaps: map[string]map[string]*corev1.ConfigMap{ + "ns1": { + "cm1": { + Data: map[string]string{ + "ca-bundle.crt": string(validCertPEM), + }, + }, + }, + "ns2": { + "cm2": { + Data: map[string]string{ + "ca-bundle.crt": string(validCertPEM2), + }, + }, + }, + }, + inputLocations: []ResourceLocation{ + {Namespace: "ns1", Name: "cm1"}, + {Namespace: "ns2", Name: "cm2"}, + }, + additionalAnnotations: certrotation.AdditionalAnnotations{}, + expectModified: true, + expectedCABundle: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{}, + }, + Data: map[string]string{ + "ca-bundle.crt": fmt.Sprintf("%s%s", string(validCertPEM), string(validCertPEM2)), + }, + }, + }, + { + name: "filter expired certificates", + destinationConfigMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cm1", + Namespace: "ns1", + Annotations: map[string]string{ + annotations.OpenShiftComponent: jiraComponent, + }, + }, + Data: map[string]string{ + "ca-bundle.crt": string(expiredCertPEM), + }, + }, + mockConfigMaps: map[string]map[string]*corev1.ConfigMap{ + "ns1": { + "cm1": { + Data: map[string]string{ + "ca-bundle.crt": string(validCertPEM), + }, + }, + }, + }, + inputLocations: []ResourceLocation{ + {Namespace: "ns1", Name: "cm1"}, + }, + additionalAnnotations: certrotation.AdditionalAnnotations{}, + expectModified: true, + expectedCABundle: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cm1", + Namespace: "ns1", + Annotations: map[string]string{ + annotations.OpenShiftComponent: jiraComponent, + }, + }, + Data: map[string]string{ + "ca-bundle.crt": string(validCertPEM), + }, + }, + }, + { + name: "not modified", + destinationConfigMap: destinationConfigMap, + mockConfigMaps: map[string]map[string]*corev1.ConfigMap{ + "ns1": { + "cm1": { + Data: map[string]string{ + "ca-bundle.crt": string(validCertPEM), + }, + }, + }, + }, + inputLocations: []ResourceLocation{ + {Namespace: "ns1", Name: "cm1"}, + }, + additionalAnnotations: certrotation.AdditionalAnnotations{ + JiraComponent: jiraComponent, + }, + expectModified: false, + expectedCABundle: destinationConfigMap, + }, + { + name: "metadata modified only", + destinationConfigMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cm1", + Namespace: "ns1", + }, + Data: map[string]string{ + "ca-bundle.crt": string(validCertPEM), + }, + }, + mockConfigMaps: map[string]map[string]*corev1.ConfigMap{ + "ns1": { + "cm1": { + Data: map[string]string{ + "ca-bundle.crt": string(validCertPEM), + }, + }, + }, + }, + inputLocations: []ResourceLocation{ + {Namespace: "ns1", Name: "cm1"}, + }, + additionalAnnotations: certrotation.AdditionalAnnotations{ + JiraComponent: jiraComponent, + }, + expectModified: true, + expectedCABundle: destinationConfigMap, + }, + { + name: "contents modified only", + destinationConfigMap: destinationConfigMap, + mockConfigMaps: map[string]map[string]*corev1.ConfigMap{ + "ns1": { + "cm1": { + Data: map[string]string{ + "ca-bundle.crt": string(validCertPEM2), + }, + }, + }, + }, + inputLocations: []ResourceLocation{ + {Namespace: "ns1", Name: "cm1"}, + }, + additionalAnnotations: certrotation.AdditionalAnnotations{ + JiraComponent: jiraComponent, + }, + expectModified: true, + expectedCABundle: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cm1", + Namespace: "ns1", + Annotations: map[string]string{ + annotations.OpenShiftComponent: jiraComponent, + }, + }, + Data: map[string]string{ + "ca-bundle.crt": string(validCertPEM2), + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + lister := &mockConfigMapLister{ + configMaps: test.mockConfigMaps, + } + + result, modified, err := CombineCABundleConfigMapsOptimistically(test.destinationConfigMap, lister, test.additionalAnnotations, test.inputLocations...) + + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + if test.expectModified != modified { + t.Errorf("Expected modified=%v but got %v", test.expectModified, modified) + } + if err == nil && result == nil { + t.Errorf("Expected result to not be nil when no error occurred") + } + + if result != nil && test.expectedCABundle != nil { + diff := cmp.Diff(&test.expectedCABundle, &result) + if diff != "" { + t.Errorf("Unexpected configmap (-want +got):\n%s", diff) + } + } + }) + } +}