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: add utility script for containerd pull #213

Draft
wants to merge 3 commits into
base: msft-main
Choose a base branch
from

Conversation

Redent0r
Copy link

@Redent0r Redent0r commented Aug 5, 2024

Add script that helps adapt containerd and docker config for containerd pull feature to work as expected.

Merge Checklist
  • Followed patch format from upstream recommendation: https://github.com/kata-containers/community/blob/main/CONTRIBUTING.md#patch-format
    • Included a single commit in a given PR - at least unless there are related commits and each makes sense as a change on its own.
  • Aware about the PR to be merged using "create a merge commit" rather than "squash and merge" (or similar)
  • genPolicy only: Ensured the tool still builds on Windows
  • The upstream/missing label (or upstream/not-needed) has been set on the PR.
Summary

Add script that helps adapt containerd and docker config for containerd pull feature to work as expected.

Test Methodology

Tested locally that I'm able to authenticate to private registry after running the script

@Redent0r Redent0r added the upstream/missing PRs that are yet to be upstreamed label Aug 5, 2024
Add script that helps adapt containerd and docker config for containerd
pull feature to work as expected.

Signed-off-by: Saul Paredes <[email protected]>
@Redent0r Redent0r force-pushed the saulparedes/add_util_setup_script branch from c36d773 to 7f872b1 Compare August 5, 2024 18:44
src/tools/genpolicy/setup_containerd_docker.sh Outdated Show resolved Hide resolved
src/tools/genpolicy/setup_containerd_docker.sh Outdated Show resolved Hide resolved
src/tools/genpolicy/setup_containerd_docker.sh Outdated Show resolved Hide resolved
src/tools/genpolicy/setup_containerd_docker.sh Outdated Show resolved Hide resolved
You may specify `-d` to use existing `containerd` installation as image manager. This method supports a wider set of images (e.g., older images with `v1` manifest). Needs `sudo` permission to access socket - e.g.,
# Use containerd to pull and manage images (required for managed identity based authentication)

Prereq: This features needs to run the following script to adapt your docker and containerd config (needs `sudo` access):

Choose a reason for hiding this comment

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

I think these changes are too intrusive to push average users to use this script.

For average users, I would prefer a list of steps in an MD file - similar to the steps from https://github.com/kata-containers/kata-containers/blob/main/docs/how-to/containerd-kata.md. Also, I would like to update such doc Upstream instead of pushing just MSFT users in this direction.

But let's talk with other folks in our team too - maybe they have different opinions.

Copy link
Author

Choose a reason for hiding this comment

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

I agree it's too much to require them to use this. This might exist for users that don't feel like doing all these steps, but they are also welcome to do them on their own. I'll add better instructions to the MD, and open a PR upstream with all squashed changes

Choose a reason for hiding this comment

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

I disagree with:

  1. Calling this script a prerequisite. We should say something like: "if you need help with your containerd settings, see this doc for related tips".
  2. Hiding a bunch of details in a script. Users that want to perform these steps should review the doc and perform just those steps that look reasonable to them.

Add guide on how to setup containerd and docker
for genpolicy containerd pull feature.

Signed-off-by: Saul Paredes <[email protected]>
You may specify `-d` to use existing `containerd` installation as image manager. This method supports a wider set of images (e.g., older images with `v1` manifest). Needs `sudo` permission to access socket - e.g.,
# Use containerd to pull and manage images (required for managed identity based authentication)

Prereq: This features needs to run the following script to adapt your docker and containerd config (needs `sudo` access):

Choose a reason for hiding this comment

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

I disagree with:

  1. Calling this script a prerequisite. We should say something like: "if you need help with your containerd settings, see this doc for related tips".
  2. Hiding a bunch of details in a script. Users that want to perform these steps should review the doc and perform just those steps that look reasonable to them.

@@ -0,0 +1,96 @@
# Manual Steps for Setting Up Containerd and Docker for Genpolicy Tool

Choose a reason for hiding this comment

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

The name of this doc should include "genpolicy".

sudo apt-get install -y containerd
```

#### Using `tdnf` (for Photon OS-based systems)

Choose a reason for hiding this comment

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

This is not an automated script, so it's OK to keep just the Ubuntu example. That's one advantage of providing a doc instead a script.

## Steps

### 1. Install Containerd

Choose a reason for hiding this comment

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

Add the OS version that you used for testing - because other versions might not work, and that's OK.


The socket file is usually `/run/containerd/containerd.sock` or `/var/run/containerd/containerd.sock`

### 6. Fix Containerd Socket File Permissions

Choose a reason for hiding this comment

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

This is not a "fix" - it's just something that might be useful for genpolicy users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream/missing PRs that are yet to be upstreamed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants