[Containerapp] `az containerapp create/up`: `--registry-server` and `--source` use managed identity for image pull by default #7972

Greedygre · 2024-09-12T05:53:37Z

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

Related command

'az containerapp containerapp create/up': --registry-server use managed identity for image pull by default
'az containerapp containerapp create': --registry-server use managed identity for image pull by default. --no-wait will not take effect with system registry identity.
'az containerapp up': Support --registry-identity, --system-assigned, --user-assigned

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? (pip install wheel==0.30.0 required)
My extension version conforms to the Extension version schema

For new extensions:

My extension description/summary conforms to the Extension Summary Guidelines.

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update src/index.json automatically.
You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify src/index.json.

azure-client-tools-bot-prd · 2024-09-12T05:53:40Z

⚠️Azure CLI Extensions Breaking Change Test

⚠️containerapp

rule cmd_name rule_message suggest_message

⚠️ 1006 - ParaAdd containerapp up cmd containerapp up added parameter registry_identity

⚠️ 1006 - ParaAdd containerapp up cmd containerapp up added parameter system_assigned

⚠️ 1006 - ParaAdd containerapp up cmd containerapp up added parameter user_assigned

azure-client-tools-bot-prd · 2024-09-12T05:53:43Z

Hi @Greedygre,
Please write the description of changes which can be perceived by customers into HISTORY.rst.
If you want to release a new extension version, please update the version in setup.py as well.

yonzhan · 2024-09-12T05:53:52Z

Containerapp

github-actions · 2024-09-12T05:54:35Z

CodeGen Tools Feedback Collection

Thank you for using our CodeGen tool. We value your feedback, and we would like to know how we can improve our product. Please take a few minutes to fill our codegen survey

github-actions · 2024-09-12T05:57:18Z

Hi @Greedygre

Release Suggestions

Module: containerapp

Update VERSION to 1.0.0b4 in src/containerapp/setup.py

Notes

For more info about extension versioning, please refer to Extension version schema

Greedygre · 2024-09-12T07:26:11Z

About src/containerapp/azext_containerapp/_utils.py, I will revert it from this PR after PR Fix create containerapp/containerappjob with msi issue merge:

LaylaLiu-gmail · 2024-09-12T09:47:07Z

src/containerapp/azext_containerapp/_up_utils.py

@@ -1247,9 +1256,13 @@ def _get_acr_from_image(cmd, app):
        app.registry_server = app.image.split("/")[
            0
        ]  # TODO what if this conflicts with registry_server param?
+
+        # If --registry-server is ACR, use system-assigned managed identity for image pull by default
+        if app.registry_identity is None and app.registry_user is None and app.registry_pass is None:


What if a public registry? Do we need system?

This logic is for command az containerapp up, if the image is from ACR, it will always look up credential and add ACR to properties.configuration.registries.
For properties.configuration.registries, it require (username and password) or identity, otherwise it throw error from DP:
(ContainerAppRegistriesPasswordSecretRefNotFound) PasswordSecretRef '' defined for registry server 'acaxinyulogtest.azurecr.io' not found.

So we need system.

For az containerapp create, if it is a public registry, without argument --registry-server the CLI will not add registry to properties.configuration.registries, then it will treat as a public image.

Greedygre · 2024-09-18T03:06:55Z

The CI failed with following error, which is not relate to our containerapp extension.

=========================== short test summary info ============================
FAILED src/standbypool/azext_standbypool/tests/latest/test_standbypool.py::StandbypoolScenario::test_standby_container_group_pool_scenarios
FAILED src/standbypool/azext_standbypool/tests/latest/test_standbypool.py::StandbypoolScenario::test_standby_virtual_machine_pool_scenarios

FumingZhang · 2024-09-18T03:17:37Z

It looks like some unrelated changes (about aks-preview) were merged into your branch?

Greedygre · 2024-09-18T04:03:41Z

It looks like some unrelated changes (about aks-preview) were merged into your branch?

Fixed.

…d identity for image pull

Juliehzl · 2024-09-25T08:18:14Z

src/containerapp/azext_containerapp/_up_utils.py

