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

add NetworkPluginMode to ManagedCluster #18735

Merged
merged 4 commits into from
Apr 21, 2022
Merged

add NetworkPluginMode to ManagedCluster #18735

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cd0b2f8
add NetworkPluginMode to ManagedCluster
tyler-lloyd Apr 18, 2022
9ab092d
add overlay example
tyler-lloyd Apr 19, 2022
2af9ad9
remove example
tyler-lloyd Apr 20, 2022
d92e311
only keep overlap as option
tyler-lloyd Apr 20, 2022
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
25 changes: 25 additions & 0 deletions ...source-manager/Microsoft.ContainerService/preview/2022-04-02-preview/managedClusters.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3343,6 +3343,10 @@
"$ref": "#/definitions/NetworkPlugin",
"description": "Network plugin used for building the Kubernetes network."
},
"networkPluginMode": {
"$ref": "#/definitions/NetworkPluginMode",
"description": "Network plugin mode used for building the Kubernetes network."
},
"networkPolicy": {
"$ref": "#/definitions/NetworkPolicy",
"description": "Network policy used for building the Kubernetes network."
Expand Down Expand Up @@ -6040,6 +6044,10 @@
"$ref": "#/definitions/NetworkPlugin",
"description": "networkPlugin for managed cluster snapshot."
},
"networkPluginMode": {
"$ref": "#/definitions/NetworkPluginMode",
"description": "networkPluginMode for managed cluster snapshot."
Copy link
Member

Choose a reason for hiding this comment

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

NetworkPluginMode (cap first letter)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any reason? shouldn't it follow the other properties in networkProfile?

Copy link
Member

Choose a reason for hiding this comment

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

I mean in the doc not the API. And... I suppose yes it should but the other fields in NetworkProfileForSnapshot don't follow the pattern of the broader Swagger I don't think? (I thought most everything was capitalized first?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean in the doc

which doc are you referring? I did notice that the enum "name" was not capitalized so here's a pr for fix that: #18884

Copy link
Member

@matthchr matthchr May 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

networkPluginMode for managed cluster snapshot.

to

NetworkPluginMode for managed cluster snapshot.

Was all I was suggesting. You're right that the other descriptions here in this particular type don't cap the first letter but I think they're all wrong actually. It's really minor though so if you think it makes more sense to leave it as is to match the other descriptions for this type feel free to do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, I added that change in the open PR linked above just now.

},
"networkPolicy": {
"$ref": "#/definitions/NetworkPolicy",
"description": "networkPolicy for managed cluster snapshot."
Expand Down Expand Up @@ -6083,6 +6091,23 @@
},
"description": "Network plugin used for building the Kubernetes network."
},
"NetworkPluginMode": {
"type": "string",
"enum": [
"Overlay"
],
"x-ms-enum": {
"name": "networkPluginMode",
"modelAsString": true,
"values": [
{
"value": "Overlay",
"description": "Pods are given IPs from the PodCIDR address space but use Azure Routing Domains rather than Kubenet reference plugins host-local and bridge."
}
]
},
"description": "The mode the network plugin should use."
Copy link
Member

Choose a reason for hiding this comment

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

Document the default if it's not specified? Also can this be used with only Azure networking mode? If so, say that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the default value will be "unspecified" but we weren't planning to make that an enum value. Still state it in the description?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, what does unspecified mean? Something different than overlay? If so then yea that's awkward to document... IMO in a perfect world we'd have a value that represented what the mode is "by default", but if we don't have that then yea leaving this undocumented is fine I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the future it will most likely be "nodeSubnet" but for now overlay is the only possible plugin mode so having everything else be "unspecified" is easier to deal with.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah in that case for now having this undocumented is fine IMO. In the future when there is a representable default for everything we should document it here, but it sounds like we're not there yet.

},
"NetworkPolicy": {
"type": "string",
"enum": [
Expand Down