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

Fix issues and limitations in account mgmt commands #58441

Open
wants to merge 4 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@billdodd
Copy link
Contributor

commented Jun 26, 2019

SUMMARY

This PR makes the following updates to the account management commands:

  1. The original code assumed the account URI path was formatted asself.root_uri + self.accounts_uri + "/" + id. This is not generally correct. Updated to lookup the proper URI.
  2. The AddUser and DeleteUser commands assumed a Redfish service that has a fixed set of account members that need to be PATCHed. Updated AddUser to POST to the Accounts collection by default and fallback to PATCH if the Allow header indicates POST is not supported or if the POST returns a 405. Updated DeleteUser to DELETE the account resource by default and fallback to PATCH if the Allow header indicates DELETE is not supported or if the DELETE returns a 405.
  3. Updated the AddUser logic in the PATCH scenario to search for the first unused account slot rather than requiring the operator to specify the account slot by Id. (Though if the account Id is specified, it is honored as before.)
  4. Added aliases to three of the existing account-related options to make it more clear that they apply to Account resource properties: target_id as an alias for id; account_username as an alias for new_username; account_password as an alias for new_password
  5. Added additional playbook example snippets in the redfish_command module EXAMPLES docstring.
  6. Idempotency: Added checks to see if the resource is already in the target state (account created, account in the specified role, etc.) and if so return success with changed set to False.
  7. Cleaned up the variable names in the user dict that is passed to the utility methods to match the module option names/aliases for consistency and to avoid confusion.
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

redfish_command.py
redfish_utils.py

ADDITIONAL INFORMATION

For Redfish services that create accounts by POSTing to the Accounts collection (which is the standard approach), it was impossible to use the AddUser command to do so. An existing, but unused account had to be specified by Id and that account would be PATCHed with the new UserName. After this PR, the module still works as before for services that need to PATCH for AddUser and DeleteUser. And it now also works for services that use POST for AddUser and DELETE for DeleteUser.

Also, for Redfish services that did not follow the hard-coded URI pattern (self.root_uri + self.accounts_uri + "/" + id), the commands would fail with a 404 status. That is now fixed.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/remote_management/redfish/redfish_command.py:310:31: E241 multiple spaces after ':'

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/remote_management/redfish/redfish_command.py:206:2: E311 EXAMPLES is not valid YAML

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

lib/ansible/modules/remote_management/redfish/redfish_command.py:206:2: error EXAMPLES: syntax error: expected '<document start>', but found '<block sequence start>'

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

@billdodd

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

ready_for_review

@billdodd

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

For consideration/discussion:

The redish_command module has commands related to the categories of Accounts, Systems, Managers, and Chassis. And it has one set of options that are passed in. Some of the options apply to all the categories and some only to one category. The original options that apply only to the Accounts category are:

  • id
  • new_username
  • new_password
  • roleid

In this PR I added an alias of target_id to the existing id option. So now the options are:

  • id (with an alias of target_id)
  • new_username
  • new_password
  • roleid

target_id is more descriptive than id, but it isn't obvious that it applies to Accounts. So maybe need a better naming convention.

Since we are dealing with the Id property of an Account resource, account_id seems a better choice than target_id. And I also think that adding account_username, account_password and account_roleid aliases for new_username, new_password and roleid respectively makes sense for consistency

After going through this option naming exercise, I realized that the new target_username option I added in the original commit for this PR was not needed. The existing new_username / account_username option can be used instead.

Will make these updates in a new commit.

In thinking about this, also consider that another PR will likely be adding additional "id" options for specifying the target system, manager and chassis. For example:

  • system_id
  • manager_id
  • chassis_id
for payload in username, pswd, roleid, enabled:
response = self.patch_request(uri, payload)
def add_user_via_patch(self, user):
response = self._find_account_uri(username=user.get('account_username'),

This comment has been minimized.

Copy link
@mraineri

mraineri Jul 2, 2019

I'm a little confused how this is supposed to work; shouldn't this try to find the "first empty slot" in the account list?

This comment has been minimized.

Copy link
@billdodd

billdodd Jul 2, 2019

Author Contributor

The original behavior/design was that the slot to be used for the new account had to be specified based on the Id property. And I didn't change that.

But that would probably be a good enhancement. Is testing that the UserName property is an empty string a reasonable test for the empty slot?

This comment has been minimized.

Copy link
@mraineri

mraineri Jul 2, 2019

Yeah, I think that's a good enough check. I would imagine a normal call to add a user would simply specify the new user info and not give an ID or existing user name.

This comment has been minimized.

Copy link
@billdodd

billdodd Jul 2, 2019

Author Contributor

Sounds good. I'll update the PR with that change.

@mraineri

This comment has been minimized.

Copy link

commented Jul 5, 2019

shipit

@ansibot ansibot added shipit and removed community_review labels Jul 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.