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

[CDN] Fixes #12152: Add custom domain BYOC support. #12648

Merged
merged 2 commits into from Jun 3, 2020

Conversation

lsmith130
Copy link
Contributor

@lsmith130 lsmith130 commented Mar 18, 2020

Adds CDN custom domain BYOC support and fixes CDN-managed certs for Verizon and Microsoft SKUs. Fixes #12152 and #9894.

History Notes:

[CDN] az cdn custom-domain enable-https: Add BYOC support.
[CDN] az cdn custom-domain enable-https: Fix enabling custom HTTPS with CDN managed certificates for Standard_Verizon and Standard_Microsoft SKUs.


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

@yonzhan yonzhan added this to the S167 milestone Mar 19, 2020
@yonzhan
Copy link
Collaborator

@yonzhan yonzhan commented Mar 19, 2020

add to S167

@yonzhan yonzhan requested a review from qianwens Mar 19, 2020
@lsmith130 lsmith130 changed the title Add CDN custom domain BYOC support. [CDN] Fixes #12152: Add custom domain BYOC support. Mar 20, 2020
@lsmith130 lsmith130 force-pushed the cdn/byoc branch 2 times, most recently from 0338ff3 to 1fc1d57 Compare Mar 21, 2020
@yonzhan yonzhan removed this from the S167 milestone Mar 28, 2020
@yonzhan yonzhan added this to the S168 milestone Mar 28, 2020
@lsmith130
Copy link
Contributor Author

@lsmith130 lsmith130 commented Apr 8, 2020

Hi again @Juliehzl, what's the timeframe for when I can get a review for this?

@@ -6718,65 +4063,6 @@ interactions:
status:
code: 200
message: OK
- request:
Copy link
Contributor

@Juliehzl Juliehzl Apr 9, 2020

Choose a reason for hiding this comment

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

Why are there so many recording deleted?

Copy link
Contributor Author

@lsmith130 lsmith130 May 7, 2020

Choose a reason for hiding this comment

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

I believe this is due to some change in the cli core, it seems like the authentication request at the beginning of some tests is no longer needed.

@yonzhan yonzhan removed this from the S168 milestone Apr 18, 2020
@yonzhan yonzhan added this to the S169 - For Build milestone Apr 18, 2020
@yonzhan
Copy link
Collaborator

@yonzhan yonzhan commented May 2, 2020

add to S169

@mmyyrroonn
Copy link
Contributor

@mmyyrroonn mmyyrroonn commented May 7, 2020

@lsmith130 Could you address the comments so that we can merge this PR to fix issues

@mmyyrroonn
Copy link
Contributor

@mmyyrroonn mmyyrroonn commented May 11, 2020

/azp run Azure.azure-cli

@azure-pipelines
Copy link

@azure-pipelines azure-pipelines bot commented May 11, 2020

Azure Pipelines successfully started running 1 pipeline(s).

@lsmith130
Copy link
Contributor Author

@lsmith130 lsmith130 commented May 11, 2020

@MyronFanQiu, I've addressed the comments and CI failures.

Copy link
Contributor

@mmyyrroonn mmyyrroonn left a comment

LGTM in general except for one question. Wait for @haroldrandom @Juliehzl

arg_group='Bring Your Own Certificate',
help='The protocol type of the certificate.',
arg_type=get_enum_type(['sni', 'ip']))
c.argument('user_cert_subscription_id',
Copy link
Contributor

@mmyyrroonn mmyyrroonn May 12, 2020

Choose a reason for hiding this comment

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

is it possible to support one secret id argument?

Copy link
Contributor Author

@lsmith130 lsmith130 May 21, 2020

Choose a reason for hiding this comment

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

This is a non-trivial request, as we would need to add extra logic to retrieve the correct resource group name for the secret, which is not included in the secret id because it doesn't use a standard Azure resource id. We would also have to correctly handle permissions issues where the user may not have permission to perform that lookup even though they do have permission to reference the key through CDN. Can we include this later as a separate PR if customers request it?

@yonzhan yonzhan removed this from the S169 - For Build milestone May 16, 2020
@yonzhan yonzhan added this to the S170 milestone May 16, 2020
@yonzhan
Copy link
Collaborator

@yonzhan yonzhan commented May 16, 2020

add to S170

accept-language:
- en-US
method: GET
uri: https://management.azure.com/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/cli_test_cdn_domain000001/providers/Microsoft.Cdn/profiles/cdnprofile1?api-version=2019-06-15-preview
Copy link

@Wyren Wyren May 19, 2020

Choose a reason for hiding this comment

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

This is a get method and a call for profile. So this doesn't match the command you're describing.

Copy link
Contributor Author

@lsmith130 lsmith130 May 21, 2020

Choose a reason for hiding this comment

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

This profile GET is only part of the setup for the test. If you look below, later requests are actually performing enableCustomHttps POST commands on the custom domains

- en-US
method: GET
uri: https://management.azure.com/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/clitest.rg000001/providers/Microsoft.Cdn/profiles/profile123?api-version=2019-06-15-preview
response:
Copy link

@Wyren Wyren May 19, 2020

Choose a reason for hiding this comment

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

Same here. This is for a pull request to fix this call and the tests seem off. It's weird.

Copy link
Contributor Author

@lsmith130 lsmith130 May 21, 2020

Choose a reason for hiding this comment

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

Again, the tests must perform required setup before testing the actual area of interest.

Copy link

@Wyren Wyren May 25, 2020

Choose a reason for hiding this comment

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

Thanks for the explanation. For some reason I assumed these were combined with the other tests considering the setup could be handled in that manor. But this makes it a lot clearer.

My apologies for the comments going a bit to far. The lock-down has put me on edge and frustration got the better of me.

accept-language:
- en-US
method: GET
uri: https://management.azure.com/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/clitest.rg000001/providers/Microsoft.Cdn/profiles/profile123/endpoints/cdn-cli-test-4/customDomains/customdomain000002?api-version=2019-06-15-preview
Copy link

@Wyren Wyren May 19, 2020

Choose a reason for hiding this comment

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

Why is this happening in multiple places...

Copy link
Contributor Author

@lsmith130 lsmith130 May 21, 2020

Choose a reason for hiding this comment

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

This GET request is retrieving the custom domain to verify its status is now correct after the previous POST enableCustomHttps request.

accept-language:
- en-US
method: GET
uri: https://management.azure.com/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/clitest.rg000001/providers/Microsoft.Cdn/profiles/profile123?api-version=2019-06-15-preview
Copy link

@Wyren Wyren May 19, 2020

Choose a reason for hiding this comment

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

Am I maybe understanding these test wrong or what's the deal.

accept-language:
- en-US
method: GET
uri: https://management.azure.com/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/clitest.rg000001/providers/Microsoft.Cdn/profiles/profile123/endpoints/cdn-cli-test-4/customDomains/customdomain000003?api-version=2019-06-15-preview
Copy link

@Wyren Wyren May 19, 2020

Choose a reason for hiding this comment

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

Do I need to put this at every enable-https request that has a get method? Because some do use the proper uri en method. So I don't know if it really is a mistake anymore. I think the message is getting through right?

Copy link
Contributor Author

@lsmith130 lsmith130 May 21, 2020

Choose a reason for hiding this comment

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

Same as above. This is a required part of the test. Since these are not unit tests it is impossible to test only the functionality of interest without making addition setup calls and reading state to verify correct behavior. Please let me know if I have not addressed your concerns in my comments.

@lsmith130
Copy link
Contributor Author

@lsmith130 lsmith130 commented May 21, 2020

@Juliehzl is it possible to approve this PR? I've addressed all questions and requested changes.

@@ -135,6 +135,33 @@ def load_arguments(self, _):
with self.argument_context('cdn custom-domain create') as c:
c.argument('location', validator=get_default_location_from_resource_group)

with self.argument_context('cdn custom-domain enable-https') as c:
c.argument('profile_name', id_part=None, help='Name of the parent profile.')
Copy link
Contributor

@Juliehzl Juliehzl May 29, 2020

Choose a reason for hiding this comment

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

Prefer ['--name', '-n' ]

Copy link
Contributor Author

@lsmith130 lsmith130 Jun 1, 2020

Choose a reason for hiding this comment

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

The name_arg_type parameter to the argument custom_domain_name already makes the parameter use --name, -n as requested. See the example for cdn custom-domain enable-https

@yonzhan yonzhan removed this from the S170 milestone May 31, 2020
@yonzhan yonzhan added this to the S171 milestone May 31, 2020
@yonzhan
Copy link
Collaborator

@yonzhan yonzhan commented May 31, 2020

add to S171

@lsmith130
Copy link
Contributor Author

@lsmith130 lsmith130 commented Jun 2, 2020

@haroldrandom could you take another look at this PR? I've addressed all comments and I believe it just needs code-owner approval before it can be merged.

@haroldrandom haroldrandom merged commit 0bb45ec into Azure:dev Jun 3, 2020
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants