-
Notifications
You must be signed in to change notification settings - Fork 614
MON-4565: Add enableUserAlertmanagerConfig to ClusterMonitoring API #2855
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,36 @@ tests: | |
| spec: | ||
| userDefined: | ||
| mode: "Disabled" | ||
| - name: Should accept userAlertmanagerConfigSelection on alertmanagerConfig | ||
| initial: | | ||
| apiVersion: config.openshift.io/v1alpha1 | ||
| kind: ClusterMonitoring | ||
| spec: | ||
| userDefined: | ||
| mode: "Disabled" | ||
| alertmanagerConfig: | ||
| deploymentMode: "DefaultConfig" | ||
| userAlertmanagerConfigSelection: Selectable | ||
| expected: | | ||
| apiVersion: config.openshift.io/v1alpha1 | ||
| kind: ClusterMonitoring | ||
| spec: | ||
| userDefined: | ||
| mode: "Disabled" | ||
| alertmanagerConfig: | ||
| deploymentMode: "DefaultConfig" | ||
| userAlertmanagerConfigSelection: Selectable | ||
| - name: Should reject invalid userAlertmanagerConfigSelection on alertmanagerConfig | ||
| initial: | | ||
| apiVersion: config.openshift.io/v1alpha1 | ||
| kind: ClusterMonitoring | ||
| spec: | ||
| userDefined: | ||
| mode: "Disabled" | ||
| alertmanagerConfig: | ||
| deploymentMode: "DefaultConfig" | ||
| userAlertmanagerConfigSelection: Enabled | ||
| expectedError: 'spec.alertmanagerConfig.userAlertmanagerConfigSelection: Unsupported value: "Enabled": supported values: "Selectable", "None"' | ||
|
Comment on lines
+21
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win Add test coverage for optional field behavior. Per the PR summary and AI-generated description, 📋 Proposed additional test case - name: Should accept alertmanagerConfig without userAlertmanagerConfigSelection
initial: |
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
spec:
userDefined:
mode: "Disabled"
alertmanagerConfig:
deploymentMode: "DefaultConfig"
expected: |
apiVersion: config.openshift.io/v1alpha1
kind: ClusterMonitoring
spec:
userDefined:
mode: "Disabled"
alertmanagerConfig:
deploymentMode: "DefaultConfig"🤖 Prompt for AI Agents |
||
| - name: Should reject ContainerResource with duplicate names | ||
| initial: | | ||
| apiVersion: config.openshift.io/v1alpha1 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -794,8 +794,37 @@ type AlertmanagerConfig struct { | |
| // When set to CustomConfig, the Alertmanager will be deployed with custom configuration. | ||
| // +optional | ||
| CustomConfig AlertmanagerCustomConfig `json:"customConfig,omitempty,omitzero"` | ||
| // userAlertmanagerConfigSelection is an optional field that controls whether user-defined | ||
| // namespaces can be selected for AlertmanagerConfig lookups on the platform Alertmanager | ||
| // instance in the `openshift-monitoring` namespace. | ||
| // Valid values are Selectable and None. | ||
| // When set to Selectable, the platform Alertmanager discovers AlertmanagerConfig resources | ||
| // in user-defined namespaces. This is equivalent to `enableUserAlertmanagerConfig: true` in | ||
| // the cluster-monitoring-config ConfigMap. | ||
|
Comment on lines
+801
to
+803
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How are these resources discovered? Is there any additional configuration options we may want to support for how these are discovered? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The (Prometheus) operator discovers all AlertmanagerConfig resources in all namespaces which are not labelled with |
||
| // When set to None, user-defined namespaces are not selected for AlertmanagerConfig lookups | ||
| // on the platform Alertmanager. This is equivalent to `enableUserAlertmanagerConfig: false` | ||
| // in the cluster-monitoring-config ConfigMap. | ||
| // This setting only applies when the user-workload monitoring Alertmanager is not enabled. | ||
| // When omitted, the default value is None. | ||
| // +optional | ||
| // +kubebuilder:validation:Enum=Selectable;None | ||
| UserAlertmanagerConfigSelection UserAlertmanagerConfigSelection `json:"userAlertmanagerConfigSelection,omitempty"` | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this be moved to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems reasonable to me. |
||
| } | ||
|
|
||
| // UserAlertmanagerConfigSelection controls whether the platform Alertmanager selects | ||
| // AlertmanagerConfig resources from user-defined namespaces. | ||
| // +enum | ||
| type UserAlertmanagerConfigSelection string | ||
|
|
||
| const ( | ||
| // UserAlertmanagerConfigSelectionSelectable enables user-defined namespaces to be selected | ||
| // for AlertmanagerConfig lookups on the platform Alertmanager. | ||
| UserAlertmanagerConfigSelectionSelectable UserAlertmanagerConfigSelection = "Selectable" | ||
| // UserAlertmanagerConfigSelectionNone disables user-defined namespaces from being selected | ||
| // for AlertmanagerConfig lookups on the platform Alertmanager. | ||
| UserAlertmanagerConfigSelectionNone UserAlertmanagerConfigSelection = "None" | ||
| ) | ||
|
|
||
| // AlertmanagerCustomConfig represents the configuration for a custom Alertmanager deployment. | ||
| // alertmanagerCustomConfig provides configuration options for the default Alertmanager instance | ||
| // that runs in the `openshift-monitoring` namespace. Use this configuration to control | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add test coverage for the "None" enum value.
The validation error on line 50 indicates that
userAlertmanagerConfigSelectionsupports two values: "Selectable" and "None". Currently only "Selectable" is tested for acceptance. Add a similar test case that verifies "None" is also accepted and preserved.📋 Proposed additional test case
🤖 Prompt for AI Agents