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

KEP: Arbitrary image pull secrets #1171

Closed
wants to merge 2 commits into from
Closed

KEP: Arbitrary image pull secrets #1171

wants to merge 2 commits into from

Conversation

sashayakovtseva
Copy link

This KEP emerged from kubernetes/kubernetes#79803

@k8s-ci-robot
Copy link
Contributor

Welcome @sashayakovtseva!

It looks like this is your first PR to kubernetes/enhancements 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/enhancements has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 26, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @sashayakovtseva. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 26, 2019
@sashayakovtseva
Copy link
Author

/sig-node

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sashayakovtseva
To complete the pull request process, please assign derekwaynecarr
You can assign the PR to them by writing /assign @derekwaynecarr in a comment when ready.

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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jul 26, 2019
@sashayakovtseva
Copy link
Author

/assign @derekwaynecarr

@dims
Copy link
Member

dims commented Jul 26, 2019

@sashayakovtseva have you seen this? #1137

@sashayakovtseva
Copy link
Author

sashayakovtseva commented Jul 26, 2019

@dims Nope, just took a look. As I understand #1137 is about connecting to kubernetes as a client, kubeconfig and etc, while here I talk about image pull credentials and kubelet component.

@sashayakovtseva sashayakovtseva changed the title Draft a KEP for arbitrary image pull secrets KEP: Arbitrary image pull secrets Jul 26, 2019
@Random-Liu
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 30, 2019
keps/sig-node/20190726-arbitrary-pull-secrets.md Outdated Show resolved Hide resolved
### Goals

- Introduce a new secret type, e.g. `kubernetes.io/regcredentials`.
It should contain data that can be mapped to the CRI's `AuthConfig` subset of fields directly.
Copy link
Member

@Random-Liu Random-Liu Jul 30, 2019

Choose a reason for hiding this comment

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

We may not want to define the schema based on AuthConfig.

CRI is a Kubelet internal API, and we can't guarantee that we won't change or extend AuthConfig in the future. If we changed it, how do we handle the version skew?

Copy link
Author

@sashayakovtseva sashayakovtseva Jul 31, 2019

Choose a reason for hiding this comment

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

Another option would be to pass pull secrets as map[string]string or smth and let CRI decide what to do, but I am not sure that is better than AuthConfig mapping.

Copy link
Member

@Random-Liu Random-Liu Jul 31, 2019

Choose a reason for hiding this comment

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

@sashayakovtseva I'm a bit confused. Does the AuthConfig object meet your requirement?

If it does, it should be equivalent to today's docker secret https://kubernetes.io/docs/tasks/configure-pod-container/pull-image-private-registry/#registry-secret-existing-credentials. Kubelet just converts docker secret into AuthConfig.

If AuthConfig works for you, you just need to fill in the value into corresponding fields in the docker secret, and then the docker secret should just work for you.

Copy link
Author

Choose a reason for hiding this comment

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

@Random-Liu AuthConfig can be used. We are using access tokens to pull private images, so AuthConfig.IdentityToken is the proper field to use IMO.

And yes, the workaround you suggest partially works. Here are the issues that should be addressed:

  1. Forcing any CRI to use secret of type kubernetes.io/dockerconfigjson doesn't match with idea of abstracting runtime with CRI. It also requires username which we don't need because of access tokens instead of username/password pair.
  2. I could not find where AuthConfig.ServerAddress is set on kubelet side. Without that it is not possible to use on-prem image registries which we want to leverage.

That is why even though with a workaround it is possible to pull a private image from default registry, I still believe this should be changed on kubelet side to fully enable runtime abstraction.

Copy link
Member

Choose a reason for hiding this comment

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

It is in the keyring https://github.com/kubernetes/kubernetes/blob/master/pkg/credentialprovider/keyring.go#L66,
and kubelet does pass it to CRI https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/kuberuntime/kuberuntime_image.go#L63.

I'm totally in to a more generic credential format in the Kubernetes api, instead of kubernetes.io/dockerconfigjson. However, we need a use case with more representativeness to define the new API, so we are waiting for a use case that can't be covered by today's kubernetes.io/dockerconfigjson. :)

Copy link
Member

@Random-Liu Random-Liu Aug 7, 2019

Choose a reason for hiding this comment

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

@sashayakovtseva Hm, I think you are right!

However, I don't understand why we don't support ServerAddress, IdentityToken and RegistryToken in the first place...I think we should fix it.

Anyway, if this is the case, there are 2 options:

  1. Fix today's kubernetes.io/dockerconfigjson, and add full docker AuthConfig support.
  2. Define a new format, and based on your proposal, it will just be equivalent to docker AuthConfig.

If so, I prefer 1). :)

Need input from @kubernetes/sig-auth-api-reviews

Copy link
Author

Choose a reason for hiding this comment

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

@Random-Liu IMO (1) should be a temporarily fix unless Kubernetes wishes to fully support only Docker containers even though there will be other CRI implementations.
I understand that technically this will work, but that is really awkward to ask e.g. Singularity users to create Docker secret to pull private images...

Copy link
Member

@Random-Liu Random-Liu Aug 8, 2019

Choose a reason for hiding this comment

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

I want 2) in the long term, but not sure whether we can define the new format correctly at the moment. Thus I prefer 1) for now.

If @kubernetes/sig-auth-api-reviews thinks that the AuthConfig format is sufficient and generic enough for Kubernetes, I'm totally fine with 2). :)

Copy link
Member

@Random-Liu Random-Liu Aug 8, 2019

Choose a reason for hiding this comment

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

BTW, the ServerAddress should be supported anyway to handle the case mentioned in containerd/cri#1227 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to @Random-Liu's suggestion.

I'd like to API to look more generic as well, but given what we have today, it's preferred if we can easily extend the existing API to support your case. If there are other use cases that cannot be easily satisfied, we can introduce a new type.

@k8s-ci-robot
Copy link
Contributor

@sashayakovtseva: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-enhancements-verify ce93e19 link /test pull-enhancements-verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Copy link
Member

@derekwaynecarr derekwaynecarr left a comment

Choose a reason for hiding this comment

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

If this was discussed in SIG Node while I was out, I apologize, but I agree with @Random-Liu . I would be interested if @mrunalp is aware of other credential formats that have been brought forward.

### Goals

- Introduce a new secret type, e.g. `kubernetes.io/regcredentials`.
It should contain data that can be mapped to the CRI's `AuthConfig` subset of fields directly.
Copy link
Member

Choose a reason for hiding this comment

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

I think @Random-Liu addressed the answer to point (2), I am missing where (1) requires a username. It's possible I am missing something. Either way, I am in favor of supporting a generic credential format and/or secret type.

- Introduce a new secret type, e.g. `kubernetes.io/regcredentials`.
It should contain data that can be mapped to the CRI's `AuthConfig` subset of fields directly.

- Do not interpret passed credentials as Docker’s when a secret of type `kubernetes.io/regcredentials`
Copy link
Member

Choose a reason for hiding this comment

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

@mrunalp are there other credential formats that have been brought forward that I may be missing?

password: <base64-encoded-password>
identityToken: <identity-token-here>
registryToken: <registry-token-here>
type: kubernetes.io/regcredentials
Copy link
Member

Choose a reason for hiding this comment

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

if the desire is to support alternative secret types, I think that is potentially a fine thing to do, but I am curious how we know if the configured runtime would now how to manage them. It would seem to me they would still require some type of schema to identify their type.

@sashayakovtseva
Copy link
Author

@derekwaynecarr Regarding point 1. Consider pull secret creation via CLI:

$ kubectl create secret docker-registry regcred --docker-server=cloud.sylabs.io  --docker-password=my-secret-token
Error: required flag(s) "docker-username" not set

Even though cloud.sylabs.io needs only token, docker-registry secret requires username to be set. Of course, some kind of stub can be used here, but again, that is generic at all.

And of course, we can circumvent it with creating docker-registry secret manually like so:

$ echo '{"auths":{"cloud.sylabs.io":{"password":"my-secret-token"}}}' | base64
eyJhdXRocyI6eyJjbG91ZC5zeWxhYnMuaW8iOnsicGFzc3dvcmQiOiJteS1zZWNyZXQtdG9rZW4ifX19Cg==

$ cat > secret.yaml <<EOF
apiVersion: v1
kind: Secret
metadata:
  name: regcred
data:
  .dockerconfigjson: eyJhdXRocyI6eyJjbG91ZC5zeWxhYnMuaW8iOnsicGFzc3dvcmQiOiJteS1zZWNyZXQtdG9rZW4ifX19Cg==
type: kubernetes.io/dockerconfigjson
EOF

$  kubectl apply -f secret.yaml

But that is not convenient for the users.
Also I have not checked yet whether this manually created secret will work at all, will do so after the weekend.

@mikedanese
Copy link
Member

Somewhat related to kubernetes/kubernetes#68810

@k8s-ci-robot k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Aug 8, 2019
@k8s-ci-robot k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Aug 8, 2019
@fejta-bot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@sashayakovtseva
Copy link
Author

According to discussion with @Random-Liu this KEP looks reasonable.
I wonder what are the next steps from my side to get it approved or updated or whatever :)

@Random-Liu
Copy link
Member

@sashayakovtseva I think if we plan to fix existing docker authconfig, I think we just need a bug report and fix it.

If we plan to have a new registry credential type, we need to go through Kubernetes api review. https://github.com/kubernetes/community/blob/master/sig-architecture/api-review-process.md

@sashayakovtseva
Copy link
Author

@Random-Liu Do I have to write that bug report? If so, to the k8s repository?

If we plan to have a new registry credential type, we need to go through Kubernetes api review.
Who is the one to decide that?

Sorry, but I don't really understand what is the action I have to take to proceed with this KEP :)
Should I wait? Should I submit smth else?

It should contain data that can be mapped to the CRI's `AuthConfig` subset of fields directly.

- Do not interpret passed credentials as Docker’s when a secret of type `kubernetes.io/regcredentials`
is used in `imagePullSecrets`. Pass it directly to the CRI implementation and let it decide how to handle them instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t see a reason to make the secret opaque. We can always change the auth config spec or introduce a new secret type - opaque secrets are against our general approach to defining points of consistency between container runtimes.

I would be supportive of letting container runtimes interpret other information about a pod in order to decide on secret values, but we’d need to be more concrete in this proposal about what that actually means.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 24, 2019
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 24, 2019
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 23, 2020
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. sig/auth Categorizes an issue or PR as relevant to SIG Auth. 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
Development

Successfully merging this pull request may close these issues.

9 participants