Skip to content

OCPBUGS-55217: CombineCABundleConfigMaps: use optimistic create/update #1936

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

OCPBUGS-55217: CombineCABundleConfigMaps: use optimistic create/update #1936

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e6d5f65
Fix order of elements in "Updating annotation" log message
vrutkovs Apr 22, 2025
42666df
crypto: error message should use duration instead of years
vrutkovs Apr 24, 2025
b50e914
CombineCABundleConfigMaps: use optimistic create/update
vrutkovs Feb 19, 2025
855d77f
Add unit test for TestCombineCABundleConfigMaps
vrutkovs Apr 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions pkg/crypto/crypto.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion pkg/crypto/crypto_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
6 changes: 3 additions & 3 deletions pkg/operator/certrotation/annotations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
61 changes: 61 additions & 0 deletions pkg/operator/resourcesynccontroller/core.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link

@everettraven everettraven May 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Would doing a conversion like string(certificates[i].Raw) make it easier to do a == based comparison?

If we were doing simpler equality based comparison of strings you could use a sets.Set[string] for tracking duplicates with set.Has(...).

I think this would reduce it to O(n):

finalCertStrings := sets.New[string]()
finalCertificates := []*x509.Certificate{}
for i := range certificates {
    if finalCertStrings.Has(string(certificates[i].Raw)) {
        continue
    } 
    finalCertificates = append(finalCertificates, certificates[i])
    finalCertStrings.Insert(string(certificates[i].Raw))
}

Although, you are trading a bit of memory for runtime performance increase this way.

I'll leave it up to you if you'd like to make any changes here, but this isn't something I would block the PR merging on.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also does []byte -> string conversion, so it will use even more memory (and use more allocations). This is not a performance critical code, so I'd rather optimize for memory usage and leave it as is. We may want to do a more comprehensive review of certificate comparison later however

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
}
Loading