Skip to content

Conversation

@wallrj
Copy link
Member

@wallrj wallrj commented Apr 12, 2023

This is in preparation for upgrading to cert-manager 1.12 where we introduce the extra go.mod files.
I want to see if those separate modules results in fewer transitive dependencies in this sample-external-issuer...

  • Upgraded the cert-manager Go dependency to 1.11
  • Use cert-manager 1.11 in E2E tests
  • Use k8s.io/utils module

Ran E2E tests locally:

$ make docker-build kind-load deploy e2e
...
Successfully tagged ghcr.io/cert-manager/sample-external-issuer/controller:v0.2.0-alpha.0-18-gab493c2
/home/richard/projects/cert-manager/sample-external-issuer/bin/kind-0.12.0 load docker-image --name sample-external-issuer-e2e ghcr.io/cert-manager/sample-external-issuer/controller:v0.2.0-alpha.0-18-gab493c2
Image: "ghcr.io/cert-manager/sample-external-issuer/controller:v0.2.0-alpha.0-18-gab493c2" with ID "sha256:c8c1cba57ffceb3c293014315611790007ca2a24fa83d494d3639ab679e2eeeb" not yet present on node "sample-external-issuer-e2e-control-plane", loading...
...
deployment.apps/sample-external-issuer-controller-manager configured
...
issuer.sample-issuer.example.com/issuer-sample condition met
certificaterequest.cert-manager.io/issuer-sample condition met
certificate.cert-manager.io/certificate-by-issuer condition met
clusterissuer.sample-issuer.example.com/clusterissuer-sample condition met
certificaterequest.cert-manager.io/clusterissuer-sample condition met
certificate.cert-manager.io/certificate-by-clusterissuer condition met
...
secret "issuer-sample-credentials" deleted

@jetstack-bot jetstack-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 12, 2023
@wallrj wallrj mentioned this pull request Apr 12, 2023
@wallrj wallrj force-pushed the cert-manager-1.11 branch from 863ad22 to ab493c2 Compare April 12, 2023 13:02
@wallrj wallrj changed the title WIP: Upgrade to cert-manager 1.11 Upgrade to cert-manager 1.11 Apr 12, 2023
@jetstack-bot jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 12, 2023
@wallrj wallrj requested a review from irbekrm April 12, 2023 13:02
# Build and install sample-external-issuer and run the E2E tests.
# This step can be run iteratively when ever you make changes to the code or to the installation manifests.
make docker-build kind-load deploy e2e
```
Copy link
Member Author

Choose a reason for hiding this comment

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

I've added this documentation for now.
I know I should add GitHub actions workflow for this, but that will be in another PR.

k8s.io/apimachinery v0.23.4
k8s.io/client-go v0.23.4
sigs.k8s.io/controller-runtime v0.11.1
github.com/cert-manager/cert-manager v1.11.1
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only line I changed manually.
The rest was changed by go mod tidy.

"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/clock"
utilerrors "k8s.io/apimachinery/pkg/util/errors"
"k8s.io/utils/clock"
Copy link
Member Author

Choose a reason for hiding this comment

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

This module got spun out some time ago.
Upgrading cert-manager brings a newer version of apimachinery which no longer has the clock package.


```bash
# Create a Kind cluster along with cert-manager.
make kind-cluster deploy-cert-manager
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work? I think the cert-manager created via make deploy-cert-manager also requires gateway-api to be installed

Copy link
Member Author

Choose a reason for hiding this comment

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

It does work...perhaps because the tests still install K8S 1.23?

$ cmctl version --short
Client Version: v1.11.0
Server Version: v1.11.1

$ kubectl version --short
Flag --short has been deprecated, and will be removed in the future. The --short output will become the default.
Client Version: v1.25.3
Kustomize Version: v4.5.7
Server Version: v1.23.4
WARNING: version difference between client (1.25) and server (1.23) exceeds the supported minor version skew of +/-1

$ make e2e
kubectl apply --filename config/samples
kubectl wait --for=condition=Ready --timeout=5s issuers.sample-issuer.example.com issuer-sample
kubectl wait --for=condition=Ready --timeout=5s  certificaterequests.cert-manager.io issuer-sample
kubectl wait --for=condition=Ready --timeout=5s  certificates.cert-manager.io certificate-by-issuer
kubectl wait --for=condition=Ready --timeout=5s clusterissuers.sample-issuer.example.com clusterissuer-sample
kubectl wait --for=condition=Ready --timeout=5s  certificaterequests.cert-manager.io clusterissuer-sample
kubectl wait --for=condition=Ready --timeout=5s  certificates.cert-manager.io certificate-by-clusterissuer
kubectl delete --filename config/samples
certificate.cert-manager.io/certificate-by-clusterissuer created
certificate.cert-manager.io/certificate-by-issuer created
certificaterequest.cert-manager.io/clusterissuer-sample created
certificaterequest.cert-manager.io/issuer-sample created
clusterissuer.sample-issuer.example.com/clusterissuer-sample created
issuer.sample-issuer.example.com/issuer-sample created
secret/clusterissuer-sample-credentials created
secret/issuer-sample-credentials created
issuer.sample-issuer.example.com/issuer-sample condition met
certificaterequest.cert-manager.io/issuer-sample condition met
certificate.cert-manager.io/certificate-by-issuer condition met
clusterissuer.sample-issuer.example.com/clusterissuer-sample condition met
certificaterequest.cert-manager.io/clusterissuer-sample condition met
certificate.cert-manager.io/certificate-by-clusterissuer condition met
certificate.cert-manager.io "certificate-by-clusterissuer" deleted
certificate.cert-manager.io "certificate-by-issuer" deleted
certificaterequest.cert-manager.io "clusterissuer-sample" deleted
certificaterequest.cert-manager.io "issuer-sample" deleted
clusterissuer.sample-issuer.example.com "clusterissuer-sample" deleted
issuer.sample-issuer.example.com "issuer-sample" deleted
secret "clusterissuer-sample-credentials" deleted
secret "issuer-sample-credentials" deleted

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh nvm, I thought these were the cert-manager/cert-manager make targets

Copy link
Member Author

Choose a reason for hiding this comment

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

$ kubectl logs -n cert-manager deploy/cert-manager | fgrep -i gateway
I0412 12:03:04.944251       1 controller.go:182] cert-manager/controller "msg"="not starting controller as it's disabled" "controller"="gateway-shim"

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I didn't realize this repo had make targets to deploy cert-manager 👍🏼

Copy link
Contributor

@irbekrm irbekrm left a comment

Choose a reason for hiding this comment

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

Thank you for leaving the code comments 👍🏼

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 12, 2023
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irbekrm, wallrj

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

The pull request process is described here

Details 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

@jetstack-bot jetstack-bot merged commit e51937a into cert-manager:main Apr 12, 2023
@wallrj wallrj deleted the cert-manager-1.11 branch April 12, 2023 13:26
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. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants