Skip to content

OSDOCS-10892 adding install config params for aws subnets #93385

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bscott-rh
Copy link
Contributor

Version(s):
4.19

Issue:
https://issues.redhat.com/browse/OSDOCS-10892

Link to docs preview:

QE review:

  • QE has approved this change.

@bscott-rh bscott-rh added this to the Planned for 4.19 GA milestone May 14, 2025
@openshift-ci openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 14, 2025
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented May 14, 2025

🤖 Thu May 15 19:35:42 - Prow CI generated the docs preview:
https://93385--ocpdocs-pr.netlify.app
Complete list of updated preview URLs: artifacts/updated_preview_urls.txt

Copy link
Member

@tthvo tthvo left a comment

Choose a reason for hiding this comment

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

Nice! I just have some comments :D

aws:
vpc:
subnets:
|Defines a list of existing subnets to be used in place of automatically generated subnets. You specify a subnet by providing the subnet ID and a list of roles that apply to that subnet. If you specify subnet IDs but do not specify roles for any subnet, the subnets' roles will be decided automatically. If you do not specify any roles, you must ensure that any other subnets in your VPC have the `kubernetes.io/cluster/<cluster-id>` or `kubernetes.io/cluster/unmanaged: true` tags, or the installation will fail.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
|Defines a list of existing subnets to be used in place of automatically generated subnets. You specify a subnet by providing the subnet ID and a list of roles that apply to that subnet. If you specify subnet IDs but do not specify roles for any subnet, the subnets' roles will be decided automatically. If you do not specify any roles, you must ensure that any other subnets in your VPC have the `kubernetes.io/cluster/<cluster-id>` or `kubernetes.io/cluster/unmanaged: true` tags, or the installation will fail.
|A list of subnets in an existing VPC to be used in place of automatically created subnets. You specify a subnet by providing the subnet ID and an optional list of roles that apply to that subnet. If you specify subnet IDs but do not specify roles for any subnet, the subnets' roles will be decided automatically. If you do not specify any roles, you must ensure that any other subnets in your VPC have the `kubernetes.io/cluster/.*: .*` or `kubernetes.io/cluster/unmanaged: true` tags.
The subnets must be part of the same `machineNetwork[].cidr` ranges that you specify.
For a standard cluster, specify a public and a private subnet for each availability zone.
For a private cluster, specify a private subnet for each availability zone.
For clusters that use AWS Local Zones, you must add AWS Local Zone subnets to this list to ensure edge machine pool creation.

Other than some nit on wordings. I want to verify the suggestions a bit:

  • Subnets' roles are optional -> should make it clear
  • kubernetes.io/cluster/<cluster-id> is actually just a tag key. A complete tag consists of key and value (for example, kubernetes.io/cluster/my-cluster: owned). So, I suggest the use of .*, similar to here. But maybe you have a better alternative 😄
  • The notes about requirements for public/private and edge zone are still valid and should keep them. Though, we can reword.

- id:
roles:
- type:
|One or more roles that apply to the subnet specified by `platform.aws.vpc.subnets.id`. If you specify a role for any subnet, each subnet must have at least one assigned role, and the "ClusterNode", "IngressControllerLB", "ControlPlaneExternalLB", and "ControlPlaneInternalLB" roles must be assigned to at least one subnet. However, if the cluster scope is internal, then the "ControlPlaneExternalLB" role is not required.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
|One or more roles that apply to the subnet specified by `platform.aws.vpc.subnets.id`. If you specify a role for any subnet, each subnet must have at least one assigned role, and the "ClusterNode", "IngressControllerLB", "ControlPlaneExternalLB", and "ControlPlaneInternalLB" roles must be assigned to at least one subnet. However, if the cluster scope is internal, then the "ControlPlaneExternalLB" role is not required.
|One or more roles that apply to the subnet specified by `platform.aws.vpc.subnets.id`. If you specify a role for any subnet, each subnet must have at least one assigned role, and the `ClusterNode`, `BootstrapNode`, `IngressControllerLB`, `ControlPlaneExternalLB`, and `ControlPlaneInternalLB` roles must be assigned to at least one subnet. However, if the cluster scope is internal, then the `ControlPlaneExternalLB` role is not required.
For subnets in AWS Local Zones, only `EdgeNode` role can be assigned.

Verification:

  • BootstrapNode is also a required role if subnet roles are specified. See EP (note: we change it to BootstrapNode instead of Bootstrap).
  • EdgeNode is exclusive for subnets in AWS Local/Wavelength zones and must be specified if such subnets are included.
  • Use backticks for code-like 😅 similar to this doc, maybe?

