Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide 3 fixes for the diff plugin #726

Merged
merged 4 commits into from
Nov 19, 2023

Conversation

Rickmarges
Copy link
Contributor

What does this PR do?

This PR contains 3 fixes for the diff plugin. I'll explain them one by one:

1. Usage access to instance_groups is removed:

When setting usage access to an instance_group (with {"role": "use", "instance_groups": ["Dummy instance group"], "user": "test@example.com"}) the diff plugin tries to remove it afterwards.
This is due to the API returning information in singular version, but as can been seen above, the role module requires the plural version.

From API:
{
  "users": [
    "test@example.com"
  ],
  "teams": [],
  "name": "Use",
  "role": "Use",
  "type": "role",
  "resource_type": "instance_group",
  "instance_group": "Dummy instance group",
  "user": "test@example.com"
}

Diff plugin creates:
{
  "role": "use",
  "instance_group": "Dummy instance group",
  "user": "test@example.com",
  "state": "absent"
}

Should be:
{
  "role": "use",
  "instance_groups": [
    "Dummy instance group"
  ],
  "user": "test@example.com",
  "state": "absent"
}

This is fixed by converting the singular versions into plural.

2. When explicitly setting state: present, the plugin creates a present and absent object in some cases

In some cases (I've seen it happen with organizations, but might happen with more) when someone explicitly sets state: present in configuration (compare_list) the diff plugin returns 2 items, one absent and one present state.

If compare list like this it works fine (no diff after first apply):
{
    "user": "test@example.com",
    "organizations": [
        "Dummy"
    ],
    "role": "admin"
},

If with explicit state present:
{
    "user": "test@example.com",
    "organizations": [
        "Dummy"
    ],
    "role": "admin",
    "state": "present"
},

Results in:
{
    "organizations": [
        "Dummy"
    ],
    "role": "admin",
    "state": "present",
    "user": "test@example.com"
},
{
    "organizations": [
        "Dummy"
    ],
    "role": "admin",
    "state": "absent",
    "user": "test@example.com"
},

This is fixed by changing the way the dicts are compared. A separate method is used where the state key is ignored.

3. Member rights to teams aren't removed

When a user should no longer be member of a certain team the configuration is removed. However, the diff plugin creates the wrong absent dict. Setting (or removing) membership access is done by using the target_team key, not team.

Created by diff plugin:
{
    "role": "member",
    "state": "absent",
    "team": "dummy-team",
    "user": "test@example.com"
}

Should be:
{
    "role": "member",
    "state": "absent",
    "target_team": "dummy-team",
    "user": "test@example.com"
}

This is fixed by checking if the team key is present and the role is member. If both are true, update the dict to change team to target_team.

How should this be tested?

Some items are automatically checked by the pipeline.
Otherwise tests can be performed based on the data provided in the code blocks above

Is there a relevant Issue open for this?

Other Relevant info, PRs, etc

@Rickmarges
Copy link
Contributor Author

Will rebase after #721 is merged (I've changed the same line as in that PR)

@Rickmarges Rickmarges force-pushed the fix_diff_plugin branch 2 times, most recently from 303f73b to 3ddfb33 Compare November 14, 2023 09:23
Marges, RSY (Rick) added 2 commits November 14, 2023 10:26
When adding or removing a user from a team you'd need to use target_team instead of team
@Rickmarges Rickmarges force-pushed the fix_diff_plugin branch 2 times, most recently from 4029df4 to de56266 Compare November 14, 2023 10:26
Copy link
Contributor

@ivarmu ivarmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catches! I think the else indentation seems to be wrong. Could you check this?

plugins/lookup/controller_object_diff.py Show resolved Hide resolved
Copy link
Contributor

@ivarmu ivarmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sean-m-sullivan
Copy link
Collaborator

Looks good to me, good additions, thanks!

@sean-m-sullivan sean-m-sullivan enabled auto-merge (squash) November 19, 2023 16:08
@sean-m-sullivan sean-m-sullivan merged commit 5e679c7 into redhat-cop:devel Nov 19, 2023
13 checks passed
przemkalit pushed a commit to przemkalit/aap_configuration that referenced this pull request Nov 22, 2024
* Add instace_group info to diff plugin

* Change way to compare dicts to ignore state key

* Ensure that key is set to target_team in case of membership removal
When adding or removing a user from a team you'd need to use target_team instead of team

---------

Co-authored-by: Marges, RSY (Rick) <rick.marges@achmea.nl>
Co-authored-by: Sean Sullivan <ssulliva@redhat.com>
przemkalit pushed a commit to przemkalit/aap_configuration that referenced this pull request Nov 22, 2024
* Add instace_group info to diff plugin

* Change way to compare dicts to ignore state key

* Ensure that key is set to target_team in case of membership removal
When adding or removing a user from a team you'd need to use target_team instead of team

---------

Co-authored-by: Marges, RSY (Rick) <rick.marges@achmea.nl>
Co-authored-by: Sean Sullivan <ssulliva@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants