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

Create Ca-certificate configmap used by token projected volume #68812

Merged
merged 1 commit into from Nov 8, 2018

Conversation

@WanLinghao
Copy link
Contributor

WanLinghao commented Sep 19, 2018

To inject ca.crt into container when projected volume was specified, configmap should be created in each namespace.This patch add a controller called "ca-configmap" to complete above job as well as some bootstrap rbac policies.

fix: #68811

NONE
@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Sep 20, 2018

I expected these pieces to be added by the admission plugin, and the backing configmaps for the CA to be created (or populated) by the controller manager (in all namespaces, not just the kube-system one), which already has that info

/assign @mikedanese

@liggitt liggitt added the sig/auth label Sep 20, 2018

@WanLinghao WanLinghao force-pushed the WanLinghao:token_projection_ca_secret_create branch from 9b83cbe to 75ae519 Sep 20, 2018

@WanLinghao WanLinghao changed the title [WIP]Create Ca-certificate secret used by token projected volume [WIP]Create Ca-certificate configmap used by token projected volume Sep 20, 2018

@WanLinghao

This comment has been minimized.

Copy link
Contributor

WanLinghao commented Sep 20, 2018

I got something pending:

  • Do we need to add labels to the configmaps created in each namespace like current serviceaccount-secret do?

  • We could just manage the configmaps via existed serviceaccount controller. Or build a independent one to do configmaps manage job. I think the independent one is better idea since it could keep the code clear and reduce the clean when we completely abandon current serviceaccount model.
    WDYT @mikedanese @liggitt

@WanLinghao

This comment has been minimized.

Copy link
Contributor

WanLinghao commented Sep 20, 2018

/retest

@WanLinghao WanLinghao force-pushed the WanLinghao:token_projection_ca_secret_create branch 2 times, most recently from 50d1a72 to 863d5f6 Sep 21, 2018

@WanLinghao

This comment has been minimized.

Copy link
Contributor

WanLinghao commented Sep 25, 2018

Sorry, not "add labels", but add something like SecretType which could distinguish if the configmap was created by controller automatically.

I got something pending:

* Do we need to add labels to the configmaps created in each namespace like current serviceaccount-secret do?
@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Sep 25, 2018

Sorry, not "add labels", but add something like SecretType which could distinguish if the configmap was created by controller automatically.

@deads2k any thoughts on the shape of this based on experience with the configmap-populating controller for the service serving cert controller?

@WanLinghao WanLinghao force-pushed the WanLinghao:token_projection_ca_secret_create branch from 00ffb4d to 1686174 Sep 25, 2018

@WanLinghao

This comment has been minimized.

Copy link
Contributor

WanLinghao commented Sep 25, 2018

/retest

1 similar comment
@WanLinghao

This comment has been minimized.

Copy link
Contributor

WanLinghao commented Sep 25, 2018

/retest

@WanLinghao WanLinghao changed the title [WIP]Create Ca-certificate configmap used by token projected volume Create Ca-certificate configmap used by token projected volume Sep 25, 2018

@WanLinghao

This comment has been minimized.

Copy link
Contributor

WanLinghao commented Nov 6, 2018

@mikedanese all comments addressed , to make it easier to review, I didn't squash those two commits. I will squash them later if you feels good about this patch

Name: RootCACertCofigMapName,
},
Data: map[string]string{
"ca.crt": string(rootCA),

This comment has been minimized.

@awly

awly Nov 6, 2018

Contributor

It bothers me that this is immutable.
It would be awesome to support cluster CA rotation at some point, maybe this can be a callback?

This comment has been minimized.

@WanLinghao

WanLinghao Nov 7, 2018

Contributor

We could just modify it when we want to support CA rotation in the future. As first version, I think it's better to keep the code simple and easy to understand. WDYT

This comment has been minimized.

@mikedanese

mikedanese Nov 7, 2018

Member

Rotation with a controller-manager restart is supported with this ¯_(ツ)_/¯

}

// RootCACertPublisher manages certificate ConfigMap objects inside Namespaces
type RootCACertPublisher struct {

This comment has been minimized.

This comment has been minimized.

@WanLinghao

WanLinghao Nov 7, 2018

Contributor

Done

@WanLinghao

This comment has been minimized.

Copy link
Contributor

WanLinghao commented Nov 7, 2018

/test pull-kubernetes-integration

sac, err := rootcacertpublisher.NewPublisher(
ctx.InformerFactory.Core().V1().ConfigMaps(),
ctx.InformerFactory.Core().V1().Namespaces(),
ctx.ClientBuilder.ClientOrDie("root-ca-crt-publisher"),

This comment has been minimized.

@mikedanese

mikedanese Nov 7, 2018

Member

root-ca-cert-publisher, missing an e

@mikedanese

This comment has been minimized.

Copy link
Member

mikedanese commented Nov 7, 2018

The modify in test file is not to test the controller we added, but to prevent error happens. Since the test I modify all assumes that no configmap exists in a newly-create namespace. adding ca.crt configmap controller breaks this obviously.

Thanks for clarifying! SGTM.

creationTimestamp: null
labels:
kubernetes.io/bootstrapping: rbac-defaults
name: system:controller:root-ca-crt-publisher

This comment has been minimized.

@mikedanese

mikedanese Nov 7, 2018

Member

@kubernetes/sig-auth-pr-reviews FYI on new controller permissions.

@mikedanese

This comment has been minimized.

Copy link
Member

mikedanese commented Nov 8, 2018

/lgtm
/approve

for commit cleanup
/hold

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 8, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikedanese, WanLinghao

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

The pull request process is described 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

To inject ca.crt into container when projected volume was specified, …
…configmap should be created in each namespace.

This patch add a controller called "root-ca-cert-publisher" to complete above job as well as some bootstrap rbac policies.

@WanLinghao WanLinghao force-pushed the WanLinghao:token_projection_ca_secret_create branch from fd57c98 to efac533 Nov 8, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Nov 8, 2018

@mikedanese

This comment has been minimized.

Copy link
Member

mikedanese commented Nov 8, 2018

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm and removed do-not-merge/hold labels Nov 8, 2018

@WanLinghao

This comment has been minimized.

Copy link
Contributor

WanLinghao commented Nov 8, 2018

/test pull-kubernetes-e2e-kops-aws

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 8, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

1 similar comment
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 8, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@mikedanese

This comment has been minimized.

Copy link
Member

mikedanese commented Nov 8, 2018

/retest

@k8s-ci-robot k8s-ci-robot merged commit 3f5db92 into kubernetes:master Nov 8, 2018

18 checks passed

cla/linuxfoundation WanLinghao authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
)

// RootCACertCofigMapName is name of the configmap which stores certificates to access api-server
const RootCACertCofigMapName = "kube-root-ca.crt"

This comment has been minimized.

@tedyu

tedyu Nov 8, 2018

typo: RootCACertCofigMapName -> RootCACertConfigMapName

This comment has been minimized.

@WanLinghao

WanLinghao Nov 9, 2018

Contributor

thank you for catching this!


switch _, err := c.cmLister.ConfigMaps(ns.Name).Get(c.configMap.Name); {
case err == nil:
return nil

This comment has been minimized.

@tedyu

tedyu Nov 8, 2018

Why would we return for this case ?

This comment has been minimized.

@WanLinghao

WanLinghao Nov 9, 2018

Contributor

Since if no error happens here, we already got a configmap, so no need to create one, just return

This comment has been minimized.

@tedyu

tedyu Nov 9, 2018

For the error case, a few lines below, we also return.
What's the purpose of the code following the switch ?

This comment has been minimized.

@WanLinghao

WanLinghao Nov 9, 2018

Contributor

Here we need to assure each namespace got one configmap named "kube-root-ca.crt". The switch here means:

  • No error indicates we already got "kube-root-ca.crt" configmap,nothing to do, return

  • Not Found Error indicates we should go ahead and create one

  • Other error, it means the GET operation itself got something wrong, it is not expected and we should return it

WanLinghao added a commit to WanLinghao/kubernetes that referenced this pull request Nov 9, 2018

WanLinghao added a commit to WanLinghao/kubernetes that referenced this pull request Nov 12, 2018

mikedanese added a commit to WanLinghao/kubernetes that referenced this pull request Nov 13, 2018

mikedanese added a commit to WanLinghao/kubernetes that referenced this pull request Nov 13, 2018

mikedanese added a commit to WanLinghao/kubernetes that referenced this pull request Nov 13, 2018

wongma7 added a commit to wongma7/kubernetes that referenced this pull request Nov 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment