From c5692891fb4a24dec11af8ca07f28d1ee1a216e1 Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Tue, 10 Dec 2024 09:15:51 +0000 Subject: [PATCH 1/5] when mgmt configmap changes update secrets --- internal/k8s/controller.go | 36 ++++++++++++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 146add526..7de47b6db 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -883,6 +883,7 @@ func (lbc *LoadBalancerController) updateAllConfigs() { var isNGINXConfigValid bool var mgmtConfigHasWarnings bool var mgmtErr error + reloadNginx := false if lbc.configMap != nil { cfgParams, isNGINXConfigValid = configs.ParseConfigMap(ctx, lbc.configMap, lbc.isNginxPlus, lbc.appProtectEnabled, lbc.appProtectDosEnabled, lbc.configuration.isTLSPassthroughEnabled, lbc.recorder) @@ -892,6 +893,15 @@ func (lbc *LoadBalancerController) updateAllConfigs() { if mgmtErr != nil { nl.Errorf(lbc.Logger, "configmap %s/%s: %v", lbc.mgmtConfigMap.GetNamespace(), lbc.mgmtConfigMap.GetName(), mgmtErr) } + // update special license secret in mgmtConfigParams + if mgmtCfgParams.Secrets.License != "" { + secret, err := lbc.client.CoreV1().Secrets(lbc.mgmtConfigMap.GetNamespace()).Get(context.TODO(), mgmtCfgParams.Secrets.License, meta_v1.GetOptions{}) + if err != nil { + nl.Errorf(lbc.Logger, "secret %s/%s: %v", lbc.mgmtConfigMap.GetNamespace(), mgmtCfgParams.Secrets.License, err) + } + lbc.specialSecrets.licenseSecret = fmt.Sprintf("%s/%s", secret.Namespace, secret.Name) + lbc.handleSpecialSecretUpdate(secret, reloadNginx) + } // update special CA secret in mgmtConfigParams if mgmtCfgParams.Secrets.TrustedCert != "" { secret, err := lbc.client.CoreV1().Secrets(lbc.mgmtConfigMap.GetNamespace()).Get(context.TODO(), mgmtCfgParams.Secrets.TrustedCert, meta_v1.GetOptions{}) @@ -901,6 +911,17 @@ func (lbc *LoadBalancerController) updateAllConfigs() { if _, hasCRL := secret.Data[configs.CACrlKey]; hasCRL { mgmtCfgParams.Secrets.TrustedCRL = secret.Name } + lbc.specialSecrets.trustedCertSecret = fmt.Sprintf("%s/%s", secret.Namespace, secret.Name) + lbc.handleSpecialSecretUpdate(secret, reloadNginx) + } + // update special ClientAuth secret in mgmtConfigParams + if mgmtCfgParams.Secrets.ClientAuth != "" { + secret, err := lbc.client.CoreV1().Secrets(lbc.mgmtConfigMap.GetNamespace()).Get(context.TODO(), mgmtCfgParams.Secrets.ClientAuth, meta_v1.GetOptions{}) + if err != nil { + nl.Errorf(lbc.Logger, "secret %s/%s: %v", lbc.mgmtConfigMap.GetNamespace(), mgmtCfgParams.Secrets.ClientAuth, err) + } + lbc.specialSecrets.clientAuthSecret = fmt.Sprintf("%s/%s", secret.Namespace, secret.Name) + lbc.handleSpecialSecretUpdate(secret, reloadNginx) } } @@ -910,7 +931,7 @@ func (lbc *LoadBalancerController) updateAllConfigs() { resourceExes := lbc.createExtendedResources(resources) - warnings, updateErr := lbc.configurator.UpdateConfig(cfgParams, mgmtCfgParams, resourceExes) + warnings, updateErr := lbc.configurator.UpdateConfig(cfgParams, mgmtCfgParams, resourceExes) // reload happens here eventTitle := "Updated" eventType := api_v1.EventTypeNormal eventWarningMessage := "" @@ -1769,7 +1790,8 @@ func (lbc *LoadBalancerController) syncSecret(task task) { lbc.secretStore.AddOrUpdateSecret(secret) if lbc.isSpecialSecret(key) { - lbc.handleSpecialSecretUpdate(secret) + reloadNginx := true + lbc.handleSpecialSecretUpdate(secret, reloadNginx) // we don't return here in case the special secret is also used in resources. } @@ -1846,7 +1868,7 @@ func (lbc *LoadBalancerController) validationTLSSpecialSecret(secret *api_v1.Sec *secretList = append(*secretList, secretName) } -func (lbc *LoadBalancerController) handleSpecialSecretUpdate(secret *api_v1.Secret) { +func (lbc *LoadBalancerController) handleSpecialSecretUpdate(secret *api_v1.Secret, reload bool) { var specialTLSSecretsToUpdate []string secretNsName := generateSecretNSName(secret) @@ -1860,6 +1882,12 @@ func (lbc *LoadBalancerController) handleSpecialSecretUpdate(secret *api_v1.Secr return } + // When the MGMT Configmap updates, we don't need to reload here, we are reloading in updateAllConfigs(). + if !reload { + lbc.recorder.Eventf(lbc.metadata.pod, api_v1.EventTypeNormal, "SecretUpdated", "the special Secret %v was updated", secretNsName) + return + } + // reload nginx when the TLS special secrets are updated switch secretNsName { case lbc.specialSecrets.licenseSecret: @@ -1881,7 +1909,7 @@ func (lbc *LoadBalancerController) handleSpecialSecretUpdate(secret *api_v1.Secr } } - lbc.recorder.Eventf(secret, api_v1.EventTypeNormal, "Updated", "the special Secret %v was updated", secretNsName) + lbc.recorder.Eventf(lbc.metadata.pod, api_v1.EventTypeNormal, "SecretUpdated", "the special Secret %v was updated", secretNsName) } // writeSpecialSecrets generates content and writes the secret to disk From 3ba0ce38a3ac798cc49344d1e3f759250080b938 Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Tue, 10 Dec 2024 09:16:41 +0000 Subject: [PATCH 2/5] add test for license key secret configmap change --- .../plus-token-name-keys.yaml | 7 ++ tests/suite/test_mgmt_configmap_keys.py | 106 ++++++++++++++++++ 2 files changed, 113 insertions(+) create mode 100644 tests/data/mgmt-configmap-keys/plus-token-name-keys.yaml create mode 100644 tests/suite/test_mgmt_configmap_keys.py diff --git a/tests/data/mgmt-configmap-keys/plus-token-name-keys.yaml b/tests/data/mgmt-configmap-keys/plus-token-name-keys.yaml new file mode 100644 index 000000000..f5d9105f6 --- /dev/null +++ b/tests/data/mgmt-configmap-keys/plus-token-name-keys.yaml @@ -0,0 +1,7 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: nginx-config-mgmt + namespace: nginx-ingress +data: + license-token-secret-name: "license-token-changed" diff --git a/tests/suite/test_mgmt_configmap_keys.py b/tests/suite/test_mgmt_configmap_keys.py new file mode 100644 index 000000000..e72343fce --- /dev/null +++ b/tests/suite/test_mgmt_configmap_keys.py @@ -0,0 +1,106 @@ +import pytest +from settings import TEST_DATA +from suite.utils.resources_utils import ( + create_license, + ensure_connection_to_public_endpoint, + get_events_for_object, + get_first_pod_name, + get_reload_count, + is_secret_present, + replace_configmap_from_yaml, + wait_before_test, +) + + +def assert_event(event_list, event_type, reason, message_substring): + """ + Assert that an event with specific type, reason, and message substring exists. + + :param event_list: List of events + :param event_type: 'Normal' or 'Warning' + :param reason: Event reason + :param message_substring: Substring expected in the event message + """ + for event in event_list: + if event.type == event_type and event.reason == reason and message_substring in event.message: + return + assert ( + False + ), f"Expected event with type '{event_type}', reason '{reason}', and message containing '{message_substring}' not found." + + +@pytest.mark.skip_for_nginx_oss +@pytest.mark.ingresses +@pytest.mark.smoke +class TestMGMTConfigMap: + @pytest.mark.parametrize( + "ingress_controller", + [ + pytest.param( + {"extra_args": ["-enable-prometheus-metrics"]}, + ) + ], + indirect=["ingress_controller"], + ) + def test_mgmt_configmap_events( + self, + cli_arguments, + kube_apis, + ingress_controller_prerequisites, + ingress_controller, + ingress_controller_endpoint, + ): + ensure_connection_to_public_endpoint( + ingress_controller_endpoint.public_ip, + ingress_controller_endpoint.port, + ingress_controller_endpoint.port_ssl, + ) + ic_pod_name = get_first_pod_name(kube_apis.v1, ingress_controller_prerequisites.namespace) + metrics_url = ( + f"http://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.metrics_port}/metrics" + ) + + print("Step 1: get reload count") + reload_count = get_reload_count(metrics_url) + + wait_before_test(1) + print(f"Step 1a: initial reload count is {reload_count}") + + print("Step 2: create duplicate existing secret with new name") + license_name = create_license( + kube_apis.v1, + ingress_controller_prerequisites.namespace, + cli_arguments["plus-jwt"], + license_token_name="license-token-changed", + ) + assert is_secret_present(kube_apis.v1, license_name, ingress_controller_prerequisites.namespace) + + print("Step 3: update the ConfigMap/license-token-secret-name to the new secret") + replace_configmap_from_yaml( + kube_apis.v1, + "nginx-config-mgmt", + ingress_controller_prerequisites.namespace, + f"{TEST_DATA}/mgmt-configmap-keys/plus-token-name-keys.yaml", + ) + + wait_before_test() + + print("Step 4: check reload count has incremented") + new_reload_count = get_reload_count(metrics_url) + print(f"Step 4a: new reload count is {new_reload_count}") + assert new_reload_count > reload_count + + print("Step 5: check pod for SecretUpdated event") + events = get_events_for_object( + kube_apis.v1, + ingress_controller_prerequisites.namespace, + ic_pod_name, + ) + + # Assert that the 'SecretUpdated' event is present + assert_event( + events, + "Normal", + "SecretUpdated", + f"the special Secret {ingress_controller_prerequisites.namespace}/{license_name} was updated", + ) From d73583083dbc09391cfdadffe03bb5d7eadc5395 Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Tue, 10 Dec 2024 09:21:48 +0000 Subject: [PATCH 3/5] remove comment --- internal/k8s/controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 7de47b6db..4f8e7105f 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -931,7 +931,7 @@ func (lbc *LoadBalancerController) updateAllConfigs() { resourceExes := lbc.createExtendedResources(resources) - warnings, updateErr := lbc.configurator.UpdateConfig(cfgParams, mgmtCfgParams, resourceExes) // reload happens here + warnings, updateErr := lbc.configurator.UpdateConfig(cfgParams, mgmtCfgParams, resourceExes) eventTitle := "Updated" eventType := api_v1.EventTypeNormal eventWarningMessage := "" From 7c2337ce3d37d139e2cc1b2872dde8d801675d30 Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Tue, 10 Dec 2024 13:06:52 +0000 Subject: [PATCH 4/5] return error and log event if TLS special secret is not valid --- internal/k8s/controller.go | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 4f8e7105f..874e673ad 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -1850,22 +1850,19 @@ func (lbc *LoadBalancerController) handleSecretUpdate(secret *api_v1.Secret, res warnings, addOrUpdateErr = lbc.configurator.AddOrUpdateResources(resourceExes, !lbc.configurator.DynamicSSLReloadEnabled()) if addOrUpdateErr != nil { nl.Errorf(lbc.Logger, "Error when updating Secret %v: %v", secretNsName, addOrUpdateErr) - lbc.recorder.Eventf(secret, api_v1.EventTypeWarning, "UpdatedWithError", "%v was updated, but not applied: %v", secretNsName, addOrUpdateErr) + lbc.recorder.Eventf(lbc.metadata.pod, api_v1.EventTypeWarning, "UpdatedWithError", "%v was updated, but not applied: %v", secretNsName, addOrUpdateErr) } lbc.updateResourcesStatusAndEvents(resources, warnings, addOrUpdateErr) } -func (lbc *LoadBalancerController) validationTLSSpecialSecret(secret *api_v1.Secret, secretName string, secretList *[]string) { - secretNsName := generateSecretNSName(secret) - +func (lbc *LoadBalancerController) validationTLSSpecialSecret(secret *api_v1.Secret, secretName string, secretList *[]string) error { err := secrets.ValidateTLSSecret(secret) if err != nil { - nl.Errorf(lbc.Logger, "Couldn't validate the special Secret %v: %v", secretNsName, err) - lbc.recorder.Eventf(secret, api_v1.EventTypeWarning, "Rejected", "the special Secret %v was rejected, using the previous version: %v", secretNsName, err) - return + return err } *secretList = append(*secretList, secretName) + return nil } func (lbc *LoadBalancerController) handleSpecialSecretUpdate(secret *api_v1.Secret, reload bool) { @@ -1932,10 +1929,20 @@ func (lbc *LoadBalancerController) writeSpecialSecrets(secret *api_v1.Secret, se func (lbc *LoadBalancerController) specialSecretValidation(secretNsName string, secret *api_v1.Secret, specialTLSSecretsToUpdate *[]string) bool { if secretNsName == lbc.specialSecrets.defaultServerSecret { - lbc.validationTLSSpecialSecret(secret, configs.DefaultServerSecretFileName, specialTLSSecretsToUpdate) + err := lbc.validationTLSSpecialSecret(secret, configs.DefaultServerSecretFileName, specialTLSSecretsToUpdate) + if err != nil { + nl.Errorf(lbc.Logger, "Couldn't validate the special Secret %v: %v", secretNsName, err) + lbc.recorder.Eventf(lbc.metadata.pod, api_v1.EventTypeWarning, "Rejected", "the special Secret %v was rejected, using the previous version: %v", secretNsName, err) + return false + } } if secretNsName == lbc.specialSecrets.wildcardTLSSecret { - lbc.validationTLSSpecialSecret(secret, configs.WildcardSecretFileName, specialTLSSecretsToUpdate) + err := lbc.validationTLSSpecialSecret(secret, configs.WildcardSecretFileName, specialTLSSecretsToUpdate) + if err != nil { + nl.Errorf(lbc.Logger, "Couldn't validate the special Secret %v: %v", secretNsName, err) + lbc.recorder.Eventf(lbc.metadata.pod, api_v1.EventTypeWarning, "Rejected", "the special Secret %v was rejected, using the previous version: %v", secretNsName, err) + return false + } } if secretNsName == lbc.specialSecrets.licenseSecret { err := secrets.ValidateLicenseSecret(secret) @@ -1954,7 +1961,12 @@ func (lbc *LoadBalancerController) specialSecretValidation(secretNsName string, } } if secretNsName == lbc.specialSecrets.clientAuthSecret { - lbc.validationTLSSpecialSecret(secret, configs.ClientAuthCertSecretFileName, specialTLSSecretsToUpdate) + err := lbc.validationTLSSpecialSecret(secret, configs.ClientAuthCertSecretFileName, specialTLSSecretsToUpdate) + if err != nil { + nl.Errorf(lbc.Logger, "Couldn't validate the special Secret %v: %v", secretNsName, err) + lbc.recorder.Eventf(lbc.metadata.pod, api_v1.EventTypeWarning, "Rejected", "the special Secret %v was rejected, using the previous version: %v", secretNsName, err) + return false + } } return true } From a65d4023d3a3890ef4c4b636c7ffabeab2cf3aef Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Tue, 10 Dec 2024 14:09:10 +0000 Subject: [PATCH 5/5] redefine how reloadNginx variable is set --- internal/k8s/controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 874e673ad..b9d4fcd2f 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -883,7 +883,7 @@ func (lbc *LoadBalancerController) updateAllConfigs() { var isNGINXConfigValid bool var mgmtConfigHasWarnings bool var mgmtErr error - reloadNginx := false + var reloadNginx bool if lbc.configMap != nil { cfgParams, isNGINXConfigValid = configs.ParseConfigMap(ctx, lbc.configMap, lbc.isNginxPlus, lbc.appProtectEnabled, lbc.appProtectDosEnabled, lbc.configuration.isTLSPassthroughEnabled, lbc.recorder)