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

Expose pod security context per component #1029

Merged
merged 6 commits into from
Sep 27, 2024

Conversation

rubenvp8510
Copy link
Collaborator

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Sep 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.10%. Comparing base (c39140e) to head (1cd1cdb).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1029      +/-   ##
==========================================
+ Coverage   73.08%   73.10%   +0.02%     
==========================================
  Files         106      106              
  Lines        6624     6630       +6     
==========================================
+ Hits         4841     4847       +6     
  Misses       1493     1493              
  Partials      290      290              
Flag Coverage Δ
unittests 73.10% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rubenvp8510 rubenvp8510 changed the title [WIP] Expose pod security context per component Expose pod security context per component Sep 15, 2024
Copy link
Collaborator

@andreasgerstmayr andreasgerstmayr left a comment

Choose a reason for hiding this comment

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

I think we should also set it on the compactor component

@rubenvp8510 rubenvp8510 force-pushed the add_pod_security_context branch 2 times, most recently from cf9ac9b to 04ecd16 Compare September 20, 2024 03:18
@rubenvp8510
Copy link
Collaborator Author

@andreasgerstmayr do you think we need to put a default value?

rubenvp8510 and others added 2 commits September 19, 2024 21:18
Co-authored-by: Andreas Gerstmayr <[email protected]>
Signed-off-by: Ruben Vargas <[email protected]>
Signed-off-by: Ruben Vargas <[email protected]>
@andreasgerstmayr
Copy link
Collaborator

@andreasgerstmayr do you think we need to put a default value?

We do set a default security context on the containers, for example here:

SecurityContext: manifestutils.TempoContainerSecurityContext(),

It has some common fields, but is not the same type. I'm not sure if we should set any defaults (RunAsNonRoot maybe? but that could break if users use a custom Tempo image).

Do you think the container security context should also be customizable? Then we should plan in advance and give the two different security contexts a distinctive name, for example containerSecurityContext and podSecurityContext in the CRD.

@rubenvp8510
Copy link
Collaborator Author

@andreasgerstmayr do you think we need to put a default value?

We do set a default security context on the containers, for example here:

SecurityContext: manifestutils.TempoContainerSecurityContext(),

It has some common fields, but is not the same type. I'm not sure if we should set any defaults (RunAsNonRoot maybe? but that could break if users use a custom Tempo image).

Do you think the container security context should also be customizable? Then we should plan in advance and give the two different security contexts a distinctive name, for example containerSecurityContext and podSecurityContext in the CRD.

I would say we can preserve this without any default value for now. and agree on the renaming.

Signed-off-by: Ruben Vargas <[email protected]>
Signed-off-by: Ruben Vargas <[email protected]>
@andreasgerstmayr andreasgerstmayr merged commit c91129c into grafana:main Sep 27, 2024
11 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.

Ingester fails on DigitalOcean Kubernetes due to permission issues
3 participants