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

[jaeger] define securityContext for each resources #527

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

Conversation

eumel8
Copy link

@eumel8 eumel8 commented Dec 17, 2023

What this PR does

At least and especially for the all-in-one installation securityContext was missing which brings a bad experienance if you start with Jaeger. This PR

  • add missing podSecurityContext/securityContext all-in-one
  • add a global defintion for podSecurityContext/securityContext
  • re-define value for each kind of workload
  • conditions for other optional resource

Which issue this PR fixes

Checklist

  • DCO signed
  • Commits are GPG signed
  • Chart Version bumped
  • Title of the PR starts with chart name ([jaeger] or [jaeger-operator])
  • README.md has been updated to match version/contain new values

Signed-off-by: Frank Kloeker <f.kloeker@telekom.de>
Signed-off-by: Frank Kloeker <f.kloeker@telekom.de>
charts/jaeger/values.yaml Outdated Show resolved Hide resolved
@@ -613,14 +639,6 @@ query:
pathType:
health:
exposed: false
resources:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove the resources for the ingress?

Copy link
Author

Choose a reason for hiding this comment

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

you mean for query, ingress has no resources. I think this was a mistake and almost twice. It was a bit confusing to walk all the values over the time. If it would make sense to have this in alphabetically order? Then I would do this on another PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the late response on this, I'd prefer refactoring in another PR.

charts/jaeger/values.yaml Outdated Show resolved Hide resolved
Signed-off-by: Frank Kloeker <f.kloeker@telekom.de>
Signed-off-by: Frank Kloeker <f.kloeker@telekom.de>
Signed-off-by: Frank Kloeker <f.kloeker@telekom.de>
Signed-off-by: Frank Kloeker <f.kloeker@telekom.de>
@eumel8
Copy link
Author

eumel8 commented Mar 15, 2024

@pavelnikolov no problem. Everyone complains the lack of open source contribution and then it takes months to review a PR. I had this very often and I ask me: What's wrong with my PR's.

@eumel8 eumel8 closed this Mar 15, 2024
@pavelnikolov pavelnikolov reopened this Mar 15, 2024
@pavelnikolov
Copy link
Contributor

I'm sorry for closing it.

Signed-off-by: Pavel Nikolov <pavelnikolov@users.noreply.github.com>
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.

[jaeger] Decide and implement default security context
2 participants