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

Document server access enhanced recording option root_path #38399

Merged
merged 6 commits into from
Apr 1, 2024
Merged

Document server access enhanced recording option root_path #38399

merged 6 commits into from
Apr 1, 2024

Conversation

gabrielcorado
Copy link
Contributor

Documents new ssh_service.enhanced_recording.root_path option added by #38066.

@gabrielcorado gabrielcorado added the no-changelog Indicates that a PR does not require a changelog entry label Feb 19, 2024
@gabrielcorado gabrielcorado self-assigned this Feb 19, 2024
@github-actions GitHub Actions
Copy link

🤖 Vercel preview here: https://docs-cynrl1kiy-goteleport.vercel.app/docs/ver/preview

@zmb3
Copy link
Collaborator

zmb3 commented Feb 19, 2024

Should we mention this anywhere outside of the inline comment in the configuration?

ptgott
ptgott reviewed Feb 20, 2024
View reviewed changes
docs/pages/includes/config-reference/ssh-service.yaml
Comment on lines +57 to +58
# Optional: Controls the path inside cgroupv2 hierarchy where Teleport
# cgroups will be placed. Default value: /teleport
Copy link
Contributor

Choose a reason for hiding this comment

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

When would a user want to change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a specific use-case where the user wants to add security enforcements (on the OS level) while running multiple SSH services on the same machine.

@gabrielcorado
Copy link
Contributor Author

@zmb3 @ptgott I've added a section about the use case of this option. I had some difficulties making it generic for our documentation. I'm open to suggestions about the content and its location. Thanks!

@github-actions GitHub Actions
Copy link

🤖 Vercel preview here: https://docs-rl0dilrh1-goteleport.vercel.app/docs/ver/preview

zmb3
zmb3 reviewed Feb 27, 2024
View reviewed changes
docs/pages/server-access/guides/bpf-session-recording.mdx Outdated
```

<Details title="Isolate session recordings">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I would use the term "session recordings" here. It makes it sound like you're specifying a directory to store session recordings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated it, and the content to relate to those resources as "system resources". This aligns with how cgroup describe its hierarchy:

cgroup is a mechanism to organize processes hierarchically and distribute system resources along the hierarchy in a controlled and configurable manner.

ptgott
ptgott approved these changes Feb 27, 2024
View reviewed changes
Copy link
Contributor

@ptgott ptgott left a comment

Choose a reason for hiding this comment

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

Approved with a comment, assuming Zac's feedback gets applied.

docs/pages/server-access/guides/bpf-session-recording.mdx Outdated

If you operate multiple Teleport instances on the same system, customize the
`root_path` configuration to change the base cgroup slice path for session
recordings resources. This ensures security isolation between recordings from
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 what "session recordings resources" means. Should this be "session recordings"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated it to match the title. More info at my comment above.

@ptgott
Copy link
Contributor

ptgott commented Feb 27, 2024

@gabrielcorado Are we backporting this PR?

@github-actions GitHub Actions
Copy link

🤖 Vercel preview here: https://docs-3mkidh7mh-goteleport.vercel.app/docs/ver/preview

@github-actions GitHub Actions
Copy link

🤖 Vercel preview here: https://docs-28lkealhg-goteleport.vercel.app/docs/ver/preview

@greedy52 greedy52 self-requested a review March 27, 2024 16:18
greedy52
greedy52 approved these changes Apr 1, 2024
View reviewed changes
Copy link
Contributor

@greedy52 greedy52 left a comment

Choose a reason for hiding this comment

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

lint seems still failing

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 1, 2024

🤖 Vercel preview here: https://docs-3o7kuh97e-goteleport.vercel.app/docs/ver/preview

@gabrielcorado gabrielcorado added this pull request to the merge queue Apr 1, 2024
Merged via the queue into master with commit 775391b Apr 1, 2024
36 checks passed
@gabrielcorado gabrielcorado deleted the gabrielcorado/docs-add-root-path-option branch April 1, 2024 19:32
@public-teleport-github-review-bot
Copy link

@gabrielcorado See the table below for backport results.

Branch Result
branch/v13 Create PR
branch/v14 Create PR
branch/v15 Create PR

This was referenced Apr 1, 2024
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greedy52 greedy52 greedy52 approved these changes

@ptgott ptgott ptgott approved these changes

@zmb3 zmb3 Awaiting requested review from zmb3

Assignees

@gabrielcorado gabrielcorado

Labels
backport/branch/v13 backport/branch/v14 backport/branch/v15 documentation no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@gabrielcorado @zmb3 @ptgott @greedy52