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.

Sign up for GitHub

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

Jump to bottom

Run collectors as nonroot user #2413

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Run collectors as nonroot user #2413

wants to merge 9 commits into from

Conversation

edeNFed
Copy link
Contributor

@edeNFed edeNFed commented Feb 9, 2025 •
edited
Loading

  • Remove odigosresource processor
  • Set log access via init container
  • Limit hostmetrics scope by mounting less directories

@edeNFed edeNFed marked this pull request as ready for review February 9, 2025 14:54
@edeNFed edeNFed requested review from RonFed and blumamir and removed request for RonFed February 9, 2025 14:54
blumamir
blumamir approved these changes Feb 10, 2025
View reviewed changes
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.

LGTM.

  1. do we still need this?
  2. wondering if we need to test this in openshift as well?

collector/Dockerfile
@@ -5,6 +5,8 @@ WORKDIR /go/src/
ARG TARGETARCH
RUN GOOS=linux GOARCH=$TARGETARCH make build-odigoscol

FROM gcr.io/distroless/base:latest
# Choose a minimal image with sh + acl utils
FROM registry.access.redhat.com/ubi9:9.5-1736404036
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we want to use redhat base image even for non-rhel images?

Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't need that

@blumamir blumamir mentioned this pull request Feb 10, 2025
Closed
4 tasks
@edeNFed
Copy link
Contributor Author

edeNFed commented Feb 11, 2025

@blumamir I rolled back the changes to hostmetrics.
will do the rest of the comments on a separate PR if that's fine with you 🙏

damemi
damemi reviewed Feb 12, 2025
View reviewed changes
collector/Dockerfile
@@ -5,6 +5,8 @@ WORKDIR /go/src/
ARG TARGETARCH
RUN GOOS=linux GOARCH=$TARGETARCH make build-odigoscol

FROM gcr.io/distroless/base:latest
# Choose a minimal image with sh + acl utils
FROM registry.access.redhat.com/ubi9:9.5-1736404036
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't need that

autoscaler/controllers/datacollection/daemonset.go
Name: "set-logs-acls",
Image: commonconfig.ControllerConfig.CollectorImage,
Command: []string{
"/bin/sh",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure, but I'll test if we'll need similar selinux changes for openshift (like in odiglet:

odigos/odiglet/pkg/instrumentation/fs/agents.go

Line 149 in 1872e35

func ApplyOpenShiftSELinuxSettings() error {
)

@BenElferink BenElferink added the enhancement New feature or request label Mar 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@damemi damemi damemi left review comments

@RonFed RonFed RonFed left review comments

@blumamir blumamir blumamir approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@edeNFed @damemi @blumamir @RonFed @BenElferink