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

Implement organization management (RBAC) #855

Merged
merged 16 commits into from
Sep 19, 2024

Conversation

gigerdo
Copy link
Member

@gigerdo gigerdo commented Sep 9, 2024

This resource allows managing the members of an organization:

  • For each member their roles can be configured.
  • Added members will be invited to the organization.
    • The user will still have to accept the invitation before they have access to the organization.
  • Removed members will be removed from the organization

(Currently all builds/tests for this PR are failing because we first need a new release of cloud-sdk-go to get the organization apis elastic/cloud-sdk-go#477)

Now includes cloud-sdk-go 1.21.0

Description

  • Regarding invitations
    • The resource tracks the invitation status for each member. Since some time can pass before the new member accepts the invitation.
    • Since invitations can't be updated, any role changes during this time will mean the invitation is cancelled and a new invitation is created.
    • After the invitation is accepted, the next apply will update the local state with the user-id. And now roles can be updated.
  • Create/delete don't really work for an organization. It already exists and can't be deleted (One can only leave an organization).
    • The default flow is to import the already existing organization using the terraform import flow.
    • It isn't possible to automatically import with the create, because then the resulting organization potentially wouldn't match the current config. So the create has to throw an error if it is called.
    • The delete does nothing currently, so it is possible to run the terraform destroy to remove the org from the state. However this is currently just to stop managing the resource in terraform, it won't actually be deleted.

Related Issues

#629

How Has This Been Tested?

  • Manual testing
  • There are no real acceptance tests because it isn't possible to automatically add new members.
  • However currently all use-cases are covered by acceptance tests that use mocked API requests.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (improves code quality but has no user-facing effect)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation

Readiness Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

This resource allows managing the members of an organization:
- Added members will be invited to the organization. The user will still have to accept the invitation before they have access to the organization.
- Removed members will be removed from the organization
- For each member their roles can be configured.
@gigerdo gigerdo marked this pull request as ready for review September 9, 2024 13:24
@gigerdo gigerdo requested a review from a team as a code owner September 9, 2024 13:24
Copy link
Member

@tobio tobio left a comment

Choose a reason for hiding this comment

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

I wonder if we'd have a better UX restructuring this one a bit:

  • An organisation data source. Like you've said there's no real configuration for an organisation, moving this to a data source would avoid the import requirement
  • Having a resource per organisation member, rather than a single resource managing all members within an organisation.

The TF module could look something like:

data "ec_organization" "myorg" {}

resource "ec_organization_member" "tobio" {
  organization_id = data.ec_organization.myorg.id
  email = "[email protected]"
  deployment_roles = [...]
  ...
}

Managing multiple members with the same permissions becomes simpler:

resource "ec_organization_member" "admins" {
  for_each = ["[email protected]", "[email protected]"]
  organization_id = ...
  email = each.key
  deployment_roles = [...]

@gigerdo
Copy link
Member Author

gigerdo commented Sep 10, 2024

I think both have their advantages. The main difference in my opinion is on the management of members:

  • Single organization resource: You must manage all members. This has the advantage that you can always be sure that the configured members are the only ones in the org.
  • Per-member resource: More flexible, you can pick which members should be managed by terraform.

In the end I decided to go with the first approach as it also makes it easier to add new members, and impossible to accidentally lose track of a member (e.g. the member exists but isn't tracked in terraform)

The `Test_modelToState` had to be updated as it hashes the deployment response (and that response now has additional fields)
@gigerdo
Copy link
Member Author

gigerdo commented Sep 11, 2024

The PR is now updated to include the right cloud-sdk-go and should compile now.

@gigerdo gigerdo requested review from tobio and a team September 12, 2024 07:59
@gigerdo
Copy link
Member Author

gigerdo commented Sep 13, 2024

Small update to the PR, I reenamed the fields:

  • for_all_deployments -> all_deployments
  • for_all_projects -> all_projects

@dimuon
Copy link
Contributor

dimuon commented Sep 16, 2024

@gigerdo , does it make sense to add unit tests to cover negative use cases?
(IIUC, TestOrganizationResourceAgainstMockedAPI covers positive use cases only).

@gigerdo
Copy link
Member Author

gigerdo commented Sep 17, 2024

@dimuon I refactored the testcases into separate tests, and added a failure case for each.

@gigerdo gigerdo requested a review from dimuon September 17, 2024 16:22
Copy link
Contributor

@dimuon dimuon left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@gigerdo gigerdo merged commit c155361 into elastic:master Sep 19, 2024
2 of 3 checks passed
@gigerdo gigerdo deleted the implement-organization-management branch September 19, 2024 13:15
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