From 6bb39495080744cb4cdd4d64121c474d0e757077 Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Wed, 31 Jan 2024 16:07:07 +1300 Subject: [PATCH 01/26] Issue 4837: chore: Rename main_test.go to flags_test.go * All the tests contained were for flags.go functions anyway * Therefore, it makes sense to rename it as such. --- cmd/nginx-ingress/{main_test.go => flags_test.go} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename cmd/nginx-ingress/{main_test.go => flags_test.go} (100%) diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/flags_test.go similarity index 100% rename from cmd/nginx-ingress/main_test.go rename to cmd/nginx-ingress/flags_test.go From 39aa5f0cab3a9be70a5a7c22b94fe518ad062d65 Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Fri, 16 Feb 2024 17:56:11 +1300 Subject: [PATCH 02/26] Issue 4837: chore: main.go refactor The objective of this change is to make functions testable and/or easy to read * replace fatal errors with formatted errors to be returned to the caller for error handling * split functions were appropriate --- cmd/nginx-ingress/main.go | 98 ++++++++++++++++++++++----------------- 1 file changed, 56 insertions(+), 42 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 2e586befe1..4872d79296 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -58,11 +58,25 @@ func main() { parseFlags() - config, kubeClient := createConfigAndKubeClient() + config, err := getClientConfig() + if err != nil { + glog.Fatalf("error creating client configuration: %v", err) + } + + kubeClient, err := getKubeClient(config) + if err != nil { + glog.Fatalf("Failed to create client: %v.", err) + } - kubernetesVersionInfo(kubeClient) + err = confirmMinimumkubernetesVersionCriteria(kubeClient) + if err != nil { + glog.Fatal(err) + } - validateIngressClass(kubeClient) + err = validateIngressClass(kubeClient) + if err != nil { + glog.Fatal(err) + } checkNamespaces(kubeClient) @@ -133,7 +147,10 @@ func main() { nginxDone := make(chan error, 1) nginxManager.Start(nginxDone) - plusClient := createPlusClient(*nginxPlus, useFakeNginxManager, nginxManager) + plusClient, err := createPlusClient(*nginxPlus, useFakeNginxManager, nginxManager) + if err != nil { + glog.Fatal(err) + } plusCollector, syslogListener, latencyCollector := createPlusAndLatencyCollectors(registry, constLabels, kubeClient, plusClient, staticCfgParams.NginxServiceMesh) cnf := configs.NewConfigurator(configs.ConfiguratorParams{ @@ -230,9 +247,7 @@ func main() { } } -func createConfigAndKubeClient() (*rest.Config, *kubernetes.Clientset) { - var config *rest.Config - var err error +func getClientConfig() (config *rest.Config, err error) { if *proxyURL != "" { config, err = clientcmd.NewNonInteractiveDeferredLoadingClientConfig( &clientcmd.ClientConfigLoadingRules{}, @@ -241,70 +256,72 @@ func createConfigAndKubeClient() (*rest.Config, *kubernetes.Clientset) { Server: *proxyURL, }, }).ClientConfig() - if err != nil { - glog.Fatalf("error creating client configuration: %v", err) - } } else { - if config, err = rest.InClusterConfig(); err != nil { - glog.Fatalf("error creating client configuration: %v", err) - } + config, err = rest.InClusterConfig() } - kubeClient, err := kubernetes.NewForConfig(config) - if err != nil { - glog.Fatalf("Failed to create client: %v.", err) - } + return config, err +} - return config, kubeClient +func getKubeClient(config *rest.Config) (kubeClient *kubernetes.Clientset, err error) { + kubeClient, err = kubernetes.NewForConfig(config) + return kubeClient, err } -func kubernetesVersionInfo(kubeClient kubernetes.Interface) { +func confirmMinimumkubernetesVersionCriteria(kubeClient kubernetes.Interface) (err error) { k8sVersion, err := k8s.GetK8sVersion(kubeClient) if err != nil { - glog.Fatalf("error retrieving k8s version: %v", err) + return fmt.Errorf("error retrieving k8s version: %v", err) } glog.Infof("Kubernetes version: %v", k8sVersion) minK8sVersion, err := util_version.ParseGeneric("1.22.0") if err != nil { - glog.Fatalf("unexpected error parsing minimum supported version: %v", err) + return fmt.Errorf("unexpected error parsing minimum supported version: %v", err) } if !k8sVersion.AtLeast(minK8sVersion) { - glog.Fatalf("Versions of Kubernetes < %v are not supported, please refer to the documentation for details on supported versions and legacy controller support.", minK8sVersion) + return fmt.Errorf("Versions of Kubernetes < %v are not supported, please refer to the documentation for details on supported versions and legacy controller support", minK8sVersion) } + return err } -func validateIngressClass(kubeClient kubernetes.Interface) { +func validateIngressClass(kubeClient kubernetes.Interface) (err error) { ingressClassRes, err := kubeClient.NetworkingV1().IngressClasses().Get(context.TODO(), *ingressClass, meta_v1.GetOptions{}) if err != nil { - glog.Fatalf("Error when getting IngressClass %v: %v", *ingressClass, err) + return fmt.Errorf("Error when getting IngressClass %v: %v", *ingressClass, err) } if ingressClassRes.Spec.Controller != k8s.IngressControllerName { - glog.Fatalf("IngressClass with name %v has an invalid Spec.Controller %v; expected %v", ingressClassRes.Name, ingressClassRes.Spec.Controller, k8s.IngressControllerName) + return fmt.Errorf("IngressClass with name %v has an invalid Spec.Controller %v; expected %v", ingressClassRes.Name, ingressClassRes.Spec.Controller, k8s.IngressControllerName) } + + return err } func checkNamespaces(kubeClient kubernetes.Interface) { if *watchNamespaceLabel != "" { - // bootstrap the watched namespace list - var newWatchNamespaces []string - nsList, err := kubeClient.CoreV1().Namespaces().List(context.TODO(), meta_v1.ListOptions{LabelSelector: *watchNamespaceLabel}) - if err != nil { - glog.Errorf("error when getting Namespaces with the label selector %v: %v", watchNamespaceLabel, err) - } - for _, ns := range nsList.Items { - newWatchNamespaces = append(newWatchNamespaces, ns.Name) - } - watchNamespaces = newWatchNamespaces - glog.Infof("Namespaces watched using label %v: %v", *watchNamespaceLabel, watchNamespaces) + watchNamespaces = getWatchedNamespaces(kubeClient) } else { checkNamespaceExists(kubeClient, watchNamespaces) } checkNamespaceExists(kubeClient, watchSecretNamespaces) } +func getWatchedNamespaces(kubeClient kubernetes.Interface) (newWatchNamespaces []string) { + // bootstrap the watched namespace list + nsList, err := kubeClient.CoreV1().Namespaces().List(context.TODO(), meta_v1.ListOptions{LabelSelector: *watchNamespaceLabel}) + if err != nil { + glog.Errorf("error when getting Namespaces with the label selector %v: %v", watchNamespaceLabel, err) + } + for _, ns := range nsList.Items { + newWatchNamespaces = append(newWatchNamespaces, ns.Name) + } + glog.Infof("Namespaces watched using label %v: %v", *watchNamespaceLabel, watchNamespaces) + + return newWatchNamespaces +} + func checkNamespaceExists(kubeClient kubernetes.Interface, namespaces []string) { for _, ns := range namespaces { if ns != "" { @@ -341,19 +358,16 @@ func createCustomClients(config *rest.Config) (dynamic.Interface, k8s_nginx.Inte return dynClient, confClient } -func createPlusClient(nginxPlus bool, useFakeNginxManager bool, nginxManager nginx.Manager) *client.NginxClient { - var plusClient *client.NginxClient - var err error - +func createPlusClient(nginxPlus bool, useFakeNginxManager bool, nginxManager nginx.Manager) (plusClient *client.NginxClient, err error) { if nginxPlus && !useFakeNginxManager { httpClient := getSocketClient("/var/lib/nginx/nginx-plus-api.sock") plusClient, err = client.NewNginxClient("http://nginx-plus-api/api", client.WithHTTPClient(httpClient)) if err != nil { - glog.Fatalf("Failed to create NginxClient for Plus: %v", err) + return plusClient, fmt.Errorf("Failed to create NginxClient for Plus: %v", err) } nginxManager.SetPlusClients(plusClient, httpClient) } - return plusClient + return plusClient, nil } func createTemplateExecutors() (*version1.TemplateExecutor, *version2.TemplateExecutor) { From f2b2873d14ce392a3e95e1ad28f4230f4490dfab Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Mon, 19 Feb 2024 15:50:10 +1300 Subject: [PATCH 03/26] Issue 4837: chore: main.go refactor The objective of this change is to make functions testable and/or easy to read * replace fatal errors with formatted errors to be returned to the caller for error handling * split functions were appropriate --- cmd/nginx-ingress/main_test.go | 1 + 1 file changed, 1 insertion(+) create mode 100644 cmd/nginx-ingress/main_test.go diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go new file mode 100644 index 0000000000..06ab7d0f9a --- /dev/null +++ b/cmd/nginx-ingress/main_test.go @@ -0,0 +1 @@ +package main From fb29d9a70af7a4ae59cdfd3994d1a0e5a914d9d6 Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Mon, 19 Feb 2024 16:01:46 +1300 Subject: [PATCH 04/26] Issue 4837: chore: main.go refactor The objective of this change is to make functions testable and/or easy to read * replace fatal errors with formatted errors to be returned to the caller for error handling * split functions were appropriate --- cmd/nginx-ingress/main.go | 141 ++++++++++++++++++++++++-------------- 1 file changed, 88 insertions(+), 53 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 4872d79296..8f76dbfac6 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -80,7 +80,14 @@ func main() { checkNamespaces(kubeClient) - dynClient, confClient := createCustomClients(config) + dynClient, err := createDynamicClient(config) + if err != nil { + glog.Fatal(err) + } + confClient, err := createConfigClient(config) + if err != nil { + glog.Fatal(err) + } constLabels := map[string]string{"class": *ingressClass} @@ -88,22 +95,35 @@ func main() { nginxManager, useFakeNginxManager := createNginxManager(managerCollector) - nginxVersion := getNginxVersionInfo(nginxManager) + nginxVersion, err := getNginxVersionInfo(nginxManager) + if err != nil { + glog.Fatal(err) + } var appProtectVersion string if *appProtect { - appProtectVersion = getAppProtectVersionInfo() + appProtectVersion, err = getAppProtectVersionInfo() + if err != nil { + glog.Fatal(err) + } } go updateSelfWithVersionInfo(kubeClient, version, appProtectVersion, nginxVersion, 10, time.Second*5) - templateExecutor, templateExecutorV2 := createTemplateExecutors() + templateExecutor := createV1TemplateExecutors() + templateExecutorV2 := createV2TemplateExecutors() aPPluginDone, aPPDosAgentDone := startApAgentsAndPlugins(nginxManager) - sslRejectHandshake := processDefaultServerSecret(kubeClient, nginxManager) + sslRejectHandshake, err := processDefaultServerSecret(kubeClient, nginxManager) + if err != nil { + glog.Fatal(err) + } - isWildcardEnabled := processWildcardSecret(kubeClient, nginxManager) + isWildcardEnabled, err := processWildcardSecret(kubeClient, nginxManager) + if err != nil { + glog.Fatal(err) + } globalConfigurationValidator := createGlobalConfigurationValidator() @@ -137,7 +157,10 @@ func main() { NginxVersion: nginxVersion, } - processNginxConfig(staticCfgParams, cfgParams, templateExecutor, nginxManager) + err = processNginxConfig(staticCfgParams, cfgParams, templateExecutor, nginxManager) + if err != nil { + glog.Fatal(err) + } if *enableTLSPassthrough { var emptyFile []byte @@ -333,29 +356,30 @@ func checkNamespaceExists(kubeClient kubernetes.Interface, namespaces []string) } } -func createCustomClients(config *rest.Config) (dynamic.Interface, k8s_nginx.Interface) { - var dynClient dynamic.Interface - var err error - if *appProtectDos || *appProtect || *ingressLink != "" { - dynClient, err = dynamic.NewForConfig(config) - if err != nil { - glog.Fatalf("Failed to create dynamic client: %v.", err) - } - } - var confClient k8s_nginx.Interface +func createConfigClient(config *rest.Config) (configClient k8s_nginx.Interface, err error) { if *enableCustomResources { - confClient, err = k8s_nginx.NewForConfig(config) + configClient, err = k8s_nginx.NewForConfig(config) if err != nil { - glog.Fatalf("Failed to create a conf client: %v", err) + return configClient, fmt.Errorf("Failed to create a conf client: %v", err) } // required for emitting Events for VirtualServer err = conf_scheme.AddToScheme(scheme.Scheme) if err != nil { - glog.Fatalf("Failed to add configuration types to the scheme: %v", err) + return configClient, fmt.Errorf("Failed to add configuration types to the scheme: %v", err) + } + } + return configClient, err +} + +func createDynamicClient(config *rest.Config) (dynClient dynamic.Interface, err error) { + if *appProtectDos || *appProtect || *ingressLink != "" { + dynClient, err = dynamic.NewForConfig(config) + if err != nil { + return dynClient, fmt.Errorf("Failed to create dynamic client: %v.", err) } } - return dynClient, confClient + return dynClient, err } func createPlusClient(nginxPlus bool, useFakeNginxManager bool, nginxManager nginx.Manager) (plusClient *client.NginxClient, err error) { @@ -370,16 +394,13 @@ func createPlusClient(nginxPlus bool, useFakeNginxManager bool, nginxManager ngi return plusClient, nil } -func createTemplateExecutors() (*version1.TemplateExecutor, *version2.TemplateExecutor) { +func createV1TemplateExecutors() *version1.TemplateExecutor { nginxConfTemplatePath := "nginx.tmpl" nginxIngressTemplatePath := "nginx.ingress.tmpl" - nginxVirtualServerTemplatePath := "nginx.virtualserver.tmpl" - nginxTransportServerTemplatePath := "nginx.transportserver.tmpl" + if *nginxPlus { nginxConfTemplatePath = "nginx-plus.tmpl" nginxIngressTemplatePath = "nginx-plus.ingress.tmpl" - nginxVirtualServerTemplatePath = "nginx-plus.virtualserver.tmpl" - nginxTransportServerTemplatePath = "nginx-plus.transportserver.tmpl" } if *mainTemplatePath != "" { @@ -388,6 +409,23 @@ func createTemplateExecutors() (*version1.TemplateExecutor, *version2.TemplateEx if *ingressTemplatePath != "" { nginxIngressTemplatePath = *ingressTemplatePath } + + templateExecutor, err := version1.NewTemplateExecutor(nginxConfTemplatePath, nginxIngressTemplatePath) + if err != nil { + glog.Fatalf("Error creating TemplateExecutor: %v", err) + } + + return templateExecutor +} + +func createV2TemplateExecutors() *version2.TemplateExecutor { + nginxVirtualServerTemplatePath := "nginx.virtualserver.tmpl" + nginxTransportServerTemplatePath := "nginx.transportserver.tmpl" + if *nginxPlus { + nginxVirtualServerTemplatePath = "nginx-plus.virtualserver.tmpl" + nginxTransportServerTemplatePath = "nginx-plus.transportserver.tmpl" + } + if *virtualServerTemplatePath != "" { nginxVirtualServerTemplatePath = *virtualServerTemplatePath } @@ -395,17 +433,12 @@ func createTemplateExecutors() (*version1.TemplateExecutor, *version2.TemplateEx nginxTransportServerTemplatePath = *transportServerTemplatePath } - templateExecutor, err := version1.NewTemplateExecutor(nginxConfTemplatePath, nginxIngressTemplatePath) - if err != nil { - glog.Fatalf("Error creating TemplateExecutor: %v", err) - } - templateExecutorV2, err := version2.NewTemplateExecutor(nginxVirtualServerTemplatePath, nginxTransportServerTemplatePath) if err != nil { glog.Fatalf("Error creating TemplateExecutorV2: %v", err) } - return templateExecutor, templateExecutorV2 + return templateExecutorV2 } func createNginxManager(managerCollector collectors.ManagerCollector) (nginx.Manager, bool) { @@ -420,26 +453,26 @@ func createNginxManager(managerCollector collectors.ManagerCollector) (nginx.Man return nginxManager, useFakeNginxManager } -func getNginxVersionInfo(nginxManager nginx.Manager) nginx.Version { - nginxInfo := nginxManager.Version() +func getNginxVersionInfo(nginxManager nginx.Manager) (nginxInfo nginx.Version, err error) { + nginxInfo = nginxManager.Version() glog.Infof("Using %s", nginxInfo.String()) if *nginxPlus && !nginxInfo.IsPlus { - glog.Fatal("NGINX Plus flag enabled (-nginx-plus) without NGINX Plus binary") + return nginxInfo, fmt.Errorf("NGINX Plus flag enabled (-nginx-plus) without NGINX Plus binary") } else if !*nginxPlus && nginxInfo.IsPlus { - glog.Fatal("NGINX Plus binary found without NGINX Plus flag (-nginx-plus)") + return nginxInfo, fmt.Errorf("NGINX Plus binary found without NGINX Plus flag (-nginx-plus)") } - return nginxInfo + return nginxInfo, err } -func getAppProtectVersionInfo() string { +func getAppProtectVersionInfo() (version string, err error) { v, err := os.ReadFile(appProtectVersionPath) if err != nil { - glog.Fatalf("Cannot detect the AppProtect version, %s", err.Error()) + return version, fmt.Errorf("Cannot detect the AppProtect version, %s", err.Error()) } - version := strings.TrimSpace(string(v)) + version = strings.TrimSpace(string(v)) glog.Infof("Using AppProtect Version %s", version) - return version + return version, err } func startApAgentsAndPlugins(nginxManager nginx.Manager) (chan error, chan error) { @@ -459,13 +492,12 @@ func startApAgentsAndPlugins(nginxManager nginx.Manager) (chan error, chan error return aPPluginDone, aPPDosAgentDone } -func processDefaultServerSecret(kubeClient *kubernetes.Clientset, nginxManager nginx.Manager) bool { - var sslRejectHandshake bool - +func processDefaultServerSecret(kubeClient *kubernetes.Clientset, nginxManager nginx.Manager) (sslRejectHandshake bool, err error) { + sslRejectHandshake = false if *defaultServerSecret != "" { secret, err := getAndValidateSecret(kubeClient, *defaultServerSecret) if err != nil { - glog.Fatalf("Error trying to get the default server TLS secret %v: %v", *defaultServerSecret, err) + return sslRejectHandshake, fmt.Errorf("Error trying to get the default server TLS secret %v: %v", *defaultServerSecret, err) } bytes := configs.GenerateCertAndKeyFileContent(secret) @@ -477,24 +509,26 @@ func processDefaultServerSecret(kubeClient *kubernetes.Clientset, nginxManager n // file doesn't exist - it is OK! we will reject TLS connections in the default server sslRejectHandshake = true } else { - glog.Fatalf("Error checking the default server TLS cert and key in %s: %v", configs.DefaultServerSecretPath, err) + return sslRejectHandshake, fmt.Errorf("Error checking the default server TLS cert and key in %s: %v", configs.DefaultServerSecretPath, err) } } } - return sslRejectHandshake + return sslRejectHandshake, nil } -func processWildcardSecret(kubeClient *kubernetes.Clientset, nginxManager nginx.Manager) bool { +func processWildcardSecret(kubeClient *kubernetes.Clientset, nginxManager nginx.Manager) (isWildcardTLSSecret bool, err error) { + isWildcardTLSSecret = false if *wildcardTLSSecret != "" { secret, err := getAndValidateSecret(kubeClient, *wildcardTLSSecret) if err != nil { - glog.Fatalf("Error trying to get the wildcard TLS secret %v: %v", *wildcardTLSSecret, err) + return isWildcardTLSSecret, fmt.Errorf("Error trying to get the wildcard TLS secret %v: %v", *wildcardTLSSecret, err) } bytes := configs.GenerateCertAndKeyFileContent(secret) nginxManager.CreateSecret(configs.WildcardSecretName, bytes, nginx.TLSSecretFileMode) } - return *wildcardTLSSecret != "" + isWildcardTLSSecret = *wildcardTLSSecret != "" + return isWildcardTLSSecret, nil } func createGlobalConfigurationValidator() *cr_validation.GlobalConfigurationValidator { @@ -521,11 +555,11 @@ func createGlobalConfigurationValidator() *cr_validation.GlobalConfigurationVali return cr_validation.NewGlobalConfigurationValidator(forbiddenListenerPorts) } -func processNginxConfig(staticCfgParams *configs.StaticConfigParams, cfgParams *configs.ConfigParams, templateExecutor *version1.TemplateExecutor, nginxManager nginx.Manager) { +func processNginxConfig(staticCfgParams *configs.StaticConfigParams, cfgParams *configs.ConfigParams, templateExecutor *version1.TemplateExecutor, nginxManager nginx.Manager) (err error) { ngxConfig := configs.GenerateNginxMainConfig(staticCfgParams, cfgParams) content, err := templateExecutor.ExecuteMainConfigTemplate(ngxConfig) if err != nil { - glog.Fatalf("Error generating NGINX main config: %v", err) + return fmt.Errorf("Error generating NGINX main config: %v", err) } nginxManager.CreateMainConfig(content) @@ -536,9 +570,10 @@ func processNginxConfig(staticCfgParams *configs.StaticConfigParams, cfgParams * if ngxConfig.OpenTracingLoadModule { err := nginxManager.CreateOpenTracingTracerConfig(cfgParams.MainOpenTracingTracerConfig) if err != nil { - glog.Fatalf("Error creating OpenTracing tracer config file: %v", err) + return fmt.Errorf("Error creating OpenTracing tracer config file: %v", err) } } + return err } func handleTermination(lbc *k8s.LoadBalancerController, nginxManager nginx.Manager, listener metrics.SyslogListener, nginxDone chan error) { From 4de9b371879505f72cd0f66b3f8786c567c050b5 Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Mon, 26 Feb 2024 16:34:56 +1300 Subject: [PATCH 05/26] Issue 4837: chore: main.go refactor The objective of this change is to make functions testable and/or easy to read * replace fatal errors with formatted errors to be returned to the caller for error handling * split functions where appropriate --- cmd/nginx-ingress/main.go | 151 +++++++++++++++++++++++--------------- 1 file changed, 92 insertions(+), 59 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 8f76dbfac6..4fae85a869 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -68,7 +68,7 @@ func main() { glog.Fatalf("Failed to create client: %v.", err) } - err = confirmMinimumkubernetesVersionCriteria(kubeClient) + err = confirmMinimumK8sVersionCriteria(kubeClient) if err != nil { glog.Fatal(err) } @@ -110,8 +110,15 @@ func main() { go updateSelfWithVersionInfo(kubeClient, version, appProtectVersion, nginxVersion, 10, time.Second*5) - templateExecutor := createV1TemplateExecutors() - templateExecutorV2 := createV2TemplateExecutors() + templateExecutor, err := createV1TemplateExecutors() + if err != nil { + glog.Fatal(err) + } + + templateExecutorV2, err := createV2TemplateExecutors() + if err != nil { + glog.Fatal(err) + } aPPluginDone, aPPDosAgentDone := startApAgentsAndPlugins(nginxManager) @@ -127,10 +134,16 @@ func main() { globalConfigurationValidator := createGlobalConfigurationValidator() - processGlobalConfiguration() + err = processGlobalConfiguration() + if err != nil { + glog.Fatal(err) + } cfgParams := configs.NewDefaultConfigParams(*nginxPlus) - cfgParams = processConfigMaps(kubeClient, cfgParams, nginxManager, templateExecutor) + cfgParams, err = processConfigMaps(kubeClient, cfgParams, nginxManager, templateExecutor) + if err != nil { + glog.Fatal(err) + } staticCfgParams := &configs.StaticConfigParams{ DisableIPV6: *disableIPV6, @@ -197,8 +210,9 @@ func main() { transportServerValidator := cr_validation.NewTransportServerValidator(*enableTLSPassthrough, *enableSnippets, *nginxPlus) virtualServerValidator := cr_validation.NewVirtualServerValidator(cr_validation.IsPlus(*nginxPlus), cr_validation.IsDosEnabled(*appProtectDos), cr_validation.IsCertManagerEnabled(*enableCertManager), cr_validation.IsExternalDNSEnabled(*enableExternalDNS)) - if *enableServiceInsight { - createHealthProbeEndpoint(kubeClient, plusClient, cnf) + err = createHealthProbeEndpoint(kubeClient, plusClient, cnf) + if err != nil { + glog.Fatal(err) } lbcInput := k8s.NewLoadBalancerControllerInput{ @@ -291,7 +305,7 @@ func getKubeClient(config *rest.Config) (kubeClient *kubernetes.Clientset, err e return kubeClient, err } -func confirmMinimumkubernetesVersionCriteria(kubeClient kubernetes.Interface) (err error) { +func confirmMinimumK8sVersionCriteria(kubeClient kubernetes.Interface) (err error) { k8sVersion, err := k8s.GetK8sVersion(kubeClient) if err != nil { return fmt.Errorf("error retrieving k8s version: %v", err) @@ -394,7 +408,7 @@ func createPlusClient(nginxPlus bool, useFakeNginxManager bool, nginxManager ngi return plusClient, nil } -func createV1TemplateExecutors() *version1.TemplateExecutor { +func createV1TemplateExecutors() (templateExecutor *version1.TemplateExecutor, err error) { nginxConfTemplatePath := "nginx.tmpl" nginxIngressTemplatePath := "nginx.ingress.tmpl" @@ -410,15 +424,16 @@ func createV1TemplateExecutors() *version1.TemplateExecutor { nginxIngressTemplatePath = *ingressTemplatePath } - templateExecutor, err := version1.NewTemplateExecutor(nginxConfTemplatePath, nginxIngressTemplatePath) + templateExecutor, err = version1.NewTemplateExecutor(nginxConfTemplatePath, nginxIngressTemplatePath) if err != nil { - glog.Fatalf("Error creating TemplateExecutor: %v", err) + + return nil, fmt.Errorf("Error creating TemplateExecutor: %v", err) } - return templateExecutor + return templateExecutor, nil } -func createV2TemplateExecutors() *version2.TemplateExecutor { +func createV2TemplateExecutors() (templateExecutorV2 *version2.TemplateExecutor, err error) { nginxVirtualServerTemplatePath := "nginx.virtualserver.tmpl" nginxTransportServerTemplatePath := "nginx.transportserver.tmpl" if *nginxPlus { @@ -433,12 +448,12 @@ func createV2TemplateExecutors() *version2.TemplateExecutor { nginxTransportServerTemplatePath = *transportServerTemplatePath } - templateExecutorV2, err := version2.NewTemplateExecutor(nginxVirtualServerTemplatePath, nginxTransportServerTemplatePath) + templateExecutorV2, err = version2.NewTemplateExecutor(nginxVirtualServerTemplatePath, nginxTransportServerTemplatePath) if err != nil { - glog.Fatalf("Error creating TemplateExecutorV2: %v", err) + return nil, fmt.Errorf("Error creating TemplateExecutorV2: %v", err) } - return templateExecutorV2 + return templateExecutorV2, nil } func createNginxManager(managerCollector collectors.ManagerCollector) (nginx.Manager, bool) { @@ -724,13 +739,26 @@ func createPlusAndLatencyCollectors( kubeClient *kubernetes.Clientset, plusClient *client.NginxClient, isMesh bool, -) (*nginxCollector.NginxPlusCollector, metrics.SyslogListener, collectors.LatencyCollector) { +) (plusCollector *nginxCollector.NginxPlusCollector, syslogListener metrics.SyslogListener, lc collectors.LatencyCollector) { + + if *enablePrometheusMetrics { + upstreamServerVariableLabels := []string{"service", "resource_type", "resource_name", "resource_namespace"} + upstreamServerPeerVariableLabelNames := []string{"pod_name"} + if isMesh { + upstreamServerPeerVariableLabelNames = append(upstreamServerPeerVariableLabelNames, "pod_owner") + } + + plusCollector = createNginxPlusCollector(registry, constLabels, kubeClient, plusClient, upstreamServerVariableLabels, upstreamServerPeerVariableLabelNames) + syslogListener, lc = createLatencyCollector(registry, constLabels, upstreamServerVariableLabels, upstreamServerPeerVariableLabelNames) + } + + return plusCollector, syslogListener, lc +} + +func createNginxPlusCollector(registry *prometheus.Registry, constLabels map[string]string, kubeClient *kubernetes.Clientset, plusClient *client.NginxClient, upstreamServerVariableLabels []string, upstreamServerPeerVariableLabelNames []string) *nginxCollector.NginxPlusCollector { + var plusCollector *nginxCollector.NginxPlusCollector var prometheusSecret *api_v1.Secret var err error - var lc collectors.LatencyCollector - lc = collectors.NewLatencyFakeCollector() - var syslogListener metrics.SyslogListener - syslogListener = metrics.NewSyslogFakeServer() if *prometheusTLSSecretName != "" { prometheusSecret, err = getAndValidateSecret(kubeClient, *prometheusTLSSecretName) @@ -739,30 +767,34 @@ func createPlusAndLatencyCollectors( } } - var plusCollector *nginxCollector.NginxPlusCollector + if *nginxPlus { + streamUpstreamServerVariableLabels := []string{"service", "resource_type", "resource_name", "resource_namespace"} + streamUpstreamServerPeerVariableLabelNames := []string{"pod_name"} + + serverZoneVariableLabels := []string{"resource_type", "resource_name", "resource_namespace"} + streamServerZoneVariableLabels := []string{"resource_type", "resource_name", "resource_namespace"} + variableLabelNames := nginxCollector.NewVariableLabelNames(upstreamServerVariableLabels, serverZoneVariableLabels, upstreamServerPeerVariableLabelNames, + streamUpstreamServerVariableLabels, streamServerZoneVariableLabels, streamUpstreamServerPeerVariableLabelNames, nil, nil) + + promlogConfig := &promlog.Config{} + logger := promlog.New(promlogConfig) + plusCollector = nginxCollector.NewNginxPlusCollector(plusClient, "nginx_ingress_nginxplus", variableLabelNames, constLabels, logger) + go metrics.RunPrometheusListenerForNginxPlus(*prometheusMetricsListenPort, plusCollector, registry, prometheusSecret) + } else { + httpClient := getSocketClient("/var/lib/nginx/nginx-status.sock") + client := metrics.NewNginxMetricsClient(httpClient) + go metrics.RunPrometheusListenerForNginx(*prometheusMetricsListenPort, client, registry, constLabels, prometheusSecret) + } + return plusCollector +} + +func createLatencyCollector(registry *prometheus.Registry, constLabels map[string]string, upstreamServerVariableLabels []string, upstreamServerPeerVariableLabelNames []string) (metrics.SyslogListener, collectors.LatencyCollector) { + var lc collectors.LatencyCollector + lc = collectors.NewLatencyFakeCollector() + var syslogListener metrics.SyslogListener + syslogListener = metrics.NewSyslogFakeServer() + if *enablePrometheusMetrics { - upstreamServerVariableLabels := []string{"service", "resource_type", "resource_name", "resource_namespace"} - upstreamServerPeerVariableLabelNames := []string{"pod_name"} - if isMesh { - upstreamServerPeerVariableLabelNames = append(upstreamServerPeerVariableLabelNames, "pod_owner") - } - if *nginxPlus { - streamUpstreamServerVariableLabels := []string{"service", "resource_type", "resource_name", "resource_namespace"} - streamUpstreamServerPeerVariableLabelNames := []string{"pod_name"} - - serverZoneVariableLabels := []string{"resource_type", "resource_name", "resource_namespace"} - streamServerZoneVariableLabels := []string{"resource_type", "resource_name", "resource_namespace"} - variableLabelNames := nginxCollector.NewVariableLabelNames(upstreamServerVariableLabels, serverZoneVariableLabels, upstreamServerPeerVariableLabelNames, - streamUpstreamServerVariableLabels, streamServerZoneVariableLabels, streamUpstreamServerPeerVariableLabelNames, nil, nil) - promlogConfig := &promlog.Config{} - logger := promlog.New(promlogConfig) - plusCollector = nginxCollector.NewNginxPlusCollector(plusClient, "nginx_ingress_nginxplus", variableLabelNames, constLabels, logger) - go metrics.RunPrometheusListenerForNginxPlus(*prometheusMetricsListenPort, plusCollector, registry, prometheusSecret) - } else { - httpClient := getSocketClient("/var/lib/nginx/nginx-status.sock") - client := metrics.NewNginxMetricsClient(httpClient) - go metrics.RunPrometheusListenerForNginx(*prometheusMetricsListenPort, client, registry, constLabels, prometheusSecret) - } if *enableLatencyMetrics { lc = collectors.NewLatencyMetricsCollector(constLabels, upstreamServerVariableLabels, upstreamServerPeerVariableLabelNames) if err := lc.Register(registry); err != nil { @@ -773,53 +805,54 @@ func createPlusAndLatencyCollectors( } } - return plusCollector, syslogListener, lc + return syslogListener, lc } -func createHealthProbeEndpoint(kubeClient *kubernetes.Clientset, plusClient *client.NginxClient, cnf *configs.Configurator) { +func createHealthProbeEndpoint(kubeClient *kubernetes.Clientset, plusClient *client.NginxClient, cnf *configs.Configurator) (err error) { if !*enableServiceInsight { - return + return nil } var serviceInsightSecret *api_v1.Secret - var err error if *serviceInsightTLSSecretName != "" { serviceInsightSecret, err = getAndValidateSecret(kubeClient, *serviceInsightTLSSecretName) if err != nil { - glog.Fatalf("Error trying to get the service insight TLS secret %v: %v", *serviceInsightTLSSecretName, err) + return fmt.Errorf("Error trying to get the service insight TLS secret %v: %v", *serviceInsightTLSSecretName, err) } } go healthcheck.RunHealthCheck(*serviceInsightListenPort, plusClient, cnf, serviceInsightSecret) + return nil } -func processGlobalConfiguration() { +func processGlobalConfiguration() (err error) { if *globalConfiguration != "" { _, _, err := k8s.ParseNamespaceName(*globalConfiguration) if err != nil { - glog.Fatalf("Error parsing the global-configuration argument: %v", err) + return fmt.Errorf("Error parsing the global-configuration argument: %v", err) } if !*enableCustomResources { - glog.Fatal("global-configuration flag requires -enable-custom-resources") + return fmt.Errorf("global-configuration flag requires -enable-custom-resources") } } + return nil } -func processConfigMaps(kubeClient *kubernetes.Clientset, cfgParams *configs.ConfigParams, nginxManager nginx.Manager, templateExecutor *version1.TemplateExecutor) *configs.ConfigParams { +func processConfigMaps(kubeClient *kubernetes.Clientset, cfgParams *configs.ConfigParams, nginxManager nginx.Manager, templateExecutor *version1.TemplateExecutor) (*configs.ConfigParams, error) { if *nginxConfigMaps != "" { ns, name, err := k8s.ParseNamespaceName(*nginxConfigMaps) if err != nil { - glog.Fatalf("Error parsing the nginx-configmaps argument: %v", err) + return nil, fmt.Errorf("Error parsing the nginx-configmaps argument: %v", err) } cfm, err := kubeClient.CoreV1().ConfigMaps(ns).Get(context.TODO(), name, meta_v1.GetOptions{}) if err != nil { - glog.Fatalf("Error when getting %v: %v", *nginxConfigMaps, err) + return nil, fmt.Errorf("Error when getting %v: %v", *nginxConfigMaps, err) } cfgParams = configs.ParseConfigMap(cfm, *nginxPlus, *appProtect, *appProtectDos, *enableTLSPassthrough) if cfgParams.MainServerSSLDHParamFileContent != nil { fileName, err := nginxManager.CreateDHParam(*cfgParams.MainServerSSLDHParamFileContent) if err != nil { - glog.Fatalf("Configmap %s/%s: Could not update dhparams: %v", ns, name, err) + return nil, fmt.Errorf("Configmap %s/%s: Could not update dhparams: %v", ns, name, err) } else { cfgParams.MainServerSSLDHParam = fileName } @@ -827,17 +860,17 @@ func processConfigMaps(kubeClient *kubernetes.Clientset, cfgParams *configs.Conf if cfgParams.MainTemplate != nil { err = templateExecutor.UpdateMainTemplate(cfgParams.MainTemplate) if err != nil { - glog.Fatalf("Error updating NGINX main template: %v", err) + return nil, fmt.Errorf("Error updating NGINX main template: %v", err) } } if cfgParams.IngressTemplate != nil { err = templateExecutor.UpdateIngressTemplate(cfgParams.IngressTemplate) if err != nil { - glog.Fatalf("Error updating ingress template: %v", err) + return nil, fmt.Errorf("Error updating ingress template: %v", err) } } } - return cfgParams + return cfgParams, nil } func updateSelfWithVersionInfo(kubeClient *kubernetes.Clientset, version, appProtectVersion string, nginxVersion nginx.Version, maxRetries int, waitTime time.Duration) { From 8ae17f9dda1b35dfdc854f165ef5b237b83fb471 Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Mon, 26 Feb 2024 20:37:00 +1300 Subject: [PATCH 06/26] Issue 4837: chore: main.go refactor Fix build failure introduced while resolving conflict following merge from main --- cmd/nginx-ingress/main.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 6251737722..39e4e8fd28 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -120,8 +120,6 @@ func main() { glog.Fatal(err) } - aPPluginDone, aPPDosAgentDone := startApAgentsAndPlugins(nginxManager) - sslRejectHandshake, err := processDefaultServerSecret(kubeClient, nginxManager) if err != nil { glog.Fatal(err) From 7b881415b9dcd807d627473e67eda2093ebe543d Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Mon, 26 Feb 2024 22:24:51 +1300 Subject: [PATCH 07/26] chore: This is pre-commit install From 8a0f5d8dbd6136c3b263cadf4d669918c4038890 Mon Sep 17 00:00:00 2001 From: Madhu Rajagopal <m.rajagopal@f5.com> Date: Tue, 27 Feb 2024 09:28:57 +1300 Subject: [PATCH 08/26] Update cmd/nginx-ingress/main.go Accept review comments regarding output parameter name. Co-authored-by: Shaun <s.odonovan@f5.com> Signed-off-by: Madhu Rajagopal <m.rajagopal@f5.com> --- cmd/nginx-ingress/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 39e4e8fd28..4f78839017 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -725,7 +725,7 @@ func createPlusAndLatencyCollectors( kubeClient *kubernetes.Clientset, plusClient *client.NginxClient, isMesh bool, -) (plusCollector *nginxCollector.NginxPlusCollector, syslogListener metrics.SyslogListener, lc collectors.LatencyCollector) { +) (plusCollector *nginxCollector.NginxPlusCollector, syslogListener metrics.SyslogListener, latencyCollector collectors.LatencyCollector) { if *enablePrometheusMetrics { upstreamServerVariableLabels := []string{"service", "resource_type", "resource_name", "resource_namespace"} From 2162fd731b7986f2e8d3fc162aaec08712278b16 Mon Sep 17 00:00:00 2001 From: Madhu Rajagopal <m.rajagopal@f5.com> Date: Tue, 27 Feb 2024 09:30:09 +1300 Subject: [PATCH 09/26] Update cmd/nginx-ingress/main.go Adopt review comments regarding golang error handling. Co-authored-by: Shaun <s.odonovan@f5.com> Signed-off-by: Madhu Rajagopal <m.rajagopal@f5.com> --- cmd/nginx-ingress/main.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 4f78839017..a9cc80866c 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -68,8 +68,7 @@ func main() { glog.Fatalf("Failed to create client: %v.", err) } - err = confirmMinimumK8sVersionCriteria(kubeClient) - if err != nil { + if err := confirmMinimumK8sVersionCriteria(kubeClient); err != nil { glog.Fatal(err) } From 4b456e63e6d4ffd5dd73dce7c9f25610c6de7453 Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Tue, 27 Feb 2024 10:29:51 +1300 Subject: [PATCH 10/26] Issue 4837: chore: main.go refactor * Fix build failure introduced after merging reviewed code changes * Address linter errors highlighted as part of pre-commit hook checks --- cmd/nginx-ingress/main.go | 58 +++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 30 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index a9cc80866c..fc8adb0ba7 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -305,17 +305,17 @@ func getKubeClient(config *rest.Config) (kubeClient *kubernetes.Clientset, err e func confirmMinimumK8sVersionCriteria(kubeClient kubernetes.Interface) (err error) { k8sVersion, err := k8s.GetK8sVersion(kubeClient) if err != nil { - return fmt.Errorf("error retrieving k8s version: %v", err) + return fmt.Errorf("error retrieving k8s version: %w", err) } glog.Infof("Kubernetes version: %v", k8sVersion) minK8sVersion, err := util_version.ParseGeneric("1.22.0") if err != nil { - return fmt.Errorf("unexpected error parsing minimum supported version: %v", err) + return fmt.Errorf("unexpected error parsing minimum supported version: %w", err) } if !k8sVersion.AtLeast(minK8sVersion) { - return fmt.Errorf("Versions of Kubernetes < %v are not supported, please refer to the documentation for details on supported versions and legacy controller support", minK8sVersion) + return fmt.Errorf("versions of kubernetes < %v are not supported, please refer to the documentation for details on supported versions and legacy controller support", minK8sVersion) } return err } @@ -323,11 +323,11 @@ func confirmMinimumK8sVersionCriteria(kubeClient kubernetes.Interface) (err erro func validateIngressClass(kubeClient kubernetes.Interface) (err error) { ingressClassRes, err := kubeClient.NetworkingV1().IngressClasses().Get(context.TODO(), *ingressClass, meta_v1.GetOptions{}) if err != nil { - return fmt.Errorf("Error when getting IngressClass %v: %v", *ingressClass, err) + return fmt.Errorf("error when getting IngressClass %v: %w", *ingressClass, err) } if ingressClassRes.Spec.Controller != k8s.IngressControllerName { - return fmt.Errorf("IngressClass with name %v has an invalid Spec.Controller %v; expected %v", ingressClassRes.Name, ingressClassRes.Spec.Controller, k8s.IngressControllerName) + return fmt.Errorf("ingressClass with name %v has an invalid Spec.Controller %v; expected %v", ingressClassRes.Name, ingressClassRes.Spec.Controller, k8s.IngressControllerName) } return err @@ -371,13 +371,13 @@ func createConfigClient(config *rest.Config) (configClient k8s_nginx.Interface, if *enableCustomResources { configClient, err = k8s_nginx.NewForConfig(config) if err != nil { - return configClient, fmt.Errorf("Failed to create a conf client: %v", err) + return configClient, fmt.Errorf("failed to create a conf client: %w", err) } // required for emitting Events for VirtualServer err = conf_scheme.AddToScheme(scheme.Scheme) if err != nil { - return configClient, fmt.Errorf("Failed to add configuration types to the scheme: %v", err) + return configClient, fmt.Errorf("failed to add configuration types to the scheme: %w", err) } } return configClient, err @@ -387,7 +387,7 @@ func createDynamicClient(config *rest.Config) (dynClient dynamic.Interface, err if *appProtectDos || *appProtect || *ingressLink != "" { dynClient, err = dynamic.NewForConfig(config) if err != nil { - return dynClient, fmt.Errorf("Failed to create dynamic client: %v.", err) + return dynClient, fmt.Errorf("failed to create dynamic client: %w", err) } } return dynClient, err @@ -398,7 +398,7 @@ func createPlusClient(nginxPlus bool, useFakeNginxManager bool, nginxManager ngi httpClient := getSocketClient("/var/lib/nginx/nginx-plus-api.sock") plusClient, err = client.NewNginxClient("http://nginx-plus-api/api", client.WithHTTPClient(httpClient)) if err != nil { - return plusClient, fmt.Errorf("Failed to create NginxClient for Plus: %v", err) + return plusClient, fmt.Errorf("failed to create NginxClient for Plus: %w", err) } nginxManager.SetPlusClients(plusClient, httpClient) } @@ -423,8 +423,7 @@ func createV1TemplateExecutors() (templateExecutor *version1.TemplateExecutor, e templateExecutor, err = version1.NewTemplateExecutor(nginxConfTemplatePath, nginxIngressTemplatePath) if err != nil { - - return nil, fmt.Errorf("Error creating TemplateExecutor: %v", err) + return nil, fmt.Errorf("error creating TemplateExecutor: %w", err) } return templateExecutor, nil @@ -447,7 +446,7 @@ func createV2TemplateExecutors() (templateExecutorV2 *version2.TemplateExecutor, templateExecutorV2, err = version2.NewTemplateExecutor(nginxVirtualServerTemplatePath, nginxTransportServerTemplatePath) if err != nil { - return nil, fmt.Errorf("Error creating TemplateExecutorV2: %v", err) + return nil, fmt.Errorf("error creating TemplateExecutorV2: %w", err) } return templateExecutorV2, nil @@ -470,9 +469,9 @@ func getNginxVersionInfo(nginxManager nginx.Manager) (nginxInfo nginx.Version, e glog.Infof("Using %s", nginxInfo.String()) if *nginxPlus && !nginxInfo.IsPlus { - return nginxInfo, fmt.Errorf("NGINX Plus flag enabled (-nginx-plus) without NGINX Plus binary") + return nginxInfo, fmt.Errorf("the NGINX Plus flag is enabled (-nginx-plus) without NGINX Plus binary") } else if !*nginxPlus && nginxInfo.IsPlus { - return nginxInfo, fmt.Errorf("NGINX Plus binary found without NGINX Plus flag (-nginx-plus)") + return nginxInfo, fmt.Errorf("found NGINX Plus binary but without NGINX Plus flag (-nginx-plus)") } return nginxInfo, err } @@ -480,7 +479,7 @@ func getNginxVersionInfo(nginxManager nginx.Manager) (nginxInfo nginx.Version, e func getAppProtectVersionInfo() (version string, err error) { v, err := os.ReadFile(appProtectVersionPath) if err != nil { - return version, fmt.Errorf("Cannot detect the AppProtect version, %s", err.Error()) + return version, fmt.Errorf("cannot detect the AppProtect version, %s", err.Error()) } version = strings.TrimSpace(string(v)) glog.Infof("Using AppProtect Version %s", version) @@ -527,7 +526,7 @@ func processDefaultServerSecret(kubeClient *kubernetes.Clientset, nginxManager n if *defaultServerSecret != "" { secret, err := getAndValidateSecret(kubeClient, *defaultServerSecret) if err != nil { - return sslRejectHandshake, fmt.Errorf("Error trying to get the default server TLS secret %v: %v", *defaultServerSecret, err) + return sslRejectHandshake, fmt.Errorf("error trying to get the default server TLS secret %v: %w", *defaultServerSecret, err) } bytes := configs.GenerateCertAndKeyFileContent(secret) @@ -539,7 +538,7 @@ func processDefaultServerSecret(kubeClient *kubernetes.Clientset, nginxManager n // file doesn't exist - it is OK! we will reject TLS connections in the default server sslRejectHandshake = true } else { - return sslRejectHandshake, fmt.Errorf("Error checking the default server TLS cert and key in %s: %v", configs.DefaultServerSecretPath, err) + return sslRejectHandshake, fmt.Errorf("error checking the default server TLS cert and key in %s: %w", configs.DefaultServerSecretPath, err) } } } @@ -551,7 +550,7 @@ func processWildcardSecret(kubeClient *kubernetes.Clientset, nginxManager nginx. if *wildcardTLSSecret != "" { secret, err := getAndValidateSecret(kubeClient, *wildcardTLSSecret) if err != nil { - return isWildcardTLSSecret, fmt.Errorf("Error trying to get the wildcard TLS secret %v: %v", *wildcardTLSSecret, err) + return isWildcardTLSSecret, fmt.Errorf("error trying to get the wildcard TLS secret %v: %w", *wildcardTLSSecret, err) } bytes := configs.GenerateCertAndKeyFileContent(secret) @@ -589,7 +588,7 @@ func processNginxConfig(staticCfgParams *configs.StaticConfigParams, cfgParams * ngxConfig := configs.GenerateNginxMainConfig(staticCfgParams, cfgParams) content, err := templateExecutor.ExecuteMainConfigTemplate(ngxConfig) if err != nil { - return fmt.Errorf("Error generating NGINX main config: %v", err) + return fmt.Errorf("error generating NGINX main config: %w", err) } nginxManager.CreateMainConfig(content) @@ -600,7 +599,7 @@ func processNginxConfig(staticCfgParams *configs.StaticConfigParams, cfgParams * if ngxConfig.OpenTracingLoadModule { err := nginxManager.CreateOpenTracingTracerConfig(cfgParams.MainOpenTracingTracerConfig) if err != nil { - return fmt.Errorf("Error creating OpenTracing tracer config file: %v", err) + return fmt.Errorf("error creating OpenTracing tracer config file: %w", err) } } return err @@ -725,7 +724,6 @@ func createPlusAndLatencyCollectors( plusClient *client.NginxClient, isMesh bool, ) (plusCollector *nginxCollector.NginxPlusCollector, syslogListener metrics.SyslogListener, latencyCollector collectors.LatencyCollector) { - if *enablePrometheusMetrics { upstreamServerVariableLabels := []string{"service", "resource_type", "resource_name", "resource_namespace"} upstreamServerPeerVariableLabelNames := []string{"pod_name"} @@ -734,10 +732,10 @@ func createPlusAndLatencyCollectors( } plusCollector = createNginxPlusCollector(registry, constLabels, kubeClient, plusClient, upstreamServerVariableLabels, upstreamServerPeerVariableLabelNames) - syslogListener, lc = createLatencyCollector(registry, constLabels, upstreamServerVariableLabels, upstreamServerPeerVariableLabelNames) + syslogListener, latencyCollector = createLatencyCollector(registry, constLabels, upstreamServerVariableLabels, upstreamServerPeerVariableLabelNames) } - return plusCollector, syslogListener, lc + return plusCollector, syslogListener, latencyCollector } func createNginxPlusCollector(registry *prometheus.Registry, constLabels map[string]string, kubeClient *kubernetes.Clientset, plusClient *client.NginxClient, upstreamServerVariableLabels []string, upstreamServerPeerVariableLabelNames []string) *nginxCollector.NginxPlusCollector { @@ -802,7 +800,7 @@ func createHealthProbeEndpoint(kubeClient *kubernetes.Clientset, plusClient *cli if *serviceInsightTLSSecretName != "" { serviceInsightSecret, err = getAndValidateSecret(kubeClient, *serviceInsightTLSSecretName) if err != nil { - return fmt.Errorf("Error trying to get the service insight TLS secret %v: %v", *serviceInsightTLSSecretName, err) + return fmt.Errorf("error trying to get the service insight TLS secret %v: %w", *serviceInsightTLSSecretName, err) } } go healthcheck.RunHealthCheck(*serviceInsightListenPort, plusClient, cnf, serviceInsightSecret) @@ -813,7 +811,7 @@ func processGlobalConfiguration() (err error) { if *globalConfiguration != "" { _, _, err := k8s.ParseNamespaceName(*globalConfiguration) if err != nil { - return fmt.Errorf("Error parsing the global-configuration argument: %v", err) + return fmt.Errorf("error parsing the global-configuration argument: %w", err) } if !*enableCustomResources { @@ -827,17 +825,17 @@ func processConfigMaps(kubeClient *kubernetes.Clientset, cfgParams *configs.Conf if *nginxConfigMaps != "" { ns, name, err := k8s.ParseNamespaceName(*nginxConfigMaps) if err != nil { - return nil, fmt.Errorf("Error parsing the nginx-configmaps argument: %v", err) + return nil, fmt.Errorf("error parsing the nginx-configmaps argument: %w", err) } cfm, err := kubeClient.CoreV1().ConfigMaps(ns).Get(context.TODO(), name, meta_v1.GetOptions{}) if err != nil { - return nil, fmt.Errorf("Error when getting %v: %v", *nginxConfigMaps, err) + return nil, fmt.Errorf("error when getting %v: %w", *nginxConfigMaps, err) } cfgParams = configs.ParseConfigMap(cfm, *nginxPlus, *appProtect, *appProtectDos, *enableTLSPassthrough) if cfgParams.MainServerSSLDHParamFileContent != nil { fileName, err := nginxManager.CreateDHParam(*cfgParams.MainServerSSLDHParamFileContent) if err != nil { - return nil, fmt.Errorf("Configmap %s/%s: Could not update dhparams: %v", ns, name, err) + return nil, fmt.Errorf("configmap %s/%s: Could not update dhparams: %w", ns, name, err) } else { cfgParams.MainServerSSLDHParam = fileName } @@ -845,13 +843,13 @@ func processConfigMaps(kubeClient *kubernetes.Clientset, cfgParams *configs.Conf if cfgParams.MainTemplate != nil { err = templateExecutor.UpdateMainTemplate(cfgParams.MainTemplate) if err != nil { - return nil, fmt.Errorf("Error updating NGINX main template: %v", err) + return nil, fmt.Errorf("error updating NGINX main template: %w", err) } } if cfgParams.IngressTemplate != nil { err = templateExecutor.UpdateIngressTemplate(cfgParams.IngressTemplate) if err != nil { - return nil, fmt.Errorf("Error updating ingress template: %v", err) + return nil, fmt.Errorf("error updating ingress template: %w", err) } } } From 668c7d31925ae29045671c7aafc73b7300ecd5df Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Tue, 27 Feb 2024 14:32:10 +1300 Subject: [PATCH 11/26] Issue 4837: chore: main.go refactor * Reduce the scope of the error returned as the error that is used here is never re-used outside the scope of the return --- cmd/nginx-ingress/main.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index fc8adb0ba7..885b136225 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -72,8 +72,7 @@ func main() { glog.Fatal(err) } - err = validateIngressClass(kubeClient) - if err != nil { + if err := validateIngressClass(kubeClient); err != nil { glog.Fatal(err) } @@ -131,8 +130,7 @@ func main() { globalConfigurationValidator := createGlobalConfigurationValidator() - err = processGlobalConfiguration() - if err != nil { + if err := processGlobalConfiguration(); err != nil { glog.Fatal(err) } @@ -167,8 +165,7 @@ func main() { NginxVersion: nginxVersion, } - err = processNginxConfig(staticCfgParams, cfgParams, templateExecutor, nginxManager) - if err != nil { + if err := processNginxConfig(staticCfgParams, cfgParams, templateExecutor, nginxManager); err != nil { glog.Fatal(err) } @@ -211,8 +208,7 @@ func main() { cr_validation.IsExternalDNSEnabled(*enableExternalDNS), ) - err = createHealthProbeEndpoint(kubeClient, plusClient, cnf) - if err != nil { + if err := createHealthProbeEndpoint(kubeClient, plusClient, cnf); err != nil { glog.Fatal(err) } From e9e37e92fd105af5bb56db588dc7a1d83ed18da2 Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Wed, 28 Feb 2024 11:07:25 +1300 Subject: [PATCH 12/26] Issue 4837: chore: main.go refactor * Add function header info where appropriate --- cmd/nginx-ingress/main.go | 47 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 885b136225..24bde7f42c 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -52,6 +52,7 @@ const ( appProtectVersionPath = "/opt/app_protect/VERSION" ) +// This is the start of the program func main() { commitHash, commitTime, dirtyBuild := getBuildInfo() fmt.Printf("NGINX Ingress Controller Version=%v Commit=%v Date=%v DirtyState=%v Arch=%v/%v Go=%v\n", version, commitHash, commitTime, dirtyBuild, runtime.GOOS, runtime.GOARCH, runtime.Version()) @@ -277,6 +278,7 @@ func main() { } } +// This function returns a k8s client object configuration func getClientConfig() (config *rest.Config, err error) { if *proxyURL != "" { config, err = clientcmd.NewNonInteractiveDeferredLoadingClientConfig( @@ -293,11 +295,14 @@ func getClientConfig() (config *rest.Config, err error) { return config, err } +// This returns a k8s client with the provided client config for interacting with the k8s API func getKubeClient(config *rest.Config) (kubeClient *kubernetes.Clientset, err error) { kubeClient, err = kubernetes.NewForConfig(config) return kubeClient, err } +// This function checks that KIC is running on at least a prescribed minimum k8s version or higher for supportability +// Anything lower throws than the prescribed version, an error is returned to the caller func confirmMinimumK8sVersionCriteria(kubeClient kubernetes.Interface) (err error) { k8sVersion, err := k8s.GetK8sVersion(kubeClient) if err != nil { @@ -316,6 +321,13 @@ func confirmMinimumK8sVersionCriteria(kubeClient kubernetes.Interface) (err erro return err } +// An Ingress resource can target a specific Ingress controller instance. +// This is useful when running multiple ingress controllers in the same cluster. +// Targeting an Ingress controller means only a specific controller should handle/implement the ingress resource. +// This can be done using either the IngressClassName field or the ingress.class annotation +// This function confirms that the Ingress resource is meant to be handled by NGINX Ingress Controller. +// Otherwise an error is returned to the caller +// This is defined in the const k8s.IngressControllerName func validateIngressClass(kubeClient kubernetes.Interface) (err error) { ingressClassRes, err := kubeClient.NetworkingV1().IngressClasses().Get(context.TODO(), *ingressClass, meta_v1.GetOptions{}) if err != nil { @@ -329,6 +341,10 @@ func validateIngressClass(kubeClient kubernetes.Interface) (err error) { return err } +// The objective of this function is to confirm the presence of the list of namepsaces in the k8s cluster. +// The list is provided via -watch-namespace and -watch-namespace-label cmdline options +// The function may log an error if it failed to get the namespace(s) via the label selector +// The secrets namespace is watched in the same vein specified via -watch-secret-namespace cmdline option func checkNamespaces(kubeClient kubernetes.Interface) { if *watchNamespaceLabel != "" { watchNamespaces = getWatchedNamespaces(kubeClient) @@ -338,6 +354,7 @@ func checkNamespaces(kubeClient kubernetes.Interface) { checkNamespaceExists(kubeClient, watchSecretNamespaces) } +// This is a helper function for fetching the all the namespaces in the cluster func getWatchedNamespaces(kubeClient kubernetes.Interface) (newWatchNamespaces []string) { // bootstrap the watched namespace list nsList, err := kubeClient.CoreV1().Namespaces().List(context.TODO(), meta_v1.ListOptions{LabelSelector: *watchNamespaceLabel}) @@ -352,6 +369,7 @@ func getWatchedNamespaces(kubeClient kubernetes.Interface) (newWatchNamespaces [ return newWatchNamespaces } +// This is a helper function for confirming the presence of input namespaces func checkNamespaceExists(kubeClient kubernetes.Interface, namespaces []string) { for _, ns := range namespaces { if ns != "" { @@ -379,6 +397,7 @@ func createConfigClient(config *rest.Config) (configClient k8s_nginx.Interface, return configClient, err } +// Creates a new dynamic client or returns an error func createDynamicClient(config *rest.Config) (dynClient dynamic.Interface, err error) { if *appProtectDos || *appProtect || *ingressLink != "" { dynClient, err = dynamic.NewForConfig(config) @@ -389,6 +408,7 @@ func createDynamicClient(config *rest.Config) (dynClient dynamic.Interface, err return dynClient, err } +// Returns a NGINX plus client config to talk to the N+ API func createPlusClient(nginxPlus bool, useFakeNginxManager bool, nginxManager nginx.Manager) (plusClient *client.NginxClient, err error) { if nginxPlus && !useFakeNginxManager { httpClient := getSocketClient("/var/lib/nginx/nginx-plus-api.sock") @@ -401,6 +421,7 @@ func createPlusClient(nginxPlus bool, useFakeNginxManager bool, nginxManager ngi return plusClient, nil } +// Returns a version 1 of the template func createV1TemplateExecutors() (templateExecutor *version1.TemplateExecutor, err error) { nginxConfTemplatePath := "nginx.tmpl" nginxIngressTemplatePath := "nginx.ingress.tmpl" @@ -425,6 +446,7 @@ func createV1TemplateExecutors() (templateExecutor *version1.TemplateExecutor, e return templateExecutor, nil } +// Returns a version 2 of the template func createV2TemplateExecutors() (templateExecutorV2 *version2.TemplateExecutor, err error) { nginxVirtualServerTemplatePath := "nginx.virtualserver.tmpl" nginxTransportServerTemplatePath := "nginx.transportserver.tmpl" @@ -448,6 +470,7 @@ func createV2TemplateExecutors() (templateExecutorV2 *version2.TemplateExecutor, return templateExecutorV2, nil } +// Returns a handle to a manager interface for managing the configuration of NGINX func createNginxManager(managerCollector collectors.ManagerCollector) (nginx.Manager, bool) { useFakeNginxManager := *proxyURL != "" var nginxManager nginx.Manager @@ -460,6 +483,7 @@ func createNginxManager(managerCollector collectors.ManagerCollector) (nginx.Man return nginxManager, useFakeNginxManager } +// Returns the NGINX version depending on OSS or Plus versions func getNginxVersionInfo(nginxManager nginx.Manager) (nginxInfo nginx.Version, err error) { nginxInfo = nginxManager.Version() glog.Infof("Using %s", nginxInfo.String()) @@ -472,6 +496,7 @@ func getNginxVersionInfo(nginxManager nginx.Manager) (nginxInfo nginx.Version, e return nginxInfo, err } +// Returns the version of App-Protect running on the system func getAppProtectVersionInfo() (version string, err error) { v, err := os.ReadFile(appProtectVersionPath) if err != nil { @@ -490,6 +515,7 @@ type childProcessConfig struct { aPDosDone chan error } +// Procedure to start the App-Protect and DOS Protect daemons func startChildProcesses(nginxManager nginx.Manager) childProcessConfig { var aPPluginDone chan error @@ -517,6 +543,8 @@ func startChildProcesses(nginxManager nginx.Manager) childProcessConfig { } } +// Applies the server secret config as provided via the cmdline option -default-server-tls-secret or the default +// Returns a boolean for rejecting the SSL handshake func processDefaultServerSecret(kubeClient *kubernetes.Clientset, nginxManager nginx.Manager) (sslRejectHandshake bool, err error) { sslRejectHandshake = false if *defaultServerSecret != "" { @@ -541,6 +569,8 @@ func processDefaultServerSecret(kubeClient *kubernetes.Clientset, nginxManager n return sslRejectHandshake, nil } +// Applies the wildcard server secret config as provided via the cmdline option -wildcard-tls-secret or the default +// Returns a boolean for rejecting the SSL handshake func processWildcardSecret(kubeClient *kubernetes.Clientset, nginxManager nginx.Manager) (isWildcardTLSSecret bool, err error) { isWildcardTLSSecret = false if *wildcardTLSSecret != "" { @@ -556,6 +586,8 @@ func processWildcardSecret(kubeClient *kubernetes.Clientset, nginxManager nginx. return isWildcardTLSSecret, nil } +// This returns a list of ports and corresponding boolean on the status of the service is enabled +// This depends on func createGlobalConfigurationValidator() *cr_validation.GlobalConfigurationValidator { forbiddenListenerPorts := map[int]bool{ 80: true, @@ -580,6 +612,7 @@ func createGlobalConfigurationValidator() *cr_validation.GlobalConfigurationVali return cr_validation.NewGlobalConfigurationValidator(forbiddenListenerPorts) } +// Generates the main NGINX config and the open tracing config func processNginxConfig(staticCfgParams *configs.StaticConfigParams, cfgParams *configs.ConfigParams, templateExecutor *version1.TemplateExecutor, nginxManager nginx.Manager) (err error) { ngxConfig := configs.GenerateNginxMainConfig(staticCfgParams, cfgParams) content, err := templateExecutor.ExecuteMainConfigTemplate(ngxConfig) @@ -663,6 +696,8 @@ func handleTermination(lbc *k8s.LoadBalancerController, nginxManager nginx.Manag os.Exit(0) } +// Handler/callback function when KIC's NGINX software declares itself ready via the -ready-status cmdline option +// This function is called by http.NewServeMux which probes the /nginx-ready endpoint via an HTTP request. func ready(lbc *k8s.LoadBalancerController) http.HandlerFunc { return func(w http.ResponseWriter, _ *http.Request) { if !lbc.IsNginxReady() { @@ -674,6 +709,7 @@ func ready(lbc *k8s.LoadBalancerController) http.HandlerFunc { } } +// This procedure creates various managers for KIC operation func createManagerAndControllerCollectors(constLabels map[string]string) (collectors.ManagerCollector, collectors.ControllerCollector, *prometheus.Registry) { var err error @@ -713,6 +749,7 @@ func createManagerAndControllerCollectors(constLabels map[string]string) (collec return mc, cc, registry } +// Creates an NGINX Plus and Latency Collector func createPlusAndLatencyCollectors( registry *prometheus.Registry, constLabels map[string]string, @@ -734,6 +771,7 @@ func createPlusAndLatencyCollectors( return plusCollector, syslogListener, latencyCollector } +// Helper function to creates an NGINX Plus Collector func createNginxPlusCollector(registry *prometheus.Registry, constLabels map[string]string, kubeClient *kubernetes.Clientset, plusClient *client.NginxClient, upstreamServerVariableLabels []string, upstreamServerPeerVariableLabelNames []string) *nginxCollector.NginxPlusCollector { var plusCollector *nginxCollector.NginxPlusCollector var prometheusSecret *api_v1.Secret @@ -767,6 +805,7 @@ func createNginxPlusCollector(registry *prometheus.Registry, constLabels map[str return plusCollector } +// Helper that returns a latency metrics collect via syslog func createLatencyCollector(registry *prometheus.Registry, constLabels map[string]string, upstreamServerVariableLabels []string, upstreamServerPeerVariableLabelNames []string) (metrics.SyslogListener, collectors.LatencyCollector) { var lc collectors.LatencyCollector lc = collectors.NewLatencyFakeCollector() @@ -787,6 +826,8 @@ func createLatencyCollector(registry *prometheus.Registry, constLabels map[strin return syslogListener, lc } +// This function starts a go routine (a lightweight thread of execution) for health checks against service insight listener ports +// An error is returned if there is a problem with the TLS secret for the Insight service func createHealthProbeEndpoint(kubeClient *kubernetes.Clientset, plusClient *client.NginxClient, cnf *configs.Configurator) (err error) { if !*enableServiceInsight { return nil @@ -803,6 +844,8 @@ func createHealthProbeEndpoint(kubeClient *kubernetes.Clientset, plusClient *cli return nil } +// Parses the cmdline option -global-configuration (requires -enable-custom-resources) +// Returns an error either if there is a problem with -global-configuration or -enable-custom-resources is not set func processGlobalConfiguration() (err error) { if *globalConfiguration != "" { _, _, err := k8s.ParseNamespaceName(*globalConfiguration) @@ -817,6 +860,8 @@ func processGlobalConfiguration() (err error) { return nil } +// Parses a ConfigMap resource for customizing NGINX configuration provided via the cmdline option -nginx-configmaps +// Returns an error if unable to parse the ConfigMap func processConfigMaps(kubeClient *kubernetes.Clientset, cfgParams *configs.ConfigParams, nginxManager nginx.Manager, templateExecutor *version1.TemplateExecutor) (*configs.ConfigParams, error) { if *nginxConfigMaps != "" { ns, name, err := k8s.ParseNamespaceName(*nginxConfigMaps) @@ -852,6 +897,8 @@ func processConfigMaps(kubeClient *kubernetes.Clientset, cfgParams *configs.Conf return cfgParams, nil } +// This function updates the labels of the NGINX Ingress Controller +// An error is returned if its unable to retrieve pod info and other problems along the way func updateSelfWithVersionInfo(kubeClient *kubernetes.Clientset, version, appProtectVersion string, nginxVersion nginx.Version, maxRetries int, waitTime time.Duration) { podUpdated := false From a60f7e27b2d19731c31555b60f02ab942cc33400 Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Wed, 28 Feb 2024 17:07:42 +1300 Subject: [PATCH 13/26] Issue 4837: chore: main.go refactor --- cmd/nginx-ingress/main_test.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index 06ab7d0f9a..ccbd617313 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -1 +1,23 @@ package main + +import ( + "flag" + "os" + "testing" + + "github.com/nginxinc/kubernetes-ingress/internal/metrics/collectors" +) + +func TestGetNginxVersionInfo(t *testing.T) { + os.Args = append(os.Args, "-nginx-plus") + os.Args = append(os.Args, "-proxy") + os.Args = append(os.Args, "test-proxy") + flag.Parse() + constLabels := map[string]string{"class": *ingressClass} + mc := collectors.NewLocalManagerMetricsCollector(constLabels) + nginxManager, _ := createNginxManager(mc) + nginxInfo, _ := getNginxVersionInfo(nginxManager) + if nginxInfo.String() == "" { + t.Errorf("Error when getting nginx version, empty string") + } +} From 05736e301e11bdca88875f811030ab904ee90631 Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Wed, 28 Feb 2024 17:11:32 +1300 Subject: [PATCH 14/26] Issue 4837: chore: main.go refactor * Added initial unit-tests for some functions --- cmd/nginx-ingress/main_test.go | 79 +++++++++++++++++++++++++++++++++- 1 file changed, 78 insertions(+), 1 deletion(-) diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index ccbd617313..9e2a33bcde 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -2,10 +2,10 @@ package main import ( "flag" + "fmt" "os" "testing" - "github.com/nginxinc/kubernetes-ingress/internal/metrics/collectors" ) func TestGetNginxVersionInfo(t *testing.T) { @@ -21,3 +21,80 @@ func TestGetNginxVersionInfo(t *testing.T) { t.Errorf("Error when getting nginx version, empty string") } } + +func TestGetAppProtectVersionInfo(t *testing.T) { + dataToWrite := "1.2.3\n" + f, err := os.CreateTemp("/var/tmp/", "dat1") + if err != nil { + fmt.Println(err) + return + } + l, err := f.WriteString(dataToWrite) + if err != nil { + fmt.Println(err) + if err := f.Close(); err != nil { + fmt.Println(err) + } + + return + } + fmt.Println(l, "bytes written successfully") + if err := f.Close(); err != nil { + fmt.Println(err) + return + } + version, err := getAppProtectVersionInfo() + if err != nil { + t.Errorf("Error reading AppProtect Version file #{err}\n") + } + + if version == "" { + t.Errorf("Error with AppProtect Version, is empty") + } +} + +func TestCreateGlobalConfigurationValidator(t *testing.T) { + globalConfiguration := conf_v1.GlobalConfiguration{ + Spec: conf_v1.GlobalConfigurationSpec{ + Listeners: []conf_v1.Listener{ + { + Name: "tcp-listener", + Port: 53, + Protocol: "TCP", + }, + { + Name: "udp-listener", + Port: 53, + Protocol: "UDP", + }, + }, + }, + } + + gcv := createGlobalConfigurationValidator() + + if err := gcv.ValidateGlobalConfiguration(&globalConfiguration); err != nil { + t.Errorf("ValidateGlobalConfiguration() returned error %v for valid input", err) + } + + incorrectGlobalConf := conf_v1.GlobalConfiguration{ + Spec: conf_v1.GlobalConfigurationSpec{ + Listeners: []conf_v1.Listener{ + { + Name: "tcp-listener", + Port: 53, + Protocol: "TCPT", + }, + { + Name: "udp-listener", + Port: 53, + Protocol: "UDP", + }, + }, + }, + } + + if err := gcv.ValidateGlobalConfiguration(&incorrectGlobalConf); err == nil { + t.Errorf("ValidateGlobalConfiguration() returned error %v for invalid input", err) + } +} From 88cfd23393654aea6db22d461ffef87cb88b5508 Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Thu, 29 Feb 2024 09:43:35 +1300 Subject: [PATCH 15/26] Issue 4837: chore: main.go refactor * Revert unit-tests to determine golangci-lint pre-commit errors --- cmd/nginx-ingress/main_test.go | 99 ---------------------------------- 1 file changed, 99 deletions(-) diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index 9e2a33bcde..06ab7d0f9a 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -1,100 +1 @@ package main - -import ( - "flag" - "fmt" - "os" - "testing" - -) - -func TestGetNginxVersionInfo(t *testing.T) { - os.Args = append(os.Args, "-nginx-plus") - os.Args = append(os.Args, "-proxy") - os.Args = append(os.Args, "test-proxy") - flag.Parse() - constLabels := map[string]string{"class": *ingressClass} - mc := collectors.NewLocalManagerMetricsCollector(constLabels) - nginxManager, _ := createNginxManager(mc) - nginxInfo, _ := getNginxVersionInfo(nginxManager) - if nginxInfo.String() == "" { - t.Errorf("Error when getting nginx version, empty string") - } -} - -func TestGetAppProtectVersionInfo(t *testing.T) { - dataToWrite := "1.2.3\n" - f, err := os.CreateTemp("/var/tmp/", "dat1") - if err != nil { - fmt.Println(err) - return - } - l, err := f.WriteString(dataToWrite) - if err != nil { - fmt.Println(err) - if err := f.Close(); err != nil { - fmt.Println(err) - } - - return - } - fmt.Println(l, "bytes written successfully") - if err := f.Close(); err != nil { - fmt.Println(err) - return - } - version, err := getAppProtectVersionInfo() - if err != nil { - t.Errorf("Error reading AppProtect Version file #{err}\n") - } - - if version == "" { - t.Errorf("Error with AppProtect Version, is empty") - } -} - -func TestCreateGlobalConfigurationValidator(t *testing.T) { - globalConfiguration := conf_v1.GlobalConfiguration{ - Spec: conf_v1.GlobalConfigurationSpec{ - Listeners: []conf_v1.Listener{ - { - Name: "tcp-listener", - Port: 53, - Protocol: "TCP", - }, - { - Name: "udp-listener", - Port: 53, - Protocol: "UDP", - }, - }, - }, - } - - gcv := createGlobalConfigurationValidator() - - if err := gcv.ValidateGlobalConfiguration(&globalConfiguration); err != nil { - t.Errorf("ValidateGlobalConfiguration() returned error %v for valid input", err) - } - - incorrectGlobalConf := conf_v1.GlobalConfiguration{ - Spec: conf_v1.GlobalConfigurationSpec{ - Listeners: []conf_v1.Listener{ - { - Name: "tcp-listener", - Port: 53, - Protocol: "TCPT", - }, - { - Name: "udp-listener", - Port: 53, - Protocol: "UDP", - }, - }, - }, - } - - if err := gcv.ValidateGlobalConfiguration(&incorrectGlobalConf); err == nil { - t.Errorf("ValidateGlobalConfiguration() returned error %v for invalid input", err) - } -} From fa3dfc9d970ae7c433dbb19d41c545946e02466d Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Thu, 29 Feb 2024 10:25:19 +1300 Subject: [PATCH 16/26] Issue 4837: chore: main_test.go unit-tests * Initial unit-tests --- cmd/nginx-ingress/main_test.go | 55 ++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index 06ab7d0f9a..20733daed9 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -1 +1,56 @@ package main + +import ( + "flag" + "fmt" + "os" + "testing" + + "github.com/nginxinc/kubernetes-ingress/internal/metrics/collectors" +) + +// Test for getNginxVersionInfo() +func TestGetNginxVersionInfo(t *testing.T) { + os.Args = append(os.Args, "-nginx-plus") + os.Args = append(os.Args, "-proxy") + os.Args = append(os.Args, "test-proxy") + flag.Parse() + constLabels := map[string]string{"class": *ingressClass} + mc := collectors.NewLocalManagerMetricsCollector(constLabels) + nginxManager, _ := createNginxManager(mc) + nginxInfo, _ := getNginxVersionInfo(nginxManager) + if nginxInfo.String() == "" { + t.Errorf("Error when getting nginx version, empty string") + } +} + +func TestGetAppProtectVersionInfo(t *testing.T) { + dataToWrite := "1.2.3\n" + f, err := os.CreateTemp("/var/tmp/", "dat1") + if err != nil { + fmt.Println(err) + return + } + l, err := f.WriteString(dataToWrite) + if err != nil { + fmt.Println(err) + if err := f.Close(); err != nil { + fmt.Println(err) + } + + return + } + fmt.Println(l, "bytes written successfully") + if err := f.Close(); err != nil { + fmt.Println(err) + return + } + version, err := getAppProtectVersionInfo() + if err != nil { + t.Errorf("Error reading AppProtect Version file #{err}\n") + } + + if version == "" { + t.Errorf("Error with AppProtect Version, is empty") + } +} From eb72565482834eb25541829d179402dcae39c116 Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Thu, 29 Feb 2024 10:53:43 +1300 Subject: [PATCH 17/26] Issue 4837: chore: main_test.go unit-tests * Fix unit-test TestGetAppProtectVersionInfo() --- cmd/nginx-ingress/main_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index 20733daed9..9248227a78 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -4,6 +4,7 @@ import ( "flag" "fmt" "os" + "path" "testing" "github.com/nginxinc/kubernetes-ingress/internal/metrics/collectors" @@ -26,7 +27,9 @@ func TestGetNginxVersionInfo(t *testing.T) { func TestGetAppProtectVersionInfo(t *testing.T) { dataToWrite := "1.2.3\n" - f, err := os.CreateTemp("/var/tmp/", "dat1") + dirPath := path.Dir(appProtectVersionPath) + versionFile := path.Base(appProtectVersionPath) + f, err := os.CreateTemp(dirPath, versionFile) if err != nil { fmt.Println(err) return From d37e3bb48f2076630f4f156b65c965f45d25f7e1 Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Thu, 29 Feb 2024 11:21:01 +1300 Subject: [PATCH 18/26] Issue 4837: chore: main_test.go unit-tests * Add unit-test for TestCreateGlobalConfigurationValidator() --- cmd/nginx-ingress/main_test.go | 48 ++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index 9248227a78..21810875b3 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -7,6 +7,8 @@ import ( "path" "testing" + conf_v1 "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1" + "github.com/nginxinc/kubernetes-ingress/internal/metrics/collectors" ) @@ -57,3 +59,49 @@ func TestGetAppProtectVersionInfo(t *testing.T) { t.Errorf("Error with AppProtect Version, is empty") } } + +func TestCreateGlobalConfigurationValidator(t *testing.T) { + globalConfiguration := conf_v1.GlobalConfiguration{ + Spec: conf_v1.GlobalConfigurationSpec{ + Listeners: []conf_v1.Listener{ + { + Name: "tcp-listener", + Port: 53, + Protocol: "TCP", + }, + { + Name: "udp-listener", + Port: 53, + Protocol: "UDP", + }, + }, + }, + } + + gcv := createGlobalConfigurationValidator() + + if err := gcv.ValidateGlobalConfiguration(&globalConfiguration); err != nil { + t.Errorf("ValidateGlobalConfiguration() returned error %v for valid input", err) + } + + incorrectGlobalConf := conf_v1.GlobalConfiguration{ + Spec: conf_v1.GlobalConfigurationSpec{ + Listeners: []conf_v1.Listener{ + { + Name: "tcp-listener", + Port: 53, + Protocol: "TCPT", + }, + { + Name: "udp-listener", + Port: 53, + Protocol: "UDP", + }, + }, + }, + } + + if err := gcv.ValidateGlobalConfiguration(&incorrectGlobalConf); err == nil { + t.Errorf("ValidateGlobalConfiguration() returned error %v for invalid input", err) + } +} From 9b17488ce61992c89af344b127d29be2e4f90d73 Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Thu, 29 Feb 2024 17:43:13 +1300 Subject: [PATCH 19/26] Issue 4837: chore: main_test.go unit-tests * Add unit-test for validateIngressClass() --- cmd/nginx-ingress/main_test.go | 60 +++++++++++++++++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index 21810875b3..c4c466a555 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -7,9 +7,14 @@ import ( "path" "testing" - conf_v1 "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1" + "github.com/nginxinc/kubernetes-ingress/internal/k8s" + networkingv1 "k8s.io/api/networking/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/kubernetes/fake" "github.com/nginxinc/kubernetes-ingress/internal/metrics/collectors" + conf_v1 "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1" ) // Test for getNginxVersionInfo() @@ -105,3 +110,56 @@ func TestCreateGlobalConfigurationValidator(t *testing.T) { t.Errorf("ValidateGlobalConfiguration() returned error %v for invalid input", err) } } + +// Test valid (nginx) and invalid (other) ingress classes +func TestValidateIngressClass(t *testing.T) { + // Define an IngressClass + { + ingressClass := &networkingv1.IngressClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx", + }, + Spec: networkingv1.IngressClassSpec{ + Controller: k8s.IngressControllerName, + }, + } + // Create a fake client + clientset := fake.NewSimpleClientset(ingressClass) + + validData := []struct { + clientset kubernetes.Interface + }{ + { + clientset: clientset, + }, + } + + if err := validateIngressClass(validData[0].clientset); err != nil { + t.Fatalf("error in ingress class, error: %v", err) + } + } + + // Test invalid case + { + ingressClass := &networkingv1.IngressClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: "not-nginx", + }, + Spec: networkingv1.IngressClassSpec{ + Controller: "www.example.com/ingress-controller", + }, + } + clientset := fake.NewSimpleClientset(ingressClass) + inValidData := []struct { + clientset kubernetes.Interface + }{ + { + clientset: clientset, + }, + } + + if err := validateIngressClass(inValidData[0].clientset); err == nil { + t.Fatalf("validateIngressClass() returned no error for invalid input, error: %v", err) + } + } +} From 86e75220db955162f95065f3d8e78e9510b82fee Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Tue, 5 Mar 2024 13:27:32 +1300 Subject: [PATCH 20/26] Issue 4837: chore: main_test.go unit-tests * Add unit-test for confirmMinimumK8sVersionCriteria() --- cmd/nginx-ingress/main_test.go | 62 +++++++++++++++++++++++++++++++--- 1 file changed, 58 insertions(+), 4 deletions(-) diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index c4c466a555..252d23a797 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -8,13 +8,15 @@ import ( "testing" "github.com/nginxinc/kubernetes-ingress/internal/k8s" - networkingv1 "k8s.io/api/networking/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/fake" "github.com/nginxinc/kubernetes-ingress/internal/metrics/collectors" conf_v1 "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1" + networkingv1 "k8s.io/api/networking/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + apiVersion "k8s.io/apimachinery/pkg/version" + fakeDisc "k8s.io/client-go/discovery/fake" ) // Test for getNginxVersionInfo() @@ -38,7 +40,7 @@ func TestGetAppProtectVersionInfo(t *testing.T) { versionFile := path.Base(appProtectVersionPath) f, err := os.CreateTemp(dirPath, versionFile) if err != nil { - fmt.Println(err) + t.Errorf("Error creating temp directory: %v", err) return } l, err := f.WriteString(dataToWrite) @@ -57,7 +59,7 @@ func TestGetAppProtectVersionInfo(t *testing.T) { } version, err := getAppProtectVersionInfo() if err != nil { - t.Errorf("Error reading AppProtect Version file #{err}\n") + t.Errorf("Error reading AppProtect Version file: %v", err) } if version == "" { @@ -163,3 +165,55 @@ func TestValidateIngressClass(t *testing.T) { } } } + +func TestMinimumK8sVersion3(t *testing.T) { + // Create a fake client. + clientset := fake.NewSimpleClientset() + + // Override the ServerVersion method on the fake Discovery client + discoveryClient, ok := clientset.Discovery().(*fakeDisc.FakeDiscovery) + if !ok { + fmt.Println("couldn't convert Discovery() to *FakeDiscovery") + } + + // This test block is when the correct/expected k8s version is returned + { + correctVersion := &apiVersion.Info{ + Major: "1", Minor: "22", GitVersion: "v1.22.2", + } + discoveryClient.FakedServerVersion = correctVersion + + // Get the server version as a sanity check + actualVersion, err := discoveryClient.ServerVersion() + if err != nil { + t.Fatalf("Failed to get server version: %v", err) + } + fmt.Printf("Kubernetes version: %s\n", actualVersion.String()) + + // Verify if the mocked server version is as expected. + if err := confirmMinimumK8sVersionCriteria(clientset); err != nil { + t.Fatalf("Error in checking minimum k8s version: %v", err) + } + } + + // This test block is when the incorrect/unexpected k8s version is returned + // i.e. not the min supported version + { + wrongVersion := &apiVersion.Info{ + Major: "1", Minor: "19", GitVersion: "v1.19.2", + } + discoveryClient.FakedServerVersion = wrongVersion + + // Get the server version as a sanity check + actualVersion, err := discoveryClient.ServerVersion() + if err != nil { + t.Fatalf("Failed to get server version: %v", err) + } + fmt.Printf("Kubernetes version: %s\n", actualVersion.String()) + + // Verify if the mocked server version returns an error as we are testing for < 1.22 (v1.19.2). + if err := confirmMinimumK8sVersionCriteria(clientset); err == nil { + t.Fatalf("Expected an error when checking minimum k8s version but got none: %v", err) + } + } +} From ffd5e524087a5df9d3822686e0722bfe55e5e603 Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Tue, 5 Mar 2024 21:34:18 +1300 Subject: [PATCH 21/26] Issue 4837: chore: main_test.go unit-tests * Fix unit-test TestGetAppProtectVersionInfo() --- cmd/nginx-ingress/main.go | 25 ++++++++++-- cmd/nginx-ingress/main_test.go | 71 ++++++++++++++++++++-------------- 2 files changed, 65 insertions(+), 31 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 24bde7f42c..bd88d73931 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -7,6 +7,7 @@ import ( "net/http" "os" "os/signal" + "path/filepath" "runtime" "strings" "syscall" @@ -52,6 +53,23 @@ const ( appProtectVersionPath = "/opt/app_protect/VERSION" ) +// FileHandle - Interface to read a file +type FileHandle interface { + ReadFile(filename string) ([]byte, error) +} + +// OSFileHandle - Struct to hold the interface to the reading a file +type OSFileHandle struct{} + +// ReadFile - Actual implementation of the interface abstraction +func (o *OSFileHandle) ReadFile(filename string) ([]byte, error) { + _, err := os.Open(filepath.Clean(filename)) + if err != nil { + return nil, err + } + return os.ReadFile(filename) +} + // This is the start of the program func main() { commitHash, commitTime, dirtyBuild := getBuildInfo() @@ -101,7 +119,8 @@ func main() { var appProtectVersion string if *appProtect { - appProtectVersion, err = getAppProtectVersionInfo() + osFileHandle := &OSFileHandle{} + appProtectVersion, err = getAppProtectVersionInfo(osFileHandle) if err != nil { glog.Fatal(err) } @@ -497,8 +516,8 @@ func getNginxVersionInfo(nginxManager nginx.Manager) (nginxInfo nginx.Version, e } // Returns the version of App-Protect running on the system -func getAppProtectVersionInfo() (version string, err error) { - v, err := os.ReadFile(appProtectVersionPath) +func getAppProtectVersionInfo(fd FileHandle) (version string, err error) { + v, err := fd.ReadFile(appProtectVersionPath) if err != nil { return version, fmt.Errorf("cannot detect the AppProtect version, %s", err.Error()) } diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index 252d23a797..09c0369075 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -1,10 +1,10 @@ package main import ( + "errors" "flag" "fmt" "os" - "path" "testing" "github.com/nginxinc/kubernetes-ingress/internal/k8s" @@ -34,37 +34,52 @@ func TestGetNginxVersionInfo(t *testing.T) { } } -func TestGetAppProtectVersionInfo(t *testing.T) { - dataToWrite := "1.2.3\n" - dirPath := path.Dir(appProtectVersionPath) - versionFile := path.Base(appProtectVersionPath) - f, err := os.CreateTemp(dirPath, versionFile) - if err != nil { - t.Errorf("Error creating temp directory: %v", err) - return - } - l, err := f.WriteString(dataToWrite) - if err != nil { - fmt.Println(err) - if err := f.Close(); err != nil { - fmt.Println(err) - } +type MockFileHandle struct { + FileContent []byte + ReadErr error +} - return - } - fmt.Println(l, "bytes written successfully") - if err := f.Close(); err != nil { - fmt.Println(err) - return - } - version, err := getAppProtectVersionInfo() - if err != nil { - t.Errorf("Error reading AppProtect Version file: %v", err) +func (m *MockFileHandle) ReadFile(_ string) ([]byte, error) { + if m.ReadErr != nil { + return nil, m.ReadErr } + return m.FileContent, nil +} - if version == "" { - t.Errorf("Error with AppProtect Version, is empty") +func TestGetAppProtectVersionInfo(t *testing.T) { + // Test for file reader returning valid/correct info and no errors + { + mockFileHandle := &MockFileHandle{ + FileContent: []byte("1.2.3\n"), + ReadErr: nil, + } + _, err := getAppProtectVersionInfo(mockFileHandle) + if err != nil { + t.Errorf("Error reading AppProtect Version file: %v", err) + } + } + // Test for file reader returning an error + { + mockFileHandle := &MockFileHandle{ + FileContent: []byte("1.2.3\n"), + ReadErr: errors.ErrUnsupported, + } + _, err := getAppProtectVersionInfo(mockFileHandle) + if err == nil { + t.Errorf("Error reading AppProtect Version file: %v", err) + } } + // Test for file reader returning an empty version + //{ + // mockFileHandle := &MockFileHandle{ + // FileContent: []byte("\n"), + // ReadErr: nil, + // } + // version, _ := getAppProtectVersionInfo(mockFileHandle) + // if version == "" { + // t.Errorf("Error with AppProtect Version, is empty") + // } + //} } func TestCreateGlobalConfigurationValidator(t *testing.T) { From 2dec9878a799ba9cbabc24ef7e3f029d5542195e Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Fri, 8 Mar 2024 14:42:48 +1300 Subject: [PATCH 22/26] Issue 4837: chore: main_test.go unit-tests * Add unit-test for getAndValidateSecret() --- cmd/nginx-ingress/main.go | 29 +++++-- cmd/nginx-ingress/main_test.go | 148 +++++++++++++++++++++++++++++++++ 2 files changed, 171 insertions(+), 6 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index bd88d73931..1174e3807b 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -53,6 +53,11 @@ const ( appProtectVersionPath = "/opt/app_protect/VERSION" ) +// KubernetesAPI - type to abstract the Kubernetes interface +type KubernetesAPI struct { + Client kubernetes.Interface +} + // FileHandle - Interface to read a file type FileHandle interface { ReadFile(filename string) ([]byte, error) @@ -567,7 +572,10 @@ func startChildProcesses(nginxManager nginx.Manager) childProcessConfig { func processDefaultServerSecret(kubeClient *kubernetes.Clientset, nginxManager nginx.Manager) (sslRejectHandshake bool, err error) { sslRejectHandshake = false if *defaultServerSecret != "" { - secret, err := getAndValidateSecret(kubeClient, *defaultServerSecret) + kAPI := &KubernetesAPI{ + Client: kubeClient, + } + secret, err := kAPI.getAndValidateSecret(*defaultServerSecret) if err != nil { return sslRejectHandshake, fmt.Errorf("error trying to get the default server TLS secret %v: %w", *defaultServerSecret, err) } @@ -593,7 +601,10 @@ func processDefaultServerSecret(kubeClient *kubernetes.Clientset, nginxManager n func processWildcardSecret(kubeClient *kubernetes.Clientset, nginxManager nginx.Manager) (isWildcardTLSSecret bool, err error) { isWildcardTLSSecret = false if *wildcardTLSSecret != "" { - secret, err := getAndValidateSecret(kubeClient, *wildcardTLSSecret) + kAPI := &KubernetesAPI{ + Client: kubeClient, + } + secret, err := kAPI.getAndValidateSecret(*wildcardTLSSecret) if err != nil { return isWildcardTLSSecret, fmt.Errorf("error trying to get the wildcard TLS secret %v: %w", *wildcardTLSSecret, err) } @@ -665,12 +676,12 @@ func getSocketClient(sockPath string) *http.Client { } // getAndValidateSecret gets and validates a secret. -func getAndValidateSecret(kubeClient *kubernetes.Clientset, secretNsName string) (secret *api_v1.Secret, err error) { +func (kAPI KubernetesAPI) getAndValidateSecret(secretNsName string) (secret *api_v1.Secret, err error) { ns, name, err := k8s.ParseNamespaceName(secretNsName) if err != nil { return nil, fmt.Errorf("could not parse the %v argument: %w", secretNsName, err) } - secret, err = kubeClient.CoreV1().Secrets(ns).Get(context.TODO(), name, meta_v1.GetOptions{}) + secret, err = kAPI.Client.CoreV1().Secrets(ns).Get(context.TODO(), name, meta_v1.GetOptions{}) if err != nil { return nil, fmt.Errorf("could not get %v: %w", secretNsName, err) } @@ -797,7 +808,10 @@ func createNginxPlusCollector(registry *prometheus.Registry, constLabels map[str var err error if *prometheusTLSSecretName != "" { - prometheusSecret, err = getAndValidateSecret(kubeClient, *prometheusTLSSecretName) + kAPI := &KubernetesAPI{ + Client: kubeClient, + } + prometheusSecret, err = kAPI.getAndValidateSecret(*prometheusTLSSecretName) if err != nil { glog.Fatalf("Error trying to get the prometheus TLS secret %v: %v", *prometheusTLSSecretName, err) } @@ -854,7 +868,10 @@ func createHealthProbeEndpoint(kubeClient *kubernetes.Clientset, plusClient *cli var serviceInsightSecret *api_v1.Secret if *serviceInsightTLSSecretName != "" { - serviceInsightSecret, err = getAndValidateSecret(kubeClient, *serviceInsightTLSSecretName) + kAPI := &KubernetesAPI{ + Client: kubeClient, + } + serviceInsightSecret, err = kAPI.getAndValidateSecret(*serviceInsightTLSSecretName) if err != nil { return fmt.Errorf("error trying to get the service insight TLS secret %v: %w", *serviceInsightTLSSecretName, err) } diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index 09c0369075..4c28707965 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -1,11 +1,22 @@ package main import ( + "bytes" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/rsa" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" "errors" "flag" "fmt" + "log" + "math/big" "os" "testing" + "time" "github.com/nginxinc/kubernetes-ingress/internal/k8s" "k8s.io/client-go/kubernetes" @@ -13,6 +24,7 @@ import ( "github.com/nginxinc/kubernetes-ingress/internal/metrics/collectors" conf_v1 "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/v1" + v1 "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" apiVersion "k8s.io/apimachinery/pkg/version" @@ -232,3 +244,139 @@ func TestMinimumK8sVersion3(t *testing.T) { } } } + +func publicKey(priv interface{}) interface{} { + switch k := priv.(type) { + case *rsa.PrivateKey: + return &k.PublicKey + case *ecdsa.PrivateKey: + return &k.PublicKey + default: + return nil + } +} + +func pemBlockForKey(priv interface{}) *pem.Block { + switch k := priv.(type) { + case *rsa.PrivateKey: + return &pem.Block{Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(k)} + case *ecdsa.PrivateKey: + b, err := x509.MarshalECPrivateKey(k) + if err != nil { + log.Fatalf("Unable to marshal ECDSA private key: %v", err) + } + return &pem.Block{Type: "EC PRIVATE KEY", Bytes: b} + default: + return nil + } +} + +func genCertKeyPair() (string, string) { + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + log.Fatal(err) + } + template := x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{ + Organization: []string{"Acme Co"}, + }, + NotBefore: time.Now(), + NotAfter: time.Now().Add(time.Hour * 24 * 180), + + KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + BasicConstraintsValid: true, + } + + derBytes, err := x509.CreateCertificate(rand.Reader, &template, &template, publicKey(priv), priv) + if err != nil { + log.Fatalf("Failed to create certificate: %s", err) + } + + out := &bytes.Buffer{} + if err = pem.Encode(out, &pem.Block{Type: "CERTIFICATE", Bytes: derBytes}); err != nil { + log.Fatal(err) + } + cert := out.String() + + out.Reset() + if err = pem.Encode(out, pemBlockForKey(priv)); err != nil { + log.Fatal(err) + } + privKey := out.String() + + return cert, privKey +} + +func TestGetAndValidateSecret(t *testing.T) { + // Test for the working case where nothing goes wrong with valid data + cert, privKey := genCertKeyPair() + { + secret := v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-secret", + Namespace: "default", + }, + Type: "kubernetes.io/tls", + Data: map[string][]byte{ + "tls.crt": []byte(cert), + "tls.key": []byte(privKey), + }, + } + + kAPI := &KubernetesAPI{ + Client: fake.NewSimpleClientset(&secret), + } + _, err := kAPI.getAndValidateSecret("default/my-secret") + if err != nil { + t.Errorf("Error in retrieving secret: %v", err) + } + } + + // Test for the non-existent secret + { + secret := v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-secret", + Namespace: "default", + }, + Type: "kubernetes.io/tls", + Data: map[string][]byte{ + "tls.crt": []byte(cert), + "tls.key": []byte(privKey), + }, + } + + kAPI := &KubernetesAPI{ + Client: fake.NewSimpleClientset(&secret), + } + _, err := kAPI.getAndValidateSecret("default/non-existent-secret") + if err == nil { + t.Errorf("Expected an error in retrieving secret but %v returned", err) + } + } + + // Test for the TLS cert/key without the key + { + secret := v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-secret", + Namespace: "default", + }, + Type: "kubernetes.io/tls", + Data: map[string][]byte{ + "tls.crt": []byte(cert), + "tls.key": []byte(""), + }, + } + + kAPI := &KubernetesAPI{ + Client: fake.NewSimpleClientset(&secret), + } + _, err := kAPI.getAndValidateSecret("default/my-secret") + if err == nil { + t.Errorf("Expected an error in retrieving secret but %v returned", err) + } + } +} From d3bf5ac39ea8837702e31c359c35faf4b5957767 Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Tue, 23 Apr 2024 14:23:14 +1200 Subject: [PATCH 23/26] Issue 4837: chore: main_test.go unit-tests * Remove superfluous logging --- cmd/nginx-ingress/main_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index 4c28707965..7e963eb836 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -211,11 +211,10 @@ func TestMinimumK8sVersion3(t *testing.T) { discoveryClient.FakedServerVersion = correctVersion // Get the server version as a sanity check - actualVersion, err := discoveryClient.ServerVersion() + _, err := discoveryClient.ServerVersion() if err != nil { t.Fatalf("Failed to get server version: %v", err) } - fmt.Printf("Kubernetes version: %s\n", actualVersion.String()) // Verify if the mocked server version is as expected. if err := confirmMinimumK8sVersionCriteria(clientset); err != nil { @@ -232,11 +231,10 @@ func TestMinimumK8sVersion3(t *testing.T) { discoveryClient.FakedServerVersion = wrongVersion // Get the server version as a sanity check - actualVersion, err := discoveryClient.ServerVersion() + _, err := discoveryClient.ServerVersion() if err != nil { t.Fatalf("Failed to get server version: %v", err) } - fmt.Printf("Kubernetes version: %s\n", actualVersion.String()) // Verify if the mocked server version returns an error as we are testing for < 1.22 (v1.19.2). if err := confirmMinimumK8sVersionCriteria(clientset); err == nil { From 5d26b05e59208fcc21bf800690c991cd8e90bc9f Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Wed, 24 Apr 2024 14:16:52 +1200 Subject: [PATCH 24/26] Issue 4837: chore: main_test.go unit-tests * Added test for getWatchedNamespaces() --- cmd/nginx-ingress/main_test.go | 95 ++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index 7e963eb836..a42c96bf67 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -2,6 +2,7 @@ package main import ( "bytes" + "context" "crypto/ecdsa" "crypto/elliptic" "crypto/rand" @@ -15,6 +16,8 @@ import ( "log" "math/big" "os" + "slices" + "strings" "testing" "time" @@ -378,3 +381,95 @@ func TestGetAndValidateSecret(t *testing.T) { } } } + +// Utility function to check if a namespace +func expectedNs(watchNsLabelList string, ns []string) bool { + wNs := strings.Split(watchNsLabelList, ",") + resultOk := false + for _, n := range wNs { + // fmt.Println("list: ", n) + nsNameWithDelimiter := strings.Split(n, "=") + // fmt.Println("WithDelimit: ", nsNameWithDelimiter) + nsNameOnly := "" + if len(nsNameWithDelimiter) > 1 { + nsNameOnly = nsNameWithDelimiter[1] + // fmt.Println("name-only: ", nsNameOnly) + } + isValid := slices.Contains(ns, nsNameOnly) + + resultOk = resultOk || isValid + } + return resultOk +} + +// This test uses a fake client to create 2 namespaces, ns1 and ns2 +// We use these objects to test the retreival of namespaces based on the +// watchedNamespacesLabel input +func TestGetWatchedNamespaces(t *testing.T) { + // Create a new fake clientset + clientset := fake.NewSimpleClientset() + ctx := context.Background() + + // Create label for test1-namespace + ns1Labels := map[string]string{ + "namespace": "ns1", + "app": "my-application", + "version": "v1", + } + + // Create the ns1 namespace using the fake clientset + _, err := clientset.CoreV1().Namespaces().Create(ctx, &v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-namespace", + Labels: ns1Labels, + }, + }, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Failed to create namespace: %v", err) + } + + // Create label for test2-namespace + ns2Labels := map[string]string{ + "namespace": "ns2", + "app": "my-application", + "version": "v1", + } + + // Create the ns2 namespace using the fake clientset + _, err = clientset.CoreV1().Namespaces().Create(ctx, &v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ns2", + Labels: ns2Labels, + }, + }, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Failed to create namespace: %v", err) + } + + // This section is testing the presence of the watchedNamespaceLabels + { + // Create a list of 'watched' namespaces + watchNsLabelList := "namespace=ns2, version=v1" + watchNamespaceLabel = &watchNsLabelList + ns := getWatchedNamespaces(clientset) + + if len(ns) == 0 { + t.Errorf("Expected namespaces-list not to be empty") + } + + resultOk := expectedNs(watchNsLabelList, ns) + if !resultOk { + t.Errorf("Expected namespaces-list to be %v, got %v", watchNsLabelList, ns) + } + } + + // This section is testing the absence (ns3) of the watchedNamespaceLabels + { + watchNsLabelList := "namespace=ns3, version=v1" + watchNamespaceLabel = &watchNsLabelList + ns := getWatchedNamespaces(clientset) + if len(ns) != 0 { + t.Errorf("Expected expected an empty namespaces-list but got %v", ns) + } + } +} From b6aacea0b7edb241ff2468297c8bd47e67868a01 Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Wed, 24 Apr 2024 15:31:40 +1200 Subject: [PATCH 25/26] Issue 4837: chore: main_test.go unit-tests * Added test for checkNamespaceExists() * checkNamespaceExists() was modified to return a boolean for the unit-test --- cmd/nginx-ingress/main.go | 9 ++++--- cmd/nginx-ingress/main_test.go | 44 +++++++++++++++++++++++++++++++++- 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 164123e1d5..5f59c3fd5c 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -384,9 +384,9 @@ func checkNamespaces(kubeClient kubernetes.Interface) { if *watchNamespaceLabel != "" { watchNamespaces = getWatchedNamespaces(kubeClient) } else { - checkNamespaceExists(kubeClient, watchNamespaces) + _ = checkNamespaceExists(kubeClient, watchNamespaces) } - checkNamespaceExists(kubeClient, watchSecretNamespaces) + _ = checkNamespaceExists(kubeClient, watchSecretNamespaces) } // This is a helper function for fetching the all the namespaces in the cluster @@ -405,15 +405,18 @@ func getWatchedNamespaces(kubeClient kubernetes.Interface) (newWatchNamespaces [ } // This is a helper function for confirming the presence of input namespaces -func checkNamespaceExists(kubeClient kubernetes.Interface, namespaces []string) { +func checkNamespaceExists(kubeClient kubernetes.Interface, namespaces []string) bool { + hasErrors := false for _, ns := range namespaces { if ns != "" { _, err := kubeClient.CoreV1().Namespaces().Get(context.TODO(), ns, meta_v1.GetOptions{}) if err != nil { glog.Warningf("Error when getting Namespace %v: %v", ns, err) } + hasErrors = hasErrors || err != nil } } + return hasErrors } func createConfigClient(config *rest.Config) (configClient k8s_nginx.Interface, err error) { diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index a42c96bf67..af2dc52351 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -420,7 +420,7 @@ func TestGetWatchedNamespaces(t *testing.T) { // Create the ns1 namespace using the fake clientset _, err := clientset.CoreV1().Namespaces().Create(ctx, &v1.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-namespace", + Name: "ns1", Labels: ns1Labels, }, }, metav1.CreateOptions{}) @@ -473,3 +473,45 @@ func TestGetWatchedNamespaces(t *testing.T) { } } } + +func TestCheckNamespaceExists(t *testing.T) { + // Create a new fake clientset + clientset := fake.NewSimpleClientset() + ctx := context.Background() + + // Create label for test1-namespace + ns1Labels := map[string]string{ + "namespace": "ns1", + "app": "my-application", + "version": "v1", + } + + // Create the ns1 namespace using the fake clientset + _, err := clientset.CoreV1().Namespaces().Create(ctx, &v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ns1", + Labels: ns1Labels, + }, + }, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Failed to create namespace: %v", err) + } + + // This block is to test the successful case i.e. where the searched namespace exists + { + nsList := []string{"ns1"} + hasErrors := checkNamespaceExists(clientset, nsList) + if hasErrors { + t.Errorf("Expected namespaces-list %v to be present, got error", nsList) + } + } + + // This block is to test the failure case i.e. where the searched namespace does not exists + { + nsList := []string{"ns2"} + hasErrors := checkNamespaceExists(clientset, nsList) + if !hasErrors { + t.Errorf("Expected namespaces-list %v to be absent, but got no errors", nsList) + } + } +} From 8567a45e62f9d342b597bac5bb977a8fdee4f911 Mon Sep 17 00:00:00 2001 From: Madhu RAJAGOPAL <m.rajagopal@f5.com> Date: Thu, 2 May 2024 15:40:30 +1200 Subject: [PATCH 26/26] Issue 4837: chore: main_test.go unit-tests * Added test for createConfigClient, processDefaultServerSecret, processWildcardSecret --- cmd/nginx-ingress/main.go | 37 +++++------ cmd/nginx-ingress/main_test.go | 110 ++++++++++++++++++++++++++++----- 2 files changed, 110 insertions(+), 37 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 5f59c3fd5c..6c8b7345cd 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -135,12 +135,12 @@ func main() { var agentVersion string if *agent { - agentVersion = getAgentVersionInfo(nginxManager) + agentVersion = nginxManager.AgentVersion() } go updateSelfWithVersionInfo(kubeClient, version, appProtectVersion, agentVersion, nginxVersion, 10, time.Second*5) - templateExecutor, err := createV1TemplateExecutors() + templateExecutorV1, err := createV1TemplateExecutors() if err != nil { glog.Fatal(err) } @@ -150,12 +150,15 @@ func main() { glog.Fatal(err) } - sslRejectHandshake, err := processDefaultServerSecret(kubeClient, nginxManager) + kAPI := &KubernetesAPI{ + Client: kubeClient, + } + sslRejectHandshake, err := kAPI.processDefaultServerSecret(nginxManager) if err != nil { glog.Fatal(err) } - isWildcardEnabled, err := processWildcardSecret(kubeClient, nginxManager) + isWildcardEnabled, err := kAPI.processWildcardSecret(nginxManager) if err != nil { glog.Fatal(err) } @@ -167,7 +170,7 @@ func main() { } cfgParams := configs.NewDefaultConfigParams(*nginxPlus) - cfgParams, err = processConfigMaps(kubeClient, cfgParams, nginxManager, templateExecutor) + cfgParams, err = kAPI.processConfigMaps(cfgParams, nginxManager, templateExecutorV1) if err != nil { glog.Fatal(err) } @@ -198,7 +201,7 @@ func main() { NginxVersion: nginxVersion, } - if err := processNginxConfig(staticCfgParams, cfgParams, templateExecutor, nginxManager); err != nil { + if err := processNginxConfig(staticCfgParams, cfgParams, templateExecutorV1, nginxManager); err != nil { glog.Fatal(err) } @@ -219,7 +222,7 @@ func main() { NginxManager: nginxManager, StaticCfgParams: staticCfgParams, Config: cfgParams, - TemplateExecutor: templateExecutor, + TemplateExecutor: templateExecutorV1, TemplateExecutorV2: templateExecutorV2, LatencyCollector: latencyCollector, LabelUpdater: plusCollector, @@ -336,7 +339,7 @@ func getKubeClient(config *rest.Config) (kubeClient *kubernetes.Clientset, err e return kubeClient, err } -// This function checks that KIC is running on at least a prescribed minimum k8s version or higher for supportability +// This function checks that NIC is running on at least a prescribed minimum k8s version or higher for supportability // Anything lower throws than the prescribed version, an error is returned to the caller func confirmMinimumK8sVersionCriteria(kubeClient kubernetes.Interface) (err error) { k8sVersion, err := k8s.GetK8sVersion(kubeClient) @@ -545,10 +548,6 @@ func getAppProtectVersionInfo(fd FileHandle) (version string, err error) { return version, err } -func getAgentVersionInfo(nginxManager nginx.Manager) string { - return nginxManager.AgentVersion() -} - type childProcesses struct { nginxDone chan error aPPluginEnable bool @@ -598,12 +597,9 @@ func startChildProcesses(nginxManager nginx.Manager) childProcesses { // Applies the server secret config as provided via the cmdline option -default-server-tls-secret or the default // Returns a boolean for rejecting the SSL handshake -func processDefaultServerSecret(kubeClient *kubernetes.Clientset, nginxManager nginx.Manager) (sslRejectHandshake bool, err error) { +func (kAPI KubernetesAPI) processDefaultServerSecret(nginxManager nginx.Manager) (sslRejectHandshake bool, err error) { sslRejectHandshake = false if *defaultServerSecret != "" { - kAPI := &KubernetesAPI{ - Client: kubeClient, - } secret, err := kAPI.getAndValidateSecret(*defaultServerSecret) if err != nil { return sslRejectHandshake, fmt.Errorf("error trying to get the default server TLS secret %v: %w", *defaultServerSecret, err) @@ -627,12 +623,9 @@ func processDefaultServerSecret(kubeClient *kubernetes.Clientset, nginxManager n // Applies the wildcard server secret config as provided via the cmdline option -wildcard-tls-secret or the default // Returns a boolean for rejecting the SSL handshake -func processWildcardSecret(kubeClient *kubernetes.Clientset, nginxManager nginx.Manager) (isWildcardTLSSecret bool, err error) { +func (kAPI KubernetesAPI) processWildcardSecret(nginxManager nginx.Manager) (isWildcardTLSSecret bool, err error) { isWildcardTLSSecret = false if *wildcardTLSSecret != "" { - kAPI := &KubernetesAPI{ - Client: kubeClient, - } secret, err := kAPI.getAndValidateSecret(*wildcardTLSSecret) if err != nil { return isWildcardTLSSecret, fmt.Errorf("error trying to get the wildcard TLS secret %v: %w", *wildcardTLSSecret, err) @@ -927,13 +920,13 @@ func processGlobalConfiguration() (err error) { // Parses a ConfigMap resource for customizing NGINX configuration provided via the cmdline option -nginx-configmaps // Returns an error if unable to parse the ConfigMap -func processConfigMaps(kubeClient *kubernetes.Clientset, cfgParams *configs.ConfigParams, nginxManager nginx.Manager, templateExecutor *version1.TemplateExecutor) (*configs.ConfigParams, error) { +func (kAPI KubernetesAPI) processConfigMaps(cfgParams *configs.ConfigParams, nginxManager nginx.Manager, templateExecutor *version1.TemplateExecutor) (*configs.ConfigParams, error) { if *nginxConfigMaps != "" { ns, name, err := k8s.ParseNamespaceName(*nginxConfigMaps) if err != nil { return nil, fmt.Errorf("error parsing the nginx-configmaps argument: %w", err) } - cfm, err := kubeClient.CoreV1().ConfigMaps(ns).Get(context.TODO(), name, meta_v1.GetOptions{}) + cfm, err := kAPI.Client.CoreV1().ConfigMaps(ns).Get(context.TODO(), name, meta_v1.GetOptions{}) if err != nil { return nil, fmt.Errorf("error when getting %v: %w", *nginxConfigMaps, err) } diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index af2dc52351..37f7dbe6ab 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -22,6 +22,7 @@ import ( "time" "github.com/nginxinc/kubernetes-ingress/internal/k8s" + "github.com/nginxinc/kubernetes-ingress/internal/nginx" "k8s.io/client-go/kubernetes" "k8s.io/client-go/kubernetes/fake" @@ -44,9 +45,14 @@ func TestGetNginxVersionInfo(t *testing.T) { mc := collectors.NewLocalManagerMetricsCollector(constLabels) nginxManager, _ := createNginxManager(mc) nginxInfo, _ := getNginxVersionInfo(nginxManager) + if nginxInfo.String() == "" { t.Errorf("Error when getting nginx version, empty string") } + + if !nginxInfo.IsPlus { + t.Errorf("Error version is not nginx-plus") + } } type MockFileHandle struct { @@ -84,17 +90,6 @@ func TestGetAppProtectVersionInfo(t *testing.T) { t.Errorf("Error reading AppProtect Version file: %v", err) } } - // Test for file reader returning an empty version - //{ - // mockFileHandle := &MockFileHandle{ - // FileContent: []byte("\n"), - // ReadErr: nil, - // } - // version, _ := getAppProtectVersionInfo(mockFileHandle) - // if version == "" { - // t.Errorf("Error with AppProtect Version, is empty") - // } - //} } func TestCreateGlobalConfigurationValidator(t *testing.T) { @@ -387,16 +382,12 @@ func expectedNs(watchNsLabelList string, ns []string) bool { wNs := strings.Split(watchNsLabelList, ",") resultOk := false for _, n := range wNs { - // fmt.Println("list: ", n) nsNameWithDelimiter := strings.Split(n, "=") - // fmt.Println("WithDelimit: ", nsNameWithDelimiter) nsNameOnly := "" if len(nsNameWithDelimiter) > 1 { nsNameOnly = nsNameWithDelimiter[1] - // fmt.Println("name-only: ", nsNameOnly) } isValid := slices.Contains(ns, nsNameOnly) - resultOk = resultOk || isValid } return resultOk @@ -515,3 +506,92 @@ func TestCheckNamespaceExists(t *testing.T) { } } } + +func TestCreateConfigClient(t *testing.T) { + *enableCustomResources = true + { + *proxyURL = "localhost" + config, err := getClientConfig() + if err != nil { + t.Errorf("Failed to get client config: %v", err) + } + + // This code block tests the working scenario + { + _, err := createConfigClient(config) + if err != nil { + t.Errorf("Failed to create client config: %v", err) + } + } + } +} + +func TestCreateNginxManager(t *testing.T) { + constLabels := map[string]string{"class": *ingressClass} + mgrCollector, _, _ := createManagerAndControllerCollectors(constLabels) + nginxMgr, _ := createNginxManager(mgrCollector) + + if nginxMgr == nil { + t.Errorf("Failed to create nginx manager") + } +} + +func TestProcessDefaultServerSecret(t *testing.T) { + kAPI := &KubernetesAPI{ + Client: fake.NewSimpleClientset(), + } + mgr := nginx.NewFakeManager("/etc/nginx") + { + sslRejectHandshake, err := kAPI.processDefaultServerSecret(mgr) + if err != nil { + t.Errorf("Failed to process default server secret: %v", err) + } + + if !sslRejectHandshake { + t.Errorf("Expected sslRejectHandshake to be false") + } + } + + { + *defaultServerSecret = "/etc/nginx/ssl/myNonExistentSecret.crt" + sslRejectHandshake, err := kAPI.processDefaultServerSecret(mgr) + if err == nil { + t.Errorf("Failed to process default server secret") + } + + if sslRejectHandshake { + t.Errorf("Expected sslRejectHandshake to be true") + } + + } +} + +func TestProcessWildcardSecret(t *testing.T) { + kAPI := &KubernetesAPI{ + Client: fake.NewSimpleClientset(), + } + mgr := nginx.NewFakeManager("/etc/nginx") + { + wildcardTLSSecret, err := kAPI.processWildcardSecret(mgr) + if err != nil { + t.Errorf("Failed to process wildcard server secret: %v", err) + } + + if wildcardTLSSecret { + t.Errorf("Expected wildcardTLSSecret to be false") + } + } + + { + *wildcardTLSSecret = "/etc/nginx/ssl/myNonExistentSecret.crt" + wildcardTLSSecret, err := kAPI.processWildcardSecret(mgr) + if err == nil { + t.Errorf("Failed to process wildcard server secret, expected error") + } + + if wildcardTLSSecret { + t.Errorf("Expected wildcardTLSSecret to be false") + } + + } +}