azurerm_redhat_openshift_cluster - support for the preconfigured_network_security_group_enabled property

[scottd018]

Community Note

• Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
• Please do not leave "+1" or "me too" comments, they generate extra noise for PR followers and do not help prioritize for review

Description

This adds support for using a preconfigured network security group for Azure Red Hat OpenShift clusters. This brings parity with the Azure CLI and the recently released BYO-NSG feature in ARO and allows users to consume this feature. See https://learn.microsoft.com/en-us/azure/openshift/howto-bring-nsg for more details.

PR Checklist

• I have followed the guidelines in our Contributing Documentation.
• I have checked to ensure there aren't other open Pull Requests for the same update/change.
• I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
• I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
• I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
  For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

• I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
• I have written new tests for my resource or datasource changes & updated any relevent documentation.
• I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
• (For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

• My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Prior to Change

Prior to this change, the automation would exit with an error similar to the following, indicating that the enabled-preconfigured-nsg flag was not passed to tell the ARO resource provider that the BYO-NSG workflow needs to be enabled:

        API Response:
        
        ----[start]----
        {
            "id": "/subscriptions/xxxx/providers/microsoft.redhatopenshift/locations/eastus/operationsstatus/7738694c-c0f3-4008-87a8-316ea61d78d5",
            "name": "7738694c-c0f3-4008-87a8-316ea61d78d5",
            "status": "Failed",
            "startTime": "2024-06-03T13:58:11.433094028Z",
            "endTime": "2024-06-03T13:58:12.839247755Z",
            "error": {
                "code": "InvalidLinkedVNet",
                "message": "The provided subnet '/subscriptions/xxxx/resourceGroups/test-rg/providers/Microsoft.Network/virtualNetworks/test-vnet/subnets/main-subnet' is invalid: must not have a network security group attached.",
                "target": "properties.masterProfile.subnetId"
            }
        }
        
        -----[end]-----

After Change

After the change, the test that was added in this PR named TestAccOpenShiftCluster_preconfiguredNetworkSecurityGroup successfully passes. This test adds in the creation of a network security group, linking it to the subnets, passing in the preconfigured_network_security_group_enabled flag, and setting the appropriate permissions as required by the ARO RP:

make acctests SERVICE='redhatopenshift' TESTARGS='-run=TestAccOpenShiftCluster_preconfiguredNetworkSecurityGroup' TESTTIMEOUT='120m'
==> 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 -run=TestAccOpenShiftCluster_preconfiguredNetworkSecurityGroup -timeout 120m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccOpenShiftCluster_preconfiguredNetworkSecurityGroup
=== PAUSE TestAccOpenShiftCluster_preconfiguredNetworkSecurityGroup
=== CONT  TestAccOpenShiftCluster_preconfiguredNetworkSecurityGroup
--- PASS: TestAccOpenShiftCluster_preconfiguredNetworkSecurityGroup (3387.58s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/redhatopenshift       3389.881s

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

• azurerm_redhat_openshift_cluster - support for the preconfigured_network_security_group_enabled property [GH-{number}]

This is a (please select all that apply):

• Bug Fix
• New Feature (ie adding a service, resource, or data source)
• Enhancement
• Breaking Change

Related Issue(s)

Fixes #25059

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

[tombuildsstuff]

hey @scottd018

Thanks for this PR - taking a look through here this mostly LGTM, if we can update nsg -> network_security_group to match other instances across the Provider and the tests pass then this should be good to go 👍

Thanks!

[scottd018]

Sounds good. Thanks @tombuildsstuff ! That's exactly what I was after was feedback while BYO-NSG was broken so I can fix it, and then test it once the resource provider is fixed. Will make the requested changes in the meantime and I can post test results once I've had a chance to test.

Thanks again!

[scottd018]

@tombuildsstuff Resolved your suggestion and used the terrafmt tool to format a test that was improperly formatted, causing the pipeline run to fail. Also updated the PR to support the new naming of the property.

Will wait for the upstream ARO RP to be fixed with NSG, run the tests, and inject the output into this PR.

[tombuildsstuff]

LGTM - thanks for making those changes @scottd018

Since we're still waiting on the RP changes to roll out, I'm going to mark this as blocked for the moment, but once that's fixed let us know and we can run the tests/proceed with this one 👍

[scottd018]

@tombuildsstuff ARO RP is now fixed. Tests have passed and the comment has been updated. I had to push a new commit, as I forgot to add the 'Network Contributor' permissions to the NSG that I created in the test, so once the pipeline for this repo pass we should be good (it looks like there may be some changes needed to re-run the pipeline). Let me know if anything else is needed from me. Thank you!

[scottd018]

@tombuildsstuff any updates on this? Just checking. Thank you!

[WadeBee-NG]

@tombuildsstuff Hi Tom, our team has been waiting for the fix. Any ETA on the PR getting merged?

[stephybun]

Tests look good, thanks @scottd018 LGTM 👍

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.