enhancements/monitoring: add proposal for early-monitoring-config-validation #1716
Conversation
| The `apiserver_admission_webhook_*` metrics should provide insights into the status of the webhook from the apiserver's perspective. For example: | ||
|
|
||
| ``` | ||
| histogram_quantile(0.99, rate(apiserver_admission_webhook_admission_duration_seconds_bucket{name="monitoringconfigmaps.openshift.io"}[5m])) |
There was a problem hiding this comment.
Perhaps we're interested in different metrics for user/platform instance too?
There was a problem hiding this comment.
you mean to identify the concerned configmap? platform or the UW one?
There was a problem hiding this comment.
No existing metrics from the API server provide such detailed information (as this would result in high cardinality, particularly for other webhooks that may be responsible for all configmaps in a cluster, for example), so the metrics would need to be added on CMO side.
In our case, even though only 2 configmaps concern us, don't you think the debug logs (shown below) are sufficient? It's true that for this, the issue should be reproducible, but wouldn't that be easy since, after all, we only have 2 configmaps to consider?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jan--f The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/lgtm |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
| Introduce early validation for changes to monitoring configurations hosted in the | ||
| `openshift-monitoring/cluster-monitoring-config` and | ||
| `openshift-user-workload-monitoring/user-workload-monitoring-config` ConfigMaps to provide | ||
| shorter feedback loops and enhance user experience. |
There was a problem hiding this comment.
How does this overlap with the on-going effort to move these configmaps to CRDs? Seems this work would be redundant once the migration to CRDs is complete. Would it not be better to focus on that effort rather than investing here?
What are the timelines for the migration project?
There was a problem hiding this comment.
Actually, as I explained on Slack (I'll explicitly mention that in the proposal) the implementation of the change proposed here was already available when I started the proposal.
See the linked openshift/cluster-monitoring-operator#2490 (PR already merged now), I worked on that during the last shiftweek.
The changes (were meant to take) took less time than the CRD effort, as they only concerned CMO + they'll prepare the way for it (CRD based config), provide a preview of what will happen with CRDs, educate users about it, and ease the migration. + CRD based config becoming GA may take some time and this would be helpful in the meantime.
Also, as I mentioned this proposal primarily serves an informational and documentary purpose for the various stakeholders., and of course, the reviews are intended to help us identify any overlooked side effects. If necessary, we can always revert the CMO PR.
(I'll try to incorporate this into the proposal)
| - This addition does not intend to replace or render obsolete the existing `UserWorkloadInvalidConfiguration`/`InvalidConfiguration` related signals in the operator status/logs/alerts. | ||
| - This proposal does not intend to prevent or postpone the planned transition to CRDs for enhanced validation capabilities. Instead, it will prepare the way for it, provide a preview of what will happen with CRDs, educate users about it, and ease the migration. | ||
| - Some ConfigMap changes may bypass the CMO validation logic if the CMO operator is down for some reason; these changes will not be validated (best-effort approach). | ||
| - ConfigMaps with invalid monitoring configurations deployed before the webhook is enabled (before upgrading to the version that enables the validation webhook on CMO) will not be flagged or adjusted. The webhook will only intervene on them during subsequent changes, if any. |
There was a problem hiding this comment.
You will need to make sure you employ a ratcheting validation technique to all updates, is that already part of the proposal?
There was a problem hiding this comment.
Don't you think the mechanism explained in Upgrade / Downgrade Strategy is sufficient?
It'll help ensure the existing configmaps are in a good shape before upgrading to 4.18 (that would ship the validation webhook).
Also, only two configmaps are concerned by this, with the informative error messages, along with the schema provided here https://docs.openshift.com/container-platform/4.17/observability/monitoring/config-map-reference-for-the-cluster-monitoring-operator.html it shouldn't be too cumbersome to adjust the configmaps if anything slipped through the mechanism in Upgrade / Downgrade Strategy
| matchConditions: | ||
| - name: 'monitoringconfigmaps' | ||
| expression: '(request.namespace == "openshift-monitoring" && request.name == "cluster-monitoring-config") | ||
| || (request.namespace == "openshift-user-workload-monitoring" && request.name | ||
| == "user-workload-monitoring-config")' |
|
|
||
| ### Removing a deprecated feature | ||
|
|
||
| Once CRD-based configuration is GA, configuration via ConfigMaps will no longer be allowed, and the webhook logic will be removed. |
There was a problem hiding this comment.
I'm not convinced that is true, the migration path for the cluster monitoring CRDs I believe is ambiguous, and, we don't know exactly when the support for configmaps will go away
There was a problem hiding this comment.
Could you elaborate? What I'm trying to say is that "once CMO no longer uses Configmaps, the webhook logic will become useless and will be removed"
I changed the wording, tell me if it's ok.
|
|
||
| Even after CMO is upgraded to a version with the webhook enabled, as long as the existing monitoring config ConfigMaps are not updated, they will not be flagged by the webhook. | ||
|
|
||
| A change in `4.17.z` will make CMO report `upgradeable=false` if the existing configs contain malformed JSON/YAML, invalid fields, no longer supported fields, or duplicated fields. We will ensure clusters reach that version before being able to upgrade to `4.18`. This will help avoid blocking implicit or unplanned changes to ConfigMaps with invalid configs during the upgrade. |
There was a problem hiding this comment.
Version numbers probably need to be updated here
There was a problem hiding this comment.
The z in 4.17.z is now known 5 and we're still aiming for 4.18
|
|
||
| Even after CMO is upgraded to a version with the webhook enabled, as long as the existing monitoring config ConfigMaps are not updated, they will not be flagged by the webhook. | ||
|
|
||
| A change in `4.17.z` will make CMO report `upgradeable=false` if the existing configs contain malformed JSON/YAML, invalid fields, no longer supported fields, or duplicated fields. We will ensure clusters reach that version before being able to upgrade to `4.18`. This will help avoid blocking implicit or unplanned changes to ConfigMaps with invalid configs during the upgrade. |
There was a problem hiding this comment.
If there's already existing bad config, and the operator is running the same checks that the webhook will run, why is the operator going degraded/not upgradeable not already a thing?
There was a problem hiding this comment.
CMO already goes degraded on bad config, see user stories and non goals, the problem is that the resulting signals show up late and can easily be missed.
|
|
||
| Upgrades will be covered by existing upgrade tests. | ||
|
|
||
| In case of a rollback, the CVO-managed `monitoringconfigmaps.openshift.io` `ValidatingWebhookConfiguration` may need to be deleted to avoid the unnecessary `timeoutSeconds: 5` overhead on each change to the monitoring config ConfigMaps. |
There was a problem hiding this comment.
You could backport a tombstone resource to the previous release which would make the CVO remove this resource if it were to see it
There was a problem hiding this comment.
Good idea, I'll look into that.
(also, I think I'm a little bit pessimistic as the server would just respond with "I don't know nothing about /validate-webhook/monitoringconfigmaps" in way less than 5s... I'll give that a try.)
|
|
||
| ## Alternatives | ||
|
|
||
| Wait for CRD based configs to be GA. |
There was a problem hiding this comment.
I'd like to see more links to this alternative within the document (so those who aren't familiar can find the other EP), and, you should also expand on why this isn't the route we are taking, why has an alternative been dismissed. I've left questions on this earlier, and as an outsider, have no context on why we aren't doing this, explain it to me in this section
There was a problem hiding this comment.
See my answer to https://github.com/openshift/enhancements/pull/1716/files#r1860750216.
I'll add the links.
|
New changes are detected. LGTM label has been removed. |
|
|
||
| ## Alternatives | ||
|
|
||
| Wait for CRD based configs to be GA. |
There was a problem hiding this comment.
See my answer to https://github.com/openshift/enhancements/pull/1716/files#r1860750216.
I'll add the links.
|
|
||
| Upgrades will be covered by existing upgrade tests. | ||
|
|
||
| In case of a rollback, the CVO-managed `monitoringconfigmaps.openshift.io` `ValidatingWebhookConfiguration` may need to be deleted to avoid the unnecessary `timeoutSeconds: 5` overhead on each change to the monitoring config ConfigMaps. |
There was a problem hiding this comment.
Good idea, I'll look into that.
(also, I think I'm a little bit pessimistic as the server would just respond with "I don't know nothing about /validate-webhook/monitoringconfigmaps" in way less than 5s... I'll give that a try.)
|
|
||
| Even after CMO is upgraded to a version with the webhook enabled, as long as the existing monitoring config ConfigMaps are not updated, they will not be flagged by the webhook. | ||
|
|
||
| A change in `4.17.z` will make CMO report `upgradeable=false` if the existing configs contain malformed JSON/YAML, invalid fields, no longer supported fields, or duplicated fields. We will ensure clusters reach that version before being able to upgrade to `4.18`. This will help avoid blocking implicit or unplanned changes to ConfigMaps with invalid configs during the upgrade. |
There was a problem hiding this comment.
CMO already goes degraded on bad config, see user stories and non goals, the problem is that the resulting signals show up late and can easily be missed.
|
|
||
| Even after CMO is upgraded to a version with the webhook enabled, as long as the existing monitoring config ConfigMaps are not updated, they will not be flagged by the webhook. | ||
|
|
||
| A change in `4.17.z` will make CMO report `upgradeable=false` if the existing configs contain malformed JSON/YAML, invalid fields, no longer supported fields, or duplicated fields. We will ensure clusters reach that version before being able to upgrade to `4.18`. This will help avoid blocking implicit or unplanned changes to ConfigMaps with invalid configs during the upgrade. |
There was a problem hiding this comment.
The z in 4.17.z is now known 5 and we're still aiming for 4.18
|
|
||
| ### Removing a deprecated feature | ||
|
|
||
| Once CRD-based configuration is GA, configuration via ConfigMaps will no longer be allowed, and the webhook logic will be removed. |
There was a problem hiding this comment.
Could you elaborate? What I'm trying to say is that "once CMO no longer uses Configmaps, the webhook logic will become useless and will be removed"
I changed the wording, tell me if it's ok.
| - name: 'not-skipped' | ||
| expression: '!has(object.metadata.labels) | ||
| || !("monitoringconfigmaps.openshift.io/skip-validate-webhook" in object.metadata.labels) | ||
| || object.metadata.labels["monitoringconfigmaps.openshift.io/skip-validate-webhook"] != "true"' |
There was a problem hiding this comment.
| - This addition does not intend to replace or render obsolete the existing `UserWorkloadInvalidConfiguration`/`InvalidConfiguration` related signals in the operator status/logs/alerts. | ||
| - This proposal does not intend to prevent or postpone the planned transition to CRDs for enhanced validation capabilities. Instead, it will prepare the way for it, provide a preview of what will happen with CRDs, educate users about it, and ease the migration. | ||
| - Some ConfigMap changes may bypass the CMO validation logic if the CMO operator is down for some reason; these changes will not be validated (best-effort approach). | ||
| - ConfigMaps with invalid monitoring configurations deployed before the webhook is enabled (before upgrading to the version that enables the validation webhook on CMO) will not be flagged or adjusted. The webhook will only intervene on them during subsequent changes, if any. |
There was a problem hiding this comment.
Don't you think the mechanism explained in Upgrade / Downgrade Strategy is sufficient?
It'll help ensure the existing configmaps are in a good shape before upgrading to 4.18 (that would ship the validation webhook).
Also, only two configmaps are concerned by this, with the informative error messages, along with the schema provided here https://docs.openshift.com/container-platform/4.17/observability/monitoring/config-map-reference-for-the-cluster-monitoring-operator.html it shouldn't be too cumbersome to adjust the configmaps if anything slipped through the mechanism in Upgrade / Downgrade Strategy
| Introduce early validation for changes to monitoring configurations hosted in the | ||
| `openshift-monitoring/cluster-monitoring-config` and | ||
| `openshift-user-workload-monitoring/user-workload-monitoring-config` ConfigMaps to provide | ||
| shorter feedback loops and enhance user experience. |
There was a problem hiding this comment.
Actually, as I explained on Slack (I'll explicitly mention that in the proposal) the implementation of the change proposed here was already available when I started the proposal.
See the linked openshift/cluster-monitoring-operator#2490 (PR already merged now), I worked on that during the last shiftweek.
The changes (were meant to take) took less time than the CRD effort, as they only concerned CMO + they'll prepare the way for it (CRD based config), provide a preview of what will happen with CRDs, educate users about it, and ease the migration. + CRD based config becoming GA may take some time and this would be helpful in the meantime.
Also, as I mentioned this proposal primarily serves an informational and documentary purpose for the various stakeholders., and of course, the reviews are intended to help us identify any overlooked side effects. If necessary, we can always revert the CMO PR.
(I'll try to incorporate this into the proposal)
|
|
||
| ## Graduation Criteria | ||
|
|
||
| The webhook is intended to go directly to `GA` and be enabled by default. |
There was a problem hiding this comment.
All new features in openshift should be gated
Well, this statement is a bit vague and not entirely accurate. I don't want to point fingers, but this isn't always strictly followed :) and sometimes it's better not to.
Allow me to explain why we're making this GA by default:
- We have ensured that the feature is thoroughly tested and passes all e2e and blocking payload tests, as well as many of the informing tests that we monitor.
- Making it 'tech preview' would just limit the clusters on which this feature could be tested.
- This is not the first time we are using validation webhooks in the monitoring stack; we already have https://github.com/openshift/cluster-monitoring-operator/tree/master/assets/admission-webhook for some of prometheus operator CRs.
- We can easily revert the implementation PR if the tests or feedback suggest that this feature shouldn't be part of 4.18.0. Additionally, the
monitoringconfigmaps.openshift.io/skip-validate-webhook: truelabel can be used to contain any issues.
We believe this feature is well defined and its potential breakages can be easily managed. Thus, it is simpler and faster to proceed with this approach.
|
|
||
| ### Topology Considerations | ||
|
|
||
| #### Hypershift / Hosted Control Planes |
There was a problem hiding this comment.
I don't think ther any special considerations are needed for Hypershift; The early validation could be used wherever CMO is deployed.
I have included additional details under ### Topology Considerations.
That being said, please feel free to notify anyone from Hypershift who you think should be directly informed about this feature. I will also try to reach out to them on Slack.
|
@machine424: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/unassign |
|
/uncc |
Implementation available here openshift/cluster-monitoring-operator#2490