📖 Proposal for ROSA networking #5381

mzazrivec · 2025-03-10T14:23:46Z

What type of PR is this?

/kind documentation

What this PR does / why we need it:

This pull request contains a proposal for implementing provisioning of networking infrastructure for ROSA HCP clusters.

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:

Checklist:

Release note:

k8s-ci-robot · 2025-03-10T14:23:49Z

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

k8s-ci-robot · 2025-03-10T14:23:54Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign andidog for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2025-03-10T14:23:56Z

Hi @mzazrivec. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

serngawy

Please add more details for the proposed CRD RosaNetwork. The proposal mention with four attributes: name, AZ count, region and CIDR block for VPC , what is the spec and status fields ?

Is it namespace scope ? cluster scope ?
What is the status conditions ? ex; validate/phases/applied ?
As you are using cloud formation, how do you propagate the errors from cloud formation template to the CR status ?
ROSAControlPlane will refer to the RosaNetwork, What fields the RosaControlplane will read to figure out the subnet-id and zones
How do you will handle deleting the RosaNetwork while there is a RosaControlPlane refer to it ?

serngawy · 2025-03-10T17:27:17Z

docs/proposal/2025-02-24-rosa-networking.md

+
+1. As a CAPA user, I want to be able to provision the network infrastructure for ROSA HCP.
+2. As a CAPA user, I want to be able to use the provisioned network infrastructure in ROSA HCP control plane.
+3. As a CAPA user, I want to be able to modify the network inrastructure previously provisioned by CAPA.


I'm not sure if that possible, you may change tags or labels but I don't think we can change VPC attributes after creating it.

Yeah, I removed the stack modification part as it can get a bit tricky.

mzazrivec · 2025-03-19T13:27:36Z

Please add more details for the proposed CRD RosaNetwork. The proposal mention with four attributes: name, AZ count, region and CIDR block for VPC , what is the spec and status fields ?

Yes, I added more details regarding the spec / status part of the new custom resource.

Is it namespace scope ? cluster scope ?

Namespaced. Added to the doc.

What is the status conditions ? ex; validate/phases/applied ?

This is the part I'm not 100% sure about, but I'd probably prefer being consistent with what CAPA already uses, e.g. something like:

status:
  conditions:
    - lastTransitionTime: "2025-03-19T16:29:24Z"
      status: "True"
      type: RosaNetworkReady
      message:
      reason:

As you are using cloud formation, how do you propagate the errors from cloud formation template to the CR status ?

Through the status.resources array. Added to the doc.

ROSAControlPlane will refer to the RosaNetwork, What fields the RosaControlplane will read to figure out the subnet-id and zones

Added to the doc.

How do you will handle deleting the RosaNetwork while there is a RosaControlPlane refer to it ?

On one hand it would make sense to prevent RosaNetwork deletion should it be referenced from an existing control plane, but not quite sure how to implement that without using some type of labeling or something else that would allow CAPA to see whether or not is the particular RosaNetwork being referenced or not. In short, I don't want to be implementing any checks in the initial implementation.

serngawy · 2025-03-19T20:09:35Z

docs/proposal/2025-02-24-rosa-networking.md

+1. A new reconciler for the new custom resource, implementing creation and deletion. The reconciler will be using an existing [CloudFormation template from ROSA CLI](https://github.com/openshift/rosa/blob/master/cmd/create/network/templates/rosa-quickstart-default-vpc/cloudformation.yaml) and will use AWS CloudFormation API to create and delete the AWS CloudFormation stack.
+
+   Outputs and resources created in the cloudformation stack will be tracked under `status` of the `RosaNetwork` type. In particular, the `status` will have the following fields:
+   * publicSubnets: array of public subnet ids


make it one list of ;

availabilityZone
privateSubnet
publicSubnet

I don't understand the above. Do you want all the AZs, private and public subnets to be rolled into one array?

Changed the status to be an array of the subnets grouped by AZs.

serngawy · 2025-03-19T20:10:42Z

docs/proposal/2025-02-24-rosa-networking.md

+Adopting the CloudFormation template used by rosa-cli would mean that CAPA and the `RosaNetwork` custom resource would be relying on a mechanism that is know to work well and any changes or fixes implemented in ROSA CLI would be picked up automatically in CAPA.
+
+In practical terms, implementation of the proposal would mean:
+1. A new namespaced custom resource definition `RosaNetwork` in CAPA with four attributes: name, AZ count, region and CIRD block for VPC. These four attributes will become the `spec` part of the new `RosaNetwork` type.


please add the go struct OR CR example for the rosaNetwork.spec

serngawy · 2025-03-19T20:13:28Z

docs/proposal/2025-02-24-rosa-networking.md

+   ```
+   status:
+     resources:
+       - resource: NATGateway1


Is this will duplication for subnet-ids ? may we don't need the list above for (zone, pub-subnet, priv-subnet) ?

Yes, duplicated, as a convenience function.

serngawy · 2025-03-19T20:18:03Z

docs/proposal/2025-02-24-rosa-networking.md

+         status: CREATE_COMPLETE
+         reason:
+   ```
+   and will be reflecting the resource definitions from the CloudFormation template (`resource`) and the values coming from AWS CloudFormation API (`id`, `status` and `reason).


please add the go struct OR CR example for the rosaNetwork.status and include the suggested conditions

serngawy · 2025-03-21T17:21:20Z

docs/proposal/20250224-rosa-networking.md

+      name: rosa-network-01
+      namespace: default
+    spec:
+      availability_zone_count: 3


Is there a reason for not specifying the regions? which regions will be used if the count is 2 ?

Checking the template here can you clarify why is the max count is 3 ? some region like us-west-2 has 4 AZ.
Lets have the availabilityZone as list of strings similar to rosaControlPlane
It should be okay if we will need to change the template a bit to specify the zones

Is there a reason for not specifying the regions? which regions will be used if the count is 2 ?

It is possible to specify the region. It is one of the input parameters of the CF template.

I'm guessing you meant AZs here, not the regions. It's not possible to specify the AZs, because that's how the CF template is written. You just specify the number of AZs you want to be used and they are being taken in the alphabetical order.

In rosa cli you can also set only the number of AZs. So I think it should be fine to implement it similarly here. If user wants more control about network, can create network and pass it to RosaControlPlane.

Checking the template here can you clarify why is the max count is 3 ? some region like us-west-2 has 4 AZ.

Don't know why 3 is the limit. But that's how the current template is.

Lets have the availabilityZone as list of strings similar to rosaControlPlane It should be okay if we will need to change the template a bit to specify the zones

That would be beyond the scope of this proposal document and the initial implementation. It's also not just a matter of changing the CF template. This change would mean that the AZs are the spec of the CR, not the status of the CR. Or maybe even both the spec and status depending on whether you want to specify the AZ count or the AZ list.

It could potentially be a future enhancement, though I'm not entirely convinced that it would all that useful. In the multi-zone cluster installation you're probably more interested in the number of zones you're going to be using rather than their names, which is how the template is written now.

To let end user has the ability to set the AZs its not beyond the proposal scope.

As endUser using us-west-2 region it has 4 AZs, Why I cannot create subnets in all AZs (forced to 3 AZs) ? Is there a technical reason for that ?

As endUser us-east-2 region, I want to specify which AZ that I can create subnet to it ? Is there technical reason that I cannot specify AZ ? (The RosaControlPlane has the option to specify the AZs and subnets)

After testing, the CLI allows attempts to create (passing validation) HCP clusters with any number of AZs (one public subnet, 4 private on 4 different AZs worked. As well as 2 different AZs/private subnets, and 3)

There seems to be no official API validation for the number of subnets/AZs, meaning we can have 1 for private link, 2 for public, but any number more for either one. I was unable to find any restrictions besides at least one for private, at least 2 for public. HCP must be MultiAZ as well so this is always the case

To let end user has the ability to set the AZs its not beyond the proposal scope.

As endUser using us-west-2 region it has 4 AZs, Why I cannot create subnets in all AZs (forced to 3 AZs) ? Is there a technical reason for that ?

That particular problem / question is not really related to this proposal nor can I really address it here.

As endUser us-east-2 region, I want to specify which AZ that I can create subnet to it ? Is there technical reason that I cannot specify AZ ? (The RosaControlPlane has the option to specify the AZs and subnets)

Well, technical reason .... There's a technical limitation right now: the CF template in rosa-cli does not allow it / support it. Things may change on that front for sure and it may be an enhancement (meaning both in rosa-cli & CAPA) in the future, but right now it's not possible.

Regarding the ability to specify explicit AZs: I created openshift/rosa#2834 and adjusted my proposal doc accordingly.

serngawy · 2025-03-21T17:31:22Z

docs/proposal/20250224-rosa-networking.md

+    spec:
+      availability_zone_count: 3
+      region: us-west-2
+      cidr_block: 10.0.0.0/16


serngawy · 2025-03-21T17:37:02Z

docs/proposal/20250224-rosa-networking.md

+     name: rosa-network-01
+     namespace: default
+   status:
+     publicSubnets:


These lists cannot tell which public, private, zone belong to each other, better to have it as below

Suggested change

publicSubnets:

subnets:

- availabilityZone: us-west-2a

privateSubnet: subnet-1d9f28ba992a83514

publicSubnet: subnet-0d9f28ba991b93514

- availabilityZone: us-west-2b

privateSubnet: subnet-1d9f28ba992a83513

publicSubnet: subnet-0d9f28ba991b93513

serngawy · 2025-03-21T17:40:57Z

docs/proposal/20250224-rosa-networking.md

+       name: hcp01-control-plane
+       namespace: default
+   spec:
+     RosaNetworkRef:


rosaNetworkRef

Suggested change

RosaNetworkRef:

rosaNetworkRef:

serngawy · 2025-03-21T17:46:05Z

docs/proposal/20250224-rosa-networking.md

+     RosaNetworkRef:
+       name: hcp01-rosa-network
+   ```
+   Should the ROSA control plane CR contain reference to ROSA network, the reconciler will read the AWS region, AZ and subnet ids parameters from the ROSA network CR.


Lets also mention that end user cannot set both RosaControlPlane->spec->subnets and rosaNetworkRef at same time.

Technically, the user can specify both, but CAPA would prefer one to the other, right?

yes, we will need to validate that in the webook raising error if user set both.

Added to the doc.

docs/proposal/20250224-rosa-networking.md

serngawy · 2025-04-02T15:52:31Z

/ok-to-test

serngawy · 2025-04-02T15:55:02Z

Thanks @mzazrivec, looks good. You may mention as well during ROSANetwork deletion a validation must be applied to check if there is a RosaControlplane refer to it.

nrb · 2025-05-13T20:01:25Z

docs/proposal/20250224-rosa-networking.md

+
+#### Functional Requirements
+
+1. Ability to create a new namespaced custom resource `RosaNetwork` with four attributes: name, AZ count, region and CIDR block for VPC.


This should probably note the AZ list, too.

@nrb yep, added to the proposal doc.

nrb · 2025-05-13T20:02:44Z

docs/proposal/20250224-rosa-networking.md

+       privateSubnet: subnet-7d7e19c0af1f4d57f
+   ```
+
+   All resources created in the cloudformation stack will be tracked under `status.resources` array:


What's the reason for setting subnets as a top-level field instead of listing them under status.resources.subnets?

@nrb in the end the subnets would be listed under both status.subnets (grouped by AZ) and status.resources. The status.resources will be a list filled with the data propagated from AWS as the stack creation progresses. My thought here was to duplicate the results we're interested in (subnets & AZs) so that we don't have to parse it out of the status.resources (i.e. a convenience function).

So if I could rephrase to confirm I understand - the subnets are promoted to a top level resource primarily for ease of readability?

@nrb Yes, I guess you could say that. Although I meant it mostly for the benefit of readibility by other controllers: AZs and subnet ids is what the ROSA controller needs to provision the actual cluster.

Do you think this is not a good idea?

docs/proposal/20250224-rosa-networking.md

nrb · 2025-05-13T20:21:02Z

docs/proposal/20250224-rosa-networking.md

+     conditions:
+     - lastTransitionTime: "2025-03-20T14:45:26Z"
+       status: "True"
+       type: RosaNetworkReady


Make sure to add a Reason field[1], even for success.

[1] https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20240916-improve-status-in-CAPI-resources.md#transition-to-kubernetes-api-conventions-aligned-conditions

If you can add the reason in here, I think that's the last of my comments.

@nrb yep, ack 😃

Not 100% sure if I understand the linked document correctly. The document says: Use of the Reason field is required (currently in Cluster API reasons is added only when condition are false)

Does that mean we can have an empty string and still show the Reason field? Or do we have to have a non-empty reason string even for success?

nrb · 2025-05-13T20:26:01Z

I agree with the general direction this proposal takes. I made some comments around clarity and explicitness, paying special attention to status.conditions since there has been a significant effort upstream to align CAPI's usage with that of the Kubernetes project itself.

The examples given in YAML are very helpful! It may also be useful to include Go definitions in order to better understand optional or mutually exclusive fields, since these can often be excluded from YAML output due to the omitEmpty tag.

k8s-ci-robot added kind/documentation Categorizes issue or PR as related to documentation. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 10, 2025

k8s-ci-robot requested review from Ankitasw and richardcase March 10, 2025 14:23

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 10, 2025

k8s-ci-robot added needs-priority needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 10, 2025

mzazrivec changed the title ~~📖 Proposal for ROSA network~~ 📖 Proposal for ROSA networking Mar 10, 2025

serngawy suggested changes Mar 10, 2025

View reviewed changes

mzazrivec force-pushed the proposal_for_rosa_networking branch from 38dadd9 to 624dd12 Compare March 19, 2025 13:16

serngawy suggested changes Mar 19, 2025

View reviewed changes

mzazrivec force-pushed the proposal_for_rosa_networking branch from 624dd12 to 8a14cc6 Compare March 21, 2025 13:58

serngawy suggested changes Mar 21, 2025

View reviewed changes

mzazrivec force-pushed the proposal_for_rosa_networking branch 2 times, most recently from a36c4cc to e9502ee Compare March 26, 2025 12:24

serngawy reviewed Mar 26, 2025

View reviewed changes

docs/proposal/20250224-rosa-networking.md Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 2, 2025

serngawy approved these changes Apr 30, 2025

View reviewed changes

PanSpagetka approved these changes May 5, 2025

View reviewed changes

nrb reviewed May 13, 2025

View reviewed changes

docs/proposal/20250224-rosa-networking.md Show resolved Hide resolved

nrb reviewed May 13, 2025

View reviewed changes

Proposal for ROSA network

655cac8

mzazrivec force-pushed the proposal_for_rosa_networking branch from e9502ee to 655cac8 Compare May 14, 2025 13:01

-     publicSubnets:
+     subnets:
+     - availabilityZone: us-west-2a
+       privateSubnet: subnet-1d9f28ba992a83514
+       publicSubnet: subnet-0d9f28ba991b93514
+     - availabilityZone: us-west-2b
+       privateSubnet: subnet-1d9f28ba992a83513
+       publicSubnet: subnet-0d9f28ba991b93513


		#### Functional Requirements

		1. Ability to create a new namespaced custom resource `RosaNetwork` with four attributes: name, AZ count, region and CIDR block for VPC.

📖 Proposal for ROSA networking #5381

Are you sure you want to change the base?

📖 Proposal for ROSA networking #5381

Uh oh!

Conversation

mzazrivec commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 10, 2025

Uh oh!

k8s-ci-robot commented Mar 10, 2025

Uh oh!

k8s-ci-robot commented Mar 10, 2025

Uh oh!

serngawy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzazrivec commented Mar 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hunterkepley Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mzazrivec commented Mar 10, 2025 •

edited

Loading

hunterkepley Mar 24, 2025 •

edited

Loading