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

WI/MI CLI Phase 1 - Base Update Functionality #3709

Merged
merged 6 commits into from
Aug 13, 2024

Conversation

tsatam
Copy link
Collaborator

@tsatam tsatam commented Jul 19, 2024

Which issue this PR addresses:

Fixes ARO-6451

What this PR does / why we need it:

Implements the ability to set additional platform workload identities on a managed identity-enabled cluster via az aro update.

Test plan for issue:

  • Unit tests for validating the --assign-platform-workload-identity argument have been updated to account for update scenarios
  • az aro update against a MIWI cluster results in an API call being made to the RP with the necessary fields populated
    • Example invocation:

      az aro update \
        --subscription "${SUBSCRIPTION_ID}" \
        --resource-group "${CLUSTER_RESOURCE_GROUP}" \
        --name azcli-testcluster \
        --assign-platform-workload-identity bar qwert \
        --assign-platform-workload-identity baz zxcvb
    • Against cluster w/ document:

      Open
      {
          "id": "/subscriptions/${SUBSCRIPTION_ID}/resourceGroups/${CLUSTER_RESOURCE_GROUP}/providers/Microsoft.RedHatOpenShift/openShiftClusters/azcli-testcluster",
          "location": "eastus",
          "identity": {
              "type": "UserAssigned",
              "userAssignedIdentities": {
                  "/subscriptions/${SUBSCRIPTION_ID}/resourceGroups/${CLUSTER_RESOURCE_GROUP}/providers/Microsoft.ManagedIdentity/userAssignedIdentities/baz": {}
              }
          },
          "properties": {
              "clusterProfile": {
                  "pullSecret": "",
                  "domain": "fv0w9cvw",
                  "version": "4.14.16",
                  "resourceGroupId": "/subscriptions/${SUBSCRIPTION_ID}/resourceGroups/aro-fv0w9cvw",
                  "fipsValidatedModules": "Disabled"
              },
              "platformWorkloadIdentityProfile": {
                  "platformWorkloadIdentities": [
                      {
                          "operatorName": "foo",
                          "resourceId": "/subscriptions/${SUBSCRIPTION_ID}/resourceGroups/${CLUSTER_RESOURCE_GROUP}/providers/Microsoft.ManagedIdentity/userAssignedIdentities/asdf"
                      },
                      {
                          "operatorName": "bar",
                          "resourceId": "/subscriptions/${SUBSCRIPTION_ID}/resourceGroups/${CLUSTER_RESOURCE_GROUP}/providers/Microsoft.ManagedIdentity/userAssignedIdentities/ghjkl"
                      }
                  ]
              },
              "networkProfile": {
                  "podCidr": "10.128.0.0/14",
                  "serviceCidr": "172.30.0.0/16",
                  "outboundType": "",
                  "preconfiguredNSG": "Disabled"
              },
              "masterProfile": {
                  "vmSize": "Standard_D8s_v3",
                  "subnetId": "/subscriptions/${SUBSCRIPTION_ID}/resourceGroups/${CLUSTER_RESOURCE_GROUP}/providers/Microsoft.Network/virtualNetworks/aro-vnet/subnets/master-subnet",
                  "encryptionAtHost": "Disabled"
              },
              "workerProfiles": [
                  {
                      "name": "worker",
                      "vmSize": "Standard_D2s_v3",
                      "diskSizeGB": 128,
                      "subnetId": "/subscriptions/${SUBSCRIPTION_ID}/resourceGroups/${CLUSTER_RESOURCE_GROUP}/providers/Microsoft.Network/virtualNetworks/aro-vnet/subnets/worker-subnet",
                      "count": 3,
                      "encryptionAtHost": "Disabled"
                  }
              ],
              "apiserverProfile": {
                  "visibility": "Public"
              },
              "ingressProfiles": [
                  {
                      "name": "default",
                      "visibility": "Public"
                  }
              ]
          }
      }
    • Produces request body:

      Open
      {
          "properties": {
              "platformWorkloadIdentityProfile": {
                  "platformWorkloadIdentities": [
                      {
                          "operatorName": "foo",
                          "resourceId": "/subscriptions/${SUBSCRIPTION_ID}/resourceGroups/${CLUSTER_RESOURCE_GROUP}/providers/Microsoft.ManagedIdentity/userAssignedIdentities/asdf"
                      },
                      {
                          "operatorName": "bar",
                          "resourceId": "/subscriptions/${SUBSCRIPTION_ID}/resourceGroups/${CLUSTER_RESOURCE_GROUP}/providers/Microsoft.ManagedIdentity/userAssignedIdentities/qwert"
                      },
                      {
                          "operatorName": "baz",
                          "resourceId": "/subscriptions/${SUBSCRIPTION_ID}/resourceGroups/${CLUSTER_RESOURCE_GROUP}/providers/Microsoft.ManagedIdentity/userAssignedIdentities/zxcvb"
                      }
                  ]
              }
          }
      }

Note

The current implementation of this command allows users to replace existing platform workload identities.
We have outlined in our CLI design doc that this behavior is not supported at this time but may be in the future. We can choose to either explicitly prevent updating existing identities within the CLI right now, or to make that API-side validation only, that can be relaxed in the future without pushing a corresponding CLI change.

Is there any documentation that needs to be updated for this PR?

Not yet

How do you know this will function as expected in production?

Preview extension-only change. We will perform comprehensive testing before releasing this extension to users.

@tsatam tsatam added hold Hold loki chainsaw Pull requests or issues owned by Team Chainsaw labels Jul 19, 2024
Copy link

Please rebase pull request.

@github-actions github-actions bot added the needs-rebase branch needs a rebase label Jul 23, 2024
@tsatam tsatam force-pushed the tsatam/ARO-6451-wimi-cli-p1-base-update-functionality branch from 9e978e8 to 65c8954 Compare July 23, 2024 16:03
@github-actions github-actions bot removed the needs-rebase branch needs a rebase label Jul 23, 2024
bitoku
bitoku previously approved these changes Jul 24, 2024
python/az/aro/azext_aro/custom.py Outdated Show resolved Hide resolved
@bitoku
Copy link
Collaborator

bitoku commented Jul 24, 2024

Sorry for the silly question, but why don't we support changing MI when updating?

@tsatam
Copy link
Collaborator Author

tsatam commented Jul 24, 2024

Sorry for the silly question, but why don't we support changing MI when updating?

The RP backend is not planning to support that ability right away, (to my understanding) it's more complex than just changing coordinates. Rather than building in something to the CLI that would prevent that, just to remove it later once the RP does support it, I am proposing we just allow the CLI to attempt to replace an identity and have the API reject the request with a user-facing error, the resulting user experience will be largely the same (potentially a few milliseconds slower).

@bitoku
Copy link
Collaborator

bitoku commented Jul 24, 2024

I understand that changing MI is not in the scope as well as changing WI.
I agree with the idea to handle change request in the RP side.

@SudoBrendan
Copy link
Collaborator

/azp run ci

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@kimorris27 kimorris27 left a comment

Choose a reason for hiding this comment

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

LGTM overall, but I think I found a semantic error we should fix.

Also, I agree that the logic preventing updates to existing MIs can go in the RP, and we have a ticket to track this issue: https://issues.redhat.com/browse/ARO-9382

python/az/aro/azext_aro/custom.py Show resolved Hide resolved
@tsatam
Copy link
Collaborator Author

tsatam commented Jul 29, 2024

/azp run ci

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Collaborator

@cadenmarchese cadenmarchese left a comment

Choose a reason for hiding this comment

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

lgtm!

@tsatam tsatam merged commit 8bcd1b6 into master Aug 13, 2024
20 checks passed
gouthamMN pushed a commit that referenced this pull request Aug 14, 2024
* Add --assign-platform-wi flag to az aro update

* Fix nil pointer dereference when converting Identity structs on API

* Restructure update command to make branching logic more clear

* Remove duplicate test

* Disallow passing in duplicate platform workload identities within the same create/update invocation

* Ensure reported duplicate platform_workload_identities list doesn't itself contain duplicates
edisonLcardenas pushed a commit that referenced this pull request Aug 16, 2024
* Add --assign-platform-wi flag to az aro update

* Fix nil pointer dereference when converting Identity structs on API

* Restructure update command to make branching logic more clear

* Remove duplicate test

* Disallow passing in duplicate platform workload identities within the same create/update invocation

* Ensure reported duplicate platform_workload_identities list doesn't itself contain duplicates
yithian pushed a commit that referenced this pull request Aug 20, 2024
* Add --assign-platform-wi flag to az aro update

* Fix nil pointer dereference when converting Identity structs on API

* Restructure update command to make branching logic more clear

* Remove duplicate test

* Disallow passing in duplicate platform workload identities within the same create/update invocation

* Ensure reported duplicate platform_workload_identities list doesn't itself contain duplicates
rhamitarora pushed a commit that referenced this pull request Aug 21, 2024
* Add --assign-platform-wi flag to az aro update

* Fix nil pointer dereference when converting Identity structs on API

* Restructure update command to make branching logic more clear

* Remove duplicate test

* Disallow passing in duplicate platform workload identities within the same create/update invocation

* Ensure reported duplicate platform_workload_identities list doesn't itself contain duplicates
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chainsaw Pull requests or issues owned by Team Chainsaw loki ready-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants