Stop using deprecated hashicorp/vault/api RawRequest functions #6601

wallrj · 2024-01-04T17:15:26Z

Fixed the following staticcheck warnings:

SA1019: v.client.RawRequest is deprecated: RawRequest exists for historical compatibility and should not be used directly. Use client.Logical().ReadRaw(...) or higher level methods instead.

The RawRequest function was deprecated in:

So in this PR I've unhidden the deprecation warning and updated the E2E test code to use https://github.com/hashicorp/vault-client-go instead of the vault/api module.

Unhide the deprecation warning
- https://github.com/cert-manager/cert-manager/actions/runs/7412656898?pr=6601
Use the higher level https://github.com/hashicorp/vault-client-go library instead

Testing

I ran the Vault E2E tests locally as follows:

kind create cluster
cmctl x install 
make e2e-build
_bin/test/e2e.test  --repo-root=/dev/null --ginkgo.focus="Vault" --ginkgo.skip="Gateway"

Running Suite: cert-manager e2e suite - /home/richard/projects/cert-manager/cert-manager
========================================================================================
Random Seed: 1704812813

Will run 166 of 791 specs
"hashicorp" already exists with the same configuration, skipping
Release "chart-vault-vault" does not exist. Installing it now.
NAME: chart-vault-vault
LAST DEPLOYED: Tue Jan  9 15:06:54 2024
NAMESPACE: e2e-vault
STATUS: deployed
REVISION: 1
NOTES:
Thank you for installing HashiCorp Vault!

Now that you have deployed Vault, you should look over the docs on using
Vault with Kubernetes available here:

https://www.vaultproject.io/docs/


Your release is named chart-vault-vault. To learn more about the release, try:

  $ helm status chart-vault-vault
  $ helm get manifest chart-vault-vault
S•••SS•••••••••[controller-runtime] log.SetLogger(...) was never called; logs will not be displayed.
Detected at:
        >  goroutine 847 [running]:
        >  runtime/debug.Stack()
        >       runtime/debug/stack.go:24 +0x5e
        >  sigs.k8s.io/controller-runtime/pkg/log.eventuallyFulfillRoot()
        >       sigs.k8s.io/controller-runtime@v0.16.3/pkg/log/log.go:60 +0xcd
        >  sigs.k8s.io/controller-runtime/pkg/log.(*delegatingLogSink).WithName(0xc000250540, {0x1dfe938, 0x14})
        >       sigs.k8s.io/controller-runtime@v0.16.3/pkg/log/deleg.go:147 +0x45
        >  github.com/go-logr/logr.Logger.WithName({{0x20c9f50, 0xc000250540}, 0x0}, {0x1dfe938?, 0xc001e51d98?})
        >       github.com/go-logr/logr@v1.4.1/logr.go:345 +0x3d
        >  sigs.k8s.io/controller-runtime/pkg/client.newClient(0x0?, {0x0, 0xc000252850, {0x0, 0x0}, 0x0, {0x0, 0x0}, 0x0})
        >       sigs.k8s.io/controller-runtime@v0.16.3/pkg/client/client.go:122 +0xec
        >  sigs.k8s.io/controller-runtime/pkg/client.New(0x1e189b7?, {0x0, 0xc000252850, {0x0, 0x0}, 0x0, {0x0, 0x0}, 0x0})
        >       sigs.k8s.io/controller-runtime@v0.16.3/pkg/client/client.go:103 +0x7d
        >  github.com/cert-manager/cert-manager/e2e-tests/framework.(*Framework).BeforeEach(0xc000340370)
        >       github.com/cert-manager/cert-manager/e2e-tests/framework/framework.go:141 +0x34c
        >  github.com/onsi/ginkgo/v2/internal.extractBodyFunction.func3({0xa03c88, 0xc002196180})
        >       github.com/onsi/ginkgo/v2@v2.13.0/internal/node.go:463 +0x13
        >  github.com/onsi/ginkgo/v2/internal.(*Suite).runNode.func3()
        >       github.com/onsi/ginkgo/v2@v2.13.0/internal/suite.go:889 +0x8d
        >  created by github.com/onsi/ginkgo/v2/internal.(*Suite).runNode in goroutine 23
        >       github.com/onsi/ginkgo/v2@v2.13.0/internal/suite.go:876 +0xd7b
•••S••W0109 15:07:35.390326  286984 warnings.go:70] tls: failed to find any PEM data in certificate input
•S•••SS••••••••••••S••W0109 15:08:10.892343  286984 warnings.go:70] tls: failed to find any PEM data in certificate input
•SSSSSSSSSSSS•••••SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS•••••SSS••••••••••SSSSSSSSSSSSSSSSSSSSSSSSSSS
•••••SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS••••••••••SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS•••••SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSproxy logs:


Ran 76 of 791 Specs in 149.628 seconds
SUCCESS! -- 76 Passed | 0 Failed | 0 Pending | 715 Skipped
PASS

/kind cleanup

NONE

Signed-off-by: Richard Wall <richard.wall@venafi.com>

wallrj · 2024-01-09T13:38:38Z

test/e2e/framework/addon/vault/setup.go

-	secretId, err := v.callVault("POST", url, "secret_id", map[string]string{})
+	// TODO: Should use Auth.AppRoleWriteSecretId instead of raw write here,
+	// but it's currently broken. See:
+	// https://github.com/hashicorp/vault-client-go/issues/249


json: cannot unmarshal number into Go struct field AppRoleWriteSecretIdResponse.data.secret_id_ttl of type string hashicorp/vault-client-go#249

wallrj · 2024-01-09T13:46:56Z

test/e2e/framework/addon/vault/setup.go

These are the all important changes but GitHub seems to be folding them. Unfold this file when reviewing.

wallrj · 2024-01-09T13:48:48Z

test/e2e/framework/addon/vault/setup.go

 	// # read the secret-id
-	url = path.Join(baseUrl, "secret-id")
-	secretId, err := v.callVault("POST", url, "secret_id", map[string]string{})


Notice that the original comment is misleading. These lines are writing (POSTing) to the secret-id endpoint.

ah, good catch. Vault seems to use POST-as-GET, it seems that's what is going on here. Maybe the comment should say:

// # read the secret-id (POST-as-GET)

Note that the error message just below also refers to "reading":

fmt.Errorf("error reading secret_id: %s", ...)

Signed-off-by: Richard Wall <richard.wall@venafi.com>

LICENSES

Signed-off-by: Richard Wall <richard.wall@venafi.com>

wallrj · 2024-01-09T15:11:26Z

test/e2e/framework/addon/vault/setup.go

+	ctx := context.Background()
+	// TODO: Should use Secrets.PkiWriteRole here,
+	// but it is broken. See:
+	// https://github.com/hashicorp/vault-client-go/issues/195


Malfunctions around boolean parameters and defaulting hashicorp/vault-client-go#195

maelvls · 2024-01-09T16:27:52Z

hey, I went thought the changes, I didn't spot anything strange. I haven't performed ad-hoc tests, nor did I run the end-to-end tests locally, I am confident CI will have caught anything off (I hope).

It must have taken some work to get there! Great work, I appreciate the self-review and the comments you added in code.

/lgtm
/approve

jetstack-bot · 2024-01-09T16:27:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maelvls

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:

~~OWNERS~~ [maelvls]

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

wallrj force-pushed the deprecations-4 branch 2 times, most recently from da2b78c to 5d5cab7 Compare January 9, 2024 11:29

Unhide hashicorp/vault/api RawRequest deprecations

88adf38

Signed-off-by: Richard Wall <richard.wall@venafi.com>

wallrj force-pushed the deprecations-4 branch 2 times, most recently from 1c97e88 to 5074209 Compare January 9, 2024 12:17

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2024

Use vault-client-go instead

3f75290

Signed-off-by: Richard Wall <richard.wall@venafi.com>

wallrj force-pushed the deprecations-4 branch from 5074209 to 274b8d9 Compare January 9, 2024 13:36

jetstack-bot added the area/acme Indicates a PR directly modifies the ACME Issuer code label Jan 9, 2024

wallrj commented Jan 9, 2024

View reviewed changes

wallrj force-pushed the deprecations-4 branch from 274b8d9 to a617808 Compare January 9, 2024 13:44

wallrj changed the title ~~WIP: Stop using deprecated hashicorp/vault/api RawRequest functions~~ Stop using deprecated hashicorp/vault/api RawRequest functions Jan 9, 2024

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 Jan 9, 2024

wallrj commented Jan 9, 2024

View reviewed changes

wallrj requested a review from maelvls January 9, 2024 13:49

wallrj added 2 commits January 9, 2024 13:56

make update-licenses

a2b5ef4

Signed-off-by: Richard Wall <richard.wall@venafi.com>

Revert bae2682

bbb511d

Signed-off-by: Richard Wall <richard.wall@venafi.com>

wallrj commented Jan 9, 2024

View reviewed changes

LICENSES Outdated Show resolved Hide resolved

wallrj mentioned this pull request Jan 9, 2024

make update-licenses is non-deterministic. #6622

Open

wallrj force-pushed the deprecations-4 branch from a617808 to bbb511d Compare January 9, 2024 14:50

Work around bugs in vault-client-sdk

38288e5

Signed-off-by: Richard Wall <richard.wall@venafi.com>

wallrj commented Jan 9, 2024

View reviewed changes

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 9, 2024

jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 9, 2024

jetstack-bot merged commit dd5f585 into cert-manager:master Jan 9, 2024
8 checks passed

wallrj deleted the deprecations-4 branch January 9, 2024 16:34

This was referenced Jan 11, 2024

feat: Add the ability to communicate with Vault via mTLS #6614

Merged

Stop using deprecated f.environment.ControlPlane.KubeCtl function #6627

Merged

JaniAnttonen mentioned this pull request Apr 12, 2024

Fix duration response types hashicorp/vault-client-go#260

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop using deprecated hashicorp/vault/api RawRequest functions #6601

Stop using deprecated hashicorp/vault/api RawRequest functions #6601

wallrj commented Jan 4, 2024 •

edited

wallrj Jan 9, 2024

wallrj Jan 9, 2024

wallrj Jan 9, 2024

maelvls Jan 9, 2024 •

edited

wallrj Jan 9, 2024

maelvls commented Jan 9, 2024

jetstack-bot commented Jan 9, 2024

Stop using deprecated hashicorp/vault/api RawRequest functions #6601

Stop using deprecated hashicorp/vault/api RawRequest functions #6601

Conversation

wallrj commented Jan 4, 2024 • edited

Testing

wallrj Jan 9, 2024

Choose a reason for hiding this comment

wallrj Jan 9, 2024

Choose a reason for hiding this comment

wallrj Jan 9, 2024

Choose a reason for hiding this comment

maelvls Jan 9, 2024 • edited

Choose a reason for hiding this comment

wallrj Jan 9, 2024

Choose a reason for hiding this comment

maelvls commented Jan 9, 2024

jetstack-bot commented Jan 9, 2024

wallrj commented Jan 4, 2024 •

edited

maelvls Jan 9, 2024 •

edited