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

Fix problem accessing private docker registries #57463

Merged
merged 1 commit into from
Dec 20, 2017
Merged

Fix problem accessing private docker registries #57463

merged 1 commit into from
Dec 20, 2017
+18 −18

Conversation

dims
Copy link
Member

@dims dims commented Dec 20, 2017

What this PR does / why we need it:
In 027c8b9, we added code to
move from .dockercfg to config.json file. But we forgot to use
the right secret type and the key to store the base64'ed creds

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #57427 #57273

Special notes for your reviewer:

Release note:

Fixes issue creating docker secrets with kubectl 1.9 for accessing docker private registries.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 20, 2017
@dims
Copy link
Member Author

dims commented Dec 20, 2017

/assign @juanvallejo
/assign @liggitt

@dims
Copy link
Member Author

dims commented Dec 20, 2017

/priority critical-urgent

@k8s-ci-robot k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Dec 20, 2017
@dims dims force-pushed the fix-accessing-private-docker-registries branch from 0b7939b to 30a0c54 Compare December 20, 2017 17:15
@juanvallejo
Copy link
Contributor

cc @smarterclayton

@liggitt
Copy link
Member

liggitt commented Dec 20, 2017

Did we verify this fixes the issue?

@liggitt
Copy link
Member

liggitt commented Dec 20, 2017

Also, it makes me unhappy that we have no end to end tests for this

@ncdc
Copy link
Member

ncdc commented Dec 20, 2017

Needs a cherrypick

@dims
Copy link
Member Author

dims commented Dec 20, 2017

@liggitt @juanvallejo looks like we are doing the right thing on the server side - https://github.com/kubernetes/kubernetes/blob/master/pkg/credentialprovider/keyring.go#L312

@dims
Copy link
Member Author

dims commented Dec 20, 2017

@ncdc ack, as soon as @juanvallejo and i verify things work

@dims
Copy link
Member Author

dims commented Dec 20, 2017

@juanvallejo @liggitt we do have at least one unit test on the server side - https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/kuberuntime/kuberuntime_image_test.go#L154

@dims
Copy link
Member Author

dims commented Dec 20, 2017

@jvallejo do you have a real registry to try with?

@dims dims force-pushed the fix-accessing-private-docker-registries branch from 30a0c54 to 6738da1 Compare December 20, 2017 17:36
@dims
Fix problem accessing private docker registries
6738da1 
In 027c8b9, we added code to
move from .dockercfg to config.json file. But we forgot to use
the right secret type and the key to store the base64'ed creds
@dims
Copy link
Member Author

dims commented Dec 20, 2017

@juanvallejo @liggitt i was able to test it using the docker hub private registry

  Type     Reason                 Age               From                   Message
  ----     ------                 ----              ----                   -------
  Normal   Scheduled              13s               default-scheduler      Successfully assigned private-reg to 192.168.0.42
  Normal   SuccessfulMountVolume  13s               kubelet, 192.168.0.42  MountVolume.SetUp succeeded for volume "default-token-h6wzd"
  Normal   Pulling                12s               kubelet, 192.168.0.42  pulling image "docker.io/dims/my-busybox:0.1"
  Normal   Pulled                 11s               kubelet, 192.168.0.42  Successfully pulled image "docker.io/dims/my-busybox:0.1"
  Normal   Created                9s (x2 over 10s)  kubelet, 192.168.0.42  Created container
  Normal   Started                9s (x2 over 10s)  kubelet, 192.168.0.42  Started container
  Normal   Pulled                 9s                kubelet, 192.168.0.42  Container image "docker.io/dims/my-busybox:0.1" already present on machine

@dims
Copy link
Member Author

dims commented Dec 20, 2017

@cblecker Can you please bless the change in the hack/ directory?

@liggitt @smarterclayton @ncdc - This one is ready to merge. i'll get the cherry pick ready

Thanks,
Dims

@dims dims mentioned this pull request Dec 20, 2017
Merged
@ncdc ncdc added this to the v1.9 milestone Dec 20, 2017
@ncdc
Copy link
Member

ncdc commented Dec 20, 2017

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Dec 20, 2017
@ncdc
Copy link
Member

ncdc commented Dec 20, 2017

/sig node

@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Dec 20, 2017
@deads2k
Copy link
Contributor

deads2k commented Dec 20, 2017

Looks like this is just correcting the secret type so that they keyring does what it is supposed to do when parsing.

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 20, 2017
@ncdc
Copy link
Member

ncdc commented Dec 20, 2017

/lgtm too

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, dims, ncdc

Associated issue: #57427

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request Needs Approval

@adohe @deads2k @dims @fabianofranz @juanvallejo @liggitt @kubernetes/sig-node-misc

Action required: This pull request must have the status/approved-for-milestone label applied by a SIG maintainer.

Pull Request Labels
  • sig/node: Pull Request will be escalated to these SIGs if needed.
  • priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
  • kind/bug: Fixes a bug discovered during the current release.
Help

@dims
Copy link
Member Author

dims commented Dec 20, 2017

@deads2k yep! line 94 and 96 in pkg/kubectl/secret_for_docker_registry.go are what fixes the problem.

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 767fde1 into kubernetes:master Dec 20, 2017
k8s-github-robot pushed a commit that referenced this pull request Dec 28, 2017
Merge pull request #57472 from dims/automated-cherry-pick-of-#57463-u…
5d58197 
…pstream-release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #57463: Fix problem accessing private docker registries

Cherry pick of #57463 on release-1.9.

#57463: Fix problem accessing private docker registries

```release-note
Fixes issue creating docker secrets with kubectl 1.9 for accessing docker private registries.
```
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.9" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

@dims dims mentioned this pull request Jan 2, 2018
Closed
@tengqm tengqm mentioned this pull request Jan 6, 2018
Closed
1 task
@dims dims mentioned this pull request Jan 9, 2018
Closed
@arno01 arno01 mentioned this pull request Jan 11, 2018
Closed
@FengyunPan FengyunPan mentioned this pull request Jan 15, 2018
Closed
@andresmgot andresmgot mentioned this pull request Mar 16, 2018
Merged
2 tasks
@juanvallejo juanvallejo mentioned this pull request Apr 2, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juanvallejo juanvallejo juanvallejo approved these changes

Assignees

@fabianofranz fabianofranz

@liggitt liggitt

@adohe-zz adohe-zz

@juanvallejo juanvallejo

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. milestone/needs-approval priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.9
Development

Successfully merging this pull request may close these issues.

Issues with private Docker Cloud repos and the 1.9.0 CLI
10 participants
@dims @juanvallejo @liggitt @ncdc @deads2k @k8s-ci-robot @k8s-github-robot @k8s-cherrypick-bot @fabianofranz @adohe-zz