From ba34c0ecf4011c2107ed70b9d5a1f342836c1832 Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Thu, 28 Nov 2024 12:23:27 +0000 Subject: [PATCH] refactor handleSpecialSecretUpdate (#6875) --- internal/configs/configurator.go | 63 ++++++++++++++------------------ internal/k8s/controller.go | 47 ++++++++++++++++++++---- 2 files changed, 67 insertions(+), 43 deletions(-) diff --git a/internal/configs/configurator.go b/internal/configs/configurator.go index c1963108a2..feb663d65a 100644 --- a/internal/configs/configurator.go +++ b/internal/configs/configurator.go @@ -295,7 +295,7 @@ func (cnf *Configurator) AddOrUpdateIngress(ingEx *IngressEx) (Warnings, error) return warnings, fmt.Errorf("error adding or updating ingress %v/%v: %w", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err) } - if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil { + if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil { return warnings, fmt.Errorf("error reloading NGINX for %v/%v: %w", ingEx.Ingress.Namespace, ingEx.Ingress.Name, err) } @@ -430,7 +430,7 @@ func (cnf *Configurator) AddOrUpdateMergeableIngress(mergeableIngs *MergeableIng return warnings, fmt.Errorf("error when adding or updating ingress %v/%v: %w", mergeableIngs.Master.Ingress.Namespace, mergeableIngs.Master.Ingress.Name, err) } - if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil { + if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil { return warnings, fmt.Errorf("error reloading NGINX for %v/%v: %w", mergeableIngs.Master.Ingress.Namespace, mergeableIngs.Master.Ingress.Name, err) } @@ -585,7 +585,7 @@ func (cnf *Configurator) AddOrUpdateVirtualServer(virtualServerEx *VirtualServer cnf.EnableReloads() } - if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil { + if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil { return warnings, fmt.Errorf("error reloading NGINX for VirtualServer %v/%v: %w", virtualServerEx.VirtualServer.Namespace, virtualServerEx.VirtualServer.Name, err) } @@ -656,7 +656,7 @@ func (cnf *Configurator) AddOrUpdateVirtualServers(virtualServerExes []*VirtualS allWeightUpdates = append(allWeightUpdates, weightUpdates...) } - if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil { + if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil { return allWarnings, fmt.Errorf("error when reloading NGINX when updating Policy: %w", err) } @@ -741,7 +741,7 @@ func (cnf *Configurator) AddOrUpdateTransportServer(transportServerEx *Transport if err != nil { return nil, fmt.Errorf("error adding or updating TransportServer %v/%v: %w", transportServerEx.TransportServer.Namespace, transportServerEx.TransportServer.Name, err) } - if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil { + if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil { return nil, fmt.Errorf("error reloading NGINX for TransportServer %v/%v: %w", transportServerEx.TransportServer.Namespace, transportServerEx.TransportServer.Name, err) } return warnings, nil @@ -909,7 +909,7 @@ func (cnf *Configurator) AddOrUpdateResources(resources ExtendedResources, reloa } if configsChanged || reloadIfUnchanged { - if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil { + if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil { return nil, fmt.Errorf("error when reloading NGINX when updating resources: %w", err) } } @@ -923,7 +923,7 @@ func (cnf *Configurator) addOrUpdateTLSSecret(secret *api_v1.Secret) string { } // AddOrUpdateSpecialTLSSecrets adds or updates a file with a TLS cert and a key from a Special TLS Secret (eg. DefaultServerSecret, WildcardTLSSecret). -func (cnf *Configurator) AddOrUpdateSpecialTLSSecrets(secret *api_v1.Secret, secretNames []string) error { +func (cnf *Configurator) AddOrUpdateSpecialTLSSecrets(secret *api_v1.Secret, secretNames []string) { l := nl.LoggerFromContext(cnf.CfgParams.Context) nl.Debugf(l, "AddOrUpdateSpecialTLSSecrets: secrets [%v]", secretNames) data := GenerateCertAndKeyFileContent(secret) @@ -931,16 +931,6 @@ func (cnf *Configurator) AddOrUpdateSpecialTLSSecrets(secret *api_v1.Secret, sec for _, secretName := range secretNames { cnf.nginxManager.CreateSecret(secretName, data, nginx.ReadWriteOnlyFileMode) } - - if !cnf.DynamicSSLReloadEnabled() { - if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil { - return fmt.Errorf("error when reloading NGINX when updating the special Secrets: %w", err) - } - } else { - nl.Debugf(l, "Skipping reload for %d special Secrets", len(secretNames)) - } - - return nil } // GenerateCertAndKeyFileContent generates a pem file content from the TLS secret. @@ -979,7 +969,7 @@ func (cnf *Configurator) DeleteIngress(key string, skipReload bool) error { } if !skipReload { - if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil { + if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil { return fmt.Errorf("error when removing ingress %v: %w", key, err) } } @@ -1002,7 +992,7 @@ func (cnf *Configurator) DeleteVirtualServer(key string, skipReload bool) error } if !skipReload { - if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil { + if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil { return fmt.Errorf("error when removing VirtualServer %v: %w", key, err) } } @@ -1021,7 +1011,7 @@ func (cnf *Configurator) DeleteTransportServer(key string) error { return fmt.Errorf("error when removing TransportServer %v: %w", key, err) } - err = cnf.reload(nginx.ReloadForOtherUpdate) + err = cnf.Reload(nginx.ReloadForOtherUpdate) if err != nil { return fmt.Errorf("error when removing TransportServer %v: %w", key, err) } @@ -1070,7 +1060,7 @@ func (cnf *Configurator) UpdateEndpoints(ingExes []*IngressEx) error { return nil } - if err := cnf.reload(nginx.ReloadForEndpointsUpdate); err != nil { + if err := cnf.Reload(nginx.ReloadForEndpointsUpdate); err != nil { return fmt.Errorf("error reloading NGINX when updating endpoints: %w", err) } @@ -1105,7 +1095,7 @@ func (cnf *Configurator) UpdateEndpointsMergeableIngress(mergeableIngresses []*M return nil } - if err := cnf.reload(nginx.ReloadForEndpointsUpdate); err != nil { + if err := cnf.Reload(nginx.ReloadForEndpointsUpdate); err != nil { return fmt.Errorf("error reloading NGINX when updating endpoints for %v: %w", mergeableIngresses, err) } @@ -1138,7 +1128,7 @@ func (cnf *Configurator) UpdateEndpointsForVirtualServers(virtualServerExes []*V return nil } - if err := cnf.reload(nginx.ReloadForEndpointsUpdate); err != nil { + if err := cnf.Reload(nginx.ReloadForEndpointsUpdate); err != nil { return fmt.Errorf("error reloading NGINX when updating endpoints: %w", err) } @@ -1185,7 +1175,7 @@ func (cnf *Configurator) UpdateEndpointsForTransportServers(transportServerExes nl.Debug(l, "No need to reload nginx") return nil } - if err := cnf.reload(nginx.ReloadForEndpointsUpdate); err != nil { + if err := cnf.Reload(nginx.ReloadForEndpointsUpdate); err != nil { return fmt.Errorf("error reloading NGINX when updating endpoints: %w", err) } return nil @@ -1272,7 +1262,8 @@ func (cnf *Configurator) DisableReloads() { cnf.isReloadsEnabled = false } -func (cnf *Configurator) reload(isEndpointsUpdate bool) error { +// Reload reloads nginx if reloads is enabled +func (cnf *Configurator) Reload(isEndpointsUpdate bool) error { if !cnf.isReloadsEnabled { return nil } @@ -1398,7 +1389,7 @@ func (cnf *Configurator) UpdateConfig(cfgParams *ConfigParams, resources Extende } cnf.nginxManager.SetOpenTracing(mainCfg.OpenTracingLoadModule) - if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil { + if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil { return allWarnings, fmt.Errorf("error when updating config from ConfigMap: %w", err) } @@ -1414,7 +1405,7 @@ func (cnf *Configurator) ReloadForBatchUpdates(batchReloadsEnabled bool) error { if !batchReloadsEnabled { return nil } - if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil { + if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil { return fmt.Errorf("error when reloading NGINX after a batch event: %w", err) } return nil @@ -1439,7 +1430,7 @@ func (cnf *Configurator) UpdateVirtualServers(updatedVSExes []*VirtualServerEx, } } - if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil { + if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil { errList = append(errList, fmt.Errorf("error when updating VirtualServer: %w", err)) } @@ -1467,7 +1458,7 @@ func (cnf *Configurator) UpdateTransportServers(updatedTSExes []*TransportServer } } - if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil { + if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil { errList = append(errList, fmt.Errorf("error when updating TransportServers: %w", err)) } @@ -1484,7 +1475,7 @@ func (cnf *Configurator) BatchDeleteVirtualServers(deletedKeys []string) []error } } - if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil { + if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil { errList = append(errList, fmt.Errorf("error when reloading NGINX for deleted VirtualServers: %w", err)) } @@ -1501,7 +1492,7 @@ func (cnf *Configurator) BatchDeleteIngresses(deletedKeys []string) []error { } } - if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil { + if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil { errList = append(errList, fmt.Errorf("error when reloading NGINX for deleted Ingresses: %w", err)) } @@ -1677,7 +1668,7 @@ func (cnf *Configurator) AddOrUpdateSpiffeCerts(svidResponse *workloadapi.X509Co cnf.nginxManager.CreateSecret(spiffeCertFileName, pemCerts, spiffeCertsFileMode) cnf.nginxManager.CreateSecret(spiffeBundleFileName, pemBundle, spiffeCertsFileMode) - err = cnf.reload(nginx.ReloadForOtherUpdate) + err = cnf.Reload(nginx.ReloadForOtherUpdate) if err != nil { return fmt.Errorf("error when reloading NGINX when updating the SPIFFE Certs: %w", err) } @@ -1822,7 +1813,7 @@ func (cnf *Configurator) AddOrUpdateAppProtectResource(resource *unstructured.Un return warnings, fmt.Errorf("error when updating %v %v/%v: %w", resource.GetKind(), resource.GetNamespace(), resource.GetName(), err) } - err = cnf.reload(nginx.ReloadForOtherUpdate) + err = cnf.Reload(nginx.ReloadForOtherUpdate) if err != nil { return warnings, fmt.Errorf("error when reloading NGINX when updating %v %v/%v: %w", resource.GetKind(), resource.GetNamespace(), resource.GetName(), err) } @@ -1837,7 +1828,7 @@ func (cnf *Configurator) AddOrUpdateResourcesThatUseDosProtected(ingExes []*Ingr return warnings, fmt.Errorf("error when updating resources that use Dos: %w", err) } - err = cnf.reload(nginx.ReloadForOtherUpdate) + err = cnf.Reload(nginx.ReloadForOtherUpdate) if err != nil { return warnings, fmt.Errorf("error when updating resources that use Dos: %w", err) } @@ -1925,7 +1916,7 @@ func (cnf *Configurator) RefreshAppProtectUserSigs( fmt.Fprintf(&builder, "app_protect_user_defined_signatures %s;\n", fName) } cnf.nginxManager.CreateAppProtectResourceFile(appProtectUserSigIndex, []byte(builder.String())) - return allWarnings, cnf.reload(nginx.ReloadForOtherUpdate) + return allWarnings, cnf.Reload(nginx.ReloadForOtherUpdate) } func appProtectDosPolicyFileName(namespace string, name string) string { @@ -1965,7 +1956,7 @@ func (cnf *Configurator) AddInternalRouteConfig() error { return fmt.Errorf("error when writing main Config: %w", err) } cnf.nginxManager.CreateMainConfig(mainCfgContent) - if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil { + if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil { return fmt.Errorf("error when reloading nginx: %w", err) } return nil diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 79d7f6b0bf..1cfe976024 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -1781,20 +1781,53 @@ func (lbc *LoadBalancerController) handleSpecialSecretUpdate(secret *api_v1.Secr var specialTLSSecretsToUpdate []string secretNsName := generateSecretNSName(secret) + if ok := lbc.specialSecretValidation(secretNsName, secret, &specialTLSSecretsToUpdate); !ok { + // if not ok bail early + return + } + + lbc.writeSpecialSecrets(secret, specialTLSSecretsToUpdate) + + // reload nginx when the TLS special secrets are updated + switch secretNsName { + case lbc.specialSecrets.defaultServerSecret, lbc.specialSecrets.wildcardTLSSecret: + if ok := lbc.performDynamicSSLReload(secret); !ok { + return + } + } + + lbc.recorder.Eventf(secret, api_v1.EventTypeNormal, "Updated", "the special Secret %v was updated", secretNsName) +} + +func (lbc *LoadBalancerController) writeSpecialSecrets(secret *api_v1.Secret, specialTLSSecretsToUpdate []string) { + lbc.configurator.AddOrUpdateSpecialTLSSecrets(secret, specialTLSSecretsToUpdate) +} + +func (lbc *LoadBalancerController) specialSecretValidation(secretNsName string, secret *api_v1.Secret, specialTLSSecretsToUpdate *[]string) bool { if secretNsName == lbc.specialSecrets.defaultServerSecret { - lbc.validationTLSSpecialSecret(secret, configs.DefaultServerSecretFileName, &specialTLSSecretsToUpdate) + lbc.validationTLSSpecialSecret(secret, configs.DefaultServerSecretFileName, specialTLSSecretsToUpdate) } if secretNsName == lbc.specialSecrets.wildcardTLSSecret { - lbc.validationTLSSpecialSecret(secret, configs.WildcardSecretFileName, &specialTLSSecretsToUpdate) + lbc.validationTLSSpecialSecret(secret, configs.WildcardSecretFileName, specialTLSSecretsToUpdate) } + return true +} - err := lbc.configurator.AddOrUpdateSpecialTLSSecrets(secret, specialTLSSecretsToUpdate) - if err != nil { - nl.Errorf(lbc.Logger, "Error when updating the special Secret %v: %v", secretNsName, err) +func (lbc *LoadBalancerController) performDynamicSSLReload(secret *api_v1.Secret) bool { + if !lbc.configurator.DynamicSSLReloadEnabled() { + return lbc.performNGINXReload(secret) + } + return true +} + +func (lbc *LoadBalancerController) performNGINXReload(secret *api_v1.Secret) bool { + secretNsName := generateSecretNSName(secret) + if err := lbc.configurator.Reload(false); err != nil { + nl.Errorf(lbc.Logger, "error when reloading NGINX when updating the special Secrets: %v", err) lbc.recorder.Eventf(secret, api_v1.EventTypeWarning, "UpdatedWithError", "the special Secret %v was updated, but not applied: %v", secretNsName, err) - return + return false } - lbc.recorder.Eventf(secret, api_v1.EventTypeNormal, "Updated", "the special Secret %v was updated", secretNsName) + return true } func generateSecretNSName(secret *api_v1.Secret) string {