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.

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

Updating Calico manifests to Calico release 2.6.2 #3869

Merged
merged 1 commit into from
Nov 22, 2017

Conversation

chrislovecnm
Copy link
Contributor

@chrislovecnm chrislovecnm commented Nov 16, 2017

Renamed the k8s-1.8 manifest to a k8s-1.7. This is required because of config
change that occurs between k8s 1.6 and k8s 1.7. This refactor will also
be re-used when Calico Kubernetes data source support is added to kops.
Updated bootstrapchannelbuilder with the new Calico version numbers.

The diffs for the k8s-1.6 version is pretty rough, tried to make it cleaner, but nada.

FIXES: #3866
FIXES: #3867

Line: https://github.com/kubernetes/kops/compare/master...chrislovecnm:calico-2.6-update?expand=1#diff-891cbc61587adb202b66b7c9bc6896daR209 is why Calico would not start on k8s 1.6 - thanks @caseydavenport

TODO
Testing K8s versions

  • test 1.5
  • test 1.6
  • test 1.7
  • test 1.8

/cc @blakebarnett @itajaja

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 16, 2017
@k8s-ci-robot
Copy link
Contributor

@chrislovecnm: GitHub didn't allow me to request PR reviews from the following users: itajaja.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

Renamed the k8s-1.8 manifest to a k8s-1.7. This is required because of config
change that occurs between k8s 1.6 and k8s 1.7. This refactor will also
be re-used when Calico Kubernetes data source support is added to kops.
Updated bootstrapchannelbuilder with the new Calico version numbers.

The diffs for the k8s-1.6 version is pretty rough, tried to make it cleaner, but nada.

FIXES: #3866

Line: https://github.com/kubernetes/kops/compare/master...chrislovecnm:calico-2.6-update?expand=1#diff-891cbc61587adb202b66b7c9bc6896daR209 is why Calico would not start on k8s 1.6 - thanks @caseydavenport

TODO
Testing K8s versions

  • test 1.5
  • test 1.6
  • test 1.7
  • test 1.8

/cc @blakebarnett @itajaja

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 16, 2017
@chrislovecnm chrislovecnm added this to the 1.8.0 milestone Nov 16, 2017
cni_network_config: |-
{
"name": "k8s-pod-network",
"cniVersion": "0.3.0",
"cniVersion": "0.1.0",
Copy link
Member

@caseydavenport caseydavenport Nov 16, 2017

Choose a reason for hiding this comment

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

Does this work?

I'd expect you to need to also remove the plugin list part.

For example:

    {
       "name": "k8s-pod-network",
       "cniVersion": "0.1.0",
       "type": "calico",
       "etcd_endpoints": "__ETCD_ENDPOINTS__",
       "log_level": "info",
       "ipam": {
         "type": "calico-ipam"
       },
       "policy": {
         "type": "k8s",
         "k8s_api_root": "https://__KUBERNETES_SERVICE_HOST__:__KUBERNETES_SERVICE_PORT__",
         "k8s_auth_token": "__SERVICEACCOUNT_TOKEN__"
       },
       "kubernetes": {
         "kubeconfig": "/etc/cni/net.d/__KUBECONFIG_FILENAME__"
       }
     }

Copy link
Member

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

I think this got switched because of this: projectcalico/calico#742 could you verify that's still the best version?

Copy link
Member

Choose a reason for hiding this comment

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

It depends on the Kubernetes version in use.

Anything less than k8s v1.7 (so v1.5, v1.6) will need CNI version 0.1.0 with the config specified here.

For Kubernets v1.7+, CNI 0.3.0 with the chained CNI config is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the template for 1.7, let me check the 1.7 template

Copy link
Member

Choose a reason for hiding this comment

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

/networking.projectcalico.org/k8s-1.6.yaml.template

Looks like it's the 1.6 template?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@caseydavenport confused. What am I missing?

Copy link
Member

Choose a reason for hiding this comment

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

It could be me that is confused, but AFAICT this is the k8s v1.6 manifest template, but the CNI config has plugin chaining in it which isn't supported until k8s v1.7.

If I'm right, then the config needs to be changed to look like the v1.5. config does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clear we need

# This ConfigMap is used to configure a self-hosted Calico installation.
kind: ConfigMap
apiVersion: v1
metadata:
  name: calico-config
  namespace: kube-system
data:
  # The calico-etcd PetSet service IP:port
  etcd_endpoints: "{{ $cluster := index .EtcdClusters 0 -}}
                      {{- range $j, $member := $cluster.Members -}}
                          {{- if $j }},{{ end -}}
                          http://etcd-{{ $member.Name }}.internal.{{ ClusterName }}:4001
                      {{- end }}"

# Configure the Calico backend to use.
  calico_backend: "bird"

  # The CNI network configuration to install on each node.
  cni_network_config: |-
    {
        "name": "k8s-pod-network",
        "type": "calico",
        "etcd_endpoints": "__ETCD_ENDPOINTS__",
        "log_level": "info",
        "ipam": {
            "type": "calico-ipam"
        },
        "policy": {
            "type": "k8s",
             "k8s_api_root": "https://__KUBERNETES_SERVICE_HOST__:__KUBERNETES_SERVICE_PORT__",
             "k8s_auth_token": "__SERVICEACCOUNT_TOKEN__"
        },
        "kubernetes": {
            "kubeconfig": "/etc/cni/net.d/__KUBECONFIG_FILENAME__"
        }
    }

ConfigMap in the 1.6 manifest?

@@ -237,30 +249,75 @@ spec:

---

# This manifest deploys the Calico policy controller on Kubernetes.
# See https://github.com/projectcalico/k8s-policy
# This deployment turns off the old "policy-controller". It should remain at 0 replicas, and then
Copy link
Member

@caseydavenport caseydavenport Nov 16, 2017

Choose a reason for hiding this comment

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

Not sure how upgrade works in kops, but we'll only need this here if it's possible to do an in-place upgrade from a v1.6 cluster with Calico v2.4 to a cluster with Calico v2.6.

It won't hurt either way.

Choose a reason for hiding this comment

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

Yeah should probably remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@blakebarnett we need to leave it, as an upgrade will not work without it. We do a kubectl apply, which will not delete old stuff.

Choose a reason for hiding this comment

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

no, I mean the new 2.6.2 version of calico-kube-controllers should be removed and we should run v2.4.x on k8s 1.6 with calico-policy-controller because it's the most widely tested configuration.

Copy link
Contributor Author

@chrislovecnm chrislovecnm Nov 16, 2017

Choose a reason for hiding this comment

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

We cannot run calico-policy-controller on a kops weave cluster. It does not work.

Choose a reason for hiding this comment

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

of course not... :) This is only part of the calico addon.

@@ -69,7 +69,7 @@ spec:
# container programs network policy and routes on each
# host.
- name: calico-node
image: quay.io/calico/node:v2.4.0
image: quay.io/calico/node:v2.6.2
Copy link
Member

Choose a reason for hiding this comment

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

If you've managed to test this and show that it works, then I'm OK with it, but note that we haven't extensively tested Calico v2.6.0 on Kubernetes < v1.6, so it's sort of uncharted territory.

I'm not aware of a reason it won't work though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we upgrade then? I think a CVE was fixed in Calico?

Choose a reason for hiding this comment

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

I think we should stick with the most tested configuration, I assume a CVE would have been fixed in 2.4.x?

Copy link
Contributor Author

@chrislovecnm chrislovecnm Nov 16, 2017

Choose a reason for hiding this comment

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

We need to go to the new policy controller for kops and gossip as well. The old policy controller does not work :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

smoke tested 1.5.8 k8s with go-guest book ... worked like a champ. Will upgrade and let Calico support or tell the users to upgrade clusters :)

Copy link
Member

Choose a reason for hiding this comment

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

@chrislovecnm sounds good to me.

@chrislovecnm chrislovecnm changed the title [WIP] Updating Calico manifests to Calico release 2.6.2 Updating Calico manifests to Calico release 2.6.2 Nov 16, 2017
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 16, 2017
@chrislovecnm
Copy link
Contributor Author

/approve

@caseydavenport or @blakebarnett can I get a lgtm?

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 16, 2017
@chrislovecnm
Copy link
Contributor Author

@justinsb I have not tested with gossip ... let me do that as well

@chrislovecnm
Copy link
Contributor Author

gossip and k8s 1.5 -> k8s 1.8 are happy now. @blakebarnett @andrewsykim @geojaz PTAL

@chrislovecnm
Copy link
Contributor Author

@caseydavenport so with the older version of Calico you cannot run a k8s 1.5 cluster with kops gossip. Does Calico support version 2.6.2 with k8s version 1.5.x?

@blakebarnett
Copy link

I don't feel like forcing the upgrade to 2.6.2 for < k8s 1.7 is a good idea. If the calico folks disagree then I'm happy.

@chrislovecnm
Copy link
Contributor Author

/approve

@caseydavenport
Copy link
Member

I don't feel like forcing the upgrade to 2.6.2 for < k8s 1.7 is a good idea. If the calico folks disagree then I'm happy.

@blakebarnett @chrislovecnm I don't fully understand the implications of not supporting the gossip DNS, but I agree that I'd feel somewhat uncomfortable forcing upgrades to Calico v2.6.2 on older Kubernetes versions (< v1.6). The only reason we should do this is if the gossip DNS is a hard requirement.

Like I said, it should "just work", but the Calico team doesn't generally support or test such a drastic mismatch of versions, so if there is an issue we'd likely just have to say "please upgrade to a modern kubernetes release".

TL;DR - I agree with @blakebarnett - we shouldn't force an upgrade of Calico on older k8s versions unless not doing so will break existing users.

@chrislovecnm
Copy link
Contributor Author

/approve cancel

@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 17, 2017
@chrislovecnm chrislovecnm force-pushed the calico-2.6-update branch 2 times, most recently from 0123e13 to 85e2117 Compare November 18, 2017 23:32
@chrislovecnm
Copy link
Contributor Author

@caseydavenport @blakebarnett I backed out the pre-1.5 changes. We will need release notes about using gossip.

@caseydavenport do I need more changes on the 1.6 manifest?

@chrislovecnm
Copy link
Contributor Author

@blakebarnett @caseydavenport PTAL

/approve

If I get a /lgtm it will merge. @justinsb is waiting on final review from you guys

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2017
etcd_endpoints: "{{ $cluster := index .EtcdClusters 0 -}}
{{- range $j, $member := $cluster.Members -}}
{{- if $j }},{{ end -}}
http://etcd-{{ $member.Name }}.internal.{{ ClusterName }}:4001
{{- end }}"

# Configure the Calico backend to use.
# Configure the Calico backend to use.
Copy link
Member

@caseydavenport caseydavenport Nov 22, 2017

Choose a reason for hiding this comment

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

comment indentation is off :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😆 yes it is ... fixing. After the push /lgtm?

Renamed the k8s-1.8 manifest to a k8s-1.7. This is required because of config
change that occurs between k8s 1.6 and k8s 1.7. This refactor will also
be re-used when Calico Kubernetes data source support is added to kops.
Updated bootstrapchannelbuilder with the new Calico version numbers.
@justinsb
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 22, 2017
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm, justinsb

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:
  • OWNERS [chrislovecnm,justinsb]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

Copy link
Member

@caseydavenport caseydavenport left a comment

Choose a reason for hiding this comment

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

lgtm

@k8s-github-robot
Copy link

Automatic merge from submit-queue.

@k8s-github-robot k8s-github-robot merged commit 8eac358 into kubernetes:master Nov 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. blocks-next cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants