[Heartbeat] Fix hint-based monitor gen

[emilioalvap]

What does this PR do?

Make hint generator more flexible by removing host/port matching requirements.
Closes #17771.
Closes #34214.

Checklist

• My code follows the style guidelines of this project
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have made corresponding change to the default configuration files
• I have added tests that prove my fix is effective or that my feature works
• I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

Deploy the following config and check that exactly one monitor is generated with an unmatched host/port:

apiVersion: apps/v1
kind: Deployment
metadata:
  name:  nginx-svc
  namespace: default
  labels:
    app:  nginx-svc
spec:
  selector:
    matchLabels:
      app: nginx-svc
  replicas: 1
  template:
    metadata:
      labels:
        app:  nginx-svc
    spec:
      containers:
      - name:  nginx-svc
        image: nginx:1.23.1-alpine
        imagePullPolicy: Always
        resources:
          requests:
            cpu: 300m
            memory: 200Mi
          limits:
            cpu: 300m
            memory: 200Mi
        ports:
        - containerPort: 3000
---
apiVersion: v1
kind: Service
metadata:
  name: nginx-svc
  namespace: default
  annotations:
    co.elastic.monitor/1.hosts: 127.0.0.1:30000
    co.elastic.monitor/1.schedule: '@every 5s'
    co.elastic.monitor/1.type: tcp
spec:
  selector:
    app: nginx-svc
  type: NodePort
  ports:
  - protocol: TCP
    port: 80
    nodePort: 30000
    name: main

[mergify]

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @emilioalvap? 🙏.
For such, you'll need to label your PR with:

• The upcoming major version of the Elastic Stack
• The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

[elasticmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2023-02-01T18:30:10.606+0000

• Duration: 48 min 44 sec

Test stats 🧪

Test	Results
Failed	0
Passed	1889
Skipped	25
Total	1914

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• /package : Generate the packages and run the E2E tests.

• /beats-tester : Run the installation tests with beats-tester.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[gsantoro]

this line would not solve an extra bug that I discovered while investigating this issue.

we could have a use case where a container export a port 80 that is remapped to a nodePort 30000 (or any other port different from 80) like in the following snippet

apiVersion: v1
kind: Service
metadata:
  name: nginx-svc
  namespace: default
  annotations:
    co.elastic.monitor/1.hosts: 127.0.0.1:30000
    co.elastic.monitor/1.schedule: '@every 5s'
    co.elastic.monitor/1.type: tcp
spec:
  selector:
    app: nginx-svc
  type: NodePort
  ports:
  - protocol: TCP
    port: 80
    nodePort: 30000
    name: main

In that case the hint port (here 30000) needs to match the node port (here 30000) and not the container port (here 80).
If the previous code is not change 80 != 30000 will skip adding this heartbeat in this case.

[gsantoro]

I believe that this extra check for the containerPort (aka port here) and the hints port is just not useful and it might cause issues when that containerPort is mapped to another port

[emilioalvap]

TBH, I'm not too worried about this scenario, considering the limitations autodiscovery actually has

[gsantoro]

I am not sure what limitations you are referring to but this is clearly a bug to me.

[gsantoro]

are we removing the check for data.host altogether?

[gsantoro]

also I don't think we should check for data.port in any part of the string. We should check it for :${data.port}

[emilioalvap]

are we removing the check for data.host altogether?

IMO, the complicated thing here is building a general case. Pods are usually addresed via ip, services can be addressed by dns, some monitors might make sense for a single pod, others will be broken for a replica set.

@andrewvc I think we have two paths to solve this, wdyt?:

• allow any value combination in without trying to match host-port, should fix errors like the ones listed here with the downside that it can have unexpected results to be handled by the user, eg: generating a unique monitor config for a replica set will delete the monitor if a pod is removed from the set.
• Fix the empty host error and effectively have an even stricter matching host-port approach.

[andrewvc]

Can you provide some more detail on the stricter matching approach? What would that look like?

[emilioalvap]

Sure, it would involve making sure that the monitor generated is dynamic in relation to the hint event data. It's basically what we do now by checking ${data.host} and ${data.port} but fixing the empty config scenario we have.

The problem with this approach is that it prevents some common use cases, that's why there're so many request to remove the restriction altogether.

[elasticmachine]

Pinging @elastic/uptime (Team:Uptime)

[andrewvc]

LGTM pending changelog