Skip to content

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

Merged
zhoxing-ms merged 30 commits intoAzure:mainfrom
UtheMan:mikolaj/add-custom-ca-to-mc
Dec 12, 2022
Merged

{AKS} Add custom ca certs to aks create and aks update#5561
zhoxing-ms merged 30 commits intoAzure:mainfrom
UtheMan:mikolaj/add-custom-ca-to-mc

Conversation

@UtheMan
Copy link
Copy Markdown
Contributor

@UtheMan UtheMan commented Nov 18, 2022


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 wangzelin007 and zhoxing-ms 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
Copy Markdown
Collaborator

yonzhan commented Nov 18, 2022

AKS

Copy link
Copy Markdown
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

Copy link
Copy Markdown
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))

Copy link
Copy Markdown
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.

@UtheMan UtheMan force-pushed the mikolaj/add-custom-ca-to-mc branch from a97d8eb to 0109638 Compare November 23, 2022 23:54
@FumingZhang
Copy link
Copy Markdown
Member

PR #5575 would resolve the serialization issue.

@UtheMan UtheMan force-pushed the mikolaj/add-custom-ca-to-mc branch from 6d22eba to ff16238 Compare November 29, 2022 04:52
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread src/aks-preview/azext_aks_preview/_params.py Outdated
Comment thread src/aks-preview/azext_aks_preview/_params.py Outdated
@UtheMan UtheMan force-pushed the mikolaj/add-custom-ca-to-mc branch from b4a7e90 to 1e86430 Compare December 8, 2022 19:20
Comment thread src/aks-preview/azext_aks_preview/_help.py Outdated
Comment thread src/aks-preview/azext_aks_preview/_help.py Outdated
Copy link
Copy Markdown
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

Comment thread src/aks-preview/HISTORY.rst
Comment thread src/aks-preview/azext_aks_preview/_params.py
Comment thread src/aks-preview/azext_aks_preview/tests/latest/test_managed_cluster_decorator.py Outdated
Comment thread src/aks-preview/azext_aks_preview/tests/latest/test_managed_cluster_decorator.py Outdated
@zhoxing-ms zhoxing-ms merged commit b860ff5 into Azure:main Dec 12, 2022
@azclibot
Copy link
Copy Markdown
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

Labels

AKS Auto-Assign Auto assign by bot

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants