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

genpolicy: hide logs by default #771

Merged
merged 1 commit into from
Aug 1, 2024
Merged

genpolicy: hide logs by default #771

merged 1 commit into from
Aug 1, 2024

Conversation

Freax13
Copy link
Contributor

@Freax13 Freax13 commented Jul 30, 2024

Logs may contain sensitive data and so they should be hidden by default. Upstream kata hides logs by default, but the Microsoft fork made them visible. This patch reverts back to hiding the logs. This won't affect the coordinator though because it uses a different set of settings. Note that this settings file is only included in release builds (.#cli-release).

Logs can be re-enabled by changing the ReadStreamRequest in settings.json to true.

Logs may contain sensitive data and so they should be hidden by
default. Upstream kata hides logs by default, but the Microsoft fork
made them visible. This patch reverts back to hiding the logs. This
won't affect the coordinator though because it uses a different set of
settings. Note that this settings file is only included in release
builds (.#cli-release).
@Freax13 Freax13 added the changelog PRs that should be part of the release notes label Jul 30, 2024
Copy link
Contributor

@burgerdev burgerdev 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 this is a good idea, but we should take a roadmap item to allow overriding this, maybe even on a per-workload basis. This also raises questions on whether I can hide the default config from users in #764.

@Freax13
Copy link
Contributor Author

Freax13 commented Jul 30, 2024

I think this is a good idea, but we should take a roadmap item to allow overriding this, maybe even on a per-workload basis. This also raises questions on whether I can hide the default config from users in #764.

Without #764, the user can just change the settings, though not on a per-workload basis. It might make sense to keep exposing the whole settings file to the user because it also allows them to change more useful settings such as ExecProcessRequest which might be useful for debugging.

@burgerdev
Copy link
Contributor

I think this is a good idea, but we should take a roadmap item to allow overriding this, maybe even on a per-workload basis. This also raises questions on whether I can hide the default config from users in #764.

Without #764, the user can just change the settings, though not on a per-workload basis. It might make sense to keep exposing the whole settings file to the user because it also allows them to change more useful settings such as ExecProcessRequest which might be useful for debugging.

Acknowledged, will consider that in my PR.

@burgerdev
Copy link
Contributor

I think we can merge this, but should probably label as breaking change.

@Freax13 Freax13 added breaking change A user-affecting breaking change and removed changelog PRs that should be part of the release notes labels Aug 1, 2024
@Freax13 Freax13 merged commit ad20de0 into main Aug 1, 2024
11 of 12 checks passed
@Freax13 Freax13 deleted the tom/no-logs-by-default branch August 1, 2024 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change A user-affecting breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants