Add feature removal validation and removability metadata

docs/developer/checks.md

@@ -15,9 +15,9 @@ Please update this file as check usage evolves.
 - **Rationale**: A validation was incorrectly removed which prevented users from creating duplicate Foreman permissions, causing upgrade failure. This check will need to be included until https://projects.theforeman.org/issues/38465 is addressed.
 
 ### check_features
-- **Description**: Ensures that all foremanctl features requested (via the `--feature` flag) are valid.
-- **Fail state**: Fails when requested features are not recognized by foremanctl.
-- **Rationale**: This check handles input sanitization for `--feature`.
+- **Description**: Ensures that all foremanctl features requested via `--add-feature` and `--remove-feature` are valid and that removals do not violate flavor or dependency rules.
+- **Fail state**: Fails when a requested feature is unknown, non-removable, protected by the current flavor, or required by another enabled feature.
+- **Rationale**: This check handles feature input sanitization. Feature state persistence remains owned by `obsah` and is committed only after the complete playbook succeeds.
 
 ### check_foreman_api
 - **Description**: Pings Foreman API (/api/v2/ping) to verify it responds. Verifies foreman_tasks service status is 'ok' when Katello/content feature is enabled.

docs/developer/deployment.md

@@ -100,6 +100,21 @@ These base features control which plugins are enabled when a feature is requeste
 
 A deployment can have multiple base features enabled.
 
+### Adding and removing features
+
+Feature selection is persisted in the `features` list in `parameters.yaml`. Add and remove flags operate on that same list, so a removal is durable without creating a separate deny-list; Obsah saves the resulting list after the playbook succeeds:
+
+```bash
+./foremanctl deploy --add-feature bmc
+./foremanctl deploy --remove-feature bmc
+```
+
+The second command removes `bmc` from the persisted user list. Running the first command again re-adds it. Repeated operations are idempotent; for a removable, non-flavor feature named by both flags, the last flag in command-line order determines whether it remains in the final list.
+
+A known removable feature that is already absent is an idempotent no-op. Unknown features, features without `removable: true`, flavor features, and features required by an enabled direct or transitive parent are rejected. Flavor features are baseline capabilities and cannot be disabled with `--remove-feature`; use a different flavor instead.
+
+Obsah updates the feature parameter file only after the playbook returns success. Validation or playbook failures leave the previous feature list in place; state-file write errors are reported by foremanctl.
+
 ### Enabling IOP
 
 IOP (Insights Operating Platform) deploys on-premise Insights services for advisor, vulnerability, and remediation. It requires internal database mode and depends on the `rh-cloud` and `katello` features.

docs/developer/feature-metadata.md

@@ -20,6 +20,7 @@ feature_two:
 The following properties are defined:
 * `description` (_String_): A human-readable description of the feature, can be used in documentation/help output.
 * `internal` (_Boolean_): Whether the feature is user visible (shows up in documentation/help) or internal (just to perform additional configuration without user interaction).
+* `removable` (_Boolean_): Whether users may request removal of the feature. Flavor features are protected and cannot be removed even when this is `true`; omitted or `false` values are not removable.
 * `foreman` (_Hash_): How this feature should be applied on the "main" system that offers the main user interaction via UI/API.
   * `plugin_name` (_String_): The name of the Foreman plugin to be enabled (via `FOREMAN_ENABLED_PLUGINS`).
      If `roles/foreman/tasks/feature/{{ foreman_plugin }}.yaml` exists, it will be executed to perform any plugin-specific setup.

docs/developer/playbooks-and-roles.md

@@ -120,11 +120,14 @@ variables:
     action: append_unique
   remove_features:
     parameter: --remove-feature
-    help: Additional features to disable in this deployment.
+    help: Features to remove from the persisted user feature list.
     action: remove
     dest: features
+    persist: false
 ```
 
+`features` is the single persisted user feature list. `--remove-feature` mutates that same list during argument parsing, while the distinct `remove_features` value contains only the raw operands for playbook validation. Because it is marked `persist: false`, a removal request is never stored as a second deny-list. A successful removal is saved by the normal `obsah` persistence path; a failed playbook leaves the previous list unchanged.
+
 ### Constraints for validating flag combinations
 
 Foremanctl uses `constraints` to enforce rules between related flags:

docs/user/parameters.md

@@ -142,12 +142,15 @@ There are multiple use cases from the users perspective that dictate what parame
 | `--foreman-fqdn` | FQDN of the Foreman server this proxy connects to | `--foreman-proxy-foreman-base-url` |
 | `--auth-bundle` | Path to the auth bundle tar file (generated by `foremanctl auth-bundle`) to be used by proxy | `--certs-tar-file` |
 | `--add-feature bmc` | Enable BMC feature | `--foreman-proxy-bmc` |
+| `--remove-feature bmc` | Remove BMC from the persisted user feature list | N/A |
 | `--add-feature content/ostree` | Enable OSTree content type | `--foreman-proxy-content-enable-ostree` |
 | `--bmc-ipmi-implementation` | IPMI implementation to use for BMC | `--foreman-proxy-bmc-default-provider` |
 | `--bmc-redfish-verify-ssl` | Verify SSL certificates for Redfish BMC connections | `--foreman-proxy-bmc-redfish-verify-ssl` |
 | `--add-feature templates` | Enable Templates feature on Smart Proxy | `--foreman-proxy-templates` |
+| `--remove-feature templates` | Remove Templates when it is not a protected flavor feature | N/A |
 | `--templates-url` | URL that hosts will use to contact the proxy for provisioning templates | `--foreman-proxy-templates-url` |
 | `--add-feature registration` | Enable Registration feature | `--foreman-proxy-registration` |
+| `--remove-feature registration` | Remove Registration when it is not a protected flavor feature | N/A |
 | `--registration-url` | URL that hosts use to reach the registration endpoint | `--foreman-proxy-registration-url` |
 
 ### Unmapped

src/features.yaml

@@ -107,6 +107,7 @@ container-gateway:
     - foreman
 bmc:
   description: Power management for bare metal hosts (IPMI, Redfish)
+  removable: true
   foreman_proxy:
     plugin_name: bmc
 webhooks:
@@ -116,10 +117,12 @@ webhooks:
   hammer: foreman_webhooks
 templates:
   description: Templates feature for foreman-proxy
+  removable: true
   foreman_proxy:
     plugin_name: templates
 registration:
   description: Host registration feature for foreman-proxy
+  removable: true
   foreman_proxy:
     plugin_name: registration
   dependencies:

src/filter_plugins/foremanctl.py

@@ -71,32 +71,47 @@ def available_foreman_plugins(_value):
     return compact_list(plugins)
 
 
-def list_all_features(enabled_features, only_enabled=False):
+def list_all_features(enabled_features, only_enabled=False, flavor_features=None):
     enabled_list = []
     available_list = []
     list_internal = os.environ.get('FOREMANCTL_FEATURES_LIST_INTERNAL', '') == 'true'
+    flavor_features_set = set(flavor_features or [])
+
     for name, meta in FEATURE_MAP.items():
         internal = meta.get('internal', False)
         if internal and not list_internal:
             continue
         description = meta.get('description', '')
+
+        if name in flavor_features_set:
+            removable = 'flavor'
+        elif meta.get('removable', False):
+            removable = 'yes'
+        else:
+            removable = 'no'
+
         if has_feature(enabled_features, name):
-            enabled_list.append((name, 'enabled', internal, description))
+            enabled_list.append((name, 'enabled', internal, removable, description))
         elif not only_enabled:
-            available_list.append((name, 'available', internal, description))
+            available_list.append((name, 'available', internal, removable, description))
 
     if not list_internal:
-        output = [f"{'FEATURE':<25} {'STATE':<12} DESCRIPTION"]
-        for name, state, _internal, description in enabled_list + available_list:
-            output.append(f"{name:<25} {state:<12} {description}")
+        output = [f"{'FEATURE':<25} {'STATE':<12} {'REMOVABLE':<13} DESCRIPTION"]
+        for name, state, _internal, removable, description in enabled_list + available_list:
+            output.append(f"{name:<25} {state:<12} {removable:<13} {description}")
     else:
-        output = [f"{'FEATURE':<25} {'STATE':<12} {'INTERNAL':<8} DESCRIPTION"]
-        for name, state, internal, description in enabled_list + available_list:
-            output.append(f"{name:<25} {state:<12} {internal:<8} {description}")
+        output = [f"{'FEATURE':<25} {'STATE':<12} {'INTERNAL':<8} {'REMOVABLE':<13} DESCRIPTION"]
+        for name, state, internal, removable, description in enabled_list + available_list:
+            output.append(f"{name:<25} {state:<12} {internal:<8} {removable:<13} {description}")
 
     return "\n".join(output)
 
 
+def is_feature_removable(feature_name):
+    """Check if a feature supports removal."""
+    return FEATURE_MAP.get(feature_name, {}).get('removable', False)
+
+
 def invalid_features(features):
     """Return a list of unknown features not defined in features.yaml."""
     return [feature for feature in features if feature not in FEATURE_MAP]
@@ -112,6 +127,63 @@ def conflicting_features(features):
     return [f"{pair[0]} conflicts with {pair[1]}" for pair in conflicts]
 
 
+def validate_feature_removals(remove_features, flavor_features):
+    """Validate requested removals without requiring them to be enabled.
+
+    A known removable feature that is already absent is an idempotent no-op.
+    Flavor features remain protected regardless of their registry metadata.
+    Returns a list of error message strings. Empty list means all valid.
+    """
+    errors = []
+    flavor_features = set(flavor_features or [])
+
+    for feature in remove_features:
+        if feature not in FEATURE_MAP:
+            errors.append(
+                f"Cannot remove unknown feature '{feature}'. "
+                f"Run 'foremanctl features' to see available features."
+            )
+        elif feature in flavor_features:
+            errors.append(
+                f"Cannot remove '{feature}' — it is a core feature of the current flavor. "
+                f"Flavor features cannot be removed."
+            )
+        elif not is_feature_removable(feature):
+            errors.append(
+                f"Cannot remove feature '{feature}' — this feature does not support removal. "
+                f"Run 'foremanctl features' to see which features can be removed."
+            )
+
+    return errors
+
+
+def unsatisfied_dependencies(enabled_features, remove_features=None):
+    """Detect removed features that are still required by an enabled feature.
+
+    ``enabled_features`` is the final feature list after command-line add/remove
+    operations. ``remove_features`` contains the raw removal operands; a feature
+    re-added later in command-line order is not considered removed. Because
+    dependencies are auto-included, a dependency is only ever "unsatisfied"
+    when the user removes it while a still-enabled feature depends on it.
+
+    Returns a list of error strings; an empty list means all removals are safe.
+    """
+    enabled_features = list(enabled_features or [])
+    remove_set = set(remove_features or []) - set(enabled_features)
+    if not remove_set:
+        return []
+
+    errors = []
+    for feature in enabled_features:
+        still_required = get_dependencies_for_feature(feature) & remove_set
+        for dependency in sorted(still_required):
+            errors.append(
+                f"Cannot remove '{dependency}' — it is required by enabled feature '{feature}'"
+            )
+
+    return errors
+
+
 def hammer_plugins(value):
     dependencies = list(get_dependencies(filter_features(value)))
     features = set(filter_features(value + dependencies))
@@ -164,6 +236,8 @@ def filters(self):
             'list_all_features': list_all_features,
             'invalid_features': invalid_features,
             'conflicting_features': conflicting_features,
+            'validate_feature_removals': validate_feature_removals,
+            'unsatisfied_dependencies': unsatisfied_dependencies,
             'has_feature': has_feature,
             'databases_for_features': databases_for_features,
             'to_postgresql_databases': to_postgresql_databases,

src/playbooks/_flavor_features/metadata.obsah.yaml

@@ -6,6 +6,7 @@ variables:
     action: append_unique
   remove_features:
     parameter: --remove-feature
-    help: Additional features to disable in this deployment.
+    help: Features to remove from the persisted user feature list.
     action: remove
     dest: features
+    persist: false

src/playbooks/features/features.yaml

@@ -14,4 +14,4 @@
   tasks:
     - name: Print features
       ansible.builtin.debug:
-        msg: "{{ enabled_features | list_all_features(list_enabled) }}"
+        msg: "{{ enabled_features | list_all_features(list_enabled, flavor_features) }}"

src/playbooks/pull-images/pull-images.yaml

@@ -11,6 +11,7 @@
     - "../../vars/base.yaml"
   become: true
   roles:
+    - role: check_features
     - role: pre_install
   post_tasks:
     - name: Deploy Foreman image units

src/requirements.txt

@@ -1,3 +1,3 @@
-obsah >= 1.11.0
+obsah @ git+https://github.com/theforeman/obsah.git@refs/pull/140/head
 requests
 requests-oauthlib

src/roles/check_features/tasks/main.yaml

@@ -1,26 +1,52 @@
 ---
+- name: Validate feature removal requests
+  ansible.builtin.assert:
+    that:
+      - check_features_removal_errors | length == 0
+    fail_msg: |
+      ERROR: Invalid feature removal request:
+      {% for error in check_features_removal_errors %}
+        - {{ error }}
+      {% endfor %}
+  vars:
+    check_features_removal_errors: "{{ remove_features | validate_feature_removals(flavor_features) }}"
+  when: remove_features | length > 0
+
+- name: Validate feature dependencies
+  ansible.builtin.assert:
+    that:
+      - check_features_dependency_errors | length == 0
+    fail_msg: |
+      ERROR: Removing features would break dependencies:
+      {% for error in check_features_dependency_errors %}
+        - {{ error }}
+      {% endfor %}
+  vars:
+    check_features_dependency_errors: "{{ enabled_features | unsatisfied_dependencies(remove_features) }}"
+  when: remove_features | length > 0
+
 - name: Validate requested features
   ansible.builtin.assert:
     that:
-      - found_invalid_features | length == 0
+      - check_features_invalid | length == 0
     fail_msg: |
-      ERROR: Unknown feature(s) requested: {{ found_invalid_features | join(', ') }}
+      ERROR: Unknown feature(s) requested: {{ check_features_invalid | join(', ') }}
 
       Run 'foremanctl features' to list all available features.
   vars:
-    found_invalid_features: "{{ features | invalid_features }}"
+    check_features_invalid: "{{ features | invalid_features }}"
   when: features | length > 0
 
 - name: Validate feature conflicts
   ansible.builtin.assert:
     that:
-      - found_conflicts | length == 0
+      - check_features_conflicts | length == 0
     fail_msg: |
       ERROR: Conflicting features detected:
-      {% for conflict in found_conflicts %}
+      {% for conflict in check_features_conflicts %}
         - {{ conflict }}
       {% endfor %}
 
       These features cannot be enabled together.
   vars:
-    found_conflicts: "{{ enabled_features | conflicting_features }}"
+    check_features_conflicts: "{{ enabled_features | conflicting_features }}"

src/vars/defaults.yml

@@ -3,5 +3,6 @@ certificates_source: default
 database_mode: internal
 tuning: default
 features: []
+remove_features: []
 enabled_features: "{{ (flavor_features + features) }}"
 air_gapped: false

tests/conftest.py

@@ -31,33 +31,48 @@ class UserParameters:
     def __init__(self, config):
         self._config = config
 
+    @staticmethod
+    def _is_internal(value):
+        return value.lower() in {'1', 'true'}
+
     @cached_property
     def features(self):
         # foremanctl outputs
-        # FEATURE                   STATE               DESCRIPTION
-        # $feature                  enabled/available   $description
+        # FEATURE                   STATE               INTERNAL  REMOVABLE       DESCRIPTION
+        # $feature                  enabled/available   0/1       flavor/yes/no    $description
+        # Internal values may be rendered as either numeric or boolean strings.
         output = subprocess.check_output(['./foremanctl', 'features'], cwd=self._config.rootdir,
                                          universal_newlines=True,
                                          env=os.environ | {'FOREMANCTL_FEATURES_LIST_INTERNAL': 'true'})
         lines = output.splitlines(keepends=False)
-        # feature, status, internal, description
-        return [line.split(None, 3) for line in lines[1:]]
+        # feature, status, internal, removable, description
+        features = []
+        for line in lines[1:]:
+            fields = line.split(None, 4)
+            features.append(fields + [''] * (5 - len(fields)))
+        return features
 
     @cached_property
     def all_available_features(self):
-        return set(feature for feature, _status, _internal, _desc in self.features)
+        return set(feature for feature, _status, _internal, _removable, _desc in self.features)
 
     @cached_property
     def available_features(self):
-        return set(feature for feature, _status, internal, _desc in self.features if internal == '0')
+        return set(
+            feature for feature, _status, internal, _removable, _desc in self.features
+            if not self._is_internal(internal)
+        )
 
     @cached_property
     def enabled_features(self):
-        return set(feature for feature, status, internal, _desc in self.features if status == 'enabled')
+        return set(feature for feature, status, _internal, _removable, _desc in self.features if status == 'enabled')
 
     @cached_property
     def user_enabled_features(self):
-        return set(feature for feature, status, internal, _desc in self.features if status == 'enabled' and internal == '0')
+        return set(
+            feature for feature, status, internal, _removable, _desc in self.features
+            if status == 'enabled' and not self._is_internal(internal)
+        )
 
     @cached_property
     def flavor(self):