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.

Sign up for GitHub

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

Jump to bottom

{AKS} Add custom ca certs to aks create and aks update #5561

Merged
merged 30 commits into from
Dec 12, 2022
Merged

{AKS} Add custom ca certs to aks create and aks update #5561

merged 30 commits into from
Dec 12, 2022

Conversation

UtheMan
Copy link
Contributor

@UtheMan UtheMan commented Nov 18, 2022 •
edited

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

Related command

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 pull request is merged into main branch, a new pull request will be created to update src/index.json automatically.
The precondition is to put your code inside this repository and upgrade the version in the pull request but do not modify src/index.json.

@ghost ghost requested review from zhoxing-ms and wangzelin007 November 18, 2022 01:24
@ghost ghost assigned zhoxing-ms Nov 18, 2022
@ghost ghost added this to the Nov 2022 (2022-12-06) milestone Nov 18, 2022
@ghost ghost added the Auto-Assign Auto assign by bot label Nov 18, 2022
@ghost ghost requested a review from yonzhan November 18, 2022 01:24
@ghost ghost added the AKS label Nov 18, 2022
@yonzhan
Copy link
Collaborator

yonzhan commented Nov 18, 2022

AKS

FumingZhang
FumingZhang reviewed Nov 22, 2022
View reviewed changes
Copy link
Member

@FumingZhang FumingZhang left a comment

Choose a reason for hiding this comment

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

For all preview API versions since 2022-09-02-preview, a new property named ManagedClusterSecurityProfileCustomCATrustCertificates is added in containerservice(AKS). The property is declared with type array, where the value of each item is of type string and format byte. Thus, the generated SDK declares the property as type [btyearray]. It could be seen that the serialization helpers that comes with the SDK could handle properties of type bytearray. But the serialization processing in knack is not competent, which would result in the following error

image

I've opened a PR #268 to fix the issue. Any comments are welcome. cc @zhoxing-ms, @jiasli

src/aks-preview/azext_aks_preview/managed_cluster_decorator.py Outdated
custom_ca_certs_file_path
)
)
custom_ca_certs = read_file_content(custom_ca_certs_file_path)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
custom_ca_certs = read_file_content(custom_ca_certs_file_path)
custom_ca_certs = str.encode(read_file_content(custom_ca_certs_file_path))

src/aks-preview/azext_aks_preview/tests/latest/data/certs.txt Outdated
@@ -0,0 +1 @@
LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUNsakNDQVg0Q0NRQzl6VUFncXFxcld6QU5CZ2txaGtpRzl3MEJBUXNGQURBTk1Rc3dDUVlEVlFRR0V3SlEKVERBZUZ3MHlNakE1TVRRd05qSXpNamRhRncweU1qQTVNVFV3TmpJek1qZGFNQTB4Q3pBSkJnTlZCQVlUQWxCTQpNSUlCSWpBTkJna3Foa2lHOXcwQkFRRUZBQU9DQVE4QU1JSUJDZ0tDQVFFQW9wS05JSWJ2dmNQQ3c5ZmM0S0xYCktEdFJab2JwNUwrLzFoQ04rM09HaGs1TnZTVHBTVXJGaWZ4cWMwbzNJRjdZa08zSzFuMmpBdkNNWE8xNkJmOWIKT0FSN1ZrQ3J3R0ZWa1hOak00d3ZYQVg4Q05OdmpxZDF6RFBYU0tkRTdXZDhrM2ZUeng2bkdVTTBVZ2xqSVBoSAp5aDRhNFp1amQ1SWcyUC9aU1gwcEdKbTQ3SlR0TXU3TURGSFZNNXdSV2NDck4vSDBUQ1lQSXZFT3MwQjhBWnhjCnAzVEY3QTZ2ZVQ1VTlwVmhRM1hsOUpONkx2dkxxUHhHM2VhMTByZHY5RFl6YWlYbVNZM3VqSTNSaTFRMTF1V0MKZHRyRklwRnU1Y0hXMk9CVytqQlh4TDB2OHhRbWt4VExpazRCUi9QTENsMzB3eEtRTnNxM3BqRGd1MG11dEt1dQo1d0lEQVFBQk1BMEdDU3FHU0liM0RRRUJDd1VBQTRJQkFRQVZFQUlzL2hMd1RWQ3dwRVhkb1hSMjRMZWxOTnVCCi84cHRLNmx5akUxMVh3Zk1OM3l5N0Yyb0IxbHJBNHJJM2o5b2JwRHNIREpCTkIxM2JpL2xLZ3ZBY2JJbi9UeXUKUktUaHRVZFBneE5ucURVeXhuYjNPb2ZNRjNnQjhlUFR1K2pacGQzenJsRXV4ZGw0MEJ5QVRDU3lPZ1I2REhNdApTRGQram95cG5PSEZBZVNNK1YwQWFUZWxYU0NLOU9BV1NBcDVlNlM3NmE2bFJ4K0Q1WGwzaEJlZEJJMHRYNTloCnRFWU5FR1phUkVsRlU3OVdjRUYwY0grWlcwK2pKOTV4RTN0aFpmZlJ6NlFJNnlGNjNtOGFDOWw5YmJkSlMyemcKWXY4VytsQ1ppLy9PRGVPQlV1Z3IrK3o5dWordkdrNDdKRFNwVjBuNEpPdW4zQUxVREowZ3FtY1MKLS0tLS1FTkQgQ0VSVElGSUNBVEUtLS0tLQo=
Copy link
Member

Choose a reason for hiding this comment

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

No need to encode the content in advance.

@FumingZhang
Copy link
Member

PR #5575 would resolve the serialization issue.

FumingZhang
FumingZhang reviewed Nov 29, 2022
View reviewed changes
src/aks-preview/azext_aks_preview/tests/latest/test_aks_commands.py Outdated
create_cmd = 'aks create --resource-group={resource_group} --name={name} ' \
'--nodepool-name {node_pool_name} -c 1 ' \
'--ssh-key-value={ssh_key_value} ' \
'--aks-custom-headers=AKSHTTPCustomFeatures=Microsoft.ContainerService/CustomCATrustPreview' \
Copy link
Member

Choose a reason for hiding this comment

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

missing trailing space, two options are combined together.

Suggested change
'--aks-custom-headers=AKSHTTPCustomFeatures=Microsoft.ContainerService/CustomCATrustPreview' \
'--aks-custom-headers=AKSHTTPCustomFeatures=Microsoft.ContainerService/CustomCATrustPreview ' \

@FumingZhang
Copy link
Member

You'll also need the recording file for the new test case test_aks_create_add_nodepool_with_custom_ca_trust_certificates.

Queued a pipeline to run it. Still running. After it succeeds, you could download the recording file from pipeline artifact and commit it.

@FumingZhang
Copy link
Member

You'll also need the recording file for the new test case test_aks_create_add_nodepool_with_custom_ca_trust_certificates.

Queued a pipeline to run it. Still running. After it succeeds, you could download the recording file from pipeline artifact and commit it.

Test failed again 😢
The transform is not applied to LRO result. Opened this draft PR #24763 to fix the issue. Pending review from cli team.

FumingZhang
FumingZhang approved these changes Dec 9, 2022
View reviewed changes
Copy link
Member

@FumingZhang FumingZhang left a comment

Choose a reason for hiding this comment

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

LGTM

src/aks-preview/HISTORY.rst Show resolved Hide resolved
src/aks-preview/azext_aks_preview/_params.py Show resolved Hide resolved
@zhoxing-ms zhoxing-ms merged commit b860ff5 into Azure:main Dec 12, 2022
@azclibot
Copy link
Collaborator

[Release] Update index.json for extension [ aks-preview ] : https://dev.azure.com/azclitools/internal/_build/results?buildId=20181&view=results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FumingZhang FumingZhang FumingZhang approved these changes

@zhoxing-ms zhoxing-ms zhoxing-ms approved these changes

@andyliuliming andyliuliming Awaiting requested review from andyliuliming andyliuliming is a code owner

@wangzelin007 wangzelin007 Awaiting requested review from wangzelin007

@yonzhan yonzhan Awaiting requested review from yonzhan

@calvinhzy calvinhzy Awaiting requested review from calvinhzy

@kairu-ms kairu-ms Awaiting requested review from kairu-ms kairu-ms is a code owner

@jsntcy jsntcy Awaiting requested review from jsntcy

@jiasli jiasli Awaiting requested review from jiasli

@evelyn-ys evelyn-ys Awaiting requested review from evelyn-ys

@necusjz necusjz Awaiting requested review from necusjz

Assignees

@zhoxing-ms zhoxing-ms

Labels
AKS Auto-Assign Auto assign by bot
Projects
None yet
Milestone
Dec 2022 (2023-01-10)
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@UtheMan @yonzhan @FumingZhang @azclibot @zhoxing-ms