Skip to content

Conversation

@Yueren-Wang
Copy link
Contributor

Description
Extend Current Regex matching to include managed hsm URIs since Managed HSM is supported in SQL now.

@yonzhan
Copy link
Collaborator

yonzhan commented Sep 10, 2020

SQL

@yonzhan yonzhan modified the milestones: S176, S177 Oct 2, 2020
@yonzhan yonzhan modified the milestones: S177, S178 Oct 24, 2020
@yonzhan yonzhan requested a review from evelyn-ys October 24, 2020 15:02
@Juliehzl
Copy link
Contributor

@bim-msft could you help take a look at hsm part?

@Juliehzl
Copy link
Contributor

Hi @kingsleyAzure, could you fix CI failure first?

@Juliehzl Juliehzl changed the title add managed hsm regex match to SQL [SQL] Add managed hsm regex match to SQL Nov 12, 2020
@Juliehzl
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Contributor

@bim-msft bim-msft left a comment

Choose a reason for hiding this comment

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

LGTM

@yonzhan yonzhan modified the milestones: S178, S179 Nov 14, 2020
@Juliehzl
Copy link
Contributor

Juliehzl commented Dec 2, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@Juliehzl
Copy link
Contributor

Juliehzl commented Dec 3, 2020

Hi @kingsleyAzure, could you take a look on style error and make CI pass?

@yonzhan yonzhan modified the milestones: S179, S180 Dec 5, 2020
@yonzhan yonzhan modified the milestones: S180, S181 Dec 26, 2020
@Yueren-Wang
Copy link
Contributor Author

Hi @Juliehzl

Can you please retry the pipeline?

@Juliehzl
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@Yueren-Wang
Copy link
Contributor Author

please rerun the pipeline. I fixed the line too long error.

@Juliehzl
Copy link
Contributor

Hi @kingsleyAzure, CI is triggered automatically with your commit. But there are more failures occurred. You could see more in Details.
I am fine with your PR, but you need to make CI pass. If you need my help, please ping me in Teams.


match = re.match(r'^https(.)+\.vault(.)+\/keys\/[^\/]+\/[0-9a-zA-Z]+$', uri)

match = re.match(r'https://(.)+\.(managedhsm.azure.net|managedhsm-preview.azure.net|vault.azure.net|vault-int.azure-int.net|vault.azure.cn|managedhsm.azure.cn|vault.usgovcloudapi.net|managedhsm.usgovcloudapi.net|vault.microsoftazure.de|managedhsm.microsoftazure.de|vault.cloudapi.eaglex.ic.gov|vault.cloudapi.microsoft.scloud)(:443)?\/keys/[^\/]+\/[0-9a-zA-Z]+$', uri)
Copy link
Contributor

Choose a reason for hiding this comment

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

please add comment to clarify what you do for regex.


match = re.match(r'^https(.)+\.vault(.)+\/keys\/[^\/]+\/[0-9a-zA-Z]+$', uri)

match = re.match(r'https://(.)+\.(managedhsm.azure.net|managedhsm-preview.azure.net|vault.azure.net|vault-int.azure-int.net|vault.azure.cn|managedhsm.azure.cn|vault.usgovcloudapi.net|managedhsm.usgovcloudapi.net|vault.microsoftazure.de|managedhsm.microsoftazure.de|vault.cloudapi.eaglex.ic.gov|vault.cloudapi.microsoft.scloud)(:443)?\/keys/[^\/]+\/[0-9a-zA-Z]+$', uri)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you support Azure Stack cloud? It seems with your current regex, only some known could are supported. If there are new cloud supported, you will need to change the regex here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently we do not support Azure stack.

@Juliehzl Juliehzl requested review from fengzhou-msft and houk-ms and removed request for mmyyrroonn January 14, 2021 06:16
@Juliehzl
Copy link
Contributor

@fengzhou-msft @houk-ms Could you help check the regex for hsm and keyvault?

@houk-ms
Copy link
Contributor

houk-ms commented Jan 14, 2021

@kingsleyAzure May I ask where did you get the list of the KeyVault endpoints?

I suggest we do some general regex here to fuzzily match vault and managedhsm for convenient maintenance, instead of enumerating all the possible host names.

@Yueren-Wang
Copy link
Contributor Author

Yueren-Wang commented Jan 14, 2021

This was retrieved from a list of supported endpoint which we tested and validated. There might be some other endpoints which gets matched by fuzz logic, but was not tested and validated. that might create trouble.

So that is why I put those validated endpoints into regex because I do not want a match or any other type of endpoints besides the one I listed in regex, even they might exist in current azure offerings.

@Yueren-Wang
Copy link
Contributor Author

The same regex match is done in powershell as well.

Copy link
Contributor

@houk-ms houk-ms left a comment

Choose a reason for hiding this comment

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

LGTM, then.

@Juliehzl Juliehzl merged commit f53441a into Azure:dev Jan 15, 2021
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