Add OpenShift service-ca support for authenticated metrics endpoints #3677
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Is there an OPRUN ticket associated with this? |
| - apiGroups: | ||
| - authentication.k8s.io | ||
| resources: | ||
| - tokenreviews | ||
| verbs: | ||
| - create | ||
| - apiGroups: | ||
| - authorization.k8s.io | ||
| resources: | ||
| - subjectaccessreviews | ||
| verbs: | ||
| - create |
There was a problem hiding this comment.
Why are these additions needed?
There was a problem hiding this comment.
This is not needed for this PR, but I noticed that we're doing apigroups/resources : * access for a lot of verbs. Ideally we'd reduce scope, starting with this change that explicitly lists the api groups and resources.
| kind: Service | ||
| metadata: | ||
| name: olm-operator-metrics | ||
| name: {{ if .Values.serviceCa.enabled }}{{ .Values.serviceCa.olmOperator.serviceName }}{{ else }}olm-operator-metrics{{ end }} |
There was a problem hiding this comment.
What if .Values.serviceCa.olmOperator.serviceName is blank? Or is that something you'll ensure downstream?
Perhaps the serviceName itself should be in the values.yaml file with a default of olm-operator-metrics?
There was a problem hiding this comment.
Kinda like what you do for service-cert-secret-name below?
|
My main comments are:
|
Adds support for OpenShift service-ca-operator alongside existing cert-manager support for authenticated metrics endpoints. This enables the metrics authentication feature to work in both upstream Kubernetes (with cert-manager) and OpenShift (with service-ca) environments. - Add explicit RBAC permissions for `tokenreviews` and `subjectaccessreviews` required by authentication filters - Add `serviceCa` configuration section to values.yaml with configurable secret/service names - Update deployment templates to support both `certManager` and `serviceCa` modes conditionally - Update service templates to conditionally add service-ca annotations when enabled - Maintain backward compatibility with existing `certManager` and `monitoring` configurations
anik120
force-pushed
the
metrics-auth-openshift-compatibility
branch
from
d8fd680 to
8f9d4d2
Compare
| - apiGroups: | ||
| - authentication.k8s.io | ||
| resources: | ||
| - tokenreviews | ||
| verbs: | ||
| - create | ||
| - apiGroups: | ||
| - authorization.k8s.io | ||
| resources: | ||
| - subjectaccessreviews | ||
| verbs: | ||
| - create |
There was a problem hiding this comment.
This is not needed for this PR, but I noticed that we're doing apigroups/resources : * access for a lot of verbs. Ideally we'd reduce scope, starting with this change that explicitly lists the api groups and resources.
| kind: Service | ||
| metadata: | ||
| name: olm-operator-metrics | ||
| name: {{ if .Values.serviceCa.enabled }}{{ .Values.serviceCa.olmOperator.serviceName }}{{ else }}olm-operator-metrics{{ end }} |
2 participants
Description of the change:
Adds support for OpenShift service-ca-operator alongside existing cert-manager support for authenticated metrics endpoints. This enables the metrics authentication feature to work in both upstream Kubernetes (with cert-manager) and OpenShift (with service-ca) environments.
Changes:
tokenreviewsandsubjectaccessreviewsrequired by authentication filtersserviceCaconfiguration section to values.yaml with configurable secret/service namescertManagerandserviceCamodes conditionallycertManagerandmonitoringconfigurationsMotivation for the change:
Testing remarks:
Reviewer Checklist
/doc[FLAKE]are truly flaky and have an issue