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

Enable preconfigured NSG flag on azure-cli #3112

Merged
merged 11 commits into from
Sep 19, 2023

Conversation

gouthamMN
Copy link
Contributor

@gouthamMN gouthamMN commented Aug 21, 2023

Which issue this PR addresses:

Fixes:
https://issues.redhat.com/browse/ARO-2591
https://issues.redhat.com/browse/ARO-2589
https://issues.redhat.com/browse/ARO-4016

What this PR does / why we need it:

  1. Bumps api version used and adds parameter --enable-preconfigured-nsg to az aro cli extension for using preconfigured NSGs. Allowed values for the flag are true or false and by default, preconfigured NSG will be disabled.
  2. Adds network contributor role to BYO-NSGs when preconfigured NSG feature is enabled.
  3. Update az cli to include subnets from workerProfilesStatus field when available.

This should probably not be merged until the next stable API is released.

Test plan for issue:

Updated/added unit tests and existing E2E tests.

Is there any documentation that needs to be updated for this PR?

Yes, the Azure Doc will need to be updated though that will be a separate effort.

petrkotas
petrkotas previously approved these changes Sep 12, 2023
Copy link
Member

@petrkotas petrkotas left a comment

Choose a reason for hiding this comment

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

Hi @gouthamMN good work. LGTM. I think we can merge as 1) the API should be already released, 2) this will affect only extension which we use for development. The same work will need to be backported to upstream: https://github.com/Azure/azure-cli to make it available to the public.

s-amann
s-amann previously approved these changes Sep 12, 2023
@gouthamMN
Copy link
Contributor Author

Hi @gouthamMN good work. LGTM. I think we can merge as 1) the API should be already released, 2) this will affect only extension which we use for development. The same work will need to be backported to upstream: https://github.com/Azure/azure-cli to make it available to the public.

ARM manifest rollout and upstreaming azure-rest-api-specs swagger is still in progress.

@SudoBrendan
Copy link
Collaborator

SudoBrendan commented Sep 14, 2023

ARM manifest rollout and upstreaming azure-rest-api-specs swagger is still in progress.

I'd agree with Petr. We should be safe to merge into this repo's /python directory, since ARO engineering should be the only ones using this code. We have historically merged here early (because we can use localhost RP instances to test), then upstreamed changes once REST Specs and ARM Manifest Rollout are complete. This process is super unintuitive, so I drew up some diagrams about it recently.

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.

This is nearly there, I left a few grammar nits and one question. I think it would be good to check with the BU on all the error messaging before we merge.

python/az/aro/azext_aro/custom.py Show resolved Hide resolved
python/az/aro/azext_aro/custom.py Show resolved Hide resolved
python/az/aro/azext_aro/custom.py Outdated Show resolved Hide resolved
python/az/aro/azext_aro/custom.py Outdated Show resolved Hide resolved
@gouthamMN gouthamMN dismissed stale reviews from s-amann and petrkotas via 6bb122a September 15, 2023 15:31
@gouthamMN
Copy link
Contributor Author

ARM manifest rollout and upstreaming azure-rest-api-specs swagger is still in progress.

I'd agree with Petr. We should be safe to merge into this repo's /python directory, since ARO engineering should be the only ones using this code. We have historically merged here early (because we can use localhost RP instances to test), then upstreamed changes once REST Specs and ARM Manifest Rollout are complete. This process is super unintuitive, so I drew up some diagrams about it recently.

@SudoBrendan I'm fine merging the changes. Thank you for updating that wiki.

@SudoBrendan SudoBrendan merged commit 118f1a8 into Azure:master Sep 19, 2023
18 checks passed
tsatam added a commit to tsatam/ARO-RP that referenced this pull request Sep 21, 2023
tsatam added a commit to tsatam/ARO-RP that referenced this pull request Sep 21, 2023
tsatam added a commit to tsatam/ARO-RP that referenced this pull request Sep 21, 2023
SudoBrendan pushed a commit that referenced this pull request Sep 21, 2023
@gouthamMN gouthamMN deleted the preconfiguredNSG-flg branch October 27, 2023 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next-release To be included in the next RP release rollout ready-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants