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

#180 k8s in custom Vnet - configurable kubeClusterCidr, kubeServiceCidr, kubeDnsServiceIp #191

Closed
wants to merge 1 commit into from

Conversation

alxlchnr
Copy link

@alxlchnr alxlchnr commented Jan 18, 2017

Problem

In #180 I adressed some harcoded IP adresses which go with the default vnet 10.0.0.0/8. But several users have reported that they have already an existing vnet which often has a different address space than the default vnet. In this case the hard coded values would not fit and cause a non working kubernetes cluster.

Solution

The idea of this PR is as follows: all hardcoded IP-Adresses should be changed to more appropriate values. In order to find the correct values it is necessary to hand over these values to acs-engine before the arm template are generated. Therefore I added three new kubernetes parameters (kubeDnsServiceIp, kubeServiceCidr, kubeClusterCidr). With these parameters I am able to find appropriate values for all previously hardcoded IP adresses.
The introduction of new kubernetes parameters may break old templates because they are not present. To overcome this issue I check for presence of the newly added parameters and if they are not present I work with default values which are the hardcoded IP adresses from the master branch

Why?

@siamak-haschemi described in #303 the manual steps which are necessary to deploy a kubernetes cluster in a custom vnet which has a different address space. In my opinion this is too much manual effort in order to deal with an apparantly frequent use case. The steps @siamak-haschemi described are the same I implemented in this PR. The PR will save future users time and manual adjustments on the generated arm templates.


This change is Reviewable

@msftclas
Copy link

Hi @alxlchnr, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@msftgits
Copy link

Hi, I am closing and re-opening this PR to bump the CLA bot. Sorry for the inconvenience!

@msftgits msftgits closed this Jan 18, 2017
@msftgits msftgits reopened this Jan 18, 2017
@msftclas
Copy link

Hi @alxlchnr, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, MSBOT;

@anhowe
Copy link
Contributor

anhowe commented Jan 24, 2017

@alxlchnr - thanks for your changes. We are CR'ing this PR this week.

@@ -49,6 +49,11 @@
"clientPrivateKey": "clientPrivateKey",
"kubeConfigCertificate": "kubeConfigCertificate",
"kubeConfigPrivateKey": "kubeConfigPrivateKey"
},
"kubeNetworkConfig": {
"kubeDnsServiceIp": "172.17.1.10",
Copy link
Contributor

Choose a reason for hiding this comment

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

How is the backward compatibility handled here ? I am not saying we need backward compatibility or not. Just wondering in your change, how this get treated ?

Copy link
Author

@alxlchnr alxlchnr Jan 25, 2017

Choose a reason for hiding this comment

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

Well I think old files won't have kubeNetworkConfig section. In this case I use the old hardcoded values as defaults in pkg/acsengine/defaults.go
So users with old template files are still able to create the deployment templates.

Copy link
Author

@alxlchnr alxlchnr Feb 21, 2017

Choose a reason for hiding this comment

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

Did my reply answer your question @rjtsdl or do you need more information?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me

@colemickens
Copy link
Contributor

@alxlchnr Is this ready for review? Can you explain the tactic you took with this, and then also rebase it?

Copy link
Contributor

@colemickens colemickens left a comment

Choose a reason for hiding this comment

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

Please address the questions Jing Tao and I have provided and we can proceed. (also rebase!)

@alxlchnr
Copy link
Author

@colemickens just answered Jing Tao's question. Also rebased

@alxlchnr
Copy link
Author

alxlchnr commented Jan 25, 2017

I will a last test, because after rebasing i get a deployment validation error.
error: InvalidTemplate : Deployment template validation failed: 'The template resource 'k8s-master-39411720-0' at line '1' and column '31824' is not valid: The language expression length limit exceeded. Limit: '16384' and actual: '16673'. . Please see https://aka.ms/arm-template-expressions for usage details.'.

It seems that the custom script content now exceeds the max. length for using resource group template functions. Do you have any suggestions on this?

@colemickens
Copy link
Contributor

@alxlchnr Unfortunately this is caused by the customData being too large. One way to workaround is to remove whitespace from the customData. We need to address this soon though, here's an issue: #216

@alxlchnr
Copy link
Author

@colemickens so this is nothing I need to worry about?

@colemickens
Copy link
Contributor

@alxlchnr Depends. If we do nothing we block merging for a non-trivial amount of time. I will attempt to create some sort of workaround today so that we can unblock you and others.

@colemickens
Copy link
Contributor

That having been said, this is not properly rebased - there are 48 commits. I can't really review this in this state.

@alxlchnr
Copy link
Author

You were right, I'm sorry. May be it's better now.

@rjtsdl rjtsdl self-assigned this Jan 26, 2017
@anhowe
Copy link
Contributor

anhowe commented Feb 7, 2017

@alxlchnr - are you able to rebase per Coles request. We want to review and merge.

@alxlchnr
Copy link
Author

alxlchnr commented Feb 7, 2017

@anhowe I just rebased. Please have a look.

@ashb
Copy link
Contributor

ashb commented Apr 4, 2017

Hi! I was after this feature and master seemed to have moved on quite a lot (to do with how the .yaml contents are included in the userdata/gzipping) so I have rebased this on master here: https://github.com/ashb/acs-engine/tree/customVnet -- I have maintained @alxlchnr's authorship on the commit.

I haven't opened a new pull request for this but I can do if that would be easier.

Update: My branch seems to create things in the right IP addresses but something isn't functioning yet. In a sample pod this fails: nc: bad address 'kubernetes.default.svc.cluster.local'

I'll continue digging and see if I can see if it's a problem with this, with my update, or just me doing something silly (quite likely, I messed up the Service Principal first time.)

I'm having problems getting the pod networking working. Without a networkPolicy specified I got the above error and the routeTable was empty, when I tried azure policy it chose IP addresses in the wrong range for the agents and it still didn't work. The testing continues.

@ashb
Copy link
Contributor

ashb commented Apr 5, 2017

Okay so my conclusion is I can't quite get this working as is, at least not using the default networkPolicy - the IPs alone aren't enough as we don't have control over things like the agent_n_Subnet params.

@colemickens
Copy link
Contributor

@ashb No network policy should work fine. If the route table was empty, it points to you not having correct service principal credentials. WIth the "azure" vnet network policy, this problem should actually be even easier since the pod ips will automatically come from the subnet range, so you'd actually need to adjust less flags.

I'd suggest focusing on the "none" network policy for now. Check your SP credentials and see if you get any further. If not, make sure your branch is pushed and ping us and we can see if we spot what's wrong.

@ashb
Copy link
Contributor

ashb commented Apr 5, 2017

@colemickens Pushed as ashb/customVnet. I'm fairly sure I've got the SP right now as the nodes are coming up and docker ps shows the usual components, kubectl get node shows the right number and a status of Ready, but I've had a day of various failure modes and I'm not quite sure where I am with... anything... any more.

A second pair of eyes would be greatly appreciated.

(The goal we are trying to achieve is to have each of our three clusters in a separate IP range. Even though they won't be speaking to each other, they will be speaking to other IaaS components in Azure and our networking team wants them to be distinct.)

@colemickens
Copy link
Contributor

@ashb Are you on the Kubernetes Slack? I can offer some interactive assistance in ~90 minutes. Otherwise...

The cluster you have now with Ready nodes, is the networking working (route table populated)? Or do you still get issues when trying to curl kubernetes for example?

@ashb
Copy link
Contributor

ashb commented Apr 5, 2017

I am, but timezones (Europe/London here) mean I'll be done in about 60 mins. I'm trying again with a clean RG and this as the (part) input to acs-engine:

{
  "apiVersion": "vlabs",
  "properties": {
    "orchestratorProfile": {
      "orchestratorType": "Kubernetes",
      "kubernetesConfig": {
        "dnsServiceIp": "192.168.1.255",
        "serviceCidr": "192.168.1.0/24"
      }
    },

I'll report how I get on. I'm hopeful this one at least should work as the service Cidr range isn't routed outside and is just iptables rules on each node.

(With apologies to @alxlchnr for hijacking his issue and spamming his inbox.)

Update: I'm not sure if this config is meant to work or not, but it doesn't. kubectl get svc shows the kubernetes service on 198.168.1.1, but there is no matching iptables rule.

On a working/stock ACS cluster I see this:

-A KUBE-SERVICES -d 10.0.0.1/32 -p tcp -m comment --comment "default/kubernetes:https cluster IP" -m tcp --dport 443 -j KUBE-SVC-NPX46M4PTMTKRN6Y
-A KUBE-SERVICES -d 10.0.0.10/32 -p udp -m comment --comment "kube-system/kube-dns:dns cluster IP" -m udp --dport 53 -j KUBE-SVC-TCOU7JCQXEZGVUNU

But on the cluster created with the above config the only thing in the KUBE-SERVICES chain is this:

-A KUBE-SERVICES -d 192.168.1.66/32 -p tcp -m comment --comment "kube-system/kubernetes-dashboard: has no endpoints" -m tcp --dport 80 -j REJECT --reject-with icmp-port-unreachable

(and kube-dns pod is failing because it tries to speak to 198.168.1.1 but gets a tcp dial timeout. Not unepxectedly.)

@ashb
Copy link
Contributor

ashb commented Apr 5, 2017

Ah progress! I have found an error message from the kube-proxy logs:

E0405 16:06:26.838902       1 proxier.go:1314] Failed to execute iptables-restore: exit status 2 (iptables-restore v1.4.21: host/network `<kubernetesDnsServiceIp>' not found

I missed a substitution somewhere!

@colemickens
Copy link
Contributor

@ashb That is related to a comment I was going to make -- I think you need to specify all of the custom cidrs, even if you're okay with the defaults. (We should also set defaults and then apply custom, but for the purpose of making progress now, I'd specify them all).

@colemickens
Copy link
Contributor

colemickens commented Apr 5, 2017

Per Slack, it sounds like @ashb has this working. I'm not sure if his their branch is up to date or if there are other fixes that haven't been pushed.

Hopefully tomorrow @ashb can go ahead and send a new PR. It will be great to have this!

@alxlchnr
Copy link
Author

alxlchnr commented Apr 5, 2017

I'm glad you got it working and the discussion on the issue is revived.
Thanks @ashb for your effort.

@anhowe
Copy link
Contributor

anhowe commented Apr 18, 2017

this was completed in k8s in custom Vnet - configurable kubeClusterCidr, kubeServiceCidr, kubeDnsServiceIp #473

@anhowe anhowe closed this Apr 18, 2017
rocketraman added a commit to rocketraman/acs-engine that referenced this pull request Aug 21, 2017
Re-implements the changes originally discussed in Azure#191.

It makes the kubernetes cluster and subnets configurable rather than
using the default 10.0.0.0/8. This is useful, for example, to allow the
kubernetes cluster to communicate with other services within Azure that
may also be within the 10.0.0.0/8 subnet.

Fixes Azure#1161

This reverts commit 49bc7d7.
rocketraman added a commit to rocketraman/acs-engine that referenced this pull request Aug 24, 2017
Re-implements the changes originally discussed in Azure#191.

It makes the kubernetes cluster and subnets configurable rather than
using the default 10.0.0.0/8. This is useful, for example, to allow the
kubernetes cluster to communicate with other services within Azure that
may also be within the 10.0.0.0/8 subnet.

Fixes Azure#1161

This reverts commit 49bc7d7.
rocketraman added a commit to rocketraman/acs-engine that referenced this pull request Aug 24, 2017
Re-implements the changes originally discussed in Azure#191.

It makes the kubernetes cluster and subnets configurable rather than
using the default 10.0.0.0/8. This is useful, for example, to allow the
kubernetes cluster to communicate with other services within Azure that
may also be within the 10.0.0.0/8 subnet.

Fixes Azure#1161

This reverts commit 49bc7d7.
rocketraman added a commit to rocketraman/acs-engine that referenced this pull request Aug 25, 2017
Re-implements the changes originally discussed in Azure#191.

It makes the kubernetes cluster and subnets configurable rather than
using the default 10.0.0.0/8. This is useful, for example, to allow the
kubernetes cluster to communicate with other services within Azure that
may also be within the 10.0.0.0/8 subnet.

Fixes Azure#1161

This reverts commit 49bc7d7.
jackfrancis pushed a commit that referenced this pull request Aug 25, 2017
Re-implements the changes originally discussed in #191.

It makes the kubernetes cluster and subnets configurable rather than
using the default 10.0.0.0/8. This is useful, for example, to allow the
kubernetes cluster to communicate with other services within Azure that
may also be within the 10.0.0.0/8 subnet.

Fixes #1161

This reverts commit 49bc7d7.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants