From 48c5f70c399aeaa45ee125deca2253a3f9ce96e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel=20Pastor=20Olivar?= Date: Wed, 9 Jun 2021 23:48:43 +0200 Subject: [PATCH] Opaque ports check (#6192) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #6177 This change adds an additional check to the data plane category which will warn users when the `opaque-ports` annotation is misconfigured on services and pods. As an example, the output looks like this: ``` $ linkerd check --proxy ... × opaque ports are properly annotated * service/emoji-svc has the annotation config.linkerd.io/opaque-ports but pod/emoji-696d9d8f95-8p94s doesn't * pod/voting-67bb58c44c-dgt46 and service/voting-svc have the annotation config.linkerd.io/opaque-ports but values don't match see https://linkerd.io/2/checks/#linkerd-opaque-ports-definition for hints ``` Signed-off-by: Miguel Ángel Pastor Olivar --- cli/cmd/check.go | 2 +- pkg/healthcheck/healthcheck.go | 93 +++++ pkg/healthcheck/healthcheck_test.go | 345 +++++++++++++++++- .../testdata/check.cni.proxy.golden | 1 + test/integration/testdata/check.proxy.golden | 1 + 5 files changed, 439 insertions(+), 3 deletions(-) diff --git a/cli/cmd/check.go b/cli/cmd/check.go index defc6ae7a..a1eb3c3ca 100644 --- a/cli/cmd/check.go +++ b/cli/cmd/check.go @@ -186,12 +186,12 @@ func configureAndRunChecks(cmd *cobra.Command, wout io.Writer, werr io.Writer, s if options.dataPlaneOnly { checks = append(checks, healthcheck.LinkerdDataPlaneChecks) checks = append(checks, healthcheck.LinkerdIdentityDataPlane) + checks = append(checks, healthcheck.LinkerdOpaquePortsDefinitionChecks) } else { checks = append(checks, healthcheck.LinkerdControlPlaneVersionChecks) } checks = append(checks, healthcheck.LinkerdCNIPluginChecks) checks = append(checks, healthcheck.LinkerdHAChecks) - } } diff --git a/pkg/healthcheck/healthcheck.go b/pkg/healthcheck/healthcheck.go index df36ce672..e008cad7e 100644 --- a/pkg/healthcheck/healthcheck.go +++ b/pkg/healthcheck/healthcheck.go @@ -136,6 +136,11 @@ const ( /// plugin is installed and ready LinkerdCNIPluginChecks CategoryID = "linkerd-cni-plugin" + // LinkerdOpaquePortsDefinitionChecks adds checks to validate that the + // "opaque ports" annotation has been defined both in the service and the + // corresponding pods + LinkerdOpaquePortsDefinitionChecks CategoryID = "linkerd-opaque-ports-definition" + // LinkerdCNIResourceLabel is the label key that is used to identify // whether a Kubernetes resource is related to the install-cni command // The value is expected to be "true", "false" or "", where "false" and @@ -1379,6 +1384,13 @@ func (hc *HealthChecker) allCategories() []*Category { return checkMisconfiguredServiceAnnotations(services) }, }, + { + description: "opaque ports are properly annotated", + hintAnchor: "linkerd-opaque-ports-definition", + check: func(ctx context.Context) error { + return hc.checkMisconfiguredOpaquePortAnnotations(ctx) + }, + }, }, false, ), @@ -2192,6 +2204,87 @@ func checkResources(resourceName string, objects []runtime.Object, expectedNames return nil } +// Check if there's a pod with the "opaque ports" annotation defined but a +// service selecting the aforementioned pod doesn't define it +func (hc *HealthChecker) checkMisconfiguredOpaquePortAnnotations(ctx context.Context) error { + services, err := hc.GetServices(ctx) + if err != nil { + return err + } + + var errStrings []string + + for _, service := range services { + if service.Spec.ClusterIP == "None" { + // skip headless services; they're handled differently + continue + } + + endpoint, err := hc.kubeAPI.CoreV1().Endpoints(service.Namespace).Get(ctx, service.Name, metav1.GetOptions{}) + + if err != nil { + return err + } + + pods := make([]*corev1.Pod, 0) + for _, subset := range endpoint.Subsets { + for _, addr := range subset.Addresses { + if addr.TargetRef != nil && addr.TargetRef.Kind == "Pod" { + pod, err := hc.kubeAPI.CoreV1().Pods(service.Namespace).Get(ctx, addr.TargetRef.Name, metav1.GetOptions{}) + if err != nil { + return err + } + pods = append(pods, pod) + } + } + } + + if mismatch := misconfiguredOpaquePortAnnotationsInService(service, pods); mismatch != nil { + errStrings = append( + errStrings, + fmt.Sprintf("\t* %s", mismatch.Error()), + ) + } + } + + if len(errStrings) >= 1 { + return fmt.Errorf(strings.Join(errStrings, "\n ")) + } + + return nil +} + +func misconfiguredOpaquePortAnnotationsInService(service corev1.Service, pods []*corev1.Pod) error { + for _, pod := range pods { + if err := misconfiguredOpaqueAnnotation(service, pod); err != nil { + return err + } + } + return nil +} + +func misconfiguredOpaqueAnnotation(service corev1.Service, pod *corev1.Pod) error { + svcAnnotation, svcAnnotationOk := service.Annotations[k8s.ProxyOpaquePortsAnnotation] + podAnnotation, podAnnotationOk := pod.Annotations[k8s.ProxyOpaquePortsAnnotation] + + if svcAnnotationOk && podAnnotationOk { + if svcAnnotation != podAnnotation { + return fmt.Errorf("pod/%s and service/%s have the annotation %s but values don't match", pod.Name, service.Name, k8s.ProxyOpaquePortsAnnotation) + } + + return nil + } + + if svcAnnotationOk { + return fmt.Errorf("service/%s has the annotation %s but pod/%s doesn't", service.Name, k8s.ProxyOpaquePortsAnnotation, pod.Name) + } + if podAnnotationOk { + return fmt.Errorf("pod/%s has the annotation %s but service/%s doesn't", pod.Name, k8s.ProxyOpaquePortsAnnotation, service.Name) + } + + return nil +} + // GetDataPlanePods returns all the pods with data plane func (hc *HealthChecker) GetDataPlanePods(ctx context.Context) ([]corev1.Pod, error) { selector := fmt.Sprintf("%s=%s", k8s.ControllerNSLabel, hc.ControlPlaneNamespace) diff --git a/pkg/healthcheck/healthcheck_test.go b/pkg/healthcheck/healthcheck_test.go index ec2566248..44933da10 100644 --- a/pkg/healthcheck/healthcheck_test.go +++ b/pkg/healthcheck/healthcheck_test.go @@ -1779,12 +1779,11 @@ func TestDataPlanePodLabels(t *testing.T) { tc := tc //pin t.Run(tc.description, func(t *testing.T) { err := checkMisconfiguredPodsLabels(tc.pods) - fmt.Println(err.Error()) if err == nil { t.Fatal("Expected error, got nothing") } - fmt.Println(err.Error()) + if err.Error() != tc.expectedErrorMsg { t.Fatalf("Unexpected error message: %s", err.Error()) } @@ -3303,6 +3302,348 @@ func TestMinReplicaCheck(t *testing.T) { } } +func TestCheckOpaquePortAnnotations(t *testing.T) { + hc := NewHealthChecker( + []CategoryID{LinkerdOpaquePortsDefinitionChecks}, + &Options{ + DataPlaneNamespace: "test-ns", + }, + ) + + var err error + + var testCases = []struct { + resources []string + expected error + }{ + { + resources: []string{` +apiVersion: v1 +kind: Service +metadata: + name: test-service-1 + namespace: test-ns +spec: + ports: + - name: elasticsearch + port: 9200 + protocol: TCP + targetPort: 9200 + selector: + service: service-1 +`, + ` +apiVersion: v1 +kind: Pod +metadata: + name: my-service-deployment + namespace: test-ns + labels: + service: service-1 +spec: + containers: + - name: test + image: "test-service" +`, + ` +apiVersion: v1 +kind: Endpoints +metadata: + annotations: + endpoints.kubernetes.io/last-change-trigger-time: "2021-06-08T08:38:16Z" + creationTimestamp: "2021-06-08T08:38:03Z" + labels: + service: test-service-1 + name: test-service-1 + namespace: test-ns +subsets: +- addresses: + - ip: 10.244.3.12 + nodeName: nodename-1 + targetRef: + kind: Pod + name: my-service-deployment + namespace: test-ns + resourceVersion: "20661" + uid: b37782aa-1458-4153-8399-dabc2b29aaae + ports: + - name: http-port + port: 8080 + protocol: TCP +`, + }, + expected: nil, + }, + { + resources: []string{` +apiVersion: v1 +kind: Service +metadata: + name: test-service-1 + namespace: test-ns + annotations: + config.linkerd.io/opaque-ports: "9200" +spec: + ports: + - name: elasticsearch + port: 9200 + protocol: TCP + targetPort: 9200 + selector: + service: service-1 +`, + ` +apiVersion: v1 +kind: Pod +metadata: + name: my-service-deployment + namespace: test-ns + service: service-1 + labels: + service: service-1 + annotations: + config.linkerd.io/opaque-ports: "9200" +spec: + containers: + - name: test + image: "test-service" +`, + ` +apiVersion: v1 +kind: Endpoints +metadata: + annotations: + endpoints.kubernetes.io/last-change-trigger-time: "2021-06-08T08:38:16Z" + creationTimestamp: "2021-06-08T08:38:03Z" + labels: + service: test-service-1 + name: test-service-1 + namespace: test-ns +subsets: +- addresses: + - ip: 10.244.3.12 + nodeName: nodename-1 + targetRef: + kind: Pod + name: my-service-deployment + namespace: test-ns + resourceVersion: "20661" + uid: b37782aa-1458-4153-8399-dabc2b29aaae + ports: + - name: http-port + port: 8080 + protocol: TCP +`, + }, + expected: nil, + }, + { + resources: []string{` +apiVersion: v1 +kind: Service +metadata: + name: test-service-1 + namespace: test-ns +spec: + ports: + - name: http + port: 9200 + protocol: TCP + targetPort: 9200 + selector: + service: service-1 +`, + ` +apiVersion: v1 +kind: Pod +metadata: + name: my-service-deployment + namespace: test-ns + service: service-1 + labels: + service: service-1 + annotations: + config.linkerd.io/opaque-ports: "9200" +spec: + containers: + - name: test + image: "test-service" +`, + ` +apiVersion: v1 +kind: Endpoints +metadata: + annotations: + endpoints.kubernetes.io/last-change-trigger-time: "2021-06-08T08:38:16Z" + creationTimestamp: "2021-06-08T08:38:03Z" + labels: + service: test-service-1 + name: test-service-1 + namespace: test-ns +subsets: +- addresses: + - ip: 10.244.3.12 + nodeName: nodename-1 + targetRef: + kind: Pod + name: my-service-deployment + namespace: test-ns + resourceVersion: "20661" + uid: b37782aa-1458-4153-8399-dabc2b29aaae + ports: + - name: http-port + port: 8080 + protocol: TCP +`, + }, + expected: fmt.Errorf("\t* pod/my-service-deployment has the annotation %s but service/test-service-1 doesn't", k8s.ProxyOpaquePortsAnnotation), + }, + { + resources: []string{` +apiVersion: v1 +kind: Service +metadata: + name: test-service-1 + namespace: test-ns + annotations: + config.linkerd.io/opaque-ports: "9200" +spec: + ports: + - name: http + port: 9200 + protocol: TCP + targetPort: 9200 + selector: + service: service-1 +`, + ` +apiVersion: v1 +kind: Pod +metadata: + name: my-service-deployment + namespace: test-ns + service: service-1 + labels: + service: service-1 +spec: + containers: + - name: test + image: "test-service" +`, + ` +apiVersion: v1 +kind: Endpoints +metadata: + annotations: + endpoints.kubernetes.io/last-change-trigger-time: "2021-06-08T08:38:16Z" + creationTimestamp: "2021-06-08T08:38:03Z" + labels: + service: test-service-1 + name: test-service-1 + namespace: test-ns +subsets: +- addresses: + - ip: 10.244.3.12 + nodeName: nodename-1 + targetRef: + kind: Pod + name: my-service-deployment + namespace: test-ns + resourceVersion: "20661" + uid: b37782aa-1458-4153-8399-dabc2b29aaae + ports: + - name: http-port + port: 8080 + protocol: TCP +`, + }, + expected: fmt.Errorf("\t* service/test-service-1 has the annotation %s but pod/my-service-deployment doesn't", k8s.ProxyOpaquePortsAnnotation), + }, + { + resources: []string{` +apiVersion: v1 +kind: Service +metadata: + name: test-service-1 + namespace: test-ns + annotations: + config.linkerd.io/opaque-ports: "9200" +spec: + ports: + - name: elasticsearch + port: 9200 + protocol: TCP + targetPort: 9200 + selector: + service: service-1 +`, + ` +apiVersion: v1 +kind: Pod +metadata: + name: my-service-deployment + namespace: test-ns + service: service-1 + labels: + service: service-1 + annotations: + config.linkerd.io/opaque-ports: "9300" +spec: + containers: + - name: test + image: "test-service" +`, + ` +apiVersion: v1 +kind: Endpoints +metadata: + annotations: + endpoints.kubernetes.io/last-change-trigger-time: "2021-06-08T08:38:16Z" + creationTimestamp: "2021-06-08T08:38:03Z" + labels: + service: test-service-1 + name: test-service-1 + namespace: test-ns +subsets: +- addresses: + - ip: 10.244.3.12 + nodeName: nodename-1 + targetRef: + kind: Pod + name: my-service-deployment + namespace: test-ns + resourceVersion: "20661" + uid: b37782aa-1458-4153-8399-dabc2b29aaae + ports: + - name: http-port + port: 8080 + protocol: TCP +`, + }, + expected: fmt.Errorf("\t* pod/my-service-deployment and service/test-service-1 have the annotation %s but values don't match", k8s.ProxyOpaquePortsAnnotation), + }, + } + + for i, tc := range testCases { + tc := tc //pin + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + hc.kubeAPI, err = k8s.NewFakeAPI(tc.resources...) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + err = hc.checkMisconfiguredOpaquePortAnnotations(context.Background()) + if err == nil && tc.expected != nil { + t.Fatalf("Expected check to be successful, got: %s", err) + } + if err != nil { + if err.Error() != tc.expected.Error() { + t.Fatalf("Expected error: %s, received: %s", tc.expected, err) + } + } + }) + } +} + type controlPlaneReplicaOptions struct { destination int identity int diff --git a/test/integration/testdata/check.cni.proxy.golden b/test/integration/testdata/check.cni.proxy.golden index ac9d79baa..36d63ea87 100644 --- a/test/integration/testdata/check.cni.proxy.golden +++ b/test/integration/testdata/check.cni.proxy.golden @@ -84,5 +84,6 @@ linkerd-data-plane √ data plane pod labels are configured correctly √ data plane service labels are configured correctly √ data plane service annotations are configured correctly +√ opaque ports are properly annotated Status check results are √ diff --git a/test/integration/testdata/check.proxy.golden b/test/integration/testdata/check.proxy.golden index f206a3613..fccf16c39 100644 --- a/test/integration/testdata/check.proxy.golden +++ b/test/integration/testdata/check.proxy.golden @@ -72,5 +72,6 @@ linkerd-data-plane √ data plane pod labels are configured correctly √ data plane service labels are configured correctly √ data plane service annotations are configured correctly +√ opaque ports are properly annotated Status check results are √