diff --git a/charts/ingress-nginx/templates/controller-daemonset.yaml b/charts/ingress-nginx/templates/controller-daemonset.yaml index a9a3dee399..86ef46194b 100644 --- a/charts/ingress-nginx/templates/controller-daemonset.yaml +++ b/charts/ingress-nginx/templates/controller-daemonset.yaml @@ -34,6 +34,9 @@ spec: labels: {{- include "ingress-nginx.labels" . | nindent 8 }} app.kubernetes.io/component: controller + {{- if not .Values.controller.disableLeaderElection }} + nginx.ingress.kubernetes.io/electionID: {{ include "ingress-nginx.controller.electionID" . }} + {{- end }} {{- with .Values.controller.labels }} {{- toYaml . | nindent 8 }} {{- end }} diff --git a/charts/ingress-nginx/templates/controller-deployment.yaml b/charts/ingress-nginx/templates/controller-deployment.yaml index 224694d1b3..d81ffb9b9d 100644 --- a/charts/ingress-nginx/templates/controller-deployment.yaml +++ b/charts/ingress-nginx/templates/controller-deployment.yaml @@ -40,6 +40,9 @@ spec: labels: {{- include "ingress-nginx.labels" . | nindent 8 }} app.kubernetes.io/component: controller + {{- if not .Values.controller.disableLeaderElection }} + nginx.ingress.kubernetes.io/electionID: {{ include "ingress-nginx.controller.electionID" . }} + {{- end }} {{- with .Values.controller.labels }} {{- toYaml . | nindent 8 }} {{- end }} diff --git a/charts/ingress-nginx/tests/controller-daemonset_test.yaml b/charts/ingress-nginx/tests/controller-daemonset_test.yaml index 9f79a3b23d..303a4e133d 100644 --- a/charts/ingress-nginx/tests/controller-daemonset_test.yaml +++ b/charts/ingress-nginx/tests/controller-daemonset_test.yaml @@ -208,3 +208,30 @@ tests: - equal: path: spec.template.spec.runtimeClassName value: myClass + + - it: should create a DaemonSet with the electionID label on the pod template when leader election is enabled + set: + controller.kind: DaemonSet + controller.disableLeaderElection: false + asserts: + - equal: + path: spec.template.metadata.labels['nginx.ingress.kubernetes.io/electionID'] + value: RELEASE-NAME-ingress-nginx-leader + + - it: should create a DaemonSet with the custom electionID label on the pod template when controller.electionID is set and leader election is enabled + set: + controller.kind: DaemonSet + controller.disableLeaderElection: false + controller.electionID: custom-election-id + asserts: + - equal: + path: spec.template.metadata.labels['nginx.ingress.kubernetes.io/electionID'] + value: custom-election-id + + - it: should create a DaemonSet without the electionID label on the pod template when leader election is disabled + set: + controller.kind: DaemonSet + controller.disableLeaderElection: true + asserts: + - notExists: + path: spec.template.metadata.labels['nginx.ingress.kubernetes.io/electionID'] diff --git a/charts/ingress-nginx/tests/controller-deployment_test.yaml b/charts/ingress-nginx/tests/controller-deployment_test.yaml index 37b6908853..a52f70ff02 100644 --- a/charts/ingress-nginx/tests/controller-deployment_test.yaml +++ b/charts/ingress-nginx/tests/controller-deployment_test.yaml @@ -231,3 +231,27 @@ tests: - equal: path: spec.template.spec.runtimeClassName value: myClass + + - it: should create a Deployment with the electionID label on the pod template when leader election is enabled + set: + controller.disableLeaderElection: false + asserts: + - equal: + path: spec.template.metadata.labels['nginx.ingress.kubernetes.io/electionID'] + value: RELEASE-NAME-ingress-nginx-leader + + - it: should create a Deployment with the custom electionID label on the pod template when controller.electionID is set and leader election is enabled + set: + controller.disableLeaderElection: false + controller.electionID: custom-election-id + asserts: + - equal: + path: spec.template.metadata.labels['nginx.ingress.kubernetes.io/electionID'] + value: custom-election-id + + - it: should create a Deployment without the electionID label on the pod template when leader election is disabled + set: + controller.disableLeaderElection: true + asserts: + - notExists: + path: spec.template.metadata.labels['nginx.ingress.kubernetes.io/electionID'] diff --git a/docs/user-guide/cli-arguments.md b/docs/user-guide/cli-arguments.md index a33e75159b..d8ae56abb8 100644 --- a/docs/user-guide/cli-arguments.md +++ b/docs/user-guide/cli-arguments.md @@ -68,6 +68,7 @@ They are set in the container spec of the `ingress-nginx-controller` Deployment | `--udp-services-configmap` | Name of the ConfigMap containing the definition of the UDP services to expose. The key in the map indicates the external port to be used. The value is a reference to a Service in the form "namespace/name:port", where "port" can either be a port name or number. | | `--update-status` | Update the load-balancer status of Ingress objects this controller satisfies. Requires setting the publish-service parameter to a valid Service reference. (default true) | | `--update-status-on-shutdown` | Update the load-balancer status of Ingress objects when the controller shuts down. Requires the update-status parameter. (default true) | +| `--use-election-id-selector-on-shutdown` | Determine if other pods are running based on the electionID label, rather than all pod labels. When true, the controller looks for pods with the specific nginx.ingress.kubernetes.io/electionID label matching the controller's election ID. When false, it uses all standard Kubernetes app labels to identify other controller pods. (default true) | | `--shutdown-grace-period` | Seconds to wait after receiving the shutdown signal, before stopping the nginx process. (default 0) | | `--size-buckets` | Set of buckets which will be used for prometheus histogram metrics such as BytesSent. (default `[10, 100, 1000, 10000, 100000, 1e+06, 1e+07]`) | | `-v, --v Level` | number for the log level verbosity | diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index 0894c0dfc1..322bebab8e 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -143,6 +143,8 @@ type Configuration struct { DisableSyncEvents bool + UseElectionIDSelectorOnShutdown bool + EnableTopologyAwareRouting bool } diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go index 20fad5afb8..8e96b4d151 100644 --- a/internal/ingress/controller/nginx.go +++ b/internal/ingress/controller/nginx.go @@ -146,12 +146,14 @@ func NewNGINXController(config *Configuration, mc metric.Collector) *NGINXContro if config.UpdateStatus { n.syncStatus = status.NewStatusSyncer(status.Config{ - Client: config.Client, - PublishService: config.PublishService, - PublishStatusAddress: config.PublishStatusAddress, - IngressLister: n.store, - UpdateStatusOnShutdown: config.UpdateStatusOnShutdown, - UseNodeInternalIP: config.UseNodeInternalIP, + Client: config.Client, + PublishService: config.PublishService, + PublishStatusAddress: config.PublishStatusAddress, + IngressLister: n.store, + UpdateStatusOnShutdown: config.UpdateStatusOnShutdown, + UseNodeInternalIP: config.UseNodeInternalIP, + UseElectionIDSelectorOnShutdown: config.UseElectionIDSelectorOnShutdown, + ElectionID: config.ElectionID, }) } else { klog.Warning("Update of Ingress status is disabled (flag --update-status)") diff --git a/internal/ingress/status/status.go b/internal/ingress/status/status.go index ef01cdd248..b7ceb6e1b0 100644 --- a/internal/ingress/status/status.go +++ b/internal/ingress/status/status.go @@ -40,6 +40,11 @@ import ( "k8s.io/ingress-nginx/pkg/apis/ingress" ) +const ( + // ElectionIDLabelKey is the label key used to identify controller pods by election ID + ElectionIDLabelKey = "nginx.ingress.kubernetes.io/electionID" +) + // UpdateInterval defines the time interval, in seconds, in // which the status should check if an update is required. var UpdateInterval = 60 @@ -68,6 +73,10 @@ type Config struct { UseNodeInternalIP bool + UseElectionIDSelectorOnShutdown bool + + ElectionID string + IngressLister ingressLister } @@ -175,6 +184,35 @@ func nameOrIPToLoadBalancerIngress(nameOrIP string) v1.IngressLoadBalancerIngres // runningAddresses returns a list of IP addresses and/or FQDN where the // ingress controller is currently running +// listControllerPods returns a list of running pods with controller labels +func (s *statusSync) listControllerPods(useElectionID bool) (*apiv1.PodList, error) { + podLabel := make(map[string]string) + + if useElectionID { + // When using electionID, only look for pods with the electionID label + // This is more specific and will correctly identify pods belonging to the same controller group + // Note: This requires the electionID label to be set on the pods (done by helm chart) + podLabel[ElectionIDLabelKey] = s.ElectionID + } else { + // As a standard, app.kubernetes.io are "reserved well-known" labels. + // In our case, we add those labels as identifiers of the Ingress + // deployment in this namespace, so we can select it as a set of Ingress instances. + // As those labels are also generated as part of a HELM deployment, we can be "safe" they + // cover 95% of the cases + for k, v := range k8s.IngressPodDetails.Labels { + // Skip labels that are frequently modified by deployment controllers + if k != "pod-template-hash" && k != "controller-revision-hash" && k != "pod-template-generation" { + podLabel[k] = v + } + } + } + return s.Client.CoreV1().Pods(k8s.IngressPodDetails.Namespace).List( + context.TODO(), + metav1.ListOptions{ + LabelSelector: labels.SelectorFromSet(podLabel).String(), + }) +} + func (s *statusSync) runningAddresses() ([]v1.IngressLoadBalancerIngress, error) { if s.PublishStatusAddress != "" { re := regexp.MustCompile(`,\s*`) @@ -191,9 +229,7 @@ func (s *statusSync) runningAddresses() ([]v1.IngressLoadBalancerIngress, error) } // get information about all the pods running the ingress controller - pods, err := s.Client.CoreV1().Pods(k8s.IngressPodDetails.Namespace).List(context.TODO(), metav1.ListOptions{ - LabelSelector: labels.SelectorFromSet(k8s.IngressPodDetails.Labels).String(), - }) + pods, err := s.listControllerPods(s.UseElectionIDSelectorOnShutdown) if err != nil { return nil, err } @@ -230,21 +266,7 @@ func (s *statusSync) runningAddresses() ([]v1.IngressLoadBalancerIngress, error) } func (s *statusSync) isRunningMultiplePods() bool { - // As a standard, app.kubernetes.io are "reserved well-known" labels. - // In our case, we add those labels as identifiers of the Ingress - // deployment in this namespace, so we can select it as a set of Ingress instances. - // As those labels are also generated as part of a HELM deployment, we can be "safe" they - // cover 95% of the cases - podLabel := make(map[string]string) - for k, v := range k8s.IngressPodDetails.Labels { - if k != "pod-template-hash" && k != "controller-revision-hash" && k != "pod-template-generation" { - podLabel[k] = v - } - } - - pods, err := s.Client.CoreV1().Pods(k8s.IngressPodDetails.Namespace).List(context.TODO(), metav1.ListOptions{ - LabelSelector: labels.SelectorFromSet(podLabel).String(), - }) + pods, err := s.listControllerPods(s.UseElectionIDSelectorOnShutdown) // Use election ID-compatible labels if configured if err != nil { return false } diff --git a/internal/ingress/status/status_test.go b/internal/ingress/status/status_test.go index 01419708b3..7cbbd25247 100644 --- a/internal/ingress/status/status_test.go +++ b/internal/ingress/status/status_test.go @@ -18,6 +18,7 @@ package status import ( "context" + "fmt" "reflect" "testing" "time" @@ -740,3 +741,216 @@ func TestIngressSliceEqual(t *testing.T) { } } } + +func TestListControllerPods(t *testing.T) { + testCases := []struct { + name string + useElectionIDSelector bool + electionID string + podLabels map[string]string + expectedLabelSelectorContains string + }{ + { + name: "use election ID selector", + useElectionIDSelector: true, + electionID: "test-election-id", + podLabels: map[string]string{ + "app": "ingress-nginx", + "pod-template-hash": "abc123", + ElectionIDLabelKey: "test-election-id", + }, + expectedLabelSelectorContains: ElectionIDLabelKey, + }, + { + name: "don't use election ID selector", + useElectionIDSelector: false, + electionID: "test-election-id", + podLabels: map[string]string{ + "app": "ingress-nginx", + "pod-template-hash": "abc123", + }, + expectedLabelSelectorContains: "app=ingress-nginx", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Create a client with a single pod + client := testclient.NewSimpleClientset( + &apiv1.PodList{Items: []apiv1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "ingress-controller", + Namespace: apiv1.NamespaceDefault, + Labels: tc.podLabels, + }, + }, + }}, + ) + + // Store the old pod details and set new ones for testing + origPodDetails := k8s.IngressPodDetails + defer func() { k8s.IngressPodDetails = origPodDetails }() + k8s.IngressPodDetails = &k8s.PodInfo{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ingress-controller", + Namespace: apiv1.NamespaceDefault, + Labels: tc.podLabels, + }, + } + + // Create a status sync with our test configuration + st := &statusSync{ + Config: Config{ + Client: client, + UseElectionIDSelectorOnShutdown: tc.useElectionIDSelector, + ElectionID: tc.electionID, + }, + } + + // Call listControllerPods + podList, err := st.listControllerPods(tc.useElectionIDSelector) + if err != nil { + t.Fatalf("listControllerPods returned an error: %v", err) + } + + // Verify the result + if tc.useElectionIDSelector { + // Should have exactly one pod when electionID selector matches + if len(podList.Items) != 1 && tc.podLabels[ElectionIDLabelKey] == tc.electionID { + t.Errorf("expected 1 pod but got %d", len(podList.Items)) + } + // Should have no pods when electionID selector doesn't match + if len(podList.Items) != 0 && tc.podLabels[ElectionIDLabelKey] != tc.electionID { + t.Errorf("expected 0 pods but got %d", len(podList.Items)) + } + } else if len(podList.Items) != 1 { + t.Errorf("expected 1 pod but got %d", len(podList.Items)) + } + }) + } +} + +// TestIsRunningMultiplePods tests the isRunningMultiplePods function with different label selectors +func TestIsRunningMultiplePods(t *testing.T) { + testCases := []struct { + name string + useElectionID bool + electionID string + podLabels []map[string]string + expectedMultiple bool + }{ + { + name: "multiple pods with same electionID", + useElectionID: true, + electionID: "test-election-id", + podLabels: []map[string]string{ + { + "app": "ingress-nginx", + "pod-template-hash": "abc123", + ElectionIDLabelKey: "test-election-id", + }, + { + "app": "ingress-nginx", + "pod-template-hash": "def456", + ElectionIDLabelKey: "test-election-id", + }, + }, + expectedMultiple: true, + }, + { + name: "multiple pods with different electionIDs", + useElectionID: true, + electionID: "test-election-id", + podLabels: []map[string]string{ + { + "app": "ingress-nginx", + "pod-template-hash": "abc123", + ElectionIDLabelKey: "test-election-id", + }, + { + "app": "ingress-nginx", + "pod-template-hash": "def456", + ElectionIDLabelKey: "other-election-id", + }, + }, + expectedMultiple: false, + }, + { + name: "single pod with matching electionID", + useElectionID: true, + electionID: "test-election-id", + podLabels: []map[string]string{ + { + "app": "ingress-nginx", + "pod-template-hash": "abc123", + ElectionIDLabelKey: "test-election-id", + }, + }, + expectedMultiple: false, + }, + { + name: "multiple pods when not using electionID", + useElectionID: false, + electionID: "test-election-id", + podLabels: []map[string]string{ + { + "app": "ingress-nginx", + "pod-template-hash": "abc123", + }, + { + "app": "ingress-nginx", + "pod-template-hash": "def456", + }, + }, + expectedMultiple: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Create pods for this test case + var pods []apiv1.Pod + for i, labels := range tc.podLabels { + pods = append(pods, apiv1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("ingress-controller-%d", i), + Namespace: apiv1.NamespaceDefault, + Labels: labels, + }, + Status: apiv1.PodStatus{ + Phase: apiv1.PodRunning, + }, + }) + } + + client := testclient.NewSimpleClientset(&apiv1.PodList{Items: pods}) + + // Store the old pod details and set new ones for testing + origPodDetails := k8s.IngressPodDetails + defer func() { k8s.IngressPodDetails = origPodDetails }() + k8s.IngressPodDetails = &k8s.PodInfo{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ingress-controller-0", + Namespace: apiv1.NamespaceDefault, + Labels: tc.podLabels[0], + }, + } + + // Create a status sync with our test configuration + st := &statusSync{ + Config: Config{ + Client: client, + UseElectionIDSelectorOnShutdown: tc.useElectionID, + ElectionID: tc.electionID, + }, + } + + // Test the isRunningMultiplePods method + result := st.isRunningMultiplePods() + if result != tc.expectedMultiple { + t.Errorf("isRunningMultiplePods returned %v but expected %v", result, tc.expectedMultiple) + } + }) + } +} diff --git a/pkg/flags/flags.go b/pkg/flags/flags.go index 376484649b..9ab4b0005f 100644 --- a/pkg/flags/flags.go +++ b/pkg/flags/flags.go @@ -232,6 +232,8 @@ Takes the form ":port". If not provided, no admission controller is starte disableSyncEvents = flags.Bool("disable-sync-events", false, "Disables the creation of 'Sync' event resources") + useElectionIDSelectorOnShutdown = flags.Bool("use-election-id-selector-on-shutdown", true, "Determine if other pods are running based on the electionID label, rather than all pod labels.") + enableTopologyAwareRouting = flags.Bool("enable-topology-aware-routing", false, "Enable topology aware routing feature, needs service object annotation service.kubernetes.io/topology-mode sets to auto or trafficDistribution.") ) @@ -377,6 +379,7 @@ https://blog.maxmind.com/2019/12/significant-changes-to-accessing-and-using-geol HealthCheckHost: *healthzHost, DynamicConfigurationRetries: *dynamicConfigurationRetries, EnableTopologyAwareRouting: *enableTopologyAwareRouting, + UseElectionIDSelectorOnShutdown: *useElectionIDSelectorOnShutdown, ListenPorts: &ngx_config.ListenPorts{ Default: *defServerPort, Health: *healthzPort,