Add Pingdom Transaction Checks #570

karlderkaefer · 2024-01-21T17:50:31Z

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

Additional Notes

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

closes #569

github-actions · 2024-01-21T18:22:15Z

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

karlderkaefer · 2024-01-21T18:23:15Z

pkg/monitors/pingdomtransaction/pingdom-transaction-monitor.go

+	return monitors
+}
+
+func (service *PingdomTransactionMonitorService) GetUrlFromSteps(id int64) string {


not sure if this required. This was a attempt before I realized that the controller is not supporting more than one provider

as far as i have seen, it supports atleast two monitors, with working on both simultaneously

yes stakater will send out request to every provider that is defined. This is not required in my opinion, since I don't the alert to be duplication on different providers, but instead I want to use different providers for different alerts.

karlderkaefer · 2024-01-22T09:38:43Z

api/v1alpha1/endpointmonitor_types.go

+
+	// `-` separated set list of integrations ids (e.g. "91166-12168")
+	// +optional
+	AlertIntegrations string `json:"alertIntegrations,omitempty"`


I personally would prefer a string array here because it's more natural for yaml and users. On the other hand, pingdom user might be used to existing format, What is your opinion?

i myself prefer arrays as well for such conditions, pushed a change recently with array for grafana cloud

Unfortunately this requires a refactoring. The problem is the alertIntegration, alertContacts and TeamAlerts are defined in Provider struct

IngressMonitorController/pkg/config/config.go

Lines 65 to 67 in 88a3c47

AlertContacts string `yaml:"alertContacts"`

AlertIntegrations string `yaml:"alertIntegrations"`

TeamAlertContacts string `yaml:"teamAlertContacts"`

This is used by all provider implementation. Probably string was used to allow more flexible setup and let the parsing upto the specific provider implementation.
I think its fine if we keep string here, also to align to existing pingdom configuration. I would like to do changes here

karlderkaefer · 2024-01-28T11:08:18Z

@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 · 2024-03-06T10:36:06Z

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

MuneebAijaz · 2024-03-06T10:38:10Z

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

MuneebAijaz · 2024-03-19T11:10:54Z

api/v1alpha1/endpointmonitor_types.go

+
+	// `-` separated set list of integrations ids (e.g. "91166-12168")
+	// +optional
+	AlertIntegrations string `json:"alertIntegrations,omitempty"`


i myself prefer arrays as well for such conditions, pushed a change recently with array for grafana cloud

MuneebAijaz · 2024-03-19T11:26:21Z

pkg/monitors/pingdomtransaction/pingdom-transaction-monitor.go

+	return monitors
+}
+
+func (service *PingdomTransactionMonitorService) GetUrlFromSteps(id int64) string {


as far as i have seen, it supports atleast two monitors, with working on both simultaneously

MuneebAijaz · 2024-03-19T11:29:43Z

pkg/monitors/pingdomtransaction/pingdom-transaction-monitor.go

+func (service *PingdomTransactionMonitorService) createTransactionCheck(monitor models.Monitor) *pingdomNew.CheckWithoutID {
+	transactionCheck := &pingdomNew.CheckWithoutID{}
+	providerConfig, _ := monitor.Config.(*endpointmonitorv1alpha1.PingdomTransactionConfig)
+	if providerConfig == nil {


shouldnt this be checked at setup level and not so down the code flow?

the providerConfig is set on alert configuration. We can not move to setup because it needs to be checked for every alert. One alternative instead of checking for existence of *endpointmonitorv1alpha1.PingdomTransactionConfig we can use the providers from the spec

IngressMonitorController/api/v1alpha1/endpointmonitor_types.go

Line 37 in f62cd9b

Providers string `json:"providers"`

It seems this field is unused in code. But it could cover both scenarios: I have a alert for multiple provider and I want to have a alert for a specific provider. I believe this would also greatly simplify the code, what do you think?

MuneebAijaz · 2024-03-19T11:36:53Z

pkg/monitors/pingdomtransaction/pingdom-transaction-monitor.go

+}
+
+// getSecretValue retrieves a secret value from Kubernetes.
+func getSecretValue(kubeClient KubernetesInterface, namespace, secretName, secretKey string) (string, error) {


do we need to define separate interfaces to do get calls here? simple get call on secret types with kubeclient can also give us secret object.

The interface is required in order to mock the kubernetes client for test. see test TestReplaceSecrets https://github.com/karlderkaefer/IngressMonitorController/blob/c06ff6e38c6e2b875d9b91ebac20e33ac2d3bdf8/pkg/monitors/pingdomtransaction/pingdom-transaction-monitor_test.go#L112

karlderkaefer · 2024-06-08T11:49:26Z

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

karlderkaefer · 2024-06-12T10:45:18Z

@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 added 17 commits November 17, 2023 18:07

feat(pingdom): add post data field (#1)

c0ca1e2

feat: add pingdom transaction checks

6efde90

feat: add pingdom transaction checks

2c0ed34

Merge branch 'master' into add-pingdom-transaction-checks

24538dc

feat: generate crds

295032a

feat: add mapping for step args

eaf216e

feat: support multiple providers

1e26926

fix: optimize log outputs

43aac90

fix: make test compability with macos arm64

3f1f566

fix: update pingdom client to latest tag

2d24ee5

fix: move util function to utils

0ef7381

chore: go vet and go fmt

38289cd

feat: update CRD for helm chart

b48165f

chore: reset helm chart spec

6b72345

fix: update golang client

24c605b

docs: add readme for pingdom transaction

a834cea

chore: update crd

8dff896

karlderkaefer commented Jan 21, 2024

View reviewed changes

chore: update log output for pingdom

d18cdc5

karlderkaefer commented Jan 22, 2024

View reviewed changes

karlderkaefer added 2 commits January 22, 2024 12:51

fix: logs need key pair values as input

2f49d66

feat: add secret template for sensitive data (#3)

ff9d48e

fix: use helm compatible template for secrets

c06ff6e

karl-johan-grahn requested a review from MuneebAijaz February 7, 2024 09:50

MuneebAijaz requested changes Mar 19, 2024

View reviewed changes

karlderkaefer added 2 commits June 8, 2024 13:52

Merge branch 'master' into add-pingdom-transaction-checks

8e9a013

chore: update go mod

08fa1fd

MuneebAijaz approved these changes Jun 12, 2024

View reviewed changes

MuneebAijaz merged commit 3016d1e into stakater:master Jun 12, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Pingdom Transaction Checks #570

Add Pingdom Transaction Checks #570

karlderkaefer commented Jan 21, 2024 •

edited

Loading

github-actions bot commented Jan 21, 2024 •

edited

Loading

karlderkaefer Jan 21, 2024

MuneebAijaz Mar 19, 2024

karlderkaefer Apr 20, 2024 •

edited

Loading

karlderkaefer Jan 22, 2024

MuneebAijaz Mar 19, 2024

karlderkaefer Jun 8, 2024

karlderkaefer commented Jan 28, 2024

karlderkaefer commented Mar 6, 2024

MuneebAijaz commented Mar 6, 2024

MuneebAijaz Mar 19, 2024

MuneebAijaz Mar 19, 2024

MuneebAijaz Mar 19, 2024

karlderkaefer Apr 20, 2024 •

edited

Loading

MuneebAijaz Mar 19, 2024

karlderkaefer Apr 20, 2024

karlderkaefer commented Jun 8, 2024

karlderkaefer commented Jun 12, 2024

	AlertContacts string `yaml:"alertContacts"`
	AlertIntegrations string `yaml:"alertIntegrations"`
	TeamAlertContacts string `yaml:"teamAlertContacts"`

Add Pingdom Transaction Checks #570

Add Pingdom Transaction Checks #570

Conversation

karlderkaefer commented Jan 21, 2024 • edited Loading

Summary

Detailed Feature List

Motivation

Testing

Additional Notes

github-actions bot commented Jan 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlderkaefer Apr 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlderkaefer commented Jan 28, 2024

karlderkaefer commented Mar 6, 2024

MuneebAijaz commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlderkaefer Apr 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlderkaefer commented Jun 8, 2024

karlderkaefer commented Jun 12, 2024

karlderkaefer commented Jan 21, 2024 •

edited

Loading

github-actions bot commented Jan 21, 2024 •

edited

Loading

karlderkaefer Apr 20, 2024 •

edited

Loading

karlderkaefer Apr 20, 2024 •

edited

Loading