Skip to content
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

Refactor handleSpecialSecretUpdate #6875

Merged
merged 4 commits into from
Nov 28, 2024
Merged

Refactor handleSpecialSecretUpdate #6875

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
b2753f5
refactor handleSpecialSecretUpdate
Nov 27, 2024
6bf247e
fix merge
Nov 27, 2024
3d30f6a
make special secrets array a pointer
pdabelf5 Nov 28, 2024
0884769
Merge branch 'main' into chore/refactor-special-secret-handling
pdabelf5 Nov 28, 2024
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
63 changes: 27 additions & 36 deletions internal/configs/configurator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@
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)
}

Expand Down Expand Up @@ -430,7 +430,7 @@
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)
}

Expand Down Expand Up @@ -585,7 +585,7 @@
cnf.EnableReloads()
}

if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil {

Check warning on line 588 in internal/configs/configurator.go

View check run for this annotation

Codecov / codecov/patch

internal/configs/configurator.go#L588 

Added line #L588 was not covered by tests
return warnings, fmt.Errorf("error reloading NGINX for VirtualServer %v/%v: %w", virtualServerEx.VirtualServer.Namespace, virtualServerEx.VirtualServer.Name, err)
}

Expand Down Expand Up @@ -656,7 +656,7 @@
allWeightUpdates = append(allWeightUpdates, weightUpdates...)
}

if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil {

Check warning on line 659 in internal/configs/configurator.go

View check run for this annotation

Codecov / codecov/patch

internal/configs/configurator.go#L659 

Added line #L659 was not covered by tests
return allWarnings, fmt.Errorf("error when reloading NGINX when updating Policy: %w", err)
}

Expand Down Expand Up @@ -741,7 +741,7 @@
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
Expand Down Expand Up @@ -909,7 +909,7 @@
}

if configsChanged || reloadIfUnchanged {
if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil {

Check warning on line 912 in internal/configs/configurator.go

View check run for this annotation

Codecov / codecov/patch

internal/configs/configurator.go#L912 

Added line #L912 was not covered by tests
return nil, fmt.Errorf("error when reloading NGINX when updating resources: %w", err)
}
}
Expand All @@ -923,24 +923,14 @@
}

// 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) {

Check warning on line 926 in internal/configs/configurator.go

View check run for this annotation

Codecov / codecov/patch

internal/configs/configurator.go#L926 

Added line #L926 was not covered by tests
l := nl.LoggerFromContext(cnf.CfgParams.Context)
nl.Debugf(l, "AddOrUpdateSpecialTLSSecrets: secrets [%v]", secretNames)
data := GenerateCertAndKeyFileContent(secret)

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.
Expand Down Expand Up @@ -979,7 +969,7 @@
}

if !skipReload {
if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil {

Check warning on line 972 in internal/configs/configurator.go

View check run for this annotation

Codecov / codecov/patch

internal/configs/configurator.go#L972 

Added line #L972 was not covered by tests
return fmt.Errorf("error when removing ingress %v: %w", key, err)
}
}
Expand All @@ -1002,7 +992,7 @@
}

if !skipReload {
if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil {

Check warning on line 995 in internal/configs/configurator.go

View check run for this annotation

Codecov / codecov/patch

internal/configs/configurator.go#L995 

Added line #L995 was not covered by tests
return fmt.Errorf("error when removing VirtualServer %v: %w", key, err)
}
}
Expand All @@ -1021,7 +1011,7 @@
return fmt.Errorf("error when removing TransportServer %v: %w", key, err)
}

err = cnf.reload(nginx.ReloadForOtherUpdate)
err = cnf.Reload(nginx.ReloadForOtherUpdate)

Check warning on line 1014 in internal/configs/configurator.go

View check run for this annotation

Codecov / codecov/patch

internal/configs/configurator.go#L1014 

Added line #L1014 was not covered by tests
if err != nil {
return fmt.Errorf("error when removing TransportServer %v: %w", key, err)
}
Expand Down Expand Up @@ -1070,7 +1060,7 @@
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)
}

Expand Down Expand Up @@ -1105,7 +1095,7 @@
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)
}

Expand Down Expand Up @@ -1138,7 +1128,7 @@
return nil
}

if err := cnf.reload(nginx.ReloadForEndpointsUpdate); err != nil {
if err := cnf.Reload(nginx.ReloadForEndpointsUpdate); err != nil {

Check warning on line 1131 in internal/configs/configurator.go

View check run for this annotation

Codecov / codecov/patch

internal/configs/configurator.go#L1131 

Added line #L1131 was not covered by tests
return fmt.Errorf("error reloading NGINX when updating endpoints: %w", err)
}

Expand Down Expand Up @@ -1185,7 +1175,7 @@
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 {

Check warning on line 1178 in internal/configs/configurator.go

View check run for this annotation

Codecov / codecov/patch

internal/configs/configurator.go#L1178 

Added line #L1178 was not covered by tests
return fmt.Errorf("error reloading NGINX when updating endpoints: %w", err)
}
return nil
Expand Down Expand Up @@ -1272,7 +1262,8 @@
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
}
Expand Down Expand Up @@ -1398,7 +1389,7 @@
}

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

Expand All @@ -1414,7 +1405,7 @@
if !batchReloadsEnabled {
return nil
}
if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil {

Check warning on line 1408 in internal/configs/configurator.go

View check run for this annotation

Codecov / codecov/patch

internal/configs/configurator.go#L1408 

Added line #L1408 was not covered by tests
return fmt.Errorf("error when reloading NGINX after a batch event: %w", err)
}
return nil
Expand All @@ -1439,7 +1430,7 @@
}
}

if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil {

Check warning on line 1433 in internal/configs/configurator.go

View check run for this annotation

Codecov / codecov/patch

internal/configs/configurator.go#L1433 

Added line #L1433 was not covered by tests
errList = append(errList, fmt.Errorf("error when updating VirtualServer: %w", err))
}

Expand Down Expand Up @@ -1467,7 +1458,7 @@
}
}

if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil {

Check warning on line 1461 in internal/configs/configurator.go

View check run for this annotation

Codecov / codecov/patch

internal/configs/configurator.go#L1461 

Added line #L1461 was not covered by tests
errList = append(errList, fmt.Errorf("error when updating TransportServers: %w", err))
}

Expand All @@ -1484,7 +1475,7 @@
}
}

if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil {

Check warning on line 1478 in internal/configs/configurator.go

View check run for this annotation

Codecov / codecov/patch

internal/configs/configurator.go#L1478 

Added line #L1478 was not covered by tests
errList = append(errList, fmt.Errorf("error when reloading NGINX for deleted VirtualServers: %w", err))
}

Expand All @@ -1501,7 +1492,7 @@
}
}

if err := cnf.reload(nginx.ReloadForOtherUpdate); err != nil {
if err := cnf.Reload(nginx.ReloadForOtherUpdate); err != nil {

Check warning on line 1495 in internal/configs/configurator.go

View check run for this annotation

Codecov / codecov/patch

internal/configs/configurator.go#L1495 

Added line #L1495 was not covered by tests
errList = append(errList, fmt.Errorf("error when reloading NGINX for deleted Ingresses: %w", err))
}

Expand Down Expand Up @@ -1677,7 +1668,7 @@
cnf.nginxManager.CreateSecret(spiffeCertFileName, pemCerts, spiffeCertsFileMode)
cnf.nginxManager.CreateSecret(spiffeBundleFileName, pemBundle, spiffeCertsFileMode)

err = cnf.reload(nginx.ReloadForOtherUpdate)
err = cnf.Reload(nginx.ReloadForOtherUpdate)

Check warning on line 1671 in internal/configs/configurator.go

View check run for this annotation

Codecov / codecov/patch

internal/configs/configurator.go#L1671 

Added line #L1671 was not covered by tests
if err != nil {
return fmt.Errorf("error when reloading NGINX when updating the SPIFFE Certs: %w", err)
}
Expand Down Expand Up @@ -1822,7 +1813,7 @@
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)

Check warning on line 1816 in internal/configs/configurator.go

View check run for this annotation

Codecov / codecov/patch

internal/configs/configurator.go#L1816 

Added line #L1816 was not covered by tests
if err != nil {
return warnings, fmt.Errorf("error when reloading NGINX when updating %v %v/%v: %w", resource.GetKind(), resource.GetNamespace(), resource.GetName(), err)
}
Expand All @@ -1837,7 +1828,7 @@
return warnings, fmt.Errorf("error when updating resources that use Dos: %w", err)
}

err = cnf.reload(nginx.ReloadForOtherUpdate)
err = cnf.Reload(nginx.ReloadForOtherUpdate)

Check warning on line 1831 in internal/configs/configurator.go

View check run for this annotation

Codecov / codecov/patch

internal/configs/configurator.go#L1831 

Added line #L1831 was not covered by tests
if err != nil {
return warnings, fmt.Errorf("error when updating resources that use Dos: %w", err)
}
Expand Down Expand Up @@ -1925,7 +1916,7 @@
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)

Check warning on line 1919 in internal/configs/configurator.go

View check run for this annotation

Codecov / codecov/patch

internal/configs/configurator.go#L1919 

Added line #L1919 was not covered by tests
}

func appProtectDosPolicyFileName(namespace string, name string) string {
Expand Down Expand Up @@ -1965,7 +1956,7 @@
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
Expand Down
47 changes: 40 additions & 7 deletions internal/k8s/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1781,20 +1781,53 @@
var specialTLSSecretsToUpdate []string
secretNsName := generateSecretNSName(secret)

if ok := lbc.specialSecretValidation(secretNsName, secret, &specialTLSSecretsToUpdate); !ok {
// if not ok bail early
return
}

Check warning on line 1787 in internal/k8s/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/k8s/controller.go#L1784-L1787 

Added lines #L1784 - L1787 were not covered by tests

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
}

Check warning on line 1796 in internal/k8s/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/k8s/controller.go#L1789-L1796 

Added lines #L1789 - L1796 were not covered by tests
}

lbc.recorder.Eventf(secret, api_v1.EventTypeNormal, "Updated", "the special Secret %v was updated", secretNsName)

Check warning on line 1799 in internal/k8s/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/k8s/controller.go#L1799 

Added line #L1799 was not covered by tests
}

func (lbc *LoadBalancerController) writeSpecialSecrets(secret *api_v1.Secret, specialTLSSecretsToUpdate []string) {
lbc.configurator.AddOrUpdateSpecialTLSSecrets(secret, specialTLSSecretsToUpdate)

Check warning on line 1803 in internal/k8s/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/k8s/controller.go#L1802-L1803 

Added lines #L1802 - L1803 were not covered by tests
}

func (lbc *LoadBalancerController) specialSecretValidation(secretNsName string, secret *api_v1.Secret, specialTLSSecretsToUpdate *[]string) bool {

Check warning on line 1806 in internal/k8s/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/k8s/controller.go#L1806 

Added line #L1806 was not covered by tests
if secretNsName == lbc.specialSecrets.defaultServerSecret {
lbc.validationTLSSpecialSecret(secret, configs.DefaultServerSecretFileName, &specialTLSSecretsToUpdate)
lbc.validationTLSSpecialSecret(secret, configs.DefaultServerSecretFileName, specialTLSSecretsToUpdate)

Check warning on line 1808 in internal/k8s/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/k8s/controller.go#L1808 

Added line #L1808 was not covered by tests
}
if secretNsName == lbc.specialSecrets.wildcardTLSSecret {
lbc.validationTLSSpecialSecret(secret, configs.WildcardSecretFileName, &specialTLSSecretsToUpdate)
lbc.validationTLSSpecialSecret(secret, configs.WildcardSecretFileName, specialTLSSecretsToUpdate)

Check warning on line 1811 in internal/k8s/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/k8s/controller.go#L1811 

Added line #L1811 was not covered by tests
}
return true

Check warning on line 1813 in internal/k8s/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/k8s/controller.go#L1813 

Added line #L1813 was not covered by tests
}

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

Check warning on line 1820 in internal/k8s/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/k8s/controller.go#L1816-L1820 

Added lines #L1816 - L1820 were not covered by tests
}

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)

Check warning on line 1826 in internal/k8s/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/k8s/controller.go#L1823-L1826 

Added lines #L1823 - L1826 were not covered by tests
lbc.recorder.Eventf(secret, api_v1.EventTypeWarning, "UpdatedWithError", "the special Secret %v was updated, but not applied: %v", secretNsName, err)
return
return false

Check warning on line 1828 in internal/k8s/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/k8s/controller.go#L1828 

Added line #L1828 was not covered by tests
}
lbc.recorder.Eventf(secret, api_v1.EventTypeNormal, "Updated", "the special Secret %v was updated", secretNsName)
return true

Check warning on line 1830 in internal/k8s/controller.go

View check run for this annotation

Codecov / codecov/patch

internal/k8s/controller.go#L1830 

Added line #L1830 was not covered by tests
}

func generateSecretNSName(secret *api_v1.Secret) string {
Expand Down
Loading