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

azurerm_redhat_openshift_cluster - support for custom managed_resource_group_name #25529

Merged
merged 6 commits into from
Jun 21, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,12 @@ type ServicePrincipal struct {
}

type ClusterProfile struct {
PullSecret string `tfschema:"pull_secret"`
Domain string `tfschema:"domain"`
ResourceGroupId string `tfschema:"resource_group_id"`
Version string `tfschema:"version"`
FipsEnabled bool `tfschema:"fips_enabled"`
PullSecret string `tfschema:"pull_secret"`
Domain string `tfschema:"domain"`
ResourceGroupName string `tfschema:"resource_group_name"`
ResourceGroupId string `tfschema:"resource_group_id"`
Version string `tfschema:"version"`
FipsEnabled bool `tfschema:"fips_enabled"`
}

type NetworkProfile struct {
Expand Down Expand Up @@ -135,6 +136,19 @@ func (r RedHatOpenShiftCluster) Arguments() map[string]*pluginsdk.Schema {
Sensitive: true,
ValidateFunc: validation.StringIsNotEmpty,
},
"resource_group_name": {
Copy link
Member

Choose a reason for hiding this comment

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

The name of this is quite confusing and clashes with the existing resource_group_id. Please give a more descriptive name for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resource_group_name is to set the resource group which will be created by Azure RedHat OpenShift RP to deploy dependency resources. I have confirmed with service team they won't support RG in another subscription so I only expose it as resource_group_name. And before we expose this resource_group_name property, the resource group name is default to aro-{domain} as portal does, where domain is another configurable property.

The resource_group_id is a computed property to display the above resource group ID.

Do we need change this to target_resource_group_name or dependency_resource_group_name? Or can we keep it similar format to resource_group_id?

Copy link
Member

Choose a reason for hiding this comment

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

Separate question, can ARO only deploy it's resources to a resource group that it creates? Or can it deploy it's resources to an already existing one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separate question, can ARO only deploy it's resources to a resource group that it creates? Or can it deploy it's resources to an already existing one?

From service team, it must be a resource group that it creates. In other word, the resource group name sent in this field must be a non-existing one.

Type: pluginsdk.TypeString,
Optional: true,
ForceNew: true,
ValidateFunc: validate.ClusterResourceGroupName,
DiffSuppressFunc: func(_, old, new string, d *pluginsdk.ResourceData) bool {
defaultResourceGroupName := fmt.Sprintf("aro-%s", d.Get("cluster_profile.0.domain").(string))
if old == defaultResourceGroupName && new == "" {
return true
}
return false
},
},
"resource_group_id": {
Type: pluginsdk.TypeString,
Computed: true,
Expand Down Expand Up @@ -479,7 +493,12 @@ func (r RedHatOpenShiftCluster) Read() sdk.ResourceFunc {
state.Tags = pointer.From(model.Tags)

if props := model.Properties; props != nil {
state.ClusterProfile = flattenOpenShiftClusterProfile(props.ClusterProfile, config)
clusterProfile, err := flattenOpenShiftClusterProfile(props.ClusterProfile, config)
if err != nil {
return fmt.Errorf("flatten cluster profile: %+v", err)
}
state.ClusterProfile = *clusterProfile

state.ServicePrincipal = flattenOpenShiftServicePrincipalProfile(props.ServicePrincipalProfile, config)
state.NetworkProfile = flattenOpenShiftNetworkProfile(props.NetworkProfile)
state.MainProfile = flattenOpenShiftMainProfile(props.MasterProfile)
Expand Down Expand Up @@ -533,20 +552,25 @@ func expandOpenshiftClusterProfile(input []ClusterProfile, subscriptionId string
fipsValidatedModules = openshiftclusters.FipsValidatedModulesEnabled
}

// the api needs a ResourceGroupId value and the portal doesn't allow you to set it but the portal returns the
// resource id being `aro-{domain}` so we'll follow that here.
resourceGroupId := commonids.NewResourceGroupID(subscriptionId, fmt.Sprintf("aro-%s", input[0].Domain)).ID()
if rg := input[0].ResourceGroupName; rg != "" {
resourceGroupId = commonids.NewResourceGroupID(subscriptionId, rg).ID()
}

return &openshiftclusters.ClusterProfile{
// the api needs a ResourceGroupId value and the portal doesn't allow you to set it but the portal returns the
// resource id being `aro-{domain}` so we'll follow that here.
ResourceGroupId: pointer.To(commonids.NewResourceGroupID(subscriptionId, fmt.Sprintf("aro-%s", input[0].Domain)).ID()),
ResourceGroupId: pointer.To(resourceGroupId),
Domain: pointer.To(input[0].Domain),
PullSecret: pointer.To(input[0].PullSecret),
FipsValidatedModules: pointer.To(fipsValidatedModules),
Version: pointer.To(input[0].Version),
}
}

func flattenOpenShiftClusterProfile(profile *openshiftclusters.ClusterProfile, config RedHatOpenShiftClusterModel) []ClusterProfile {
func flattenOpenShiftClusterProfile(profile *openshiftclusters.ClusterProfile, config RedHatOpenShiftClusterModel) (*[]ClusterProfile, error) {
if profile == nil {
return []ClusterProfile{}
return &[]ClusterProfile{}, nil
}

// pull secret isn't returned by the API so pass the existing value along
Expand All @@ -560,15 +584,28 @@ func flattenOpenShiftClusterProfile(profile *openshiftclusters.ClusterProfile, c
fipsEnabled = *profile.FipsValidatedModules == openshiftclusters.FipsValidatedModulesEnabled
}

return []ClusterProfile{
resourceGroupId, err := commonids.ParseResourceGroupIDInsensitively(*profile.ResourceGroupId)
teowa marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, fmt.Errorf("parsing resourceGroupId: %+v", err)
teowa marked this conversation as resolved.
Show resolved Hide resolved
}

resourceGroupIdString := ""
resourceGroupName := ""
if resourceGroupId != nil {
resourceGroupIdString = resourceGroupId.ID()
resourceGroupName = resourceGroupId.ResourceGroupName
}

return &[]ClusterProfile{
{
PullSecret: pullSecret,
Domain: pointer.From(profile.Domain),
FipsEnabled: fipsEnabled,
ResourceGroupId: pointer.From(profile.ResourceGroupId),
Version: pointer.From(profile.Version),
PullSecret: pullSecret,
Domain: pointer.From(profile.Domain),
FipsEnabled: fipsEnabled,
ResourceGroupId: resourceGroupIdString,
ResourceGroupName: resourceGroupName,
Version: pointer.From(profile.Version),
},
}
}, nil
}

func expandOpenshiftServicePrincipalProfile(input []ServicePrincipal) *openshiftclusters.ServicePrincipalProfile {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,21 @@ func TestAccOpenShiftCluster_requiresImport(t *testing.T) {
})
}

func TestAccOpenShiftCluster_basicResourceGroupName(t *testing.T) {
data := acceptance.BuildTestData(t, "azurerm_redhat_openshift_cluster", "test")
r := OpenShiftClusterResource{}

data.ResourceTest(t, r, []acceptance.TestStep{
{
Config: r.basicResourceGroupName(data),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
),
},
data.ImportStep("service_principal.0.client_secret"),
})
}

func (t OpenShiftClusterResource) Exists(ctx context.Context, clients *clients.Client, state *pluginsdk.InstanceState) (*bool, error) {
id, err := openshiftclusters.ParseProviderOpenShiftClusterID(state.ID)
if err != nil {
Expand Down Expand Up @@ -704,6 +719,59 @@ resource "azurerm_redhat_openshift_cluster" "test" {
`, r.template(data), data.RandomInteger, data.RandomString)
}

func (r OpenShiftClusterResource) basicResourceGroupName(data acceptance.TestData) string {
return fmt.Sprintf(`
%[1]s

resource "azurerm_redhat_openshift_cluster" "test" {
name = "acctestaro%[2]d"
location = azurerm_resource_group.test.location
resource_group_name = azurerm_resource_group.test.name

cluster_profile {
domain = "aro-%[3]s.com"
version = "4.13.23"
resource_group_name = "acctestrg-aro-infra-%[3]s"
}

network_profile {
pod_cidr = "10.128.0.0/14"
service_cidr = "172.30.0.0/16"
}

main_profile {
vm_size = "Standard_D8s_v3"
subnet_id = azurerm_subnet.main_subnet.id
}

api_server_profile {
visibility = "Public"
}

ingress_profile {
visibility = "Public"
}

worker_profile {
vm_size = "Standard_D4s_v3"
disk_size_gb = 128
node_count = 3
subnet_id = azurerm_subnet.worker_subnet.id
}

service_principal {
client_id = azuread_application.test.application_id
client_secret = azuread_service_principal_password.test.value
}

depends_on = [
"azurerm_role_assignment.role_network1",
"azurerm_role_assignment.role_network2",
]
}
`, r.template(data), data.RandomInteger, data.RandomString)
}

func (OpenShiftClusterResource) template(data acceptance.TestData) string {
return fmt.Sprintf(`
provider "azurerm" {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package validate

import (
"fmt"
"regexp"
"strings"
)

func ClusterResourceGroupName(v interface{}, k string) (warnings []string, errors []error) {
value := v.(string)

if len(value) > 90 {
errors = append(errors, fmt.Errorf("%q may not exceed 90 characters in length", k))
}

if strings.HasSuffix(value, ".") {
errors = append(errors, fmt.Errorf("%q may not end with a period", k))
}

if len(value) == 0 {
errors = append(errors, fmt.Errorf("%q cannot be blank", k))
} else if matched := regexp.MustCompile(`^[0-9a-z-._()]+$`).Match([]byte(value)); !matched {
// regex pulled from https://docs.microsoft.com/en-us/rest/api/resources/resourcegroups/createorupdate
// ARO only allow for lower cases https://github.com/Azure/ARO-RP/blob/e5c40654277c77fe78ba669610ac05774e448683/pkg/frontend/openshiftcluster_putorpatch.go#L189
errors = append(errors, fmt.Errorf("%q may only contain lowercase alpha characters, digit, dash, underscores, parentheses and periods", k))
}

return warnings, errors
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
package validate

import "testing"

func TestClusterResourceGroupName(t *testing.T) {
cases := []struct {
Input string
Valid bool
}{
{
Input: "",
Valid: false,
},

{
Input: "2",
Valid: true,
},

{
Input: "a.1",
Valid: true,
},

{
Input: "a-a",
Valid: true,
},

{
Input: "a.",
Valid: false,
},

{
Input: "2a-9",
Valid: true,
},

{
Input: "2a-9_",
Valid: true,
},

// upper case
{
Input: "2A",
Valid: false,
},

// 90 chars
{
Input: "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijkl",
Valid: true,
},

// 91 chars
{
Input: "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklm",
Valid: false,
},
}
for _, tc := range cases {
t.Logf("[DEBUG] Testing Value %s", tc.Input)
_, errors := ClusterResourceGroupName(tc.Input, "test")
valid := len(errors) == 0

if tc.Valid != valid {
t.Fatalf("Expected %t but got %t", tc.Valid, valid)
}
}
}
2 changes: 2 additions & 0 deletions website/docs/r/redhat_openshift_cluster.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,8 @@ A `cluster_profile` block supports the following:

* `fips_enabled` - (Optional) Whether Federal Information Processing Standard (FIPS) validated cryptographic modules are used. Defaults to `false`. Changing this forces a new resource to be created.

* `resource_group_name` - (Optional) The name of a Resource Group which will be created and used to deploy the infrastructure of Azure Red Hat OpenShift Cluster. The value should not contain uppercase characters. Defaults to `aro-{domain}`. Changing this forces a new resource to be created.

---

A `network_profile` block supports the following:
Expand Down
Loading