Add new VaultAuthGlobal type #735

benashz · 2024-05-09T20:29:25Z

The resource provide a resource holding Vault auth configuration that can be shared across VaultAuth resources. A VaultAuth instance only needs to provide the authentication method and a valid vaultAuthGlobalRef. VSO will automatically merge the VaultAuthGlobal with the referring VaultAuth. This allows for a VaultAuth instance to inherit some global authentication configuration.

The VaultAuthGlobal resource can be configured with one or more Vault auth method specific configuration.

Given the following VaultAuthGlobal:

apiVersion: secrets.hashicorp.com/v1beta1
kind: VaultAuthGlobal
metadata:
  name: default
  namespace: tenant-ns
spec:
  defaultAuthMethod: kubernetes
  kubernetes:
    audiences:
    - vault
    namespace: vault-tenant-ns
    mount: demo-auth-mount
    role: auth-role
    serviceAccount: default
    tokenExpirationSeconds: 600

The referring VaultAuth would look like:

apiVersion: secrets.hashicorp.com/v1beta1
kind: VaultAuth
metadata:
  name: default
  namespace: tenant-ns
spec:
  vaultAuthGlobalRef: default

If you wanted to override the kubernetes.role the VaultAuth would look like:

apiVersion: secrets.hashicorp.com/v1beta1
kind: VaultAuth
metadata:
  name: default
  namespace: tenant-ns
spec:
  vaultAuthGlobalRef: default
  kubernetes:
    role: other-auth-role

The referring VaultAuth's configuration always overrides its VaultAuthGlobal's configuration.

benashz · 2024-05-13T19:13:01Z

Relates to #341

The resource provide a resource holding Vault auth configuration that can be shared across VaultAuth resources. A VaultAuth instance only needs to provide the authentication method and a valid vaultAuthGlobalRef. VSO will automatically merge the VaultAuthGlobal with the referring VaultAuth. This allows for a VaultAuth instance to inherit some global authentication configuration. The VaultAuthGlobal resource can be configured with one or more Vault auth method specific configuration. Given the following VaultAuthGlobal: ``` apiVersion: secrets.hashicorp.com/v1beta1 kind: VaultAuthGlobal metadata: name: default namespace: tenant-ns spec: kubernetes: audiences: - vault namespace: demo-ns mount: demo-auth-mount role: auth-role serviceAccount: default tokenExpirationSeconds: 600 ``` The referring VaultAuth would look like: ``` apiVersion: secrets.hashicorp.com/v1beta1 kind: VaultAuth metadata: name: default namespace: tenant-ns spec: method: kubernetes vaultAuthGlobalRef: default ```

tvoran

This is looking good to me, my only real concern is that VaultConnectionRef in VaultAuthGlobal doesn't seem to be used.

tvoran · 2024-05-16T18:55:36Z

api/v1beta1/vaultauth_types.go

+	// method will be used. This can be used as a default Vault namespace for all
+	// auth methods.


Is this really a default for all auth methods? That sounds like something that would apply in VaultAuthGlobal but not VaultAuth.

tvoran · 2024-05-16T20:56:09Z

controllers/vaultauth_controller.go

+		} else {
+			o = mObj
+			condition.Message = fmt.Sprintf("%s:%s:%d",
+


Suggested change

Fixed in 6c44b68

tvoran · 2024-05-16T20:56:51Z

controllers/vaultauth_controller.go

-		FilterFunc:   filterOldCacheRefs,
-		PruneStorage: true,
-	}); err != nil {
+	// hash the VaultAut


Suggested change

// hash the VaultAut

// hash the VaultAuth

Fixed in 6c44b68

tvoran · 2024-05-16T21:06:16Z

controllers/vaultauth_controller.go

+		Watches(
+			&secretsv1beta1.VaultAuthGlobal{},
+			NewEnqueueRefRequestsHandler(VaultAuthGlobal, r.referenceCache, nil, nil),
+			// builder.WithPredicates(&predicate.GenerationChangedPredicate{}),


Still needed?

Dropped in 6c44b68

tvoran · 2024-05-16T21:34:04Z

internal/common/common.go

+	cObj.Spec.Headers = firstNonZeroLen(mapLenFunc[string, string],
+		cObj.Spec.Headers, globalAuthHeaders, gObj.Spec.DefaultHeaders)
+	cObj.Spec.Params = firstNonZeroLen(mapLenFunc[string, string],
+		cObj.Spec.Params, globalAuthParams, gObj.Spec.DefaultParams)


For headers and params, another strategy would be to combine the global and cObj items, where cObj.Spec.Headers could add to and override globalAuthHeaders items, for example. Have we considered that style of merge?

It was definitely a consideration to take the union of all params/headers, but I hesitant, since there would be no way for a referring VaultAuth to omit specific keys without extending the functionality of the vaultAuthGlobalRef. One thing that we could do is to allow for configurable merge strategies to be set in the VaultAuthSpec. I can put up a spike PR for that.

@tvoran put up a spike PR here: #757

tvoran · 2024-05-16T21:44:48Z

api/v1beta1/vaultauth_types.go

-	Valid bool   `json:"valid"`
-	Error string `json:"error"`
+	Valid      bool               `json:"valid"`
+	Error      string             `json:"error"`


I know this was already there, but shouldn't this be tagged omitempty? As in an empty Error with Valid=true would be a valid status?

SGTM, updated in 6c44b68

tvoran · 2024-05-16T22:02:38Z

api/v1beta1/vaultauth_types.go

+	Valid      bool               `json:"valid"`
+	Error      string             `json:"error"`
+	Conditions []metav1.Condition `json:"conditions,omitempty"`
+	SpecHash   string             `json:"specHash,omitempty"`
 }


Is there a way for users to see what the merged VaultAuth spec is? I wonder if in the future putting the full merged spec in the status would make sense. Would be a a little crowded though.

It was definitely a consideration, since the hash of the spec is rather opaque :) I think it could make things easier to debug. I can spike in another PR to see what we think. Since we are introducing Conditions here, we could always extend it later as well.

controllers/vaultauth_controller.go

api/v1beta1/vaultauthglobal_types.go

Fixes: - vaultConnectionRef not be inherited from the global auth - firstNonZeroLen() always returned last - misc doc updates and improvements - some fixes to the credentials providers

…nfig

thyton

It's looking good for me.

thyton · 2024-05-18T00:03:08Z

internal/common/common_test.go

+		{
+			name: "invalid-nil-auth-config",
+			c:    builder.Build(),
+			gObj: &secretsv1beta1.VaultAuthGlobal{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "baz",
+					Name:      "buz",
+				},
+			},
+			o: &secretsv1beta1.VaultAuth{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "baz",
+					Name:      "foo",
+				},
+				Spec: secretsv1beta1.VaultAuthSpec{
+					VaultAuthGlobalRef: "buz",
+				},
+			},
+			wantErr: func(t assert.TestingT, err error, i ...interface{}) bool {
+				return assert.ErrorContains(t, err, "no auth method set in VaultAuth baz/foo")
+			},
+		},


Could we have more test coverages for other auth methods for invalid auth config cases after merged with VaultAuthGlobal?

Seems reasonable

tvoran

I'm not quite sure I understand the usage of conditions on the VaultAuth status, but this all seems to work for me.

tvoran · 2024-05-21T17:28:13Z

api/v1beta1/vaultauthglobal_types.go

+	// unset - disallow all namespaces except the Operator's the VaultAuthMethod's
+	// namespace, this is the default behavior.


except the Operator's the VaultAuthMethod's namespace

Which is it?

tvoran · 2024-05-21T23:15:19Z

controllers/vaultauth_controller.go

+			Type:               "VaultAuthGlobalRef",
+			Status:             "True",
+			ObservedGeneration: o.Generation,
+			LastTransitionTime: metav1.NewTime(nowFunc()),


This doesn't seem like a transition time, more like an evaluation time?

tvoran · 2024-05-21T23:16:14Z

controllers/vaultauth_controller.go

 	var errs error
+
+	var conditions []metav1.Condition


Should this be adding onto the existing conditions on the object's status? It looks like this starts over with a single entry every evaluation.

tvoran · 2024-05-21T23:26:34Z

controllers/vaultauth_controller.go

+			Status:             "True",
+			ObservedGeneration: o.Generation,
+			LastTransitionTime: metav1.NewTime(nowFunc()),
+			Reason:             "VaultAuthGlobalRef",


I wonder if Type and Reason should be different?

benashz force-pushed the VAULT-23381/add-support-global-vault-auth-config branch 2 times, most recently from 74dca96 to 7ecc7c0 Compare May 9, 2024 20:35

benashz linked an issue May 9, 2024 that may be closed by this pull request

Allow setting VaultAuth spec kubernetes mount as a default global setting #274

Open

benashz force-pushed the VAULT-23381/add-support-global-vault-auth-config branch 5 times, most recently from 4406bf0 to 071ba35 Compare May 13, 2024 16:03

benashz added this to the v0.7.0 milestone May 13, 2024

benashz force-pushed the VAULT-23381/add-support-global-vault-auth-config branch 2 times, most recently from 303ce2f to 6eaec64 Compare May 13, 2024 19:07

benashz added the vault-auth-globals label May 13, 2024

benashz force-pushed the VAULT-23381/add-support-global-vault-auth-config branch from 6eaec64 to 84003e2 Compare May 14, 2024 11:55

benashz changed the title ~~WIP: Add new VaultAuthGlobal type~~ Add new VaultAuthGlobal type May 14, 2024

benashz force-pushed the VAULT-23381/add-support-global-vault-auth-config branch from 84003e2 to 86c22bc Compare May 14, 2024 14:00

benashz force-pushed the VAULT-23381/add-support-global-vault-auth-config branch from 86c22bc to b0a73a4 Compare May 14, 2024 15:13

benashz marked this pull request as ready for review May 14, 2024 16:43

benashz requested a review from a team as a code owner May 14, 2024 16:43

Add validation method to all auth config structs

f00a5a6

benashz requested review from tvoran and thyton May 15, 2024 13:05

tvoran reviewed May 16, 2024

View reviewed changes

benashz added 2 commits May 17, 2024 09:05

Post review updates

6c44b68

Fixes: - vaultConnectionRef not be inherited from the global auth - firstNonZeroLen() always returned last - misc doc updates and improvements - some fixes to the credentials providers

Merge branch 'main' into VAULT-23381/add-support-global-vault-auth-co…

d3d4f4e

…nfig

thyton reviewed May 18, 2024

View reviewed changes

tvoran approved these changes May 21, 2024

View reviewed changes

benashz modified the milestones: v0.7.0, v0.8.0 May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new VaultAuthGlobal type #735

Add new VaultAuthGlobal type #735

benashz commented May 9, 2024 •

edited

benashz commented May 13, 2024

tvoran left a comment

tvoran May 16, 2024

tvoran May 16, 2024

benashz May 17, 2024

tvoran May 16, 2024

benashz May 17, 2024

tvoran May 16, 2024

benashz May 17, 2024

tvoran May 16, 2024

benashz May 17, 2024

benashz May 21, 2024

tvoran May 16, 2024

benashz May 17, 2024

tvoran May 16, 2024

benashz May 17, 2024 •

edited

thyton left a comment •

edited

thyton May 18, 2024

benashz May 21, 2024

tvoran left a comment

tvoran May 21, 2024

tvoran May 21, 2024

tvoran May 21, 2024

tvoran May 21, 2024 •

edited

		// method will be used. This can be used as a default Vault namespace for all
		// auth methods.

		// unset - disallow all namespaces except the Operator's the VaultAuthMethod's
		// namespace, this is the default behavior.

Add new VaultAuthGlobal type #735

Are you sure you want to change the base?

Add new VaultAuthGlobal type #735

Conversation

benashz commented May 9, 2024 • edited

benashz commented May 13, 2024

tvoran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benashz May 17, 2024 • edited

Choose a reason for hiding this comment

thyton left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvoran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvoran May 21, 2024 • edited

Choose a reason for hiding this comment

benashz commented May 9, 2024 •

edited

benashz May 17, 2024 •

edited

thyton left a comment •

edited

tvoran May 21, 2024 •

edited