RBAC on unstable fails to load

[amanda11]

SUMMARY

Provide a quick summary of your bug report.

STACKSTORM VERSION

Paste the output of st2 --version:

OS, environment, install method

Ansible CI/CD has been failing for 3 days on the smoke tests at the RBAC apply defintions

Steps to reproduce the problem

Perform a ansible install with environment -e st2_rbac_enable=True. At the point it loads the rbac definition file with the smoke test resource it fails.
Think this is probably true for any RBAC on unstable.

Expected Results

Load should complete.

Actual Results

The ansible script fails on the rbac. Reproducing this on a clean VM and running the ansible install then we see error:

2022-07-22 11:29:13,398 INFO [-] Loading role definitions from "/opt/stackstorm/rbac/roles/"
Traceback (most recent call last):
  File "/bin/st2-apply-rbac-definitions", line 15, in <module>
    sys.exit(apply_rbac_definitions.main(sys.argv[1:]))
  File "/opt/stackstorm/st2/lib/python3.8/site-packages/st2rbac_backend/cmd/apply_rbac_definitions.py", line 60, in main
    apply_definitions()
  File "/opt/stackstorm/st2/lib/python3.8/site-packages/st2rbac_backend/cmd/apply_rbac_definitions.py", line 42, in apply_definitions
    result = loader.load()
  File "/opt/stackstorm/st2/lib/python3.8/site-packages/st2rbac_backend/loader.py", line 61, in load
    result["roles"] = self.load_role_definitions()
  File "/opt/stackstorm/st2/lib/python3.8/site-packages/st2rbac_backend/loader.py", line 79, in load_role_definitions
    role_definition_api = self.load_role_definition_from_file(file_path=file_path)
  File "/opt/stackstorm/st2/lib/python3.8/site-packages/st2rbac_backend/loader.py", line 159, in load_role_definition_from_file
    role_definition_api = role_definition_api.validate()
  File "/opt/stackstorm/st2/lib/python3.8/site-packages/st2common/models/api/rbac.py", line 170, in validate
    cleaned = super(RoleDefinitionFileFormatAPI, self).validate()
  File "/opt/stackstorm/st2/lib/python3.8/site-packages/st2common/models/api/base.py", line 75, in validate
    cleaned = util_schema.validate(
  File "/opt/stackstorm/st2/lib/python3.8/site-packages/st2common/util/schema/__init__.py", line 370, in validate
    instance = assign_default_values(instance=instance, schema=schema)
  File "/opt/stackstorm/st2/lib/python3.8/site-packages/st2common/util/schema/__init__.py", line 255, in assign_default_values
    instance[property_name] = assign_default_values(
  File "/opt/stackstorm/st2/lib/python3.8/site-packages/st2common/util/schema/__init__.py", line 249, in assign_default_values
    array_instance = instance.get(property_name, None)
AttributeError: 'list' object has no attribute 'get'

There is only one role defined, and it looks good:

$ cat /opt/stackstorm/rbac/roles/smoke_tests_rbac_basic.yaml 
---

name: smoke_tests_rbac_basic
description: This role has access only to action core.local in pack 'core'
permission_grants:
  - permission_types:
    - action_execute
    - action_view
    resource_uid: action:core:local
  - permission_types:
    - runner_type_list

[amanda11]

@cognifloyd Could this be related to the output schema changes that went in via: #5319

It's around the right time, as these tests were reliable before. And it looks like similar area of code. Wondering if the RBAC schema is incorrect as per the updates to the schema checking?

Do we need to define the schemas that are defined in code, e.g.

st2/st2common/st2common/models/api/rbac.py

Line 116 in 8726092

schema = {

Or the problem migth be that the schema has a default of [] on the permission_types, and the code for setting defaults doesn't cope with setting that?

[cognifloyd]

@amanda11 wow. So, it looks like this bug was present before, BUT by fixing a different bug, I uncovered this bug.

Looks like:

• schema for an array of objects
  • a property in the items schema is also an array
    • 💥

Before, it was only applying defaults to each of the items schema properties if, and only if, the property's schema included properties which effectively meant it only set defaults for objects and ignored setting defaults within array properties before.

I think I see what the fix requires, hopefully I can submit a PR to add tests for that scenario and fix that today or tomorrow.

[cognifloyd]

OK. I think the best way to fix this is to merge these two assign_default_values implementations:

• st2/st2common/st2common/util/schema/__init__.py

  Lines 202 to 205 in 3e1a052

  	def assign_default_values(instance, schema):
  	"""
  	Assign default values on the provided instance based on the schema default specification.
  	"""

• st2/st2common/st2common/util/config_loader.py

  Lines 297 to 305 in 7110c75

  	def _assign_default_values(self, schema, config):
  	"""
  	Assign default values for particular config if default values are provided in the config
  	schema and a value is not specified in the config.
  	Note: This method mutates config argument in place.
  	:rtype: ``dict|list``
  	"""

I've spent a lot of time revising the pack config loader and ensuring it can handle many different schema types in #5225 and #5321, so I'd like to move that implementation from st2common.util.config_loader.ContentPackConfigLoader to st2common.util.schema.

[cognifloyd]

It looks like the schema handling in ContentPackConfigLoader is not compatible with the schema handling in st2common.util.schema. I tried to combine them in a couple of different ways without success. They each make very different assumptions about:

• what the type keyword can be (a simple type in pack config, but it might be a list of types in other schemas),
• how to handle None values (pack config eagerly descends into sub schemas, but util stops when something is None).

So, I'm refactoring assign_default_values to separate objects vs arrays handling in #5685
My first goal was to break it up so I can understand what it is doing more easily. With the latest refactoring attempt, all existing tests are passing. Next, I want to write a test that fails without my refactoring, and then try with the refactoring to see if anything else needs to be adjusted.

[cognifloyd]

Cool. I have a reproducer test that fails on master and passes with my changes. Now pushing the commits one at a time to show the progression in CI.