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

Move cert-manager to apps #2374

Merged
merged 1 commit into from
Jul 21, 2021

Conversation

jimdaga
Copy link

@jimdaga jimdaga commented Jul 21, 2021

Resolves:

Notes:

  • It's my understanding that cert-manager may be sunset soon, so keeping the changes limited in this PR.

Changes:

  • Move cert-manager directory under the apps/cert-manager path.
  • Add default deploy.sh script.
  • Update various README notes:
    • Link to the current version of docs for the running version of cert-manager (v0.13)
    • Add note about having to setup clusterrolebinding
    • Update with the deploy.sh info

Question

  • Are we OK with leaving the clusterrolebinding step outside of deploy.sh?

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 21, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @jimdaga. 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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. area/apps/cert-manager cert-manager, code in apps/cert-manager/ labels Jul 21, 2021
@k8s-ci-robot k8s-ci-robot requested review from munnerz and thockin July 21, 2021 03:15
@k8s-ci-robot k8s-ci-robot added wg/k8s-infra size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 21, 2021
@jimdaga jimdaga marked this pull request as ready for review July 21, 2021 03:17
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 21, 2021
@spiffxp
Copy link
Member

spiffxp commented Jul 21, 2021

/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 21, 2021
@ameukam
Copy link
Member

ameukam commented Jul 21, 2021

/lgtm
/approve
/hold

Leave the /hold cancel to @spiffxp. :-)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 21, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 21, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ameukam, jimdaga

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2021
@spiffxp
Copy link
Member

spiffxp commented Jul 21, 2021

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 21, 2021
@k8s-ci-robot k8s-ci-robot merged commit 9555daf into kubernetes:main Jul 21, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Jul 21, 2021
@jimdaga jimdaga deleted the issue-2150/move-certmanager branch July 21, 2021 16:42
@spiffxp
Copy link
Member

spiffxp commented Jul 21, 2021

Hm

$ ./deploy.sh
Warning: apiextensions.k8s.io/v1beta1 CustomResourceDefinition is deprecated in v1.16+, unavailable in v1.22+; use apiextensions.k8s.io/v1 CustomResourceDefinition
customresourcedefinition.apiextensions.k8s.io/certificaterequests.cert-manager.io configured
customresourcedefinition.apiextensions.k8s.io/certificates.cert-manager.io configured
customresourcedefinition.apiextensions.k8s.io/challenges.acme.cert-manager.io configured
customresourcedefinition.apiextensions.k8s.io/clusterissuers.cert-manager.io configured
customresourcedefinition.apiextensions.k8s.io/issuers.cert-manager.io configured
customresourcedefinition.apiextensions.k8s.io/orders.acme.cert-manager.io configured
namespace/cert-manager unchanged
serviceaccount/cert-manager-cainjector unchanged
serviceaccount/cert-manager unchanged
serviceaccount/cert-manager-webhook unchanged
Warning: rbac.authorization.k8s.io/v1beta1 ClusterRole is deprecated in v1.17+, unavailable in v1.22+; use rbac.authorization.k8s.io/v1 ClusterRole
clusterrole.rbac.authorization.k8s.io/cert-manager-cainjector unchanged
Warning: rbac.authorization.k8s.io/v1beta1 ClusterRoleBinding is deprecated in v1.17+, unavailable in v1.22+; use rbac.authorization.k8s.io/v1 ClusterRoleBinding
clusterrolebinding.rbac.authorization.k8s.io/cert-manager-cainjector unchanged
clusterrolebinding.rbac.authorization.k8s.io/cert-manager-webhook:auth-delegator configured
clusterrole.rbac.authorization.k8s.io/cert-manager-webhook:webhook-requester unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-controller-issuers unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-controller-clusterissuers unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-controller-certificates unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-controller-orders unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-controller-challenges unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-controller-ingress-shim unchanged
clusterrolebinding.rbac.authorization.k8s.io/cert-manager-controller-issuers unchanged
clusterrolebinding.rbac.authorization.k8s.io/cert-manager-controller-clusterissuers unchanged
clusterrolebinding.rbac.authorization.k8s.io/cert-manager-controller-certificates unchanged
clusterrolebinding.rbac.authorization.k8s.io/cert-manager-controller-orders unchanged
clusterrolebinding.rbac.authorization.k8s.io/cert-manager-controller-challenges unchanged
clusterrolebinding.rbac.authorization.k8s.io/cert-manager-controller-ingress-shim unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-view unchanged
clusterrole.rbac.authorization.k8s.io/cert-manager-edit unchanged
service/cert-manager unchanged
service/cert-manager-webhook unchanged
deployment.apps/cert-manager-cainjector configured
deployment.apps/cert-manager unchanged
deployment.apps/cert-manager-webhook configured
Warning: admissionregistration.k8s.io/v1beta1 MutatingWebhookConfiguration is deprecated in v1.16+, unavailable in v1.22+; use admissionregistration.k8s.io/v1 MutatingWebhookConfiguration
mutatingwebhookconfiguration.admissionregistration.k8s.io/cert-manager-webhook configured
Warning: admissionregistration.k8s.io/v1beta1 ValidatingWebhookConfiguration is deprecated in v1.16+, unavailable in v1.22+; use admissionregistration.k8s.io/v1 ValidatingWebhookConfiguration
validatingwebhookconfiguration.admissionregistration.k8s.io/cert-manager-webhook configured
clusterissuer.cert-manager.io/letsencrypt-prod configured
clusterissuer.cert-manager.io/letsencrypt-staging configured
clusterissuer.cert-manager.io/selfsigning-issuer unchanged
the namespace from the provided object "kube-system" does not match the namespace "cert-manager". You must pass '--namespace=kube-system' to perform this operation.
the namespace from the provided object "kube-system" does not match the namespace "cert-manager". You must pass '--namespace=kube-system' to perform this operation.
the namespace from the provided object "kube-system" does not match the namespace "cert-manager". You must pass '--namespace=kube-system' to perform this operation.
the namespace from the provided object "kube-system" does not match the namespace "cert-manager". You must pass '--namespace=kube-system' to perform this operation.
the namespace from the provided object "kube-system" does not match the namespace "cert-manager". You must pass '--namespace=kube-system' to perform this operation.

Diffing output of kubectl get clusterissuer -A from before and after

10c10
<     generation: 2
---
>     generation: 3
12c12
<     resourceVersion: "254365400"
---
>     resourceVersion: "436255683"
31c31
<             name: k8s-io-v6
---
>             name: k8s-io
53c53
<     generation: 2
---
>     generation: 3
55c55
<     resourceVersion: "114571644"
---
>     resourceVersion: "436255688"
79d78
<           - smyk.la

@spiffxp
Copy link
Member

spiffxp commented Jul 21, 2021

Two things jump out to me:

cert-manager.yaml has resources specified in multiple namespaces

  • I don't want to munge cert-manager.yaml too much or try to figure out if it can be redone to not use kube-system
  • We could make deploy.sh not use a namespace flag here and just trust the resources to do the right thing
  • We could split the file into cluster-scoped and namespace-scoped (similar to what I've done for a non-/apps thing in prow: dir-per-namespace resources deployed to cluster #2371)

the change k8s-io-v6 to k8s-io

  • well, since that's the way it used to be in prod, I have manually edited it back
$ k edit clusterissuer letsencrypt-prod
clusterissuer.cert-manager.io/letsencrypt-prod edited
$ k get ingress -A | grep k8s-io
k8s-io-canary         k8s-io                 <none>   *       34.102.239.89        80, 443   518d
k8s-io-canary         k8s-io-v6              <none>   *       2600:1901:0:552b::   80, 443   264d
k8s-io-prod           k8s-io                 <none>   *       34.107.204.206       80        518d
k8s-io-prod           k8s-io-v6              <none>   *       2600:1901:0:26f3::   80        264d

$ kubectl describe ingress -n k8s-io-prod k8s-io
Name:             k8s-io
Namespace:        k8s-io-prod
Address:          34.107.204.206
Default backend:  k8s-io:http (10.40.6.12:80,10.40.8.10:80)
Rules:
  Host        Path  Backends
  ----        ----  --------
  *           *     k8s-io:http (10.40.6.12:80,10.40.8.10:80)
Annotations:  ingress.gcp.kubernetes.io/pre-shared-cert: mcrt-2ddd9946-3dce-45be-9642-0de890720aed
              ingress.kubernetes.io/backends: {"k8s1-ea949c44-k8s-io-prod-k8s-io-80-6bdf511a":"HEALTHY"}
              ingress.kubernetes.io/forwarding-rule: k8s-fw-k8s-io-prod-k8s-io--ea949c440a044527
              ingress.kubernetes.io/https-forwarding-rule: k8s-fws-k8s-io-prod-k8s-io--ea949c440a044527
              ingress.kubernetes.io/https-target-proxy: k8s-tps-k8s-io-prod-k8s-io--ea949c440a044527
              ingress.kubernetes.io/ssl-cert: mcrt-2ddd9946-3dce-45be-9642-0de890720aed
              ingress.kubernetes.io/target-proxy: k8s-tp-k8s-io-prod-k8s-io--ea949c440a044527
              ingress.kubernetes.io/url-map: k8s-um-k8s-io-prod-k8s-io--ea949c440a044527
              kubernetes.io/ingress.class: gce
              kubernetes.io/ingress.global-static-ip-name: k8s-io-ingress-prod
              networking.gke.io/managed-certificates: k8s-io-prod
Events:
  Type    Reason  Age                    From                     Message
  ----    ------  ----                   ----                     -------
  Normal  Sync    63s (x989 over 6d20h)  loadbalancer-controller  Scheduled for sync

$ kubectl describe ingress -n k8s-io-prod k8s-io-v6
Name:             k8s-io-v6
Namespace:        k8s-io-prod
Address:          2600:1901:0:26f3::
Default backend:  k8s-io:http (10.40.6.12:80,10.40.8.10:80)
Rules:
  Host        Path  Backends
  ----        ----  --------
  *           *     k8s-io:http (10.40.6.12:80,10.40.8.10:80)
Annotations:  ingress.gcp.kubernetes.io/pre-shared-cert: mcrt-2ddd9946-3dce-45be-9642-0de890720aed
              ingress.kubernetes.io/backends: {"k8s1-ea949c44-k8s-io-prod-k8s-io-80-6bdf511a":"HEALTHY"}
              ingress.kubernetes.io/forwarding-rule: k8s-fw-k8s-io-prod-k8s-io-v6--ea949c440a044527
              ingress.kubernetes.io/https-forwarding-rule: k8s-fws-k8s-io-prod-k8s-io-v6--ea949c440a044527
              ingress.kubernetes.io/https-target-proxy: k8s-tps-k8s-io-prod-k8s-io-v6--ea949c440a044527
              ingress.kubernetes.io/ssl-cert: mcrt-2ddd9946-3dce-45be-9642-0de890720aed
              ingress.kubernetes.io/target-proxy: k8s-tp-k8s-io-prod-k8s-io-v6--ea949c440a044527
              ingress.kubernetes.io/url-map: k8s-um-k8s-io-prod-k8s-io-v6--ea949c440a044527
              kubernetes.io/ingress.class: gce
              kubernetes.io/ingress.global-static-ip-name: k8s-io-ingress-prod-v6
              networking.gke.io/managed-certificates: k8s-io-prod
Events:
  Type    Reason  Age                    From                     Message
  ----    ------  ----                   ----                     -------
  Normal  Sync    73s (x989 over 6d20h)  loadbalancer-controller  Scheduled for sync
  • well, I don't think that messed with anything, and so I'm back to "are we even using this anymore"... but happy enough to call this done (I will PR in the manual edit I did)

@ameukam
Copy link
Member

ameukam commented Jul 21, 2021

@spiffxp the certificate remaining managed by cert-manager is k8s-io-canary. It's a self-signed certificate. Plan is to replace it with 10-year certificate manually created : #2173

@jimdaga
Copy link
Author

jimdaga commented Jul 21, 2021

@spiffxp It looks like cert-manager.yaml is just a clone of https://github.com/jetstack/cert-manager/releases/download/v0.13.1/cert-manager.yaml with one change.

diff ~/Downloads/cert-manager.yaml ~/git/k8s.io/cert-manager/cert-manager.yaml
6277c6277
<
---
>
6324a6325
>           - --max-concurrent-challenges=20
6337c6338
<
---
>
6397c6398
<
---
>
6468c6469
<
---
>

Per the cert-manager offical docs, we don't need namespace:
https://cert-manager.io/v0.13-docs/installation/kubernetes/#installing-with-regular-manifests

$ kubectl apply --validate=false -f https://github.com/jetstack/cert-manager/releases/download/v0.13.1/cert-manager.yaml

So I think removing the namespace logic from deploy.sh will be fine.

@jimdaga
Copy link
Author

jimdaga commented Jul 21, 2021

PR with that change if you agree with removing namespace: #2379

@spiffxp
Copy link
Member

spiffxp commented Jul 21, 2021

You beat me to the deploy.sh change, I'll rebase #2380 on top of that

@spiffxp
Copy link
Member

spiffxp commented Jul 21, 2021

Are we OK with leaving the clusterrolebinding step outside of deploy.sh?

I will look at this next, but am not sure how deeply it's worth answering this if we're going to get rid of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apps/cert-manager cert-manager, code in apps/cert-manager/ cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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.

4 participants