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 support for machine_selector_files #1225

Merged
merged 1 commit into from Oct 9, 2023

Conversation

jiaqiluo
Copy link
Member

@jiaqiluo jiaqiluo commented Sep 12, 2023

Issue:

#1194

Problem

machine_selector_files can be used, combined with the machine_selector_config, to achieve the goal of configuring the API audit logging on REK2/K3s clusters.

The support for setting machine_selector_files is missing on the TF side.

Solution

This PR adds support for the machine_selector_files filed on the Cluster_v2 resource.

Testing

Engineering Testing

Manual Testing

Below are the TF files I used for testing the new fields

Details

provider "rancher2" {
  api_url   = var.rancher_api_url
  token_key = var.rancher_admin_bearer_token
  insecure  = true
}

# Create amazonec2 cloud credential
resource "rancher2_cloud_credential" "foo" {
  name = "foo"
  amazonec2_credential_config {
    access_key = var.aws_access_key
    secret_key = var.aws_secret_key
  }
}

# Create amazonec2 machine config v2
resource "rancher2_machine_config_v2" "foo" {
  generate_name = "jiaqi-machine"
  amazonec2_config {
    ami            = var.aws_ami
    region         = var.aws_region
    security_group = [var.aws_security_group_name]
    subnet_id      = var.aws_subnet_id
    vpc_id         = var.aws_vpc_id
    zone           = var.aws_zone_letter
    root_size      = var.aws_root_size
  }
}

resource "rancher2_secret_v2" "foo" {
  cluster_id = "local"
  name = "config-file-1"
  namespace = "fleet-default"
  data = {
    audit-policy = "testing file for machine selector files \n"
  }
  annotations = {
    "rke.cattle.io/object-authorized-for-clusters" = "rke2-1"
  }
}


# Create a new rancher v2 amazonec2 RKE2 Cluster v2
resource "rancher2_cluster_v2" "jiaqi-rke2" {
  name = var.rke2_cluster_name
  kubernetes_version = "v1.25.13+rke2r1"
  enable_network_policy = false
  default_cluster_role_for_project_members = "user"
  rke_config {
    machine_pools {
      name = "pool1"
      cloud_credential_secret_name = rancher2_cloud_credential.foo.id
      control_plane_role = true
      etcd_role = true
      worker_role = true
      quantity = 1
      machine_config {
        kind = rancher2_machine_config_v2.foo.kind
        name = rancher2_machine_config_v2.foo.name
      }
    }
    machine_selector_files {
      machine_label_selector {
        match_labels = {
          "rke.cattle.io/control-plane-role" = "true"
        }
      }
      file_sources {
        secret {
          name = "config-file-1"
          default_permissions = "644"
          items {
            key = "audit-policy"
            path ="/etc/rancher/rke2/custom/test-policy.yaml"
            permissions = "666"
          }
        }
      }
    }
  }
}

Automated Testing

The existing tests are updated.

QA Testing Considerations

  • Provision a new cluster with utilizing machine_selector_files
  • Upgrade an ending cluster from not utilizing to utilizing machine_selector_files

Regressions Considerations

Cluster provisioning or upgrading fails

@jiaqiluo jiaqiluo force-pushed the machine-selector-files branch 3 times, most recently from 0741412 to 1ac9ecb Compare September 13, 2023 22:37
@jiaqiluo jiaqiluo marked this pull request as ready for review September 13, 2023 23:15
@jiaqiluo jiaqiluo requested review from a-blender and a team September 13, 2023 23:15
docs/resources/cluster_v2.md Outdated Show resolved Hide resolved
docs/resources/cluster_v2.md Outdated Show resolved Hide resolved
docs/resources/cluster_v2.md Outdated Show resolved Hide resolved
rancher2/schema_cluster_v2_rke_config_files.go Outdated Show resolved Hide resolved
rancher2/structure_cluster_v2_rke_config_system_config.go Outdated Show resolved Hide resolved
rancher2/structure_cluster_v2_rke_config_system_config.go Outdated Show resolved Hide resolved
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.

Lgtm long as all the tests pass

docs/resources/cluster_v2.md Outdated Show resolved Hide resolved
@a-blender a-blender changed the title add the support for machine_selector_files Add support for machine_selector_files Sep 28, 2023
@snasovich snasovich requested a review from a team September 29, 2023 14:36
Comment on lines +115 to +123
"machine_selector_files": {
Type: schema.TypeList,
Optional: true,
Computed: true,
Description: "Cluster V2 machine selector files",
Elem: &schema.Resource{
Schema: clusterV2RKEConfigMachineSelectorFilesFields(),
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

As we are adding new fields do we need to add them for both schema and v0 schema, ain't we only supposed to add breaking changes and its migration logic?

Honestly Idk but it doesn't seems right.

@a-blender

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a grey area in this repo. I agree that, technically, we should not modify the old v0 schema, but in practice, I have seen multiple PRs that make changes to it. (example1 example2 ).

This makes me think that it is expected, or our practice, to add new optional fields to both the v0 and v1 schema.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe if we modify a field, we only change the type in the new v1 (or vx) schema and add migration logic so the TF provider knows what to do on upgrade of the provider. Correct, new fields should be added to all schema versions.

@jiaqiluo jiaqiluo merged commit e28bd24 into rancher:master Oct 9, 2023
1 check passed
@jiaqiluo jiaqiluo deleted the machine-selector-files branch October 9, 2023 17:13
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

3 participants