@@ -409,7 +409,10 @@ def __init__(
        env_vars=None,
        workload_profile_name=None,
        ingress=None,
-        force_single_container_updates=None
+        force_single_container_updates=None,
+        registry_identity=None,


should this parameter be exposed?

For az containerapp up, we expose new arguments --registry-identity, --system-assigned, --user-assigned

I do not see --registry-identity added in params.py

For --registry-identity, it is Inherited from azure-cli az containerapp:

Juliehzl · 2024-09-25T08:21:19Z

src/containerapp/azext_containerapp/_up_utils.py

        parsed = urlparse(app.image)
        registry_name = (parsed.netloc if parsed.scheme else parsed.path).split(".")[0]
-        if app.registry_user is None or app.registry_pass is None:


what if only app.registry_user provided?

It will be handled in create_containerapp or update_containerapp, we will look up creds for it, also I has test case cover it: test_containerapp_registry_acr_look_up_credentical

Juliehzl · 2024-09-25T08:26:37Z

src/containerapp/azext_containerapp/containerapp_decorator.py

@@ -849,6 +880,7 @@ def parent_construct_payload(self):
        self.set_up_registry_identity()

    def construct_payload(self):
+        self.set_up_system_assigned_identity_as_default_if_using_acr()


why not put after parent_construct_payload()?

The set_up_system_assigned_identity_as_default_if_using_acr will set --identity to system as default, it should be execute before parent_construct_payload()

Juliehzl · 2024-09-25T08:28:21Z

src/containerapp/azext_containerapp/containerapp_decorator.py

+        if self.get_argument_source():
+            _get_registry_details_without_get_creds(self.cmd, app, self.get_argument_source())
+        if self.get_argument_repo():


what about both --source and --repo provided?

We don't allowed input --source and --repo together:
When input --source --repo together, throw:
Cannot use --source and --repo together. Can either deploy from a local directory or a Github repo

For --repo, it will create github action with sourcecontrols, it use the ACR registry to create github action.
registry_username=app.registry_user,

The sourcecontrols doesn't support registry with registry identity. It only support registry username and password.

Juliehzl · 2024-09-25T08:33:06Z

src/containerapp/azext_containerapp/containerapp_decorator.py

+        system_sp = safe_get(self.containerapp_def, "identity", "principalId")
+
+        # create system service principalId
+        if system_sp is None and is_registry_msi_system(identity):


in this condition, only when customer specify system as identity, we will create the system principalId? should we also create in default scenario?

Yes, we did.
For the default behavior, in the set_up_system_assigned_identity_as_default_if_using_acr, it will set identity to system, then in here it will create the system principalId too.
I have test to cover it: test_containerapp_registry_identity_system

Juliehzl · 2024-09-25T08:35:42Z

src/containerapp/azext_containerapp/containerapp_decorator.py

+                    return
+                registry_exists = False
+                for r in registries_def:
+                    if r['server'].lower() == self.get_argument_registry_server().lower():


what if no 'server' in r?

Is there such a scenario? Is it a legal scenario?

Juliehzl · 2024-09-25T08:55:12Z

src/containerapp/azext_containerapp/custom.py

+        if source:
+            _get_registry_details_without_get_creds(cmd, app, source)
+        if repo:


what if both source and repo provided?

This is not allowed:
When input --source --repo together, throw:
Cannot use --source and --repo together. Can either deploy from a local directory or a Github repo

code in _validate_up_args

Juliehzl · 2024-09-25T08:56:08Z

src/containerapp/azext_containerapp/custom.py


    force_single_container_updates = False
    if source:
+        app.get_acr_creds = False


once source is provided, you will never try to get acr creds?

with --source,
If customer not provider --registry-username, we will never try to get acr creds.
If customer provider --registry-username or --registry-password, we will try to get acr creds in the next stepscreate_containerapp or update_containerapp.

Juliehzl · 2024-09-26T02:46:29Z

src/containerapp/azext_containerapp/_up_utils.py

-            self.cmd.cli_ctx, registry_name
-        )
+        if self.get_acr_creds:
+            self.registry_user, self.registry_pass, _ = _get_acr_cred(


the format looks strange

…by mistake

zhoxing-ms · 2024-10-08T06:27:01Z

src/containerapp/azext_containerapp/_params.py

+        c.argument('user_assigned', nargs='+', help="Space-separated user identities to be assigned.")
+        c.argument('system_assigned', help="Boolean indicating whether to assign system-assigned identity.", action='store_true')


Could you refer to the guideline https://github.com/Azure/azure-cli/blob/dev/doc/managed_identity_command_guideline.md to design these managed identity related parameters?

The new user_assigned and system_assigned parameters for the az containerapp up command are consistent with those for az containerapp create. We need to change them together, otherwise the user experience will be inconsistent.

OK, sounds good

azure-client-tools-bot-prd bot added this to the September 2024 (2024-10-08) milestone Sep 12, 2024

microsoft-github-policy-service bot requested review from yonzhan, wangzelin007, yanzhudd, zhoxing-ms, jsntcy, ruslany, sanchitmehta, ebencarek, JennyLawrance, howang-ms, vinisoto, chinadragon0515, vturecek, torosent, pagariyaalok, Juliehzl and jijohn14 September 12, 2024 05:53

microsoft-github-policy-service bot added the Auto-Assign Auto assign by bot label Sep 12, 2024

microsoft-github-policy-service bot assigned zhoxing-ms Sep 12, 2024

microsoft-github-policy-service bot added the ContainerApp label Sep 12, 2024

LaylaLiu-gmail reviewed Sep 12, 2024

View reviewed changes

Greedygre force-pushed the xinyu/20240829_registry_server_default_system_assign branch from 680f092 to c459d1e Compare September 18, 2024 03:09

Greedygre requested a review from necusjz as a code owner September 18, 2024 03:09

github-actions bot added the release-version-block Updates do not qualify release version rules. NOTE: please do not edit it manually. label Sep 18, 2024

fix

de7d0bf

Greedygre force-pushed the xinyu/20240829_registry_server_default_system_assign branch from c459d1e to de7d0bf Compare September 18, 2024 04:01

fix history

0ab62f6

github-actions bot removed the release-version-block Updates do not qualify release version rules. NOTE: please do not edit it manually. label Sep 18, 2024

xinyululala added 6 commits September 23, 2024 16:15

merge main

d61be9a

change to --no-wait will not take effect with system registry identity

b355438

fix test and rerun for -no-wait

8e08333

fix history

fbe0d15

az containerapp up/create/update --source: use system-assigned manage…

d72f552

…d identity for image pull

rerun tests and fix

1f7bd17

Greedygre changed the title ~~[Containerapp] az containerapp create/up: --registry-server use managed identity for image pull by default~~ [Containerapp] az containerapp create/up: --registry-server and --source use managed identity for image pull by default Sep 25, 2024

update history

490eb34

Juliehzl reviewed Sep 25, 2024

View reviewed changes

Juliehzl reviewed Sep 26, 2024

View reviewed changes

Juliehzl approved these changes Sep 29, 2024

View reviewed changes

fix ConnectedContainerAppNotSupportIdentity and ex overwrite to None …

3e3e148

…by mistake

zhoxing-ms reviewed Oct 8, 2024

View reviewed changes

zhoxing-ms approved these changes Oct 8, 2024

View reviewed changes

zhoxing-ms merged commit 3005636 into Azure:main Oct 8, 2024
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Containerapp] `az containerapp create/up`: `--registry-server` and `--source` use managed identity for image pull by default #7972

