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

Add inherited_cluster_roles attribute to global_role #1242

Merged

Conversation

pmatseykanets
Copy link
Contributor

@pmatseykanets pmatseykanets commented Oct 4, 2023

Issue:

rancher/rancher#42967

Problem

inheritedClusterRoles is a new field in global roles. We should be able to set it using rancher terraform provider when creating/updating a global role resource.

Solution

A new attribute inherited_cluster_roles was added to the global role.

Testing

Check that inherited_cluster_roles attribute is correctly populated when a global role is created/updated.

Engineering Testing

Manual Testing

Used a local build of the rancher terraform provider against a local k8s cluster v1.27.4+k3s1 running rancher v2.8 HEAD (c7af81ec9c55b) to create and update a global role with inherited cluster roles.

resource "rancher2_global_role" "test-global-role" {
    name = "test-global-role"
    description = "test-global-role"
    new_user_default = false
    rules {
      api_groups = ["*"]
      resources = ["secrets"]
      verbs = ["create"]
    }
    inherited_cluster_roles = ["cluster-owner"]
}
terraform plan -var-file rancher.tfvars -auto-approve
...
Terraform will perform the following actions:

  # rancher2_global_role.test-global-role will be created
  + resource "rancher2_global_role" "test-global-role" {
      + annotations             = (known after apply)
      + builtin                 = (known after apply)
      + description             = "test-global-role"
      + id                      = (known after apply)
      + inherited_cluster_roles = [
          + "cluster-owner",
        ]
      + labels                  = (known after apply)
      + name                    = "test-global-role"
      + new_user_default        = false

      + rules {
          + api_groups = [
              + "*",
            ]
          + resources  = [
              + "secrets",
            ]
          + verbs      = [
              + "create",
            ]
        }
    }

Plan: 1 to add, 0 to change, 0 to destroy.
rancher2_global_role.test-global-role: Creating...
rancher2_global_role.test-global-role: Creation complete after 4s [id=gr-xt2jb]

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.
k get globalrole gr-xt2jb -o yaml
apiVersion: management.cattle.io/v3
description: test-global-role
displayName: test-global-role
inheritedClusterRoles:
- cluster-owner
kind: GlobalRole
...

Automated Testing

Unit tests updated to cover the new attribute.

QA Testing Considerations

Regressions Considerations

@pmatseykanets pmatseykanets self-assigned this Oct 4, 2023
go.mod Show resolved Hide resolved
Copy link

@MbolotSuse MbolotSuse left a comment

Choose a reason for hiding this comment

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

Couple of small issues, and I think we also need to update the docs/resources/role_template.md file as well.

rancher2/resource_rancher2_global_role.go Outdated Show resolved Hide resolved
rancher2/schema_global_role.go Outdated Show resolved Hide resolved
@pmatseykanets pmatseykanets marked this pull request as ready for review October 6, 2023 16:03
Copy link

@crobby crobby left a comment

Choose a reason for hiding this comment

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

Aside from the outstanding question on versioning for the rancher bits, I think this looks good.

Copy link
Contributor

@eliyamlevy eliyamlevy left a comment

Choose a reason for hiding this comment

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

LGTM other than rancher version bits as well. Not sure how we handle versioning in this repo.

Copy link
Contributor

@thatmidwesterncoder thatmidwesterncoder left a comment

Choose a reason for hiding this comment

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

echoing LGTM but not sure about the version bumps.

Copy link
Contributor

@a-blender a-blender left a comment

Choose a reason for hiding this comment

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

@pmatseykanets Rancher version bump lgtm, on second thought I think it's the safest option to have TF reference the rancher/rancher commit where the backend for InheritedGlobalRoles was merged in. Can't be 100% sure the provider supports the later commits yet. Good to merge

@a-blender a-blender merged commit e7f4fa8 into rancher:master Oct 18, 2023
1 check passed
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.

None yet

6 participants