Add the recreateOption to the object template

When a user needs to update an object's immutable fields, the object
must be replaced. The user may opt-in to setting recreateOption on an
object template to "IfRequired" and "Always". When set to "IfRequired",
normal updates proceed when possible.

Relates:
https://issues.redhat.com/browse/ACM-11846

Signed-off-by: mprahl <mprahl@users.noreply.github.com>

api/v1/configurationpolicy_types.go

@@ -195,6 +195,15 @@ type ObjectTemplate struct {
 
  MetadataComplianceType MetadataComplianceType `json:"metadataComplianceType,omitempty"`
 
+ // RecreateOption describes whether to delete and recreate an object when an update is required. `IfRequired`
+ // will recreate the object when updating an immutable field. `Always` will always recreate the object if a mismatch
+ // is detected. `RecreateOption` has no effect when the `remediationAction` is `inform`. `IfRequired` has no effect
+ // on clusters without dry run update support. The default value is `None`.
+ //
+ //+kubebuilder:validation:Enum=None;IfRequired;Always
+ //+kubebuilder:default=None
+ RecreateOption RecreateOption `json:"recreateOption,omitempty"`
+
  // ObjectDefinition defines required fields for the object
  // +kubebuilder:pruning:PreserveUnknownFields
  ObjectDefinition runtime.RawExtension `json:"objectDefinition"`
@@ -340,6 +349,14 @@ func (c ComplianceType) IsMustNotHave() bool {
 // +kubebuilder:validation:Enum=MustHave;Musthave;musthave;MustOnlyHave;Mustonlyhave;mustonlyhave
 type MetadataComplianceType string
 
+type RecreateOption string
+
+const (
+ None RecreateOption = "None"
+ IfRequired RecreateOption = "IfRequired"
+ Always RecreateOption = "Always"
+)
+
 // RelatedObject is the list of objects matched by this Policy resource.
 type RelatedObject struct {
  //

controllers/configurationpolicy_controller.go

@@ -408,6 +408,19 @@ func (r *ConfigurationPolicyReconciler) shouldEvaluatePolicy(
  return true
  }
 
+ // If there was a timeout during a recreate, immediately evaluate the policy regardless of the evaluation interval.
+ if policy.Status.ComplianceState == policyv1.NonCompliant {
+ for _, details := range policy.Status.CompliancyDetails {
+ for _, condition := range details.Conditions {
+ if condition.Reason == "K8s update template error" && strings.Contains(
+ condition.Message, "timed out waiting for the object to delete during recreate",
+ ) {
+ return true
+ }
+ }
+ }
+ }
+
  lastEvaluated, err := time.Parse(time.RFC3339, policy.Status.LastEvaluated)
  if err != nil {
  log.Error(err, "The policy has an invalid status.lastEvaluated value. Will evaluate it now.")
@@ -1769,9 +1782,11 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
  if exists && obj.shouldExist {
  log.V(2).Info("The object already exists. Verifying the object fields match what is desired.")
 
- var throwSpecViolation, triedUpdate, updatedObj bool
+ var throwSpecViolation, triedUpdate bool
  var msg, diff string
+ var updatedObj *unstructured.Unstructured
 
+ created := false
  uid := string(obj.existingObj.GetUID())
 
  if evaluated, compliant := r.alreadyEvaluated(obj.policy, obj.existingObj); evaluated {
@@ -1791,15 +1806,18 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
  throwSpecViolation, msg, diff, triedUpdate, updatedObj = r.checkAndUpdateResource(
  obj, objectT, remediation,
  )
+
+ if updatedObj != nil && string(updatedObj.GetUID()) != uid {
+ uid = string(updatedObj.GetUID())
+ created = true
+ }
  }
 
  if triedUpdate && !strings.Contains(msg, "Error validating the object") {
  // The object was mismatched and was potentially fixed depending on the remediation action
  result.events = append(result.events, objectTmplEvalEvent{false, reasonWantFoundNoMatch, ""})
  }
 
- created := false
-
  if throwSpecViolation {
  var resultReason, resultMsg string
 
@@ -1820,7 +1838,7 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(
  } else {
  // it is a must have and it does exist, so it is compliant
  if remediation.IsEnforce() {
- if updatedObj {
+ if updatedObj != nil {
  result.events = append(result.events, objectTmplEvalEvent{true, reasonUpdateSuccess, ""})
  } else {
  result.events = append(result.events, objectTmplEvalEvent{true, reasonWantFoundExists, ""})
@@ -2569,7 +2587,13 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
  obj singleObject,
  objectT *policyv1.ObjectTemplate,
  remediation policyv1.RemediationAction,
-) (throwSpecViolation bool, message string, diff string, updateNeeded bool, updateSucceeded bool) {
+) (
+ throwSpecViolation bool,
+ message string,
+ diff string,
+ updateNeeded bool,
+ updatedObj *unstructured.Unstructured,
+) {
  complianceType := strings.ToLower(string(objectT.ComplianceType))
  mdComplianceType := strings.ToLower(string(objectT.MetadataComplianceType))
 
@@ -2597,7 +2621,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
  if obj.existingObj == nil {
  log.Info("Skipping update: Previous object retrieval from the API server failed")
 
- return false, "", "", false, false
+ return false, "", "", false, nil
  }
 
  var res dynamic.ResourceInterface
@@ -2615,10 +2639,11 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
  obj.desiredObj, obj.existingObj, existingObjectCopy, complianceType, mdComplianceType, !r.DryRunSupported,
  )
  if message != "" {
- return true, message, "", true, false
+ return true, message, "", true, nil
  }
 
  recordDiff := objectT.RecordDiffWithDefault()
+ var needsRecreate bool
 
  if updateNeeded {
  mismatchLog := "Detected value mismatch"
@@ -2631,7 +2656,7 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
  if err := r.validateObject(obj.existingObj); err != nil {
  message := fmt.Sprintf("Error validating the object %s, the error is `%v`", obj.name, err)
 
- return true, message, "", updateNeeded, false
+ return true, message, "", updateNeeded, nil
  }
  }
 
@@ -2646,16 +2671,6 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
  DryRun: []string{metav1.DryRunAll},
  })
  if err != nil {
- // If an inform policy and the update is forbidden (i.e. modifying Pod spec fields), then return
- // noncompliant since that confirms some fields don't match.
- if k8serrors.IsForbidden(err) {
- log.Info(fmt.Sprintf("Dry run update failed with error: %s", err.Error()))
-
- r.setEvaluatedObject(obj.policy, obj.existingObj, false)
-
- return true, "", "", false, false
- }
-
  // If it's a conflict, refetch the object and try again.
  if k8serrors.IsConflict(err) {
  log.Info("The object was updating during the evaluation. Trying again.")
@@ -2668,32 +2683,70 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
  }
  }
 
- message := getUpdateErrorMsg(err, obj.existingObj.GetKind(), obj.name)
- if message == "" {
- message = fmt.Sprintf(
- "Error issuing a dry run update request for the object `%v`, the error is `%v`",
- obj.name,
- err,
- )
+ // Handle all errors not related to updating immutable fields here
+ if !k8serrors.IsInvalid(err) {
+ message := getUpdateErrorMsg(err, obj.existingObj.GetKind(), obj.name)
+ if message == "" {
+ message = fmt.Sprintf(
+ "Error issuing a dry run update request for the object `%v`, the error is `%v`",
+ obj.name,
+ err,
+ )
+ }
+
+ // If the user specifies an unknown or invalid field, it comes back as a bad request.
+ if k8serrors.IsBadRequest(err) {
+ r.setEvaluatedObject(obj.policy, obj.existingObj, false)
+ }
+
+ return true, message, "", updateNeeded, nil
  }
 
- return true, message, "", updateNeeded, false
- }
+ // If an update is invalid (i.e. modifying Pod spec fields), then return noncompliant since that
+ // confirms some fields don't match and can't be fixed with an update. If a recreate option is
+ // specified, then the update may proceed when enforced.
+ needsRecreate = true
+ recreateOption := objectT.RecreateOption
 
- removeFieldsForComparison(dryRunUpdatedObj)
+ if isInform || !(recreateOption == policyv1.Always || recreateOption == policyv1.IfRequired) {
+ log.Info(fmt.Sprintf("Dry run update failed with error: %s", err.Error()))
 
- if reflect.DeepEqual(dryRunUpdatedObj.Object, existingObjectCopy.Object) {
- log.Info(
- "A mismatch was detected but a dry run update didn't make any changes. Assuming the object is " +
- "compliant.",
- )
+ r.setEvaluatedObject(obj.policy, obj.existingObj, false)
 
- r.setEvaluatedObject(obj.policy, obj.existingObj, true)
+ // Remove noisy fields such as managedFields from the diff
+ removeFieldsForComparison(existingObjectCopy)
+ removeFieldsForComparison(obj.existingObj)
 
- return false, "", "", false, false
- }
+ diff = handleDiff(log, recordDiff, true, existingObjectCopy, obj.existingObj)
+
+ if !isInform {
+ // Don't include the error message in the compliance status because that can be very long. The
+ // user can check the diff or the logs for more information.
+ message = fmt.Sprintf(
+ `%s cannot be updated, likely due to immutable fields not matching, you may `+
+ `set spec["object-templates"][].recreateOption to recreate the object`,
+ getMsgPrefix(&obj),
+ )
+ }
+
+ return true, message, diff, false, nil
+ }
+ } else {
+ removeFieldsForComparison(dryRunUpdatedObj)
+
+ if reflect.DeepEqual(dryRunUpdatedObj.Object, existingObjectCopy.Object) {
+ log.Info(
+ "A mismatch was detected but a dry run update didn't make any changes. Assuming the object " +
+ "is compliant.",
+ )
 
- diff = handleDiff(log, recordDiff, isInform, existingObjectCopy, dryRunUpdatedObj)
+ r.setEvaluatedObject(obj.policy, obj.existingObj, true)
+
+ return false, "", "", false, nil
+ }
+
+ diff = handleDiff(log, recordDiff, isInform, existingObjectCopy, dryRunUpdatedObj)
+ }
  } else if recordDiff == policyv1.RecordDiffLog || (isInform && recordDiff == policyv1.RecordDiffInStatus) {
  // Generate and log the diff for when dryrun is unsupported (i.e. OCP v3.11)
  mergedObjCopy := obj.existingObj.DeepCopy()
@@ -2706,18 +2759,61 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
  if isInform {
  r.setEvaluatedObject(obj.policy, obj.existingObj, false)
 
- return true, "", diff, false, false
+ return true, "", diff, false, nil
  }
 
  // If it's not inform (i.e. enforce), update the object
- log.Info("Updating the object based on the template definition")
+ var err error
+
+ // At this point, if a recreate is needed, we know the user opted in, otherwise, the dry run update
+ // failed and would have returned before now.
+ if needsRecreate || objectT.RecreateOption == policyv1.Always {
+ log.Info(
+ "Deleting and recreating the object based on the template definition",
+ "recreateOption", objectT.RecreateOption,
+ )
+
+ err = res.Delete(context.TODO(), obj.name, metav1.DeleteOptions{})
+ if err != nil && !k8serrors.IsNotFound(err) {
+ message = fmt.Sprintf(`%s failed to delete when recreating with the error %v`, getMsgPrefix(&obj), err)
+
+ return true, message, "", updateNeeded, nil
+ }
+
+ attempts := 0
+
+ for {
+ updatedObj, err = res.Create(context.TODO(), &obj.desiredObj, metav1.CreateOptions{})
+ if !k8serrors.IsAlreadyExists(err) {
+ // If there is no error or the error is unexpected, break for the error handling below
+ break
+ }
+
+ attempts++
+
+ if attempts >= 3 {
+ message = fmt.Sprintf(
+ `%s timed out waiting for the object to delete during recreate, will retry on the next `+
+ `policy evaluation`,
+ getMsgPrefix(&obj),
+ )
+
+ return true, message, "", updateNeeded, nil
+ }
+
+ time.Sleep(time.Second)
+ }
+ } else {
+ log.Info("Updating the object based on the template definition")
+
+ updatedObj, err = res.Update(context.TODO(), obj.existingObj, metav1.UpdateOptions{
+ FieldValidation: metav1.FieldValidationStrict,
+ })
+ }
 
- updatedObj, err := res.Update(context.TODO(), obj.existingObj, metav1.UpdateOptions{
- FieldValidation: metav1.FieldValidationStrict,
- })
  if err != nil {
  if k8serrors.IsConflict(err) {
- log.Info("The object updating during the evaluation. Trying again.")
+ log.Info("The object updated during the evaluation. Trying again.")
 
  rv, getErr := res.Get(context.TODO(), obj.existingObj.GetName(), metav1.GetOptions{})
  if getErr == nil {
@@ -2727,19 +2823,23 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
  }
  }
 
+ action := "update"
+
+ if needsRecreate || objectT.RecreateOption == policyv1.Always {
+ action = "recreate"
+ }
+
  message := getUpdateErrorMsg(err, obj.existingObj.GetKind(), obj.name)
  if message == "" {
- message = fmt.Sprintf("Error updating the object `%v`, the error is `%v`", obj.name, err)
+ message = fmt.Sprintf("%s failed to %s with the error `%v`", getMsgPrefix(&obj), action, err)
  }
 
- return true, message, diff, updateNeeded, false
+ return true, message, diff, updateNeeded, nil
  }
 
  if !statusMismatch {
  r.setEvaluatedObject(obj.policy, updatedObj, true)
  }
-
- updateSucceeded = true
  } else {
  if throwSpecViolation && recordDiff != policyv1.RecordDiffNone {
  // The spec didn't require a change but throwSpecViolation indicates the status didn't match. Handle
@@ -2754,7 +2854,17 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
  r.setEvaluatedObject(obj.policy, obj.existingObj, !throwSpecViolation)
  }
 
- return throwSpecViolation, "", diff, updateNeeded, updateSucceeded
+ return throwSpecViolation, "", diff, updateNeeded, updatedObj
+}
+
+func getMsgPrefix(obj *singleObject) string {
+ var namespaceMsg string
+
+ if obj.namespaced {
+ namespaceMsg = fmt.Sprintf(" in namespace %s", obj.namespace)
+ }
+
+ return fmt.Sprintf(`%s [%s]%s`, obj.gvr.Resource, obj.name, namespaceMsg)
 }
 
 // handleDiff will generate the diff and then log it or return it based on the input recordDiff value. If recordDiff

deploy/crds/kustomize_configurationpolicy/policy.open-cluster-management.io_configurationpolicies.yaml

@@ -177,6 +177,18 @@ spec:
  - InStatus
  - None
  type: string
+ recreateOption:
+ default: None
+ description: |-
+ RecreateOption describes whether to delete and recreate an object when an update is required. `IfRequired`
+ will recreate the object when updating an immutable field. `Always` will always recreate the object if a mismatch
+ is detected. `RecreateOption` has no effect when the `remediationAction` is `inform`. `IfRequired` has no effect
+ on clusters without dry run update support. The default value is `None`.
+ enum:
+ - None
+ - IfRequired
+ - Always
+ type: string
  required:
  - complianceType
  - objectDefinition

deploy/crds/policy.open-cluster-management.io_configurationpolicies.yaml

@@ -184,6 +184,18 @@ spec:
  - InStatus
  - None
  type: string
+ recreateOption:
+ default: None
+ description: |-
+ RecreateOption describes whether to delete and recreate an object when an update is required. `IfRequired`
+ will recreate the object when updating an immutable field. `Always` will always recreate the object if a mismatch
+ is detected. `RecreateOption` has no effect when the `remediationAction` is `inform`. `IfRequired` has no effect
+ on clusters without dry run update support. The default value is `None`.
+ enum:
+ - None
+ - IfRequired
+ - Always
+ type: string
  required:
  - complianceType
  - objectDefinition