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

Azure Service Mesh - BYO CA, Egress Gateway and Mesh Revisions #24453

Merged
merged 5 commits into from
Jun 21, 2023
Merged

Azure Service Mesh - BYO CA, Egress Gateway and Mesh Revisions #24453

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a8cf6b9
Azure Service Mesh - BYO CA and Egress Gateway
deveshdama Jun 15, 2023
513b6a6
prettier fix.
deveshdama Jun 15, 2023
f562172
adding changes for for revisions and meshversioncompatibilityprofile
deveshdama Jun 20, 2023
f184555
adding a missing parameter
deveshdama Jun 20, 2023
1ddd0d9
Changes as per lint failures
deveshdama Jun 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 75 additions & 1 deletion ...ce-manager/Microsoft.ContainerService/aks/preview/2023-06-02-preview/managedClusters.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -601,7 +601,7 @@
"Create Managed Cluster with Node Public IP Prefix": {
"$ref": "./examples/ManagedClustersCreate_NodePublicIPPrefix.json"
},
"Create Managed Cluster with Azure KeyVault Secrets Provider Addon": {
"Create Managed Cluster with Azure Key Vault Secrets Provider Addon": {
"$ref": "./examples/ManagedClustersCreate_AzureKeyvaultSecretsProvider.json"
},
"Create Managed Cluster with FIPS enabled OS": {
Expand Down Expand Up @@ -7710,6 +7710,9 @@
"properties": {
"components": {
"$ref": "#/definitions/IstioComponents"
},
"certificateAuthority": {
"$ref": "#/definitions/IstioCertificateAuthority"
}
}
},
Expand All @@ -7727,6 +7730,14 @@
}
}
},
"egressGateways": {
"type": "array",
"description": "Istio egress gateways.",
"items": {
"$ref": "#/definitions/IstioEgressGateway"
},
"x-ms-identifiers": []
},
"IstioIngressGateway": {
"type": "object",
"description": "Istio ingress gateway configuration. For now, we support up to one external ingress gateway named `aks-istio-ingressgateway-external` and one internal ingress gateway named `aks-istio-ingressgateway-internal`.",
Expand Down Expand Up @@ -7763,6 +7774,69 @@
"enabled"
]
},
"IstioCertificateAuthority": {
"type": "object",
"description": "Istio Service Mesh Certificate Authority (CA) configuration. For now, we only support plugin certificates as described here https://aka.ms/asm-plugin-ca",
"properties": {
"plugin": {
"$ref": "#/definitions/IstioPluginCertificateAuthority"
}
}
},
"IstioPluginCertificateAuthority": {
"type": "object",
"description": "Plugin certificates information for Service Mesh.",
"properties": {
"keyVaultId": {
"type": "string",
"format": "arm-id",
"x-ms-arm-id-details": {
"allowedResources": [
{
"type": "Microsoft.KeyVault/vaults"
}
]
},
"description": "The resource ID of the Key Vault."
},
"certObjectName": {
"type": "string",
"description": "Intermediate certificate object name in Azure Key Vault."
},
"keyObjectName": {
"type": "string",
"description": "Intermediate certificate private key object name in Azure Key Vault."
},
"rootCertObjectName": {
"type": "string",
"description": "Root certificate object name in Azure Key Vault."
},
"certChainObjectName": {
"type": "string",
"description": "Certificate chain object name in Azure Key Vault."
}
}
},
"IstioEgressGateway": {
"type": "object",
"description": "Istio egress gateway configuration.",
"properties": {
"enabled": {
"type": "boolean",
"description": "Whether to enable the egress gateway."
},
"nodeSelector": {
"type": "object",
"additionalProperties": {
"type": "string"
},
"description": "NodeSelector for scheduling the egress gateway."
Comment on lines +8013 to +8018
Copy link
Member

Choose a reason for hiding this comment

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

There's a LintDiff error about this block. A recently added rule AvoidAdditionalProperties attempts to limit such usage. In addition, I checked the swagger of other services, and there are not many similar usages (except for tags).

If you insist on using such a structure (map[string]string), there is a high probability that you need to clarify the specific reasons to the ARM reviewer when performing the final ARM review before the swagger release.

Copy link
Member

Choose a reason for hiding this comment

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

Maps are pretty standard in JSON - I'm surprised the Swagger team is pushing this rule? What's the reasoning behind it? The Azure guidelines state the exact opposite of this:

See here

✔️ YOU MAY use JSON arrays if maintaining an order of values is required. Avoid arrays in other situations since arrays can be difficult and inefficient to work with, especially with JSON Merge Patch where the entire array needs to be read prior to any operation being applied to it.

The only alternative to this map structure here would be an array, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

@FumingZhang @matthchr this is in line with nodeLabels under AgentPoolProperties:

azure-rest-api-specs/specification/containerservice/resource-manager/Microsoft.ContainerService/aks/stable/2023-03-01/managedClusters.json

Line 2828 in e2b5f93

"nodeLabels": {

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitions must not have properties named "additionalProperties" except for user defined tags.

^ I think labels used for NodeSelectors would fall under here?

}
},
"required": [
"enabled"
]
},
"KubernetesSupportPlan": {
"type": "string",
"description": "Different support tiers for AKS managed clusters",
Expand Down