Skip to content
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

Updates for OpenShift compatibility #2348

Merged
merged 12 commits into from
Jan 31, 2025

Conversation

damemi
Copy link
Contributor

@damemi damemi commented Jan 29, 2025

OpenShift seems to enable OwnerReferencesPermissionEnforcement by default, which gives errors like this when setting OwnerReferences:

2025-01-29T19:42:38Z	ERROR	Reconciler error	{"controller": "startlangdetection-source", "controllerGroup": "odigos.io", "controllerKind": "Source", "Source": {"name":"source-xsbv2","namespace":"test-project"}, "namespace": "test-project", "name": "source-xsbv2", "reconcileID": "c36405f6-40bb-4eb0-9d5d-fe4bc7802994", "error": "instrumentationconfigs.odigos.io \"deployment-inventory\" is forbidden: cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't set finalizers on: , <nil>"}

This basically means wherever we update an ownerreference with BlockOwnerDeletion: true (which is set by the controller-runtime helpers), we also need permission to update the finalizers for whatever we're making the owner.

I found this in these places:

  • When the autoscaler creates a collector configmap, the owner of the ConfigMap is the CollectorsGroups. So we need to be able to update collectorsgroups/finalizers
  • When the Instrumentor creates an InstrumentationConfig, the owner of the InstrumentationConfig is the workload. So we need finalizers for deployments,statefulsets,and daemonsets
  • When the odiglet creates an InstrumentationInstance, the owner of the InstrumentationInstance is also the workload. Same as above

This also adds the SELinux volume mount from #1131 to the Odiglet init container added in #1355, along with making the init container privileged and adding logging for when the SELinux commands aren't found (this was failing silently)

This also updates the --openshift flag to automatically use the *-ubi9 images. If no --image-prefix is set, the images will be pulled from registry.connect.redhat.com/odigos.

@RonFed
Copy link
Collaborator

RonFed commented Jan 29, 2025

The owners of instrumentation instances are pods, not workloads.

Does setting the finalizers on the workload happen today for setups that are not OpenShift?
I wonder if the owner of instrumentation config should be the Source

@damemi
Copy link
Contributor Author

damemi commented Jan 29, 2025

@RonFed I don't think this admission enforcement is on by default in most clusters. We don't even necessarily have to set the finalizers to trigger this, it's that we are trying to set ownerreferences with BlockDeletion.

Source shouldn't be the ownerreference for the instrumentationconfig, because Sources can persist beyond a workload existing. InstrumentationConfig should be deleted when the workload is deleted

Updated to use Pods instead of workloads for odiglet

@damemi damemi changed the title Add finalizers permissions for OwnerReferencesPermissionEnforcement Updates for OpenShift compatibility Jan 30, 2025
@damemi damemi force-pushed the finalizers-permissions branch 2 times, most recently from cc72bc8 to b45fbba Compare January 30, 2025 15:07
@damemi damemi force-pushed the finalizers-permissions branch from b45fbba to 2800f74 Compare January 30, 2025 15:49
@damemi damemi force-pushed the finalizers-permissions branch 2 times, most recently from ce3a1c8 to cf9296c Compare January 30, 2025 19:54
@damemi damemi force-pushed the finalizers-permissions branch from cf9296c to cb97ce2 Compare January 30, 2025 21:45
Copy link
Collaborator

@blumamir blumamir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a difficult task, great work.

Copy link
Collaborator

@RonFed RonFed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm,
left a few small comments.
Also, worth making sure that this change won't break users that use the image prefix option - it looks to me like it won't but just making sure.
In the CLI we are using the same GetImageName and in the helm there is the new util function which looks to preserve the current behavior.

@@ -4,6 +4,8 @@ import (
"k8s.io/apimachinery/pkg/util/version"
)

const RedHatImagePrefix = "registry.connect.redhat.com/odigos"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this looks specific to the CLI, maybe we should move this constant there,
edit: I see that a lot of the consts here are used only in the CLI, so I guess we can keep it here for consistency

@damemi damemi enabled auto-merge (squash) January 31, 2025 16:06
@damemi damemi merged commit 3808517 into odigos-io:main Jan 31, 2025
44 of 46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants