From d7d4b74f789921a4e94ad575ba4cc91de67634ae Mon Sep 17 00:00:00 2001 From: Kate Osborn Date: Thu, 3 Sep 2020 17:55:13 -0600 Subject: [PATCH] Add pod_owner label to metrics when -spire-agent-address is set --- cmd/nginx-ingress/main.go | 5 +- internal/configs/config_params.go | 2 +- internal/configs/configurator.go | 16 +++- internal/configs/configurator_test.go | 12 +-- internal/configs/ingress.go | 8 +- internal/configs/ingress_test.go | 24 +++--- internal/configs/virtualserver.go | 20 ++++- internal/configs/virtualserver_test.go | 2 +- internal/k8s/controller.go | 68 +++++++++++++++-- internal/k8s/controller_test.go | 102 +++++++++++++++++++++++++ 10 files changed, 220 insertions(+), 39 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index d63af89e4a..2e7a93123e 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -500,7 +500,7 @@ func main() { StubStatusOverUnixSocketForOSS: *enablePrometheusMetrics, TLSPassthrough: *enableTLSPassthrough, EnableSnippets: *enableSnippets, - SpiffeCerts: *spireAgentAddress != "", + NginxServiceMesh: *spireAgentAddress != "", MainAppProtectLoadModule: *appProtect, EnableLatencyMetrics: *enableLatencyMetrics, } @@ -548,6 +548,9 @@ func main() { if *enablePrometheusMetrics { upstreamServerVariableLabels := []string{"service", "resource_type", "resource_name", "resource_namespace"} upstreamServerPeerVariableLabelNames := []string{"pod_name"} + if staticCfgParams.NginxServiceMesh { + upstreamServerPeerVariableLabelNames = append(upstreamServerPeerVariableLabelNames, "pod_owner") + } if *nginxPlus { serverZoneVariableLabels := []string{"resource_type", "resource_name", "resource_namespace"} variableLabelNames := nginxCollector.NewVariableLabelNames(upstreamServerVariableLabels, serverZoneVariableLabels, upstreamServerPeerVariableLabelNames) diff --git a/internal/configs/config_params.go b/internal/configs/config_params.go index b8723901f1..14d65f2f90 100644 --- a/internal/configs/config_params.go +++ b/internal/configs/config_params.go @@ -109,7 +109,7 @@ type StaticConfigParams struct { StubStatusOverUnixSocketForOSS bool TLSPassthrough bool EnableSnippets bool - SpiffeCerts bool + NginxServiceMesh bool EnableInternalRoutes bool MainAppProtectLoadModule bool PodName string diff --git a/internal/configs/configurator.go b/internal/configs/configurator.go index 056c14e6bf..26d8053e2b 100644 --- a/internal/configs/configurator.go +++ b/internal/configs/configurator.go @@ -164,9 +164,13 @@ func (cnf *Configurator) updateIngressMetricsLabels(ingEx *IngressEx, upstreams newUpstreamsNames = append(newUpstreamsNames, u.Name) for _, server := range u.UpstreamServers { s := fmt.Sprintf("%v:%v", server.Address, server.Port) - podName := ingEx.PodsByIP[s] + podInfo := ingEx.PodsByIP[s] labelKey := fmt.Sprintf("%v/%v", u.Name, s) - upstreamServerPeerLabels[labelKey] = []string{podName} + upstreamServerPeerLabels[labelKey] = []string{podInfo.Name} + if cnf.staticCfgParams.NginxServiceMesh { + ownerLabelVal := fmt.Sprintf("%s/%s", podInfo.OwnerType, podInfo.OwnerName) + upstreamServerPeerLabels[labelKey] = append(upstreamServerPeerLabels[labelKey], ownerLabelVal) + } newPeers[labelKey] = true newPeersIPs = append(newPeersIPs, labelKey) } @@ -314,9 +318,13 @@ func (cnf *Configurator) updateVirtualServerMetricsLabels(virtualServerEx *Virtu newUpstreams[u.Name] = true newUpstreamsNames = append(newUpstreamsNames, u.Name) for _, server := range u.Servers { - podName := virtualServerEx.PodsByIP[server.Address] + podInfo := virtualServerEx.PodsByIP[server.Address] labelKey := fmt.Sprintf("%v/%v", u.Name, server.Address) - upstreamServerPeerLabels[labelKey] = []string{podName} + upstreamServerPeerLabels[labelKey] = []string{podInfo.Name} + if cnf.staticCfgParams.NginxServiceMesh { + ownerLabelVal := fmt.Sprintf("%s/%s", podInfo.OwnerType, podInfo.OwnerName) + upstreamServerPeerLabels[labelKey] = append(upstreamServerPeerLabels[labelKey], ownerLabelVal) + } newPeers[labelKey] = true newPeersIPs = append(newPeersIPs, labelKey) } diff --git a/internal/configs/configurator_test.go b/internal/configs/configurator_test.go index 6b24d3ebd7..4235a89037 100644 --- a/internal/configs/configurator_test.go +++ b/internal/configs/configurator_test.go @@ -640,9 +640,9 @@ func TestUpdateIngressMetricsLabels(t *testing.T) { }, }, }, - PodsByIP: map[string]string{ - "10.0.0.1:80": "pod-1", - "10.0.0.2:80": "pod-2", + PodsByIP: map[string]PodInfo{ + "10.0.0.1:80": {Name: "pod-1"}, + "10.0.0.2:80": {Name: "pod-2"}, }, } @@ -800,9 +800,9 @@ func TestUpdateVirtualServerMetricsLabels(t *testing.T) { Host: "example.com", }, }, - PodsByIP: map[string]string{ - "10.0.0.1:80": "pod-1", - "10.0.0.2:80": "pod-2", + PodsByIP: map[string]PodInfo{ + "10.0.0.1:80": {Name: "pod-1"}, + "10.0.0.2:80": {Name: "pod-2"}, }, } diff --git a/internal/configs/ingress.go b/internal/configs/ingress.go index 28aa17a626..7b10fcdd07 100644 --- a/internal/configs/ingress.go +++ b/internal/configs/ingress.go @@ -26,7 +26,7 @@ type IngressEx struct { Endpoints map[string][]string HealthChecks map[string]*api_v1.Probe ExternalNameSvcs map[string]bool - PodsByIP map[string]string + PodsByIP map[string]PodInfo AppProtectPolicy *unstructured.Unstructured AppProtectLogConf *unstructured.Unstructured AppProtectLogDst string @@ -250,7 +250,7 @@ func generateNginxCfg(ingEx *IngressEx, pems map[string]string, apResources map[ Namespace: ingEx.Ingress.Namespace, Annotations: ingEx.Ingress.Annotations, }, - SpiffeClientCerts: staticParams.SpiffeCerts && !cfgParams.SpiffeServerCerts, + SpiffeClientCerts: staticParams.NginxServiceMesh && !cfgParams.SpiffeServerCerts, } } @@ -478,10 +478,10 @@ func generateNginxCfgForMergeableIngresses(mergeableIngs *MergeableIngresses, ma Upstreams: upstreams, Keepalive: keepalive, Ingress: masterNginxCfg.Ingress, - SpiffeClientCerts: staticParams.SpiffeCerts && !baseCfgParams.SpiffeServerCerts, + SpiffeClientCerts: staticParams.NginxServiceMesh && !baseCfgParams.SpiffeServerCerts, } } func isSSLEnabled(isSSLService bool, cfgParams ConfigParams, staticCfgParams *StaticConfigParams) bool { - return isSSLService || staticCfgParams.SpiffeCerts && !cfgParams.SpiffeServerCerts + return isSSLService || staticCfgParams.NginxServiceMesh && !cfgParams.SpiffeServerCerts } diff --git a/internal/configs/ingress_test.go b/internal/configs/ingress_test.go index f93109cea6..ba1c24e010 100644 --- a/internal/configs/ingress_test.go +++ b/internal/configs/ingress_test.go @@ -766,7 +766,7 @@ func TestGenerateNginxCfgForSpiffe(t *testing.T) { } apResources := make(map[string]string) - result := generateNginxCfg(&cafeIngressEx, pems, apResources, false, configParams, false, false, "", &StaticConfigParams{SpiffeCerts: true}) + result := generateNginxCfg(&cafeIngressEx, pems, apResources, false, configParams, false, false, "", &StaticConfigParams{NginxServiceMesh: true}) if !reflect.DeepEqual(result, expected) { t.Errorf("generateNginxCfg returned \n%v, but expected \n%v", result, expected) @@ -788,7 +788,7 @@ func TestGenerateNginxCfgForInternalRoute(t *testing.T) { } apResources := make(map[string]string) - result := generateNginxCfg(&cafeIngressEx, pems, apResources, false, configParams, false, false, "", &StaticConfigParams{SpiffeCerts: true, EnableInternalRoutes: true}) + result := generateNginxCfg(&cafeIngressEx, pems, apResources, false, configParams, false, false, "", &StaticConfigParams{NginxServiceMesh: true, EnableInternalRoutes: true}) if !reflect.DeepEqual(result, expected) { t.Errorf("generateNginxCfg returned \n%+v, but expected \n%+v", result, expected) @@ -799,61 +799,61 @@ func TestIsSSLEnabled(t *testing.T) { type testCase struct { IsSSLService, SpiffeServerCerts, - SpiffeCerts, + NginxServiceMesh, Expected bool } var testCases = []testCase{ { IsSSLService: false, SpiffeServerCerts: false, - SpiffeCerts: false, + NginxServiceMesh: false, Expected: false, }, { IsSSLService: false, SpiffeServerCerts: true, - SpiffeCerts: true, + NginxServiceMesh: true, Expected: false, }, { IsSSLService: false, SpiffeServerCerts: false, - SpiffeCerts: true, + NginxServiceMesh: true, Expected: true, }, { IsSSLService: false, SpiffeServerCerts: true, - SpiffeCerts: false, + NginxServiceMesh: false, Expected: false, }, { IsSSLService: true, SpiffeServerCerts: true, - SpiffeCerts: true, + NginxServiceMesh: true, Expected: true, }, { IsSSLService: true, SpiffeServerCerts: false, - SpiffeCerts: true, + NginxServiceMesh: true, Expected: true, }, { IsSSLService: true, SpiffeServerCerts: true, - SpiffeCerts: false, + NginxServiceMesh: false, Expected: true, }, { IsSSLService: true, SpiffeServerCerts: false, - SpiffeCerts: false, + NginxServiceMesh: false, Expected: true, }, } for i, tc := range testCases { - actual := isSSLEnabled(tc.IsSSLService, ConfigParams{SpiffeServerCerts: tc.SpiffeServerCerts}, &StaticConfigParams{SpiffeCerts: tc.SpiffeCerts}) + actual := isSSLEnabled(tc.IsSSLService, ConfigParams{SpiffeServerCerts: tc.SpiffeServerCerts}, &StaticConfigParams{NginxServiceMesh: tc.NginxServiceMesh}) if actual != tc.Expected { t.Errorf("isSSLEnabled returned %v but expected %v for the case %v", actual, tc.Expected, i) } diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 82e3a73e38..d668b818b6 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -29,6 +29,22 @@ var incompatibleLBMethodsForSlowStart = map[string]bool{ "random two least_time=last_byte": true, } +// MeshPodOwner contains the type and name of the K8s resource that owns the pod. +// This owner information is needed for NGINX Service Mesh metrics. +type MeshPodOwner struct { + // OwnerType is one of the following: statefulset, daemonset, deployment. + OwnerType string + // OwnerName is the name of the statefulset, daemonset, or deployment. + OwnerName string +} + +// PodInfo contains the name of the Pod and the MeshPodOwner information +// which is used for NGINX Service Mesh metrics. +type PodInfo struct { + Name string + MeshPodOwner +} + // VirtualServerEx holds a VirtualServer along with the resources that are referenced in this VirtualServer. type VirtualServerEx struct { VirtualServer *conf_v1.VirtualServer @@ -37,7 +53,7 @@ type VirtualServerEx struct { VirtualServerRoutes []*conf_v1.VirtualServerRoute ExternalNameSvcs map[string]bool Policies map[string]*conf_v1alpha1.Policy - PodsByIP map[string]string + PodsByIP map[string]PodInfo } func (vsx *VirtualServerEx) String() string { @@ -168,7 +184,7 @@ func newVirtualServerConfigurator(cfgParams *ConfigParams, isPlus bool, isResolv isTLSPassthrough: staticParams.TLSPassthrough, enableSnippets: staticParams.EnableSnippets, warnings: make(map[runtime.Object][]string), - spiffeCerts: staticParams.SpiffeCerts, + spiffeCerts: staticParams.NginxServiceMesh, } } diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index 00c23a6a72..3b89a23ac1 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -699,7 +699,7 @@ func TestGenerateVirtualServerConfigWithSpiffeCerts(t *testing.T) { isPlus := false isResolverConfigured := false tlsPemFileName := "" - staticConfigParams := &StaticConfigParams{TLSPassthrough: true, SpiffeCerts: true} + staticConfigParams := &StaticConfigParams{TLSPassthrough: true, NginxServiceMesh: true} vsc := newVirtualServerConfigurator(&baseCfgParams, isPlus, isResolverConfigured, staticConfigParams) result, warnings := vsc.GenerateVirtualServerConfig(&virtualServerEx, tlsPemFileName) if !reflect.DeepEqual(result, expected) { diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 52346c3e22..11cacad7ce 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -89,6 +89,8 @@ var ( type podEndpoint struct { Address string PodName string + // MeshPodOwner is used for NGINX Service Mesh metrics + configs.MeshPodOwner } // LoadBalancerController watches Kubernetes API and @@ -2360,7 +2362,7 @@ func (lbc *LoadBalancerController) createIngress(ing *networking.Ingress) (*conf ingEx.Endpoints = make(map[string][]string) ingEx.HealthChecks = make(map[string]*api_v1.Probe) ingEx.ExternalNameSvcs = make(map[string]bool) - ingEx.PodsByIP = make(map[string]string) + ingEx.PodsByIP = make(map[string]configs.PodInfo) if ing.Spec.Backend != nil { podEndps := []podEndpoint{} @@ -2393,7 +2395,10 @@ func (lbc *LoadBalancerController) createIngress(ing *networking.Ingress) (*conf if lbc.isNginxPlus || lbc.isLatencyMetricsEnabled { for _, endpoint := range podEndps { - ingEx.PodsByIP[endpoint.Address] = endpoint.PodName + ingEx.PodsByIP[endpoint.Address] = configs.PodInfo{ + Name: endpoint.PodName, + MeshPodOwner: endpoint.MeshPodOwner, + } } } } @@ -2440,7 +2445,10 @@ func (lbc *LoadBalancerController) createIngress(ing *networking.Ingress) (*conf if lbc.isNginxPlus || lbc.isLatencyMetricsEnabled { for _, endpoint := range podEndps { - ingEx.PodsByIP[endpoint.Address] = endpoint.PodName + ingEx.PodsByIP[endpoint.Address] = configs.PodInfo{ + Name: endpoint.PodName, + MeshPodOwner: endpoint.MeshPodOwner, + } } } } @@ -2551,7 +2559,7 @@ func (lbc *LoadBalancerController) createVirtualServer(virtualServer *conf_v1.Vi endpoints := make(map[string][]string) externalNameSvcs := make(map[string]bool) - podsByIP := make(map[string]string) + podsByIP := make(map[string]configs.PodInfo) for _, u := range virtualServer.Spec.Upstreams { endpointsKey := configs.GenerateEndpointsKey(virtualServer.Namespace, u.Service, u.Subselector, u.Port) @@ -2579,7 +2587,10 @@ func (lbc *LoadBalancerController) createVirtualServer(virtualServer *conf_v1.Vi if lbc.isNginxPlus || lbc.isLatencyMetricsEnabled { for _, endpoint := range podEndps { - podsByIP[endpoint.Address] = endpoint.PodName + podsByIP[endpoint.Address] = configs.PodInfo{ + Name: endpoint.PodName, + MeshPodOwner: endpoint.MeshPodOwner, + } } } } @@ -2667,7 +2678,10 @@ func (lbc *LoadBalancerController) createVirtualServer(virtualServer *conf_v1.Vi if lbc.isNginxPlus || lbc.isLatencyMetricsEnabled { for _, endpoint := range podEndps { - podsByIP[endpoint.Address] = endpoint.PodName + podsByIP[endpoint.Address] = configs.PodInfo{ + Name: endpoint.PodName, + MeshPodOwner: endpoint.MeshPodOwner, + } } } } @@ -2831,10 +2845,14 @@ func getEndpointsBySubselectedPods(targetPort int32, pods []*api_v1.Pod, svcEps for _, address := range subset.Addresses { if address.IP == pod.Status.PodIP { addr := fmt.Sprintf("%v:%v", pod.Status.PodIP, targetPort) - + ownerType, ownerName := getPodOwnerTypeAndName(pod) podEnd := podEndpoint{ Address: addr, PodName: getPodName(address.TargetRef), + MeshPodOwner: configs.MeshPodOwner{ + OwnerType: ownerType, + OwnerName: ownerName, + }, } endps = append(endps, podEnd) } @@ -2960,7 +2978,12 @@ func (lbc *LoadBalancerController) getEndpointsForPort(endps api_v1.Endpoints, i addr := fmt.Sprintf("%v:%v", address.IP, port.Port) podEnd := podEndpoint{ Address: addr, - PodName: address.TargetRef.Name, + } + if address.TargetRef != nil { + parentType, parentName := lbc.getPodOwnerTypeAndNameFromAddress(address.TargetRef.Namespace, address.TargetRef.Name) + podEnd.OwnerType = parentType + podEnd.OwnerName = parentName + podEnd.PodName = address.TargetRef.Name } endpoints = append(endpoints, podEnd) } @@ -2972,6 +2995,35 @@ func (lbc *LoadBalancerController) getEndpointsForPort(endps api_v1.Endpoints, i return nil, fmt.Errorf("No endpoints for target port %v in service %s", targetPort, svc.Name) } +func (lbc *LoadBalancerController) getPodOwnerTypeAndNameFromAddress(ns, name string) (parentType, parentName string) { + obj, exists, err := lbc.podLister.GetByKey(fmt.Sprintf("%s/%s", ns, name)) + if err != nil { + glog.Warningf("could not get pod by key %s/%s: %v", ns, name, err) + return "", "" + } + if exists { + pod := obj.(*api_v1.Pod) + return getPodOwnerTypeAndName(pod) + } + return "", "" +} + +func getPodOwnerTypeAndName(pod *api_v1.Pod) (parentType, parentName string) { + parentType = "deployment" + for _, owner := range pod.GetOwnerReferences() { + parentName = owner.Name + if owner.Controller != nil && *owner.Controller { + if owner.Kind == "StatefulSet" || owner.Kind == "DaemonSet" { + parentType = strings.ToLower(owner.Kind) + } + if owner.Kind == "ReplicaSet" && strings.HasSuffix(owner.Name, pod.Labels["pod-template-hash"]) { + parentName = strings.TrimSuffix(owner.Name, "-"+pod.Labels["pod-template-hash"]) + } + } + } + return parentType, parentName +} + func (lbc *LoadBalancerController) getServicePortForIngressPort(ingSvcPort intstr.IntOrString, svc *api_v1.Service) *api_v1.ServicePort { for _, port := range svc.Spec.Ports { if (ingSvcPort.Type == intstr.Int && port.Port == int32(ingSvcPort.IntValue())) || (ingSvcPort.Type == intstr.String && port.Name == ingSvcPort.String()) { diff --git a/internal/k8s/controller_test.go b/internal/k8s/controller_test.go index cb047d58b0..9aff71452b 100644 --- a/internal/k8s/controller_test.go +++ b/internal/k8s/controller_test.go @@ -1986,6 +1986,7 @@ func TestFormatWarningsMessages(t *testing.T) { } func TestGetEndpointsBySubselectedPods(t *testing.T) { + boolPointer := func(b bool) *bool { return &b } tests := []struct { desc string targetPort int32 @@ -1998,6 +1999,10 @@ func TestGetEndpointsBySubselectedPods(t *testing.T) { expectedEps: []podEndpoint{ { Address: "1.2.3.4:80", + MeshPodOwner: configs.MeshPodOwner{ + OwnerType: "deployment", + OwnerName: "deploy-1", + }, }, }, }, @@ -2010,6 +2015,15 @@ func TestGetEndpointsBySubselectedPods(t *testing.T) { pods := []*v1.Pod{ { + ObjectMeta: meta_v1.ObjectMeta{ + OwnerReferences: []meta_v1.OwnerReference{ + { + Kind: "Deployment", + Name: "deploy-1", + Controller: boolPointer(true), + }, + }, + }, Status: v1.PodStatus{ PodIP: "1.2.3.4", }, @@ -2231,6 +2245,94 @@ func TestCreatePolicyMap(t *testing.T) { } } +func TestGetPodOwnerTypeAndName(t *testing.T) { + tests := []struct { + desc string + expType string + expName string + pod *v1.Pod + }{ + { + desc: "deployment", + expType: "deployment", + expName: "deploy-name", + pod: &v1.Pod{ObjectMeta: createTestObjMeta("Deployment", "deploy-name", true)}, + }, + { + desc: "stateful set", + expType: "statefulset", + expName: "statefulset-name", + pod: &v1.Pod{ObjectMeta: createTestObjMeta("StatefulSet", "statefulset-name", true)}, + }, + { + desc: "daemon set", + expType: "daemonset", + expName: "daemonset-name", + pod: &v1.Pod{ObjectMeta: createTestObjMeta("DaemonSet", "daemonset-name", true)}, + }, + { + desc: "replica set with no pod hash", + expType: "deployment", + expName: "replicaset-name", + pod: &v1.Pod{ObjectMeta: createTestObjMeta("ReplicaSet", "replicaset-name", false)}, + }, + { + desc: "replica set with pod hash", + expType: "deployment", + expName: "replicaset-name", + pod: &v1.Pod{ + ObjectMeta: createTestObjMeta("ReplicaSet", "replicaset-name-67c6f7c5fd", true), + }, + }, + { + desc: "nil controller should use default values", + expType: "deployment", + expName: "deploy-name", + pod: &v1.Pod{ + ObjectMeta: meta_v1.ObjectMeta{ + OwnerReferences: []meta_v1.OwnerReference{ + { + Name: "deploy-name", + Controller: nil, + }, + }, + }, + }, + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + actualType, actualName := getPodOwnerTypeAndName(test.pod) + if actualType != test.expType { + t.Errorf("getPodOwnerTypeAndName() returned %s for owner type but expected %s", actualType, test.expType) + } + if actualName != test.expName { + t.Errorf("getPodOwnerTypeAndName() returned %s for owner name but expected %s", actualName, test.expName) + } + }) + } +} + +func createTestObjMeta(kind, name string, podHashLabel bool) meta_v1.ObjectMeta { + controller := true + meta := meta_v1.ObjectMeta{ + OwnerReferences: []meta_v1.OwnerReference{ + { + Kind: kind, + Name: name, + Controller: &controller, + }, + }, + } + if podHashLabel { + meta.Labels = map[string]string{ + "pod-template-hash": "67c6f7c5fd", + } + } + return meta +} + func policyMapToString(policies map[string]*conf_v1alpha1.Policy) string { var keys []string for k := range policies {