[Containerapp] `az containerapp create/up`: `--registry-server` and `--source` use managed identity for image pull by default #7972

Greedygre commented Sep 12, 2024 •

edited

Loading

azure-client-tools-bot-prd bot commented Sep 12, 2024 •

edited

Loading

azure-client-tools-bot-prd bot commented Sep 12, 2024

yonzhan commented Sep 12, 2024

github-actions bot commented Sep 12, 2024

github-actions bot commented Sep 12, 2024 •

edited

Loading

Greedygre commented Sep 12, 2024

LaylaLiu-gmail Sep 12, 2024

Greedygre Sep 13, 2024

Greedygre commented Sep 18, 2024

FumingZhang commented Sep 18, 2024

Greedygre commented Sep 18, 2024

Juliehzl Sep 25, 2024

Greedygre Sep 25, 2024

Juliehzl Sep 26, 2024

Greedygre Sep 26, 2024 •

edited

Loading

Juliehzl Sep 25, 2024

Greedygre Sep 25, 2024

Juliehzl Sep 25, 2024

Greedygre Sep 25, 2024

Juliehzl Sep 25, 2024

Greedygre Sep 25, 2024

Juliehzl Sep 25, 2024

Greedygre Sep 25, 2024 •

edited

Loading

Juliehzl Sep 25, 2024

Greedygre Sep 25, 2024

Juliehzl Sep 25, 2024

Greedygre Sep 25, 2024

Juliehzl Sep 25, 2024

Greedygre Sep 25, 2024 •

edited

Loading

Juliehzl Sep 26, 2024

zhoxing-ms Oct 8, 2024

Greedygre Oct 8, 2024

zhoxing-ms Oct 8, 2024

		c.argument('user_assigned', nargs='+', help="Space-separated user identities to be assigned.")
		c.argument('system_assigned', help="Boolean indicating whether to assign system-assigned identity.", action='store_true')

[Containerapp] az containerapp create/up: --registry-server and --source use managed identity for image pull by default #7972

[Containerapp] az containerapp create/up: --registry-server and --source use managed identity for image pull by default #7972

Conversation

Greedygre commented Sep 12, 2024 • edited Loading

Related command

General Guidelines

About Extension Publish

azure-client-tools-bot-prd bot commented Sep 12, 2024 • edited Loading

azure-client-tools-bot-prd bot commented Sep 12, 2024

yonzhan commented Sep 12, 2024

github-actions bot commented Sep 12, 2024

CodeGen Tools Feedback Collection

github-actions bot commented Sep 12, 2024 • edited Loading

Release Suggestions

Module: containerapp

Notes

Greedygre commented Sep 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Greedygre commented Sep 18, 2024

FumingZhang commented Sep 18, 2024

Greedygre commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Greedygre Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Greedygre Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Greedygre Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[Containerapp] `az containerapp create/up`: `--registry-server` and `--source` use managed identity for image pull by default #7972

[Containerapp] `az containerapp create/up`: `--registry-server` and `--source` use managed identity for image pull by default #7972

Greedygre commented Sep 12, 2024 •

edited

Loading

azure-client-tools-bot-prd bot commented Sep 12, 2024 •

edited

Loading

github-actions bot commented Sep 12, 2024 •

edited

Loading

Greedygre Sep 26, 2024 •

edited

Loading

Greedygre Sep 25, 2024 •

edited

Loading

Greedygre Sep 25, 2024 •

edited

Loading