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

api: Define OperatorsAuthentication model #950

Merged
merged 1 commit into from
Dec 10, 2024
Merged

Conversation

mbarnes
Copy link
Collaborator

@mbarnes mbarnes commented Dec 9, 2024

What this PR does

Adds PlatformProfile.operatorsAuthentication field to the Azure API, structured to resemble the azure.operators_authentication OCM schema (with minor tweaks to eliminate redundancy in field names, and simplifications where data is provided through ARM-defined models or headers)

Jira: ARO-10911 - Frontend API Changes to support managed identities
Link to demo recording:

Special notes for your reviewer

  • The main file to focus on is hcpCluster-models.tsp. The rest of the changes are generated from it.
  • There is no plumbing to Cluster Service yet. I want to get initial sign-off on the API changes first.
  • Visibility flags may need to be tweaked. Obviously the operator arrays should be replaceable for OpenShift versions beyond the initial install, but I'm not sure what other fields we should allow to be patched.

/** The control plane operator name */
operatorName: string;

...UserAssignedManagedIdentity;
Copy link
Contributor

Choose a reason for hiding this comment

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

the client-id and principal-id will be duplicated in both the identity{} field and this field if we choose to specify it here.

properties: {
    "managedIdentityProfile": {
        "controlPlaneOperators": [
             {
             "operatorName": "csi-disk-driver", 
             "resourceId": "/subscriptions/.../Microsoft.Identity/managedIdentity/csi-disk-driver"
             "principalId": "csi-disk-driver",
             "clientId": "csi-disk-driver"
             }, 
             {...}
         ]
    }   
},
identity: {
    "/subscriptions/.../Microsoft.Identity/managedIdentity/csi-disk-driver": {
        "principalId": "csi-disk-driver",
        "clientId": "csi-disk-driver"
    }, 
    "/subscriptions/.../csi-file-driver": {
        "principalId": "csi-file-driver",
        "clientId": "csi-file-driver"
    }
}

Copy link
Collaborator Author

@mbarnes mbarnes Dec 10, 2024

Choose a reason for hiding this comment

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

Ah, I had no idea what the top-level identity section would contain. In that case we can have both data and control plane operators be a map of operatorName -> resourceId where resourceId is a lookup key for the identity section (validation would fail if key is not found).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, maybe the serviceManagedIdentity field just needs to be a resource ID lookup key as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just to call out here, the RP does not currently parse the top-level identity section at all so we'll need to get that implemented immediately in addition to the plumbing needed for this new API.

@miguelsorianod
Copy link
Collaborator

I see some of the attributes have removed parts of the names compared to in the CS API. In #858 (comment) I described and reasoned why in the CS API we intentionally kept parts of the names that initially might seem "redundant"

@mbarnes
Copy link
Collaborator Author

mbarnes commented Dec 10, 2024

I described and reasoned why in the CS API we intentionally kept parts of the names that initially might seem "redundant"

I'm neutral on naming and will change it based on consensus, but FTR I felt the purpose of the operator maps were clear enough from the enclosing scopes.

properties: {
  platform: {
    operatorsAuthentication: {
      managedIdentities: {
        controlPlaneOperators: {...}
        dataPlaneOperators: {...}
      }
    }
  }
}

I'm considering renaming the managedIdentities section to userAssignedIdentities for additional clarity.

Copy link

Please rebase pull request.

This is an extension to PlatformProfile that defines user-assigned
managed identities for individual OpenShift cluster operators.
@mbarnes mbarnes force-pushed the operators-authentication branch from 65cb766 to 46acea7 Compare December 10, 2024 16:36
@mbarnes mbarnes enabled auto-merge December 10, 2024 16:36
Copy link
Collaborator

@SudoBrendan SudoBrendan left a comment

Choose a reason for hiding this comment

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

I think this is clear on the various kinds of auth required and what fields should be provided for each. We should probably get BU feedback on names just to be sure, unless that's tracked somewhere I'm unaware of?

@mbarnes mbarnes merged commit e4f974f into main Dec 10, 2024
19 checks passed
@mbarnes mbarnes deleted the operators-authentication branch December 10, 2024 17:41
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.

4 participants