Skip to content
This repository has been archived by the owner on Mar 9, 2022. It is now read-only.

Add pull image authentication. #88

Merged
merged 1 commit into from
Jun 22, 2017
Merged

Add pull image authentication. #88

merged 1 commit into from
Jun 22, 2017

Conversation

Random-Liu
Copy link
Member

Fixes #58.

I've tried this PR with a private image taotaotheripper/node-problem-detector:v0.2, it works.

However, the node e2e test is still failing because of containerd/containerd#1064, which should possibly be fixed in containerd.

Signed-off-by: Lantao Liu lantaol@google.com

@Random-Liu Random-Liu added this to the v0.1.0-alpha.1 milestone Jun 22, 2017
@Random-Liu
Copy link
Member Author

CRI validation test result:

Summarizing 5 Failures:

[Fail] [k8s.io] Security Context runtime should support container with security context [It] runtime should support RunAsUserName [security context] 
/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/container.go:326

[Fail] [k8s.io] Streaming runtime should support streaming interfaces [It] runtime should support exec [Conformance] 
/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/streaming.go:127

[Fail] [k8s.io] Security Context runtime should support container with security context [It] runtime should support RunAsUser [security context] 
/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/container.go:326

[Fail] [k8s.io] Streaming runtime should support streaming interfaces [It] runtime should support attach [Conformance] 
/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/streaming.go:183

[Fail] [k8s.io] Streaming runtime should support streaming interfaces [It] runtime should support portforward [Conformance] 
/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/streaming.go:231

Ran 35 of 36 Specs in 33.203 seconds
FAIL! -- 30 Passed | 5 Failed | 0 Pending | 1 Skipped --- FAIL: TestE2ECRI (33.20s)
FAIL

Ginkgo ran 1 suite in 33.27664581s
Test Suite Failed
exit status 1

@Random-Liu Random-Liu mentioned this pull request Jun 22, 2017
Closed
42 tasks
@Random-Liu
Copy link
Member Author

Random-Liu commented Jun 22, 2017 •
edited
Loading

After this release, all v0.1.0-alpha.1 goals are achieved. I'll cut the v0.1.0-alpha.1 release.

After that we should be focusing on documentation before v0.1.0.

If containerd cuts 1.0.0-alpha this week, which is unlikely, we'll update containerd api early next week before v0.1.0. Or else, we'll stick with current containerd version, and update after v0.1.0.

@kubernetes-incubator/maintainers-cri-containerd

mikebrow
mikebrow approved these changes Jun 22, 2017
View reviewed changes
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM just a couple nits

pkg/server/image_pull_test.go Outdated
}{
"should not return error if auth config is nil": {
auth: nil,
expectErr: false,
Copy link
Member

Choose a reason for hiding this comment

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

not necessary, false is the default...

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

pkg/server/image_pull_test.go
testPasswd := "password"
testAuthLen := base64.StdEncoding.EncodedLen(len(testUser + ":" + testPasswd))
testAuth := make([]byte, testAuthLen)
base64.StdEncoding.Encode(testAuth, []byte(testUser+":"+testPasswd))
Copy link
Member

@mikebrow mikebrow Jun 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

maybe add a bad auth here to get closer to 100% test coverage on the api

For example:
invalidAuthLen := base64.StdEncoding.EncodedLen(len(testUser + "%" + testPasswd))
invalidDecodeAuth := make([]byte, invalidAuthLen)
base64.StdEncoding.Encode(invalidDecodeAuth, []byte(testUser+"%"+testPasswd)) // % is an invalid base64 char...and an invalid separator

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@Random-Liu
Add pull image authentication.
d5674be 
Signed-off-by: Lantao Liu <lantaol@google.com>
@Random-Liu
Copy link
Member Author

Apply LGTM based on #88 (review).

@Random-Liu Random-Liu merged commit 6de96a5 into containerd:master Jun 22, 2017
@Random-Liu Random-Liu deleted the add-image-authentication branch June 22, 2017 18:49
lanchongyizu pushed a commit to lanchongyizu/cri-containerd that referenced this pull request Sep 3, 2017
@Random-Liu
Merge pull request containerd#88 from Random-Liu/add-image-authentica…
4d6bf55 
…tion

Add pull image authentication.
dcantah pushed a commit to dcantah/cri that referenced this pull request Jan 25, 2021
@katiewasnothere
Merge pull request containerd#88 from katiewasnothere/support_update_…
aeec7b2 
…wcow

Add CRI support for windows container/pod updates
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mikebrow mikebrow mikebrow approved these changes

Assignees

@mikebrow mikebrow

Labels
cncf-cla: yes lgtm
Projects
None yet
Milestone
v0.1.0-alpha.1
Development

Successfully merging this pull request may close these issues.

Pull Image Authentication
3 participants
@Random-Liu @mikebrow @k8s-ci-robot