Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5605 +/- ##
==========================================
+ Coverage 53.28% 54.68% +1.40%
==========================================
Files 272 279 +7
Lines 29537 31174 +1637
==========================================
+ Hits 15739 17049 +1310
- Misses 12983 13205 +222
- Partials 815 920 +105 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
22ad098 to
35b0abc
Compare
|
|
||
| // Resource group name where the Aro-hcp will be attached to it. | ||
| ResourceGroup string `json:"resourceGroup,omitempty"` | ||
|
|
There was a problem hiding this comment.
IMHO we will also need the:
tenant_id,subscription_id,managed_resource_group_name(not onlyresource_group_name)
to create a cluster, isn't it?
There was a problem hiding this comment.
yes, those info are provided at the azureclusteridentity
| // PlatformProfile represents the Azure platform configuration. | ||
| type PlatformProfile struct { | ||
| // Azure subnet id | ||
| Subnet string `json:"subnet,omitempty"` |
There was a problem hiding this comment.
Why Subnet & SubnetRef ?
subnet id or cluster id or node pool id ? I'm not sure base on: https://github.com/Azure/ARO-HCP/blob/main/cluster-service/cluster-creation.md#creating-node-pools
Replicas & autorepare will be used from capi MachinePool?
There was a problem hiding this comment.
subnetRef will be referring to VirtualNetworksSubnet CR by name.
yes, replicas defined at CAPI machinePool CR
docs/proposals/20250425-aro-hcp.md
Outdated
| ControlPlaneOperators map[string]string `json:"controlPlaneOperators,omitempty"` | ||
|
|
||
|
|
||
| // DataPlaneOperators ref to Microsoft.ManagedIdentity/userAssignedIdentities | ||
| DataPlaneOperators map[string]string `json:"dataPlaneOperators,omitempty"` | ||
|
|
||
|
|
||
| // ServiceManagedIdentity ref to Microsoft.ManagedIdentity/userAssignedIdentities | ||
| ServiceManagedIdentity string `json:"serviceManagedIdentity,omitempty"` |
There was a problem hiding this comment.
What are the keys in those maps?
What about to replace maps with structures for it?
There was a problem hiding this comment.
I updated the managed Identities to include all the required identities names
| ServiceManagedIdentity string `json:"serviceManagedIdentity,omitempty"` | ||
| } | ||
|
|
||
| type ControlPlaneOperators struct { |
There was a problem hiding this comment.
Are we not planning to reuse the HyperShift API for this, rather than duplicating API efforts?
There was a problem hiding this comment.
no, we will re-use the Hypershift API. The naming for CRD fields is a bit different but internally with API call will set the naming as here
| KmsManagedIdentities string `json:"kmsManagedIdentities,omitempty"` | ||
| } | ||
|
|
||
| type DataPlaneOperators struct { |
There was a problem hiding this comment.
Similar question as above, re: reusing HyperShift API
| ServiceManagedIdentity string `json:"serviceManagedIdentity,omitempty"` | ||
| } | ||
|
|
||
| type ControlPlaneOperators struct { |
There was a problem hiding this comment.
Should we also list the exact role definitions each one of these will get?
There was a problem hiding this comment.
not sure if I understand, The field desc mentioning the role definition "control-plane" and similar for every UAMI
There was a problem hiding this comment.
Every control plane and data plane are assigned a specific Azure role definition(s) - CNTRLPLANE-171.
For example, the ingress managed identity is only assigned the "Azure Red Hat OpenShift Cluster Ingress Operator" role over the resource groups containing the VNET and the DNS Zone.
There was a problem hiding this comment.
okay, that is fine we will use the same role naming and keys. Would you mention the ref for it from the Hypershift repo . I don't think the Jira url above is accessible for everyone.
There was a problem hiding this comment.
The mapping of control plane to role definition to resource group does not reside from the HyperShift repo. I am not sure if it is available in some public fashion on the ARO side 🤔
There was a problem hiding this comment.
There was a problem hiding this comment.
FWIW, we do not set the role definitions in HyperShift. That is assumed to be done prior to creating the HostedCluster.
The HyperShift link you mentioned is related to the HyperShift CLI, which does set the role definitions but that is only for development and testing use.
There was a problem hiding this comment.
hmm, so we need to have this defined some where yes/no ?
Also this is more like implementation details for the UserAssignedIdentity CR that will be created we will need to set the right spec info
There was a problem hiding this comment.
I think it would be nice to be defined somewhere.
Maybe it goes with the UserAssignedIdentity CR; is that defined somewhere?
|
/assign |
docs/proposals/20250425-aro-hcp.md
Outdated
| ControlPlaneManagedIdentities string `json:"controlPlaneOperatorsManagedIdentities,omitempty"` | ||
|
|
||
| // ClusterApiAzureManagedIdentities "cluster-api-azure" Microsoft.ManagedIdentity/userAssignedIdentities | ||
| ClusterApiAzureManagedIdentities string `json:clusterApiAzureManagedIdentities",omitempty"` |
There was a problem hiding this comment.
sometimes is here badly formatted json option:
json:clusterApiAzureManagedIdentities",omitempty" -> json:"clusterApiAzureManagedIdentities,omitempty"
The " should be moved after json: . There is such a typo multiple times.
marek-veber
left a comment
There was a problem hiding this comment.
identityRef - need to be *corev1.ObjectReference in go
and in yaml:
identityRef:
kind: AzureClusterIdentity
name: aro-identity
namespace: default
docs/proposals/20250425-aro-hcp.md
Outdated
|
|
||
| // IdentityRef is a reference to an identity to be used when reconciling the aro control plane. | ||
| // If no identity is specified, the default identity for this controller will be used. | ||
| IdentityRef *infrav1.AzureClusterIdentity `json:"identityRef,omitempty"` |
There was a problem hiding this comment.
| IdentityRef *infrav1.AzureClusterIdentity `json:"identityRef,omitempty"` | |
| IdentityRef *corev1.ObjectReference `json:"identityRef,omitempty"` |
| identityRef: | ||
| name: aro-identity |
There was a problem hiding this comment.
| identityRef: | |
| name: aro-identity | |
| identityRef: | |
| kind: AzureClusterIdentity | |
| name: aro-identity | |
| namespace: default |
Signed-off-by: serngawy <serngawy@gmail.com>
|
Is this ready for final review? @serngawy |
yes |
| CloudNetworkConfigManagedIdentities string `json:"cloudNetworkConfigManagedIdentities,omitempty"` | ||
|
|
||
| // KmsManagedIdentities "kms" Microsoft.ManagedIdentity/userAssignedIdentities | ||
| KmsManagedIdentities string `json:"kmsManagedIdentities,omitempty"` |
There was a problem hiding this comment.
This is not possible to use with openshift 4.18.1
There was a problem hiding this comment.
Correct. ARO HCP is only supported for OpenShift 4.19+
|
/lgtm |
|
@marek-veber: changing LGTM is restricted to collaborators DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/assign @jackfrancis |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
/reopen |
|
@willie-yao: Reopened this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/remove-lifecycle rotten |
What type of PR is this?
/kind documentation
What this PR does / why we need it:
Proposal for ARO-HCP feature
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
TODOs:
Release note: