New Resource: `azurerm_redhat_openshift_cluster` #24375

teowa · 2024-01-03T05:56:03Z

supersede #20266

Reference:

Notes:

For internal server error mentioned in previous PR, it is caused by deletion of Azure Red Hat OpenShift RP app. And resource created in acctest cannot be deleted if the RP app is deleted. From service team, the deletion of RP app is not expected. The app is created when registering Microsoft.RedHatOpenShift resource provider for subscription. To recover the deleted app, re-register the Microsoft.RedHatOpenShift resource provider. In Terraform config, we should NOT use resource for the azuread_service_principal, data source should be used.

   // This is the RedHatOpenShift service principal id
- resource "azuread_service_principal" "redhatopenshift" {
-   application_id = "f1dd0a37-89c6-4e07-bcd1-ffd3d43d8875"
-   use_existing   = true
- }
+ data "azuread_service_principal" "redhatopenshift" {
+  application_id = "f1dd0a37-89c6-4e07-bcd1-ffd3d43d8875"
+ }

For PUT-GET inconsistence issue for workerProfile mentioned in New Resource: azurerm_redhat_openshift_cluster #20266 (review), this is fixed in 2023-09-04 version of API. A new workerProfilesStatus field is added to display seperated workers.
Updated to use hashicorp/go-azure-sdk and typed sdk.

Test:

ARM_TEST_LOCATION="westus" make acctests SERVICE="redhatopenshift" TESTARGS="-parallel 20 -run TestAccOpenShiftCluster_" TESTTIMEOUT='2h'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/redhatopenshift -parallel 20 -run TestAccOpenShiftCluster_ -timeout 2h -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccOpenShiftCluster_basic
=== PAUSE TestAccOpenShiftCluster_basic
=== RUN   TestAccOpenShiftCluster_update
=== PAUSE TestAccOpenShiftCluster_update
=== RUN   TestAccOpenShiftCluster_private
=== PAUSE TestAccOpenShiftCluster_private
=== RUN   TestAccOpenShiftCluster_userDefinedRouting
=== PAUSE TestAccOpenShiftCluster_userDefinedRouting
=== RUN   TestAccOpenShiftCluster_encryptionAtHost
=== PAUSE TestAccOpenShiftCluster_encryptionAtHost
=== RUN   TestAccOpenShiftCluster_basicWithFipsEnabled
=== PAUSE TestAccOpenShiftCluster_basicWithFipsEnabled
=== RUN   TestAccOpenShiftCluster_requiresImport
=== PAUSE TestAccOpenShiftCluster_requiresImport
=== CONT  TestAccOpenShiftCluster_basic
=== CONT  TestAccOpenShiftCluster_requiresImport
=== CONT  TestAccOpenShiftCluster_encryptionAtHost
=== CONT  TestAccOpenShiftCluster_update
=== CONT  TestAccOpenShiftCluster_private
=== CONT  TestAccOpenShiftCluster_userDefinedRouting
=== CONT  TestAccOpenShiftCluster_basicWithFipsEnabled
--- PASS: TestAccOpenShiftCluster_basic (3348.99s)
--- PASS: TestAccOpenShiftCluster_requiresImport (3701.20s)
--- PASS: TestAccOpenShiftCluster_userDefinedRouting (3759.67s)
--- PASS: TestAccOpenShiftCluster_encryptionAtHost (3850.19s)
--- PASS: TestAccOpenShiftCluster_private (3862.10s)
--- PASS: TestAccOpenShiftCluster_basicWithFipsEnabled (3874.02s)
--- PASS: TestAccOpenShiftCluster_update (4961.78s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/redhatopenshift       4961.804s

…profile

katbyte · 2024-01-08T03:53:49Z

@teowa - have they resolved the general issue of the service always returning an uninformative "internal server error" that doesn't indicate what the problem is?

teowa · 2024-01-08T07:43:55Z

@katbyte The internal server error still exists for now, if the RedHatOpenShift Service Principal is deleted. From service team, the SP is not expected to be deleted and it is created during RP registration.

ms-zhenhua

Hey @teowa,

Thanks for this PR - I've taken a look through and left some comments inline. If we can fix those up, this should be good to go 👍

Thanks!

ms-zhenhua · 2024-01-08T08:16:45Z

internal/services/redhatopenshift/client/client.go

+}
+
+func NewClient(o *common.ClientOptions) (*Client, error) {
+	openshiftClustersClient, err := openshiftclusters.NewOpenShiftClustersClientWithBaseURI(o.Environment.ResourceManager)


Suggested change

openshiftClustersClient, err := openshiftclusters.NewOpenShiftClustersClientWithBaseURI(o.Environment.ResourceManager)

openShiftClustersClient, err := openshiftclusters.NewOpenShiftClustersClientWithBaseURI(o.Environment.ResourceManager)

ms-zhenhua · 2024-01-08T08:19:00Z

internal/services/redhatopenshift/redhat_openshift_cluster_resource.go

+
+type WorkerProfile struct {
+	VmSize                  string `tfschema:"vm_size"`
+	DiskSizeGb              int64  `tfschema:"disk_size_gb"`


Suggested change

DiskSizeGb int64 `tfschema:"disk_size_gb"`

DiskSizeGb int `tfschema:"disk_size_gb"`

(just passing through) but FWIW these should be int64 and not int - since we're expecting int64's everywhere else (SDK etc) even if that's downcast to a smaller variable size

ms-zhenhua · 2024-01-08T08:19:10Z

internal/services/redhatopenshift/redhat_openshift_cluster_resource.go

+type WorkerProfile struct {
+	VmSize                  string `tfschema:"vm_size"`
+	DiskSizeGb              int64  `tfschema:"disk_size_gb"`
+	NodeCount               int64  `tfschema:"node_count"`


Suggested change

NodeCount int64 `tfschema:"node_count"`

NodeCount int `tfschema:"node_count"`

ms-zhenhua · 2024-01-08T08:32:32Z

internal/services/redhatopenshift/redhat_openshift_cluster_resource.go

+						ForceNew: true,
+						Default:  false,
+					},
+					"pull_secret": {


should pull_secret be a sensitive value?

yes, this is sensitive

ms-zhenhua · 2024-01-08T08:45:51Z

internal/services/redhatopenshift/redhat_openshift_cluster_resource.go

+
+			parameters := openshiftclusters.OpenShiftCluster{
+				Name:     pointer.To(id.OpenShiftClusterName),
+				Location: azure.NormalizeLocation(config.Location),


Suggested change

Location: azure.NormalizeLocation(config.Location),

Location: location.Normalize(config.Location),

ms-zhenhua · 2024-01-09T02:40:06Z