Skip to content

End-to-end tests: use Vault 1.12.1 instead of the outdated Vault 1.2.3#5604

Merged
jetstack-bot merged 1 commit intocert-manager:masterfrom
maelvls:upgrade-vault-in-e2e
Dec 13, 2022
Merged

End-to-end tests: use Vault 1.12.1 instead of the outdated Vault 1.2.3#5604
jetstack-bot merged 1 commit intocert-manager:masterfrom
maelvls:upgrade-vault-in-e2e

Conversation

@maelvls
Copy link
Copy Markdown
Member

@maelvls maelvls commented Nov 25, 2022

The main reason for bumping Vault's version is because 1.2.3 is not compatible with the config parameter disable_iss_validation, which is needed for accommodating the future tests coming in #5502 that rely on bound tokens and static tokens.

For context, Vault 1.2.3 was released on Sep 9, 2019 (1) but disable_iss_validation was only added on July 21st, 2020 in Vault 1.5.0.

Due to a breaking change that happened in Vault 1.5.0 (2) in which Vault started loading the pod's token instead of using the token to be reviewed for authenticating, I had to tweak the end-to-end tests. I renamed serviceAccountName and namespace to boundSA and boundNS, since these two variables now refer to the service account of the token to be reviewed, but not the service account to be used for authenticating. The service account used for authenticating is now the pod's service account.

Alternatively, I could have prevented the service account from being mounted to the pod, but I figured that having the two service accounts separated is what most people do (although both ways are fine by me).

In order to upgrade to Vault 1.12.1, I had to change a bit how the end-to-end tests are

/kind cleanup

NONE

@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/testing Issues relating to testing approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 25, 2022
@maelvls maelvls force-pushed the upgrade-vault-in-e2e branch from 626202d to 05fdf51 Compare November 25, 2022 16:25
@SgtCoDFish
Copy link
Copy Markdown
Member

I'll take a look at this next week - not really got enough time left today to do it justice. Thanks for raising 👍

@maelvls maelvls marked this pull request as draft November 25, 2022 19:00
@jetstack-bot jetstack-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 25, 2022
@SgtCoDFish
Copy link
Copy Markdown
Member

The test failure seems legitimate to me, so I'll hold off on looking for now!

@maelvls maelvls force-pushed the upgrade-vault-in-e2e branch from 05fdf51 to 74ae2a5 Compare November 28, 2022 15:16
@jetstack-bot jetstack-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 28, 2022
@maelvls maelvls marked this pull request as ready for review November 29, 2022 10:03
@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 Nov 29, 2022
@maelvls maelvls force-pushed the upgrade-vault-in-e2e branch 3 times, most recently from 5464430 to c3718c6 Compare December 2, 2022 15:33
The main reason for bumping Vault's version is because 1.2.3 is not
compatible with the config parameter `disable_iss_validation`, which is
needed for accommodating the future tests [1] that rely on bound tokens
and static tokens.

For context, Vault 1.2.3 was released on Sep 9, 2019 [2] but
`disable_iss_validation` was only added on July 21st, 2020 in Vault
1.5.0.

Due to a breaking change that happened in Vault 1.5.0 [3] in which Vault
started loading the pod's token instead of using the same token (to be
reviewed) for authenticating. An alternative solution could have been to
prevent the service account from being mounted to the pod, but I figured
that having the two service accounts separated is a better practice.

[1]: cert-manager#5502
[2]: hashicorp/vault@c14bd9a2
[3]: https://github.com/hashicorp/vault/blob/main/CHANGELOG.md#150

Signed-off-by: Maël Valais <mael@vls.dev>
@maelvls maelvls force-pushed the upgrade-vault-in-e2e branch from c3718c6 to f4f72c1 Compare December 2, 2022 15:36
Copy link
Copy Markdown
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Tests pass so this seems good to merge as far as I'm concerned! Thanks 😁

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 13, 2022
@jetstack-bot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maelvls, SgtCoDFish

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

@SgtCoDFish
Copy link
Copy Markdown
Member

/test pull-cert-manager-master-make-test

@jetstack-bot jetstack-bot merged commit a1391d6 into cert-manager:master Dec 13, 2022
@jetstack-bot jetstack-bot added this to the v1.10 milestone Dec 13, 2022
@SgtCoDFish SgtCoDFish modified the milestones: v1.10, v1.11 Jan 6, 2023
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/testing Issues relating to testing 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. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants