SPIRE-365: create-only mode false status is not set on the main CR once the create-only mode is disabled

[PillaiManish]

This PR fixes the status of the main CR to revert the status of the CREATE_ONLY_MODE when it is set to false.

[openshift-ci-robot]

@PillaiManish: This pull request references SPIRE-365 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set.

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

Walkthrough

The PR enhances CREATE_ONLY_MODE environment variable handling by introducing synchronized one-time logging for invalid values, improving case-insensitivity and whitespace tolerance in the IsInCreateOnlyMode function, and refactoring the controller to derive CreateOnlyMode status from environment variables rather than aggregating operand states.

Changes

Cohort / File(s)	Summary
Utils package enhancements pkg/controller/utils/utils.go, pkg/controller/utils/utils_test.go	Added sync.Once guard for single-warning logging on invalid CREATE_ONLY_MODE values. Enhanced IsInCreateOnlyMode to handle case-insensitivity, whitespace trimming, and log invalid values once via ctrl.Log. Expanded test coverage with additional cases for mixed-case, whitespace handling, and edge cases.
Controller reconciliation logic refactoring pkg/controller/zero-trust-workload-identity-manager/controller.go	Replaced operand-aggregated CreateOnlyMode logic with environment-driven approach. Removed per-operand CreateOnlyMode state tracking fields (anyCreateOnlyModeEnabled, anyOperandHasCreateOnlyCondition). Simplified reconciliation to directly use IsInCreateOnlyMode() for condition updates and logging.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

✨ Finishing touches

• 📝 Generate docstrings

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[openshift-ci-robot]

@PillaiManish: This pull request references SPIRE-365 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set.

Details

In response to this:

This PR fixes the status of the main CR to revert the status of the CREATE_ONLY_MODE when it is set to false.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[sayak-redhat]

@anirudhAgniRedhat @PillaiManish

• 1. Case-sensitive: "TRUE"/"True" silently ignored (only "true" works)
• 2. Invalid values change state: "random" turns TRUE→FALSE (should keep current state)
• 3. No warning logs for invalid values

[sayak-redhat]

@PillaiManish @anirudhAgniRedhat

Enhancement: Accept case-insensitive "TRUE"/"FALSE" for CREATE_ONLY_MODE env var using strings.EqualFold(). Any random/invalid value will default to false (disabled) for safety.

CREATE_ONLY_MODE env var handling

• Accepts: true/True/TRUE → Enables create-only mode
• Accepts: false/False/FALSE → Disables create-only mode
• Any other value (yes, 1, random, etc.) → Defaults to DISABLED

The check is case-insensitive. Recommend documenting "true" or "false" as accepted values.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

pkg/controller/utils/utils_test.go (2)

780-784: Test case name is slightly misleading.

The test case "env var not set" actually sets the env var to an empty string via t.Setenv. While functionally equivalent due to os.Getenv behavior, the test name could be clearer. Consider renaming to "env var set to empty string" or adding a separate test that truly unsets the variable using os.Unsetenv.

---

804-809: The sync.Once guard limits testability of the warning log.

The logInvalidCreateOnlyModeOnce in utils.go ensures the warning is logged only once. Since this is package-level state, only the first test encountering an invalid value will trigger the log. Consider adding a comment documenting this limitation, or exposing a test hook to reset the guard for comprehensive log verification.

pkg/controller/zero-trust-workload-identity-manager/controller.go (1)

312-321: Minor: IsInCreateOnlyMode() called twice per reconciliation.

utils.IsInCreateOnlyMode() is called both in setCreateOnlyModeCondition (line 201) and again at line 316. While functionally correct (env var is stable), consider returning the value from setCreateOnlyModeCondition or calling it once and passing to both locations to avoid redundant processing.

♻️ Optional refactor

-// setCreateOnlyModeCondition sets the CreateOnlyMode condition on the main CR based on the environment variable
-func setCreateOnlyModeCondition(statusMgr *status.Manager, existingConditions []metav1.Condition) {
-	createOnlyMode := utils.IsInCreateOnlyMode()
+// setCreateOnlyModeCondition sets the CreateOnlyMode condition on the main CR based on the environment variable
+// Returns the current create-only mode status
+func setCreateOnlyModeCondition(statusMgr *status.Manager, existingConditions []metav1.Condition) bool {
+	createOnlyMode := utils.IsInCreateOnlyMode()

 	if createOnlyMode {
 		statusMgr.AddCondition(CreateOnlyMode, utils.CreateOnlyModeEnabled,
 			"Create-only mode is enabled: Updates are not reconciled to existing resources",
 			metav1.ConditionTrue)
 	} else {
 		// Only set to False if we previously had it set to True (to show the transition)
 		existingCondition := apimeta.FindStatusCondition(existingConditions, CreateOnlyMode)
 		if existingCondition != nil && existingCondition.Status == metav1.ConditionTrue {
 			statusMgr.AddCondition(CreateOnlyMode, utils.CreateOnlyModeDisabled,
 				"Create-only mode is disabled",
 				metav1.ConditionFalse)
 		}
 	}
+	return createOnlyMode
 }

Then in Reconcile:

 	// Set CreateOnlyMode condition based on environment variable (simpler than aggregating from operands)
-	setCreateOnlyModeCondition(statusMgr, config.Status.ConditionalStatus.Conditions)
+	createOnlyModeEnabled := setCreateOnlyModeCondition(statusMgr, config.Status.ConditionalStatus.Conditions)

 	// Check create-only mode from environment variable for logging and OLM update
-	createOnlyModeEnabled := utils.IsInCreateOnlyMode()
 	r.log.Info("Aggregated operand status", "allReady", result.allReady, "notCreated", result.notCreatedCount, "failed", result.failedCount, "createOnlyModeEnabled", createOnlyModeEnabled, "anyOperandExists", result.anyOperandExists)

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 0dcaf41 and 7e43361.

📒 Files selected for processing (3)

• pkg/controller/utils/utils.go
• pkg/controller/utils/utils_test.go
• pkg/controller/zero-trust-workload-identity-manager/controller.go

🧰 Additional context used

📓 Path-based instructions (1)

**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

• pkg/controller/utils/utils.go
• pkg/controller/utils/utils_test.go
• pkg/controller/zero-trust-workload-identity-manager/controller.go

🔇 Additional comments (7)

pkg/controller/utils/utils.go (3)

9-14: LGTM!

The new imports for sync and controller-runtime are appropriate for the sync.Once guard and structured logging functionality.

---

28-29: LGTM!

The sync.Once guard at package level is appropriate for ensuring the warning is logged only once per process lifetime, preventing log spam during repeated reconciliation cycles.

---

233-255: LGTM!

The implementation correctly addresses all reviewer feedback from the PR:

• Case-insensitive handling via strings.ToUpper
• Whitespace tolerance via strings.TrimSpace
• Invalid values safely default to disabled (false)
• One-time warning log for invalid inputs with helpful guidance on valid values

pkg/controller/zero-trust-workload-identity-manager/controller.go (4)

199-216: LGTM!

This function correctly addresses the PR's main objective: setting CreateOnlyMode to False when the mode is disabled after being enabled. The transition check on line 210 prevents cluttering the status when the mode was never enabled.

---

328-343: LGTM!

The simplified structs correctly remove the per-operand CreateOnlyMode tracking fields, aligning with the new environment-driven approach.

---

475-492: LGTM!

The updated comments clearly explain that ZTWIM now checks the environment variable directly, while this function still extracts operand-level CreateOnlyMode conditions for visibility purposes.

---

386-392: LGTM!

The return correctly reflects the simplified aggregate result structure.

[PillaiManish]

/retest

[PillaiManish]

/retest

[openshift-ci]

@PillaiManish: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[sayak-redhat]

lgtm

[sayak-redhat]

/verified by qe

[openshift-ci-robot]

@sayak-redhat: This PR has been marked as verified by qe.

Details

In response to this:

/verified by qe

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[sayak-redhat]

/hold

[anirudhAgniRedhat]

/lgtm

[sayak-redhat]

/verified by qe

test cases covered in MD file inside the jira
https://issues.redhat.com/browse/SPIRE-368

[openshift-ci-robot]

@sayak-redhat: This PR has been marked as verified by qe.

Details

In response to this:

/verified by qe

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[anirudhAgniRedhat]

/approve
Thanks for the PR

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anirudhAgniRedhat, PillaiManish

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 [anirudhAgniRedhat]

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

[sayak-redhat]

/unhold