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

test:e2e: Add cri auth test #290

Merged
merged 1 commit into from
Dec 15, 2022
Merged

Conversation

darfux
Copy link
Contributor

@darfux darfux commented Dec 15, 2022

Make the original k8s-e2e workflow to be a reusable workflow template, which is used by cri case and kubeconf case with different auth-type input.

Differences between cri case and kubeconf case:

  • CRI snapshotter spec needs extra containerd socket mount
  • CRI snapshotter spec has no cluster role for secrets
  • CRI snapshotter spec uses --enable-cri-keychain instead of --enable-kubeconfig-keychain
  • In the Run E2E test step, cri case needs to change the image service endpoint of kubelet to nydus-snapshotter

Other changes:

  • Add imagePullSecrets to test pod spec and change it's namespace to nydus-system. So that kubelet can get the image secret under both cri&kubeconf cases. The auth info in the containerd config file is no longer needed and has been removed.
  • Add a ps -ef info log in the Dump logs step

Signed-off-by: Li Yuxuan liyuxuan.darfux@bytedance.com

@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2022

Codecov Report

Base: 33.88% // Head: 33.88% // No change to project coverage 👍

Coverage data is based on head (779226f) compared to base (cbaaea1).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #290   +/-   ##
=======================================
  Coverage   33.88%   33.88%           
=======================================
  Files          30       30           
  Lines        3223     3223           
=======================================
  Hits         1092     1092           
  Misses       2019     2019           
  Partials      112      112           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@darfux darfux force-pushed the add-cri-auth-e2e branch 3 times, most recently from 779226f to a0d1580 Compare December 15, 2022 02:35
@darfux
Copy link
Contributor Author

darfux commented Dec 15, 2022

Add e2e test for PR #282 /cc @changweige

@changweige
Copy link
Member

@liubin Do you have time to take a look at this PR too?

@@ -91,10 +89,8 @@ jobs:

[plugins."io.containerd.grpc.v1.cri".registry.mirrors."${registry_ip}:5000"]
endpoint = ["http://${registry_ip}:5000"]
[plugins."io.containerd.grpc.v1.cri".registry.configs."${registry_ip}:5000".auth]
username = "${{ env.DOCKER_USER }}"
password = "${{ env.DOCKER_PASSWORD }}"
Copy link
Contributor

@liubin liubin Dec 15, 2022

Choose a reason for hiding this comment

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

Is this not required?
I see the CI passed, but it seems both cri and kubeconf are useing the same node to run test.
You added image-service-endpoint to kubelet, but did not delete it when running kubeconfg test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From using-jobs-in-a-workflow and using-a-github-hosted-runner:

Each job runs in a runner environment specified by runs-on.
When the job begins, GitHub automatically provisions a new VM for that job.

I think each job will run in a standalone runner environment(VM) and don't share any data 🤔

Is this not required?

Under kubeconf case, kubelet can get the image secret from imagePullSecrets as well and pass it to containerd now.

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 tried to run a single kubeconf case: https://github.com/darfux/nydus-snapshotter/actions/runs/3701944536/jobs/6271709388 and it seems still OK. @liubin FYI😀

Make the original k8s-e2e workflow to be a reusable workflow template,
which is used by cri case and kubeconf case with different `auth-type`
input.

Differences between cri case and kubeconf case:
- CRI snapshotter spec needs extra containerd socket mount
- CRI snapshotter spec has no cluster role for secrets
- CRI snapshotter spec uses `--enable-cri-keychain` instead of
`--enable-kubeconfig-keychain`
- In the `Run E2E test` step, cri case needs to change the image service
endpoint of kubelet to nydus-snapshotter

Other changes:
- Add `imagePullSecrets` to test pod spec and change it's namespace to
`nydus-system`. So that kubelet can get the image secret under both
cri&kubeconf cases. The auth info in the containerd config file is no
longer needed and has been removed.
- Add a `ps -ef` info log in the `Dump logs` step

Signed-off-by: Li Yuxuan <liyuxuan.darfux@bytedance.com>
@changweige changweige merged commit 6c0b103 into containerd:main Dec 15, 2022
@darfux darfux deleted the add-cri-auth-e2e branch December 15, 2022 08:18
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.

None yet

4 participants