Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Add ACI Connector #1844

Merged
merged 17 commits into from
Dec 1, 2017
Merged

Add ACI Connector #1844

merged 17 commits into from
Dec 1, 2017

Conversation

sozercan
Copy link
Member

What this PR does / why we need it:
Adds ACI Connector as a configurable add-on in ACS-engine

@@ -130,8 +131,13 @@ var (
},
//KubernetesSpecConfig - Due to Chinese firewall issue, the default containers from google is blocked, use the Chinese local mirror instead
KubernetesSpecConfig: KubernetesSpecConfig{
KubernetesImageBase: "crproxy.trafficmanager.net:6000/google_containers/",
TillerImageBase: "crproxy.trafficmanager.net:6000/kubernetes-helm/",
KubernetesImageBase: "crproxy.trafficmanager.net:6000/google_containers/",
Copy link
Member

Choose a reason for hiding this comment

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

Let's punt on modifying the China cloud spec, we usually defer to the China team to manage this. If you'd like, you could open up an issue to add ACI connector addon capability to China cloud and we can work with them on it.

Name string `json:"name,omitempty"`
Enabled *bool `json:"enabled,omitempty"`
Containers []KubernetesContainerSpec `json:"containers,omitempty"`
Environment []KubernetesEnvironmentSpec `json:"environment,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to make this a map[string]string so it has general purpose value. KubernetesAddon is meant to be a generic interface for configuring an addon.

Copy link
Member

@jackfrancis jackfrancis Nov 29, 2017

Choose a reason for hiding this comment

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

How about Config map[string]string `json:"config,omitempty"

@ghost ghost assigned sozercan Nov 29, 2017
@ghost ghost added the in progress label Nov 29, 2017
@@ -136,6 +137,7 @@ var kubernetesAddonYamls15 = map[string]string{
"MASTER_ADDON_KUBERNETES_DASHBOARD_DEPLOYMENT_B64_GZIP_STR": "kubernetesmasteraddons-kubernetes-dashboard-deployment1.5.yaml",
masterAddonAzureStorageClasses: masterAddonAzureStorageClassesFileUnmanaged,
"MASTER_ADDON_TILLER_DEPLOYMENT_B64_GZIP_STR": "kubernetesmasteraddons-tiller-deployment1.5.yaml",
"MASTER_ADDON_ACI_CONNECTOR_DEPLOYMENT_B64_GZIP_STR": "kubernetesmasteraddons-aci-connector-deployment1.5.yaml",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this separate file? I don't think we are supporting K8s 1.5 and your yaml is the same for the 1.5 and the non-1.5 version, yes?

DefaultACIConnectorAddonsConfig = api.KubernetesAddon{
Name: DefaultACIConnectorAddonName,
Enabled: pointerToBool(api.DefaultACIConnectorAddonEnabled),
Config: map[string]string{},
Copy link
Member

@jackfrancis jackfrancis Nov 30, 2017

Choose a reason for hiding this comment

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

Add the default vals to Config here as well. E.g.,

Config: map[string]string{
    "clientId": <defaultVal>,
    "clientKey": <defaultVal>,
   <etc...>
}

@ghost ghost assigned jackfrancis Nov 30, 2017
@@ -635,6 +635,10 @@ func convertAddonsToAPI(v *vlabs.KubernetesConfig, a *KubernetesConfig) {
MemoryLimits: v.Addons[i].Containers[j].MemoryLimits,
})
}

Copy link
Member

Choose a reason for hiding this comment

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

@sozercan I added the below lines to adapt the KubernetesAddons default assignment convenience function to include Config key/vals as well

Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @sozercan !

@jackfrancis jackfrancis merged commit 6d1a3c2 into Azure:master Dec 1, 2017
@ghost ghost removed the in progress label Dec 1, 2017
@sozercan sozercan deleted the aci-connector branch December 1, 2017 17:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants