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

[Spring-cloud] Add support for AppMSI #4598

Merged

Conversation

jiec-msft
Copy link
Contributor

@jiec-msft jiec-msft commented Mar 30, 2022


Note

  1. Part of change is also extracted into another PR to help review: Pull Request: [Spring-Cloud] Switch to 2022-03-01-preview sdk for msi command groups.
  2. And, we only need to merge current PR([Spring-cloud] Add support for AppMSI #4598).
  3. Release date: after backend for Azure Spring Cloud has released, around 3/31 ~ 4/1

This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your PR is merged into master branch, a new PR will be created to update src/index.json automatically.
The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify src/index.json.

… and re-generate the recording-test yaml files
Add helps for params in identity remove
Add validators for params in identity remove
Add unit test for params validator
spring-cloud app identity remove
spring-cloud app identity remove
spring-cloud app identity remove
1) Refine help info for command
2) Add parameters
3) Add validator
4) Add UT for validator
1. Add param
2. Add help info
3. Add validator
4. Add UT for validator
implement and add partial recording test
add recording test for create app with user assigned managed identities
add recording test for create app with both assigned managed identities
1. add parameters
2. add validator
3. add method in help
4. add command
5. add stub implementation
6. add UT for validator
add recording test for force-set
@ghost ghost added Spring Cloud Spring Cloud related issues Auto-Assign Auto assign by bot labels Mar 30, 2022
@ghost ghost requested review from zhoxing-ms, wangzelin007 and yonzhan March 30, 2022 06:24
@ghost ghost assigned zhoxing-ms Mar 30, 2022
@ghost ghost added this to the Mar 2022 (2022-04-06) milestone Mar 30, 2022
@jiec-msft jiec-msft marked this pull request as ready for review March 30, 2022 06:37
@yonzhan
Copy link
Collaborator

yonzhan commented Mar 30, 2022

Spring-cloud

Comment on lines +294 to +304
helps['spring-cloud app identity force-set'] = """
type: command
short-summary: Force set managed identities on an app.
examples:
- name: Force remove all managed identities on an app.
text: az spring-cloud app identity force-set -n MyApp -s MyCluster -g MyResourceGroup --system-assigned disable --user-assigned disable
- name: Force remove all user-assigned managed identities on an app, and enable or keep system-assigned managed identity.
text: az spring-cloud app identity force-set -n MyApp -s MyCluster -g MyResourceGroup --system-assigned enable --user-assigned disable
- name: Force remove system-assigned managed identity on an app, and assign or keep user-assigned managed identities.
text: az spring-cloud app identity force-set -n MyApp -s MyCluster -g MyResourceGroup --system-assigned disable --user-assigned IdentityResourceId1 IdentityResourceId2
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this command? Can its function be replaced by the spring-cloud app identity remove command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally we don't need these commands. It's related to a data consistency problem among managed identity RP and my RP. There can be case when user identity is assigned to an app in my side, but deleted in managed identity RP due to data in-consistency. These commands leverage put method to reset data back with one command.

app identity remove/assign is done with patch http method, there can be case, there is no identity assigned to an app in my RP, but managed identity RP still has some identity assigned to the app.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks~

Comment on lines 80 to 81
if _app_not_updatable(app):
raise CLIError("Failed to remove managed identities since app is in {} state.".format(app.properties.provisioning_state))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use the specific error type instead of CLIError?

Copy link
Contributor Author

@jiec-msft jiec-msft Mar 30, 2022

Choose a reason for hiding this comment

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

updated all 9 occurrences.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Assign Auto assign by bot do-not-merge Spring Cloud Spring Cloud related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants