feat: Add ObservabilityUI plugins API by jgbernalp · Pull Request #434 · rhobs/observability-operator

jgbernalp · 2024-03-12T16:47:34Z

This PR adds the Observability UI CRDs.

https://issues.redhat.com/browse/OU-357

Every ObservabilityUIPlugin, creates the following resources:

Deployment to host a lightweight backend for the plugin assets
Service to expose the deployment to the console
Service account
ConsolePlugin CR so the console knows in which service the plugin lives
Optional: ClusterRole and ClusterRoleBinding for some plugins so their backend can watch configmaps in a namespace

And patches the console to add the plugin so is enabled in the web console

openshift-ci · 2024-03-12T16:47:46Z

Hi @jgbernalp. Thanks for your PR.

I'm waiting for a rhobs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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/test-infra repository.

jan--f · 2024-03-13T14:42:04Z

Generally this looks good, I might have requests on some details later.
One question we should discuss: currently obo is Openshift agnostic and can be installed on a vanilla k8s. We make use of that in development during e2e testing with a kind cluster. If at all possible I'd like to preserve that feature. Iiuc the dependencies with this PR are the ConsolePlugin kind and serving cert annotations in the service.
Do you think there is a way to degrade gracefully if we're not on OCP, i.e. the ConsolePlugin is not present?
Does the Service require and https endpoint or could this be implemented differently? So far we leave any tls setups to the user via service-side-apply, though we certainly have considered encrypted by default before.

jgbernalp · 2024-03-13T15:05:49Z

Generally this looks good, I might have requests on some details later. One question we should discuss: currently obo is Openshift agnostic and can be installed on a vanilla k8s. We make use of that in development during e2e testing with a kind cluster. If at all possible I'd like to preserve that feature. Iiuc the dependencies with this PR are the ConsolePlugin kind and serving cert annotations in the service. Do you think there is a way to degrade gracefully if we're not on OCP, i.e. the ConsolePlugin is not present? Does the Service require and https endpoint or could this be implemented differently? So far we leave any tls setups to the user via service-side-apply, though we certainly have considered encrypted by default before.

Thanks for the initial check. I agree, we should verify whether the cluster supports the ConsolePlugin and, if it's not present, skip the reconciliation process. Imo since the service is relevant only within an OpenShift cluster where plugins are available, opting to skip reconciliation seems to be an adequate solution also for the serving cert annotation.

danielmellado · 2024-03-13T15:07:58Z

I was also about to suggest to drop openshift-specific names like groupAPI but it just makes sense to completely fall back if on non-openshift cluster. +1!

periklis · 2024-03-13T20:47:54Z

Generally this looks good, I might have requests on some details later. One question we should discuss: currently obo is Openshift agnostic and can be installed on a vanilla k8s. We make use of that in development during e2e testing with a kind cluster. If at all possible I'd like to preserve that feature. Iiuc the dependencies with this PR are the ConsolePlugin kind and serving cert annotations in the service. Do you think there is a way to degrade gracefully if we're not on OCP, i.e. the ConsolePlugin is not present? Does the Service require and https endpoint or could this be implemented differently? So far we leave any tls setups to the user via service-side-apply, though we certainly have considered encrypted by default before.

Sharing my experience on how we isolate OpenShift features in Loki Operator the last two years:

First things first, having an operator registering CRDs in the client side scheme even if the CRDs do not existing on the APIserver side is not a harmful thing. It get's only harmful if you start a controller starting to sync/watch for such resources.
What worked good for us are two things. First we publish three almost bundles (community, community-openshift, openshift). The bundles are identical except for:
- Container images: community and community-openshift reference images on dockerhub based on alpine. Openshift is a bundle only populated at and by Red Hat using RHEL-based images and other productization bits (annotations, CVP, etc.)
- Feature gates: This used to be the controller-runtime project config that migrated into the operator code base because it is migrated (i.e. community, community-openshift, openshift). The feature gates include a separate OpenShift only section that handles features like service-ca, CCO, etc. Regardless these some of the generic feature gates are opened all together because we know we have support on OpenShift only OOTB (e.g. ServiceMonitors, TLS Endpoints in ServiceMonitors, Restricted Pod Security, Alerts)
Controlling the feature gates per bundle gives us the maximum control on how much we can automate on the target cluster. The Community OpenShift and the OpenShift bundles basically go to the maximum because we can safely assume that Prometheus, ServiceCA, etc. exist on the platform. On the other hand the Community bundle is the bare minimum that one can install from here and the user can adapt the Feature Gates at will. The only exception to the rule is the dependency to Cert-Manager (and this is only a recommendation) for TLS certificates used for our Webhooks.
Based on feature gates we manage individual runtime characterists:
- Starting controllers: For example only on OpenShift the dashboards-controller
- Resource Registration: Based on the gates we register watches/owns handlers for each controller, e.g. we watch for the cluster Proxy (used for container proxy configuration) or APIServer (used for TLS Profiles) objects only in OpenShift.
- Extra Webhook Validators: For example only on OpenShift we enable the validations of alerting rules according to the official style guide.
- Manage Resources: On OpenShift we replace Ingress for Route and add support for CCO's CredentialRequest using in ARO and ROSA.
Anything we put in manifests is pure k8s from the beginning. This means we can test this on kind. Anything OpenShift specific is preserved in dedicated subpackages that is applied by patching the structs with mergo.

