IngressMonitorController icon indicating copy to clipboard operation
IngressMonitorController copied to clipboard

Add Pingdom Transaction Checks

Open karlderkaefer opened this issue 1 year ago • 4 comments

Summary

This pull request addresses issue #553 #569 . The main changes are:

  • Add new Provider PingdomTransaction
  • Use Golang Client which supports Transaction Check
  • Controller now supports more than one provider, before it was not possible to have more than one
  • Refactor log messages
  • Use pointer for service monitors for Reconciler to improve performance
  • provide a secret template function to be able to read sensitive data from secrets
  • Added Documentation and Examples

Detailed Feature List

Multi Provider Support:

It was not possible to add more than one provider. For example if you have provider config

providers:
  - name: PingdomTransaction
    apiURL: "https://api.pingdom.com/api/3.1"
    apiToken: <API_TOKEN>
  - name: Pingdom
    apiURL: "https://api.pingdom.com/api/3.1"
    apiToken: <API_TOKEN>
enableMonitorDeletion: true

If you add monitor regardless of the provider spec, the create and update events were fired to all available providers. This lead API errors and unwanted creation of monitors

We will now find the right service monitor proxy depending on the spec and sent create, update and delete events only to suitable providers. In case the provider could not be detected we use the first configured provider, which is default for endpoint monitors without specific provider spec.

Further more I added a enum for different providers

The existing spec does not assign a check to a service provider directly on the CRDs. We have check with either a provider config or without. This will lead to more code. It would be probably better to introduce a required field spec.monitorType to map each check explicitly to a provider. However that would introduce a breaking change (wanted to avoid that)

New Provider for Pingdom Transaction I created a new provider and package for Pingdom transaction provider since the fields are significantly different than Uptime checks. Also we need another client to do the API calls. So it's probably best if separate by packages

Using Pointer for Monitor Proxy The service monitor proxies are initialised on startup. It would make more sense for me if we use pointer here to avoid allocation of new objects. What do you think?

Refactor Logging Output

  • in case the api of the pingdom failed to create a check it was not clear by which check it was caused -> adding check name
{"logger":"pingdom","msg":"Tag string should not contain spaces. Not applying tags for monitor: sapcl-group-bmw-us10-monitoring"}
  • clear some log outputs that are not relevant to the user

Secret Template to retrieve sensitive Data We have the requirement to set sensitive data such as password for some fields. We would like to avoid to store them in endpoint monitor CRD. Instead we just define a secret reference e.g. value: '{{secret:my-secret-name:my-secret-key}}'. The secret will be retrieved and is replacing the template before the post request is sent to Pingdom.

Minor fixes

  • make envtest setup compatible with macos arm64
  • validation on CRD level notified the user on apply phase about unmatching fields
The EndpointMonitor "manual-pingdom-transaction-check-karl" is invalid: spec.pingdomTransactionConfig.interval: Unsupported value: 22: supported values: "5", "10", "20", "60", "720", "1440"

Motivation

#569

Testing

  • I did run my newly created test pingdom-transaction-monitor_test.go to ensure a basic create and delete works
  • I did apply and tested the full spec for transaction check (see readme)
apiVersion: endpointmonitor.stakater.com/v1alpha1
kind: EndpointMonitor
metadata:
  name: manual-pingdom-transaction-check-karl
spec:
  url: https://www.google.com
  pingdomTransactionConfig:
    steps:
      - function: go_to
        args:
          url: https://www.google.com
      - function: fill
        args:
          input: textarea[name=q]
          value: kubernetes
      - function: submit
        args:
          form: form
      - function: exists
        args:
          element: '#rso'
    alertContacts: "14901866"
    alertIntegrations: "132625"
    #teamAlertContacts: "14901866"
    custom_message: "This is a custom message"
    paused: false
    region: us-east
    interval: 60
    send_notification_when_down: 3
    severity_level: low
    tags:
      - testing
      - manual
---
apiVersion: endpointmonitor.stakater.com/v1alpha1
kind: EndpointMonitor
metadata:
  name: manual-pingdom-uptime-check-without-spec-karl
spec:
  url: "https://www.google.com"
---
apiVersion: endpointmonitor.stakater.com/v1alpha1
kind: EndpointMonitor
metadata:
  name: manual-pingdom-uptime-check-with-spec-karl
spec:
  url: "https://www.google.com"
  pingdomConfig:
    notifyWhenBackUp: true
    alertContacts: "14901866"
    alertIntegrations: "132625"
    tags: "testing,manual"
  • I ensured that check creation works even if you have multiple providers
  • I tested that we have no breaking change - an endpoint monitor potentially can have no defined providerSpec. It will now use the first defined provider in case no spec was found
  • I tested this version in my cluster and verified all functions and log outputs during my manual tests

Pingdom-Transaction-Check

Additional Notes

Include any additional notes or comments you have about the changes made.

closes #569

karlderkaefer avatar Jan 21 '24 17:01 karlderkaefer

@karlderkaefer Image is available for testing. docker pull stakater/ingressmonitorcontroller:SNAPSHOT-PR-570-08fa1fdc

github-actions[bot] avatar Jan 21 '24 18:01 github-actions[bot]

@karl-johan-grahn can you have look? I wanted to minimize changes but in the end still a lot. If splitting of the PR helps let me know

karlderkaefer avatar Jan 28 '24 11:01 karlderkaefer

Hi @MuneebAijaz is there an update on the review status?

karlderkaefer avatar Mar 06 '24 10:03 karlderkaefer

@karlderkaefer apologies for the delay, this is scheduled for next week

MuneebAijaz avatar Mar 06 '24 10:03 MuneebAijaz

@MuneebAijaz I have answered all comments, can we get an update on the review?

karlderkaefer avatar Jun 08 '24 11:06 karlderkaefer

@MuneebAijaz I appreciate the approval very much. Although we have tested these changes intensively, If there any bug related I'm happy to fix. you can mention me or @dennis-ge

karlderkaefer avatar Jun 12 '24 10:06 karlderkaefer