roles:
- type:
|One or more roles that apply to the subnet specified by `platform.aws.vpc.subnets.id`. If you specify a role for any subnet, each subnet must have at least one assigned role, and the "ClusterNode", "IngressControllerLB", "ControlPlaneExternalLB", and "ControlPlaneInternalLB" roles must be assigned to at least one subnet. However, if the cluster scope is internal, then the "ControlPlaneExternalLB" role is not required.
|List of one or more role types. Valid values include "ClusterNode", "EdgeNode", "Bootstrap", "IngressControllerLB", "ControlPlaneExternalLB", and "ControlPlaneInternalLB".
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
|List of one or more role types. Valid values include "ClusterNode", "EdgeNode", "Bootstrap", "IngressControllerLB", "ControlPlaneExternalLB", and "ControlPlaneInternalLB".
|List of one or more role types. Valid values include `ClusterNode`, `EdgeNode`, `BootstrapNode`, `IngressControllerLB`, `ControlPlaneExternalLB`, and `ControlPlaneInternalLB`.

BootstrapNode is the right one (i.e. we divert a bit from the EP which says Bootstrap)

Comment on lines 479 to 487
- id: subnet-<id1>
roles:
- type: IngressControllerLB
- type: ControlPlaneLB
- type: Bootstrap
- id: subnet-<id2>
roles:
- type: ControlPlaneLB
- type: ClusterNode
Copy link
Member

Choose a reason for hiding this comment

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

platform:
  aws:
    vpc:
      subnets:
        - id: subnet-<id1>
        - id: subnet-<id2>
        - id: subnet-<id3>

I suggest we can keep it simple by giving an example of specifying subnets without roles here.

  • It is same as before but following new field convention.
  • Since there is no explanation of such subnet roles here, it's best not to make it complex. And roles are optional also.
  • Side note: There is no ControlPlaneLB role btw :D You might see it somewhere because we are lazy to say/type ControlPlaneExternal and ControlPlaneInternalLB haha

@bscott-rh bscott-rh force-pushed the OSDOCS-10892-install branch from ff57e0e to d6ce7c7 Compare May 15, 2025 19:26
Copy link

openshift-ci bot commented May 15, 2025

@bscott-rh: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

Copy link
Member

@tthvo tthvo left a comment

Choose a reason for hiding this comment

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

/lgtm

Looks good to me! I'd leave it to others to have another look and approve :D

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 15, 2025
@tthvo
Copy link
Member

tthvo commented May 15, 2025

/cc @yunjiang29

@openshift-ci openshift-ci bot requested a review from yunjiang29 May 15, 2025 20:19
@yunjiang29
Copy link
Contributor

/lgtm

@bscott-rh bscott-rh added the peer-review-needed Signifies that the peer review team needs to review this PR label May 16, 2025
@skopacz1 skopacz1 added the peer-review-in-progress Signifies that the peer review team is reviewing this PR label May 16, 2025
Copy link
Contributor

@skopacz1 skopacz1 left a comment

Choose a reason for hiding this comment

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

Just a few nits, otherwise lgtm!

- id:
roles:
- type:
|One or more roles that apply to the subnet specified by `platform.aws.vpc.subnets.id`. If you specify a role for any subnet, each subnet must have at least one assigned role, and the `ClusterNode`, `IngressControllerLB`, `ControlPlaneExternalLB`, `BootstrapNode` and `ControlPlaneInternalLB` roles must be assigned to at least one subnet. However, if the cluster scope is internal, then the `ControlPlaneExternalLB` role is not required.
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs an Oxford comma here:

Suggested change
|One or more roles that apply to the subnet specified by `platform.aws.vpc.subnets.id`. If you specify a role for any subnet, each subnet must have at least one assigned role, and the `ClusterNode`, `IngressControllerLB`, `ControlPlaneExternalLB`, `BootstrapNode` and `ControlPlaneInternalLB` roles must be assigned to at least one subnet. However, if the cluster scope is internal, then the `ControlPlaneExternalLB` role is not required.
|One or more roles that apply to the subnet specified by `platform.aws.vpc.subnets.id`. If you specify a role for any subnet, each subnet must have at least one assigned role, and the `ClusterNode`, `IngressControllerLB`, `ControlPlaneExternalLB`, `BootstrapNode`, and `ControlPlaneInternalLB` roles must be assigned to at least one subnet. However, if the cluster scope is internal, then the `ControlPlaneExternalLB` role is not required.

Comment on lines +482 to +484
- id: subnet-<id1>
- id: subnet-<id2>
- id: subnet-<id3>
Copy link
Contributor

Choose a reason for hiding this comment

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

Idk if this would look better or worse, but maybe an underscore between ID and the numbers?

Suggested change
- id: subnet-<id1>
- id: subnet-<id2>
- id: subnet-<id3>
- id: subnet-<id_1>
- id: subnet-<id_2>
- id: subnet-<id_3>

Up to you

@skopacz1 skopacz1 added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-in-progress Signifies that the peer review team is reviewing this PR peer-review-needed Signifies that the peer review team needs to review this PR labels May 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-4.19 lgtm Indicates that a PR is ready to be merged. peer-review-done Signifies that the peer review team has reviewed this PR size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants