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.

Sign up for GitHub

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

Jump to bottom

HOSTEDCP-1544: Allow user to specify subnet ID for Hosted Cluster & NodePool Creation #3945

Merged
merged 9 commits into from May 8, 2024
Merged

HOSTEDCP-1544: Allow user to specify subnet ID for Hosted Cluster & NodePool Creation #3945

merged 9 commits into from May 8, 2024

Conversation

bryan-cox
Copy link
Member

@bryan-cox bryan-cox commented Apr 25, 2024 •
edited

What this PR does / why we need it:
This PR primarily fixes the misunderstand of how a resource group provided by a client will be used in ARO HCP. We originally implemented BYO resource group as a resource group in which to create all cloud infrastructure. That is not correct understanding and not how it will be used in ARO HCP.

The expected setup should be a client brings us a resource group with a existing vnet, subnet, and NSG already setup. ARO HCP would then create a managed resource group with all the cloud infrastructure items in it and use the client's vnet, subnet, and NSG from the provided resource group.

Essentially, this PR:

  • removes subnet name from the API in favor of adding the subnet ID in the API instead
  • removes the vnet name from the API in favor of using the vnet ID in the API instead
  • updates the azure cloud provider config to use the a separate resource group for the vnet and NSG
  • updates the HyperShift CLI to use the vnet ID, subnet ID, and NSG ID instead of name
  • grabs the resource group the VNET and NSG is in from a VNET ID

Which issue(s) this PR fixes:
Fixes HOSTEDCP-1544

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 25, 2024
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 25, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Apr 25, 2024 •
edited by openshift-ci bot

@bryan-cox: This pull request references HOSTEDCP-1544 which is a valid jira issue.

In response to this:

What this PR does / why we need it:
TBD

Which issue(s) this PR fixes:
Fixes HOSTEDCP-1544

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Apr 25, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added do-not-merge/needs-area area/ci-tooling Indicates the PR includes changes for CI or tooling labels Apr 25, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Apr 25, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bryan-cox

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot added area/cli Indicates the PR includes changes for CLI area/documentation Indicates the PR includes changes for documentation approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/needs-area labels Apr 25, 2024
@bryan-cox bryan-cox changed the title HOSTEDCP-1544: Allow user to specify subnet ID for NodePool Creation HOSTEDCP-1544: Allow user to specify subnet ID for Hosted Cluster & NodePool Creation Apr 25, 2024
@openshift-ci openshift-ci bot added the area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release label Apr 25, 2024
@netlify Netlify
Copy link

netlify bot commented Apr 25, 2024 •
edited

✅ Deploy Preview for hypershift-docs ready!

Name Link
🔨 Latest commit 021003d
🔍 Latest deploy log https://app.netlify.com/sites/hypershift-docs/deploys/663a5e2cdb9bb60008cd927f
😎 Deploy Preview https://deploy-preview-3945--hypershift-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@openshift-ci openshift-ci bot added area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release area/testing Indicates the PR includes changes for e2e testing labels Apr 25, 2024
@bryan-cox bryan-cox force-pushed the HOSTEDCP-1544 branch 6 times, most recently from 2d54f06 to e9da826 Compare April 29, 2024 17:15
stevekuznetsov
stevekuznetsov reviewed Apr 29, 2024
View reviewed changes
Copy link
Contributor

@stevekuznetsov stevekuznetsov left a comment

Choose a reason for hiding this comment

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

Do we have an end-to-end test that captures the new flow with a pre-existing client group?

api/hypershift/v1beta1/hostedcluster_types.go Show resolved Hide resolved
api/hypershift/v1beta1/hostedcluster_types.go Outdated Show resolved Hide resolved
api/hypershift/v1beta1/hostedcluster_types.go Outdated Show resolved Hide resolved
cmd/infra/azure/create.go Show resolved Hide resolved
cmd/infra/azure/create.go Show resolved Hide resolved
control-plane-operator/controllers/hostedcontrolplane/cloud/azure/providerconfig.go Show resolved Hide resolved
@bryan-cox bryan-cox force-pushed the HOSTEDCP-1544 branch 4 times, most recently from 64f73b2 to 807fbb4 Compare April 30, 2024 15:02
@openshift-ci-robot
Copy link

openshift-ci-robot commented Apr 30, 2024 •
edited by openshift-ci bot

@bryan-cox: This pull request references HOSTEDCP-1544 which is a valid jira issue.

In response to this:

What this PR does / why we need it:
This PR:

  • removes subnet name from the API in favor of adding the subnet ID to the API instead
  • adds a new API field called ClientResourceGroupName which represents a client's resource group which should contain their vnet, subnet, and any NSG
  • updates the azure cloud provider config to use the client's resource group for the vnet and NSG if one was provided
  • updates the HyperShift CLI to use the vnet, subnet, and NSG from a client's resource group if provided through a flag

Which issue(s) this PR fixes:
Fixes HOSTEDCP-1544

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Apr 30, 2024 •
edited by openshift-ci bot

@bryan-cox: This pull request references HOSTEDCP-1544 which is a valid jira issue.

In response to this:

What this PR does / why we need it:
This PR primarily fixes the misunderstand of how a resource group provided by a client will be used in ARO HCP. We originally implemented BYO resource group as a resource group in which to create all cloud infrastructure. That is not correct understanding and not how it will be used in ARO HCP.

The expected setup should be a client brings us a resource group with a existing vnet, subnet, and/or NSG already setup. ARO HCP would then create a managed resource group with all the cloud infrastructure items in it and use the client's vnet, subnet, and/or NSG from their client resource group.

Essentially, this PR:

  • removes subnet name from the API in favor of adding the subnet ID to the API instead
  • adds a new API field called ClientResourceGroupName which represents a client's resource group which should contain their vnet, subnet, and any NSG
  • updates the azure cloud provider config to use the client's resource group for the vnet and NSG if one was provided
  • updates the HyperShift CLI to use the vnet, subnet, and NSG from a client's resource group if provided through a flag

Which issue(s) this PR fixes:
Fixes HOSTEDCP-1544

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

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 openshift-eng/jira-lifecycle-plugin repository.

@bryan-cox
Copy link
Member Author

/test unit
/test verify
/test images
/test e2e-aws

@openshift-ci-robot
Copy link

openshift-ci-robot commented May 7, 2024 •
edited by openshift-ci bot

@bryan-cox: This pull request references HOSTEDCP-1544 which is a valid jira issue.

In response to this:

What this PR does / why we need it:
This PR primarily fixes the misunderstand of how a resource group provided by a client will be used in ARO HCP. We originally implemented BYO resource group as a resource group in which to create all cloud infrastructure. That is not correct understanding and not how it will be used in ARO HCP.

The expected setup should be a client brings us a resource group with a existing vnet, subnet, and NSG already setup. ARO HCP would then create a managed resource group with all the cloud infrastructure items in it and use the client's vnet, subnet, and NSG from the provided resource group.

Essentially, this PR:

  • removes subnet name from the API in favor of adding the subnet ID in the API instead
  • removes the vnet name from the API in favor of using the vnet ID in the API instead
  • updates the azure cloud provider config to use the a separate resource group for the vnet and NSG
  • updates the HyperShift CLI to use the vnet ID, subnet ID, and NSG ID instead of name

Which issue(s) this PR fixes:
Fixes HOSTEDCP-1544

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link

openshift-ci-robot commented May 7, 2024 •
edited by openshift-ci bot

@bryan-cox: This pull request references HOSTEDCP-1544 which is a valid jira issue.

In response to this:

What this PR does / why we need it:
This PR primarily fixes the misunderstand of how a resource group provided by a client will be used in ARO HCP. We originally implemented BYO resource group as a resource group in which to create all cloud infrastructure. That is not correct understanding and not how it will be used in ARO HCP.

The expected setup should be a client brings us a resource group with a existing vnet, subnet, and NSG already setup. ARO HCP would then create a managed resource group with all the cloud infrastructure items in it and use the client's vnet, subnet, and NSG from the provided resource group.

Essentially, this PR:

  • removes subnet name from the API in favor of adding the subnet ID in the API instead
  • removes the vnet name from the API in favor of using the vnet ID in the API instead
  • updates the azure cloud provider config to use the a separate resource group for the vnet and NSG
  • updates the HyperShift CLI to use the vnet ID, subnet ID, and NSG ID instead of name
  • grabs the resource group the VNET and NSG is in from a VNET ID

Which issue(s) this PR fixes:
Fixes HOSTEDCP-1544

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

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 openshift-eng/jira-lifecycle-plugin repository.

@bryan-cox
Copy link
Member Author

/test unit
/test images
/test verify

@miguelsorianod
Copy link

The documentation of the attributes in the AzurePlatformSpec type looks good to me now, assuming the behavior matches the description of the doc 👍 .

Just a minor comment about the doc of AzurePlatformSpec. At the end of the paragraph it mentions:
An existing cloud resource is expected to exist under the same SubscriptionID and within the same ResourceGroupName.

Maybe this part can be removed?

@bryan-cox
Add additional details to AzurePlatform API
345b804 
Signed-off-by: Bryan Cox <brcox@redhat.com>
@bryan-cox
Update HyperShift CLI to accept only subnet ID
7d43d78 
Replaces the ability to specify the subnet name in the HyperShift CLI
with only the ability to specify the subnet ID.

Signed-off-by: Bryan Cox <brcox@redhat.com>
@bryan-cox
Copy link
Member Author

/test unit
/test images
/test verify

@bryan-cox
Get subnet name from its ID for Azure cloud config
0d37b88 
Signed-off-by: Bryan Cox <brcox@redhat.com>
@bryan-cox
Support BYO network security group
5651785 
Supports bring your own (BYO) network security group (NSG); else the
HyperShift CLI will create one for you. The Azure cloud provider
configuration is updated to indicate which resource group the NSG in
a BYO NSG scenario.

Signed-off-by: Bryan Cox <brcox@redhat.com>
@bryan-cox
Use SecurityGroupID in CLI and CPO
e05760b 
Signed-off-by: Bryan Cox <brcox@redhat.com>
@bryan-cox
Get VNET name and RG info from the VNET ID
344741c 
Signed-off-by: Bryan Cox <brcox@redhat.com>
@bennerv
Copy link

bennerv commented May 7, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 7, 2024
@bryan-cox bryan-cox marked this pull request as ready for review May 8, 2024 00:57
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 8, 2024
@openshift-ci openshift-ci bot requested review from enxebre and sjenning May 8, 2024 00:58
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 3037c8a and 2 for PR HEAD 344741c in total

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented May 8, 2024

@bryan-cox: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-ibmcloud-iks 344741c link false /test e2e-ibmcloud-iks
ci/prow/e2e-ibmcloud-roks 344741c link false /test e2e-ibmcloud-roks

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

@openshift-merge-bot openshift-merge-bot bot merged commit 983b05b into openshift:main May 8, 2024
13 of 15 checks passed
@miguelsorianod
Copy link

lgtm 👍

@openshift-bot
Copy link

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-hypershift-container-v4.16.0-202405080914.p0.g983b05b.assembly.stream.el9 for distgit hypershift.
All builds following this will include this PR.

@bryan-cox bryan-cox deleted the HOSTEDCP-1544 branch May 8, 2024 12:32
enxebre pushed a commit to enxebre/hypershift that referenced this pull request May 8, 2024
@openshift-merge-bot @enxebre
Merge pull request openshift#3945 from bryan-cox/HOSTEDCP-1544
909d70f 
HOSTEDCP-1544: Allow user to specify subnet ID for Hosted Cluster & NodePool Creation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevekuznetsov stevekuznetsov stevekuznetsov left review comments

@bennerv bennerv bennerv left review comments

@miguelsorianod miguelsorianod miguelsorianod left review comments

@Patryk-Stefanski Patryk-Stefanski Patryk-Stefanski left review comments

@enxebre enxebre Awaiting requested review from enxebre

@sjenning sjenning Awaiting requested review from sjenning

Assignees

@bennerv bennerv

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/ci-tooling Indicates the PR includes changes for CI or tooling area/cli Indicates the PR includes changes for CLI area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release area/documentation Indicates the PR includes changes for documentation area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release area/testing Indicates the PR includes changes for e2e testing jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@bryan-cox @openshift-ci-robot @miguelsorianod @bennerv @openshift-bot @stevekuznetsov @Patryk-Stefanski