Vault k8s auth by JoshVanL · Pull Request #2040 · cert-manager/cert-manager

JoshVanL · 2019-09-06T14:39:22Z

fixes #647
Taking over from #649

Adds Kubernetes authentication type for Vault Issue

jetstack-bot · 2019-09-06T14:39:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoshVanL

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:

~~OWNERS~~ [JoshVanL]

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

JoshVanL · 2019-09-06T14:40:17Z

@kunickiaj

JoshVanL · 2019-09-06T14:42:04Z

/hold

JoshVanL · 2019-09-09T08:10:26Z

/hold cancel
/unassign
/assign @munnerz

JoshVanL · 2019-09-09T08:20:47Z

🙃
/retest

pkg/apis/certmanager/v1alpha1/types_issuer.go

munnerz · 2019-09-09T08:41:36Z

pkg/issuer/vault/setup.go

-		v.issuer.GetSpec().Vault.Auth.AppRole.SecretRef.Name == "" {
+		v.issuer.GetSpec().Vault.Auth.AppRole.SecretRef.Name == "" &&
+		v.issuer.GetSpec().Vault.Auth.Kubernetes.Role == "" &&
+		v.issuer.GetSpec().Vault.Auth.Kubernetes.SecretRef.Name == "" {


Instead of checking particular fields of these different auth types, we should make each auth type structure a pointer and then just check if they are nil here. This is super messy right now and could really confuse users IMO.

munnerz · 2019-09-09T08:42:34Z

pkg/issuer/vault/setup.go

+		v.issuer.GetSpec().Vault.Auth.Kubernetes.SecretRef.Name != "" &&
+		(v.issuer.GetSpec().Vault.Auth.AppRole.RoleId != "" ||
+			v.issuer.GetSpec().Vault.Auth.AppRole.SecretRef.Name != "" ||
+			v.issuer.GetSpec().Vault.Auth.TokenSecretRef.Name != "") {


I'm not even going to try and parse this combination of &&, (, ) and || to understand this 🙈

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

munnerz · 2019-09-09T08:46:46Z

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

+
+// CleanKubernetesRole cleans up the ClusterRoleBinding and ServiceAccount for Kubernetes auth delegation
+func (v *VaultInitializer) CleanKubernetesRole(client kubernetes.Interface, namespace, roleName, serviceAccountName string) error {
+	client.RbacV1().ClusterRoleBindings().Delete(fmt.Sprintf("%s:%s:auth-delegator", namespace, serviceAccountName), nil)


nit (not essential given this is test code) - is there any way to not have to re-template this? It's a bit annoying and given we don't check errors from this method, if we change something in future it's unlikely we'll notice this line breaking.

JoshVanL · 2019-09-20T14:47:53Z

/retest

munnerz · 2019-09-23T11:12:56Z

pkg/internal/apis/certmanager/types_issuer.go

+	// `/v1/auth/foo/login`. If unspecified, the default value "kubernetes" will
+	// be used.
+	// +optional
+	Path string `json:"mountPath"`


Add ,omitempty to optional fields 😄

Signed-off-by: Adam Kunicki <adam@streamsets.com>

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

munnerz · 2019-09-23T17:04:08Z

pkg/apis/acme/v1alpha2/types_issuer.go

 	apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-
-	cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1"


nit: unnecessary/incorrect change

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

JoshVanL · 2019-09-24T09:53:59Z

/retest

munnerz · 2019-09-25T08:51:27Z

/lgtm

retest-bot · 2019-09-25T09:53:13Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels Sep 6, 2019

jetstack-bot requested review from munnerz and vdesjardins September 6, 2019 14:39

JoshVanL mentioned this pull request Sep 6, 2019

Adds Kubernetes authentication type for Vault Issuer #649

Closed

jetstack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 6, 2019

JoshVanL force-pushed the vault-k8s-auth branch from 26aa55e to 02f85b5 Compare September 6, 2019 16:39

munnerz added this to the v0.11 milestone Sep 7, 2019

jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 9, 2019

jetstack-bot assigned munnerz Sep 9, 2019

munnerz reviewed Sep 9, 2019

View reviewed changes

jetstack-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 9, 2019

JoshVanL force-pushed the vault-k8s-auth branch from 4d29860 to 723b675 Compare September 9, 2019 12:26

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 9, 2019

JoshVanL force-pushed the vault-k8s-auth branch from 723b675 to ae1af2f Compare September 9, 2019 12:38

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

JoshVanL force-pushed the vault-k8s-auth branch from ae1af2f to 82ea2f4 Compare September 9, 2019 12:46

JoshVanL force-pushed the vault-k8s-auth branch 3 times, most recently from 4e08719 to c4b55c9 Compare September 20, 2019 13:12

JoshVanL force-pushed the vault-k8s-auth branch from c4b55c9 to 0324959 Compare September 20, 2019 15:16

munnerz reviewed Sep 23, 2019

View reviewed changes

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2019

Adam Kunicki and others added 11 commits September 23, 2019 13:33

Added Kubernetes authentication type for Vault Issuer

7058dfc

Signed-off-by: Adam Kunicki <adam@streamsets.com>

Adds vault kubernetes auth tests

9aea714

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Adds vault issuer Kubernetes auth docs

a98561b

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Adds internal vault issuer Kubernetes auth types

452e42b

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Updates vault certificaterequest controller test

05b5f44

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Addresses comments and makes vault auth pointers

f2f5fcc

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Adds all vault Auth methods to secret checks and stops panic

9b827ee

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Set role name correctly for vault addon with Kube Auth

f6e1964

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Increase scope of vault addon tokenreview permissions to cluster

5f25ffa

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Adds review comments

9ffaae5

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

Update generated

68f9546

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

JoshVanL force-pushed the vault-k8s-auth branch from dcc570f to 5ae6be9 Compare September 23, 2019 13:51

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2019

munnerz reviewed Sep 23, 2019

View reviewed changes

Updates apis to v1alpha2/meta

e1875bc

Signed-off-by: JoshVanL <vleeuwenjoshua@gmail.com>

JoshVanL force-pushed the vault-k8s-auth branch from 5ae6be9 to e1875bc Compare September 23, 2019 17:21

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 25, 2019

jetstack-bot merged commit 816bbf5 into cert-manager:master Sep 25, 2019

jetstack-bot mentioned this pull request Oct 21, 2019

Update apiVersion for issuer example in upgrade 0.7 to 0.8 docs #2263

Closed

mbrancato mentioned this pull request Nov 10, 2019

Service Account authentication to Vault #2345

Closed

Conversation

JoshVanL commented Sep 6, 2019

Uh oh!

jetstack-bot commented Sep 6, 2019

Uh oh!

JoshVanL commented Sep 6, 2019

Uh oh!

JoshVanL commented Sep 6, 2019

Uh oh!

JoshVanL commented Sep 9, 2019

Uh oh!

JoshVanL commented Sep 9, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

munnerz Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

munnerz Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

munnerz Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

JoshVanL commented Sep 20, 2019

Uh oh!

munnerz Sep 23, 2019

Choose a reason for hiding this comment

Uh oh!

munnerz Sep 23, 2019

Choose a reason for hiding this comment

Uh oh!

JoshVanL commented Sep 24, 2019

Uh oh!

munnerz commented Sep 25, 2019

Uh oh!

retest-bot commented Sep 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants