Include conditions on supported types #1058

benashz · 2025-04-24T13:39:07Z

Adds support for setting the status conditions on the following types:

vaultstaticsecrets
vaultpkisecrets
vaultdynamicsecrets
hcpauths
hcpvaultsecretsapps

Adds support for setting the status conditions on the following types: - vaultstaticsecrets - vaultpkisecrets - vaultdynamicsecrets - hcpauths - hcpvaultsecretsapps

mtparet · 2025-06-25T08:28:13Z

Would be awesome to get it merged!

dm3ch · 2025-06-27T00:35:03Z

@benashz Sorry for bothering you, but is there any reason why this PR is still a draft?

Is something is still missing or non functional or it's finished and just awaiting a merge?

tvoran · 2025-07-30T19:22:44Z

api/v1beta1/hcpauth_types.go

+	// Conditions hold information that be used by other apps to determine the health
+	// the resource instance.


Suggested change

// Conditions hold information that be used by other apps to determine the health

// the resource instance.

// Conditions hold information that be used by other apps to determine the health

// of the resource instance.

Also missing a "can" here -- "that can be used by other apps"

tvoran · 2025-07-30T19:25:26Z

controllers/common.go

 		// we key conditions on their type and reason
 		// e.g: type=VaultAuthGlobal reason=Available, ...


It looks like this isn't quite true anymore? i.e. the key seems to just be Type now on line 295.

tvoran · 2025-07-30T19:27:26Z

controllers/common_test.go

@@ -388,6 +388,7 @@ func Test_waitForStoppedCh(t *testing.T) {
 }

 func TestVaultAuthReconciler_updateConditions(t *testing.T) {
+	t.Skip("This test is not working as expected. It is failing with the following error: expected 1 conditions, got 2")


Is this going to be addressed in this PR? I don't see updateCondtions() being tested anywhere else.

tvoran · 2025-07-30T19:43:28Z

controllers/vaultconnection_controller.go

+			o.Status.Conditions = append(o.Status.Conditions,
+				newConditionNow(o, "VaultPing", consts.ReasonInvalidConfiguration,
+					metav1.ConditionTrue, "Vault ping, address=%s",
+					o.Spec.Address),
+			)


Should the the reason be something besides InvalidConfiguration here? Seems like the ping was successful at this point?

tvoran · 2025-07-30T19:45:10Z

controllers/vaultconnection_controller.go

@@ -108,16 +136,29 @@ func (r *VaultConnectionReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 		errs = errors.Join(errs, err)
 	}

-	if err := r.updateStatus(ctx, o); err != nil {
-		errs = errors.Join(errs, err)
+	// TODO: cleanup error reporting


Can this comment be more specific about what needs cleanup and why?

tvoran · 2025-07-30T19:56:26Z

controllers/vaultpkisecret_controller.go

+	conditions = append(conditions,
+		newSyncCondition(o, metav1.ConditionTrue,
+			"Secret synced, horizon=%s", horizon),
+	)


It looks like this change to conditions isn't used.

tvoran · 2025-07-30T19:57:32Z

controllers/vaultpkisecret_controller.go

 	r.recordEvent(o, reason, fmt.Sprintf("Secret synced, horizon=%s", horizon))
 	logger.Info("Successfully updated the secret", "horizon", horizon)
 	return ctrl.Result{
 		RequeueAfter: horizon,
 	}, nil
 }

+func (r *VaultPKISecretReconciler) syncSecret(ctx context.Context, o *secretsv1beta1.VaultPKISecret) (*vault.PKICertResponse, error) {


Is this function going to be used somewhere?

tvoran · 2025-07-30T20:00:37Z

controllers/vaultstaticsecret_controller.go

+		if err := r.updateStatus(ctx, o, newSyncCondition(o, metav1.ConditionFalse, "Failed to sync the secret, horizon=%s, err=%s", horizon, err)); err != nil {
+			return ctrl.Result{}, err
+		}
+
+		if err := r.updateStatus(ctx, o, newSyncCondition(o, metav1.ConditionFalse, "Failed to sync the secret, horizon=%s, err=%s", horizon, err)); err != nil {
+			return ctrl.Result{}, err
+		}


Duplicate?

Suggested change

if err := r.updateStatus(ctx, o, newSyncCondition(o, metav1.ConditionFalse, "Failed to sync the secret, horizon=%s, err=%s", horizon, err)); err != nil {

return ctrl.Result{}, err

}

if err := r.updateStatus(ctx, o, newSyncCondition(o, metav1.ConditionFalse, "Failed to sync the secret, horizon=%s, err=%s", horizon, err)); err != nil {

return ctrl.Result{}, err

}

if err := r.updateStatus(ctx, o, newSyncCondition(o, metav1.ConditionFalse, "Failed to sync the secret, horizon=%s, err=%s", horizon, err)); err != nil {

return ctrl.Result{}, err

}

digivava · 2025-07-30T21:15:19Z

controllers/common.go

 	for _, newCond := range updates {
 		// we key conditions on their type and reason
 		// e.g: type=VaultAuthGlobal reason=Available, ...
-		key := fmt.Sprintf("%s/%s", newCond.Type, newCond.Reason)
+		// if newCond.Reason == "" {


This was probably meant to be removed

digivava · 2025-07-30T21:34:08Z

controllers/vaultconnection_controller.go

+		o.Status.Conditions = append(o.Status.Conditions,
+			newConditionNow(o, consts.TypeResourceValidation, consts.ReasonInvalidConfiguration,
+				metav1.ConditionFalse, "Failed to validate resource, address=%s, errs=%s",
+				o.Spec.Address, errs),


Just double-checking, does this work okay if errs is nil? Since this is an || conditional, it can hit this piece of code if the status is invalid but errs is nil. Should we populate the error with something if it's the !*o.Status.Valid case?

benashz mentioned this pull request Apr 24, 2025

VSS: Add status fields #1057

Closed

benashz added this to the v0.11.0 milestone Apr 24, 2025

sergeyshevch mentioned this pull request Apr 24, 2025

Status fields for resource health in VaultStaticSecret and VaultDynamicSecret resources #538

Open

Include conditions on supported types

3d75f0c

Adds support for setting the status conditions on the following types: - vaultstaticsecrets - vaultpkisecrets - vaultdynamicsecrets - hcpauths - hcpvaultsecretsapps

benashz force-pushed the VAULT-22933/adopt-status-conditions-for-all-crds branch from 0052f71 to 3d75f0c Compare April 24, 2025 13:55

benashz linked an issue Apr 24, 2025 that may be closed by this pull request

Status fields for resource health in VaultStaticSecret and VaultDynamicSecret resources #538

Open

benashz assigned digivava May 13, 2025

benashz requested review from tvoran, digivava and Zlaticanin and removed request for tvoran July 8, 2025 22:26

benashz marked this pull request as ready for review July 30, 2025 17:37

benashz requested a review from a team as a code owner July 30, 2025 17:37

benashz requested a review from tvoran July 30, 2025 17:37

tvoran reviewed Jul 30, 2025

View reviewed changes

digivava reviewed Jul 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Include conditions on supported types #1058

Include conditions on supported types #1058

Uh oh!

benashz commented Apr 24, 2025

Uh oh!

mtparet commented Jun 25, 2025

Uh oh!

dm3ch commented Jun 27, 2025

Uh oh!

tvoran Jul 30, 2025

Uh oh!

digivava Jul 30, 2025

Uh oh!

tvoran Jul 30, 2025

Uh oh!

tvoran Jul 30, 2025

Uh oh!

tvoran Jul 30, 2025

Uh oh!

tvoran Jul 30, 2025

Uh oh!

tvoran Jul 30, 2025

Uh oh!

tvoran Jul 30, 2025

Uh oh!

tvoran Jul 30, 2025

Uh oh!

digivava Jul 30, 2025

Uh oh!

digivava Jul 30, 2025 •

edited

Loading

Uh oh!

Uh oh!

		// Conditions hold information that be used by other apps to determine the health
		// the resource instance.

		// we key conditions on their type and reason
		// e.g: type=VaultAuthGlobal reason=Available, ...

Include conditions on supported types #1058

Are you sure you want to change the base?

Include conditions on supported types #1058

Uh oh!

Conversation

benashz commented Apr 24, 2025

Uh oh!

mtparet commented Jun 25, 2025

Uh oh!

dm3ch commented Jun 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

digivava Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

digivava Jul 30, 2025 •

edited

Loading