simonpasquier · 2024-03-14T09:14:52Z

Reading quickly through the discussion, I think that the Loki approach is lean and flexible (thanks @periklis for the details!). FWIW the upstream Prometheus operator does something different: it introspects CRDs + RBAC permissions to adjust the behavior at runtime (e.g. conditional start of some controllers) but the situation isn't the same since we don't control the deployment model (can be custom installation, Helm, OLM, ...) and have to deal with users not updating everything when they upgrade to a new operator version.

One question would be regarding HyperShift: do you foresee (or have you experienced) a situation where the operator depends on a feature that exists in classic OCP but not in HyperShift?

jgbernalp · 2024-03-14T09:32:53Z

Thnx @periklis for the suggestion, I was already working on the feature gates based on Loki Operator example. 7862c0d adds them.

periklis · 2024-03-14T10:55:07Z

One question would be regarding HyperShift: do you foresee (or have you experienced) a situation where the operator depends on a feature that exists in classic OCP but not in HyperShift?

Not yet! But from experience with ARO/ROSA I can foresee the following workflow, assuming Service Delivery is managing the Loki Operator installation for customers:

Open the Loki Operator feature gates as a separate API (e.g. similar to openshift/api's FeatureGates or via this one) to manage on runtime
Or provide a mechanism for HIVE/HyperShift/whatever to populate a separate ConfigMap for our feature gates.

In general our support matrix for feature gates settings in Loki Operator has been static and in turn weakly tested to users manipulating them before installing the operator. We suspect with quite a confidence that some combinations will break the operator entirely. Therefore we give them a static but tested configmap upfront in the bundle as per later being treated immutable by OLM.

pkg/apis/observability-ui/v1alpha1/types.go

danielmellado · 2024-04-04T09:07:19Z

/ok-to-test

docs/user-guides/observability-ui-plugins.md

tremes · 2024-04-04T13:21:54Z

pkg/apis/observability-ui/v1alpha1/types.go

+	// +required
+	// +kubebuilder:validation:Required
+	// +kubebuilder:validation:Enum=dashboards
+	Type UIPluginType `json:"type"`


I was experimenting with ObservabilityUIPlugin creation in an OpenShift cluster and even though the type is displayed as required (in UI), I can create a plugin with an empty value.

Perhaps the // +kubebuilder:validation:Enum=dashboards should be applied to UIPluginType type (example here)

I think this is still problematic (enum required validation). Maybe the ObservabilityUIPluginSpec should not have omitempty for validation to happen and it should probably be // +kubebuilder:validation:Enum=Dashboards

Maybe we could also declare Dashboards as the default value, but I don't know what the future plans are.

@tremes wdyt about the current version here?

deploy/operator/kustomization.yaml

cmd/operator/main.go

simonpasquier · 2024-04-18T14:29:22Z

docs/user-guides/observability-ui-plugins.md

+
+### Dashboards
+
+The plugin will search for datasources as ConfigMaps in the `openshift-config-managed` namespace with the `console.openshift.io/dashboard-datasource: 'true'` label. The namespace `openshift-config-managed` is required, more details on how to create a datasource ConfigMap can be found in the [console-dashboards-plugin](https://github.com/openshift/console-dashboards-plugin/blob/main/docs/add-datasource.md)


we might need to update this section if the plugin searches only in the COO namespace.

The plugin will search only in the openshift-config-managed namespace. This can be adjusted in the future to be a configurable value. But now it follows the same namespace as the console dashboards.

pkg/controllers/uiplugin/components.go

simonpasquier · 2024-04-18T14:39:25Z

pkg/controllers/uiplugin/plugin_info_builder.go

+	case uiv1alpha1.TypeDashboards:
+		{
+			readerRoleName := plugin.Name + "-datasource-reader"
+			datasourcesNamespace := "openshift-config-managed"


just for my own education, this is currently hardcoded by the plugin?

This can be configured in the plugin using the --dashboards-namespace flag but its default is openshift-config-managed. At this point there is a single deployment template used for any plugin. This might change when we add support for other plugins that need specific options.

simonpasquier · 2024-04-18T15:49:08Z

pkg/controllers/uiplugin/plugin_info_builder.go

+					},
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      readerRoleName,
+						Namespace: datasourcesNamespace,


if the role is limited to the COO namespace, shouldn't we adjust the --dashboards-namespace plugin flags accordingly?
https://github.com/openshift/console-dashboards-plugin/blob/6800afc1e9bd785e31bf0a7a4b7d282d60eb0b1a/cmd/plugin-backend.go#L17

simonpasquier · 2024-04-18T15:51:19Z

pkg/controllers/uiplugin/controller.go

+// RBAC for managing observability ui plugin objects
+//+kubebuilder:rbac:groups=apps,resources=deployments,verbs=list;watch;create;update;patch;delete
+//+kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles;rolebindings,verbs=list;watch;create;update;delete;patch
+//+kubebuilder:rbac:groups="",resources=serviceaccounts;services;configmaps,verbs=get;list;watch;create;update;patch;delete


Do we really need create/update/patch/delete permissions?
It can be adjusted as a follow-up and in fact, we probably want to review/tidy up the operator permissions after the PR merges.

I created a follow up task https://issues.redhat.com/browse/COO-121

simonpasquier · 2024-04-18T15:52:39Z

deploy/operator/observability-operator-cluster-role.yaml

+  resources:
+  - uiplugins
+  verbs:
+  - create


same remark here regarding permissions

I created a follow up task https://issues.redhat.com/browse/COO-121

…lers

IIUC the concern was about the UIPlugin resource being namespace-scoped, it's now cluster-scoped.

simonpasquier · 2024-04-19T13:21:44Z

pkg/reconciler/reconciler.go

 func (r Updater) Reconcile(ctx context.Context, c client.Client, scheme *runtime.Scheme) error {
-	if r.resourceOwner.GetNamespace() == r.resource.GetNamespace() {
+	// If the resource owner is in the same namespace as the resource, or if the resource owner is cluster scoped set the owner reference.
+	if r.resourceOwner.GetNamespace() == r.resource.GetNamespace() || r.resourceOwner.GetNamespace() == "" {


simonpasquier · 2024-04-19T15:07:35Z

I didn't test/review in depth the latest version of the PR but you've got my virtual /lgtm :)

danielmellado · 2024-04-22T06:33:45Z

Besides the nit issue on the commit-lint IMHO this is ready to merge. @jgbernalp mind addressing that? Tnx!

openshift-ci · 2024-04-22T09:19:17Z

@jgbernalp: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/observability-operator-e2e	`090b61e`	link	true	`/test observability-operator-e2e`

Full PR test history. Your PR dashboard.

Details

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/test-infra repository. I understand the commands that are listed here.

jan--f · 2024-04-22T13:57:56Z

Works as expected, follow-up tasks for some minor nits are created and tracked. Good to go, thanks everyone!
/lgtm

jan--f · 2024-04-22T13:58:50Z

/approve

openshift-ci · 2024-04-22T14:00:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jan--f, jgbernalp

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [jan--f]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jgbernalp requested a review from a team as a code owner March 12, 2024 16:47

jgbernalp requested review from danielmellado and jan--f and removed request for a team March 12, 2024 16:47

openshift-ci bot added the needs-ok-to-test label Mar 12, 2024

jgbernalp force-pushed the add-observability-ui-plugins branch 2 times, most recently from f0c64c4 to b81e843 Compare March 13, 2024 14:31

openshift-merge-robot added the needs-rebase label Mar 13, 2024

jgbernalp force-pushed the add-observability-ui-plugins branch from b81e843 to 6707faf Compare March 13, 2024 14:35

openshift-merge-robot removed the needs-rebase label Mar 13, 2024

jan--f mentioned this pull request Mar 13, 2024

feat: Add support for LoggingStack and ObservabilityUIPlugin resources #417

Closed

tremes reviewed Mar 14, 2024

View reviewed changes

pkg/apis/observability-ui/v1alpha1/types.go Show resolved Hide resolved

jgbernalp force-pushed the add-observability-ui-plugins branch from 7862c0d to eb5f959 Compare March 14, 2024 13:54

openshift-merge-robot added the needs-rebase label Mar 17, 2024

jgbernalp force-pushed the add-observability-ui-plugins branch from eb5f959 to c588944 Compare March 19, 2024 11:06

openshift-merge-robot removed the needs-rebase label Mar 19, 2024

tremes mentioned this pull request Mar 26, 2024

PoC for general processing of status conditions for multiple operands #447

Closed

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels Apr 4, 2024

tremes reviewed Apr 4, 2024

View reviewed changes

docs/user-guides/observability-ui-plugins.md Outdated Show resolved Hide resolved

tremes reviewed Apr 4, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase label Apr 18, 2024

jan--f reviewed Apr 18, 2024

View reviewed changes

deploy/operator/kustomization.yaml Outdated Show resolved Hide resolved

jan--f reviewed Apr 18, 2024

View reviewed changes

cmd/operator/main.go Outdated Show resolved Hide resolved

simonpasquier reviewed Apr 18, 2024

View reviewed changes

simonpasquier mentioned this pull request Apr 18, 2024

COO-117: test: add test to detect OwnerRefInvalidNamespace event #465

Merged

jgbernalp force-pushed the add-observability-ui-plugins branch from 53789b5 to adece23 Compare April 18, 2024 18:17

jgbernalp added 3 commits April 18, 2024 20:17

feat: add observability ui plugins api

fa28067

feat: add feature gates to enable openshift specific apis and control…

730c85c

…lers

feat: rename ui plugins group name

bc3b3a7

jgbernalp force-pushed the add-observability-ui-plugins branch from adece23 to 6e99265 Compare April 18, 2024 18:19

openshift-merge-robot removed the needs-rebase label Apr 18, 2024

simonpasquier reviewed Apr 19, 2024

View reviewed changes

jgbernalp force-pushed the add-observability-ui-plugins branch from 4ccc116 to 090b61e Compare April 22, 2024 08:11

feat: make UIPlugin cluster scoped

a7e6188

jgbernalp force-pushed the add-observability-ui-plugins branch from 090b61e to a7e6188 Compare April 22, 2024 08:26

openshift-ci bot assigned jan--f Apr 22, 2024

openshift-ci bot added the lgtm label Apr 22, 2024

openshift-ci bot added the approved label Apr 22, 2024

jan--f approved these changes Apr 22, 2024

View reviewed changes

jan--f merged commit 92bae83 into rhobs:main Apr 22, 2024


		### Dashboards

		The plugin will search for datasources as ConfigMaps in the `openshift-config-managed` namespace with the `console.openshift.io/dashboard-datasource: 'true'` label. The namespace `openshift-config-managed` is required, more details on how to create a datasource ConfigMap can be found in the [console-dashboards-plugin](https://github.com/openshift/console-dashboards-plugin/blob/main/docs/add-datasource.md)

Conversation

jgbernalp commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Mar 12, 2024

Uh oh!

jan--f commented Mar 13, 2024

Uh oh!

jgbernalp commented Mar 13, 2024

Uh oh!

danielmellado commented Mar 13, 2024

Uh oh!

periklis commented Mar 13, 2024

Uh oh!

simonpasquier commented Mar 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgbernalp commented Mar 14, 2024

Uh oh!

periklis commented Mar 14, 2024

Uh oh!

Uh oh!

danielmellado commented Apr 4, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonpasquier commented Apr 19, 2024

Uh oh!

danielmellado commented Apr 22, 2024

Uh oh!

openshift-ci bot commented Apr 22, 2024

Uh oh!

jan--f commented Apr 22, 2024

Uh oh!

jan--f commented Apr 22, 2024

Uh oh!

openshift-ci bot commented Apr 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jgbernalp commented Mar 12, 2024 •

edited

Loading

simonpasquier commented Mar 14, 2024 •

edited

Loading