Manifest templates #1884

spikecurtis · 2018-04-12T23:28:56Z

Description

Reworks generation of various calico.yaml and canal.yaml manifests to be generated with Jekyll's templating system.

Todos

Tests

Release Note

None required

spikecurtis · 2018-04-12T23:29:18Z

@ozdanborne you can just look at the second commit in this series

ozdanborne · 2018-04-13T17:03:53Z

@spikecurtis Just wanted to bring up whitespace trimming:

In Liquid, you can include a hyphen in your tag syntax {{-, -}}, {%-, and -%} to strip whitespace from the left or right side of a rendered tag.

There's some places in the manifest where you've put if statements on their own line. By adding a whitespace trimmer, there won't be a blank line in the rendered manifests.

Other places, some of the if logic can be indented and on different lines if you use the whitespace trimmers, making it easier to read.

I'll make some individual comments to point out examples.

ozdanborne · 2018-04-13T17:04:52Z

When do you plan to get this in? You may want to announce a freeze on manifest changes soon, otherwise you'll have a hard time rebasing.

ozdanborne · 2018-04-13T17:11:49Z

_includes/master/manifests/calico-config.yaml

@@ -0,0 +1,127 @@
+# This ConfigMap is used to configure a self-hosted {{include.prodname}} installation.


How about a jekyll comment at the top of each of these include manifests that explains which .include flags it accepts. For this manifest, for example:

{% comment %} calico-config.yaml acccepts the following include flags: | Name | Accepted Values | |-------------|-----------------| | datastore | kdd, etcd | | typha | true, false | | network | calico, flannel | | calico_ipam | true, false | {% endcomment %}

spikecurtis · 2018-04-13T17:18:29Z

I planned on getting it in ASAP. I'm assuming that CNX doesn't use these manifests (or not master, at any rate).

ozdanborne · 2018-04-13T17:18:47Z

master/getting-started/kubernetes/installation/hosted/istio-inject-configmap.yaml

+        image: docker.io/istio/proxy_init:0.6.0
+        args:
+        - "-p"
+        - {{ .MeshConfig.ProxyListenPort }}


I think jekyll is going to try to render this. So you'll need to put it in a {% raw %}{{ .MeshConfig.ProxyListenPort }}{% endraw %}

Never mind! Looks like you did above 👍

ozdanborne · 2018-04-13T18:13:10Z

I'm worried that Canal and Calico are too different to control with if/else. Perhaps we want to split the configmap into calico-config.yaml and canal-config.yaml, and consider doing the same for the Daemonset as well.

Edit: I'm less certain about splitting the daemonset but the calico-config and canal-config seem like it would be a clean way to simplify it.

spikecurtis · 2018-04-13T18:18:08Z

Canal + etcd seems to be the odd one. Canal + KDD is very similar to Calico + KDD.

I was wondering if it is really necessary to be that way, and maybe Canal + etcd just never got updated...

ozdanborne · 2018-04-13T18:24:37Z

_includes/master/manifests/calico-node.yaml

+            # No IP address needed.
+            - name: IP
+              value: ""
+{% else %}


This 'else' is for the if include.network == 'calico' on line 112. It is quite difficult to track that down since these if statements aren't indented.

Since I don't think jekyll has a way for us to indent the if/else logic, perhaps we should avoid nested if statements and long if/else conditions when possible, instead opting to end them and explitly starting a new if with its exact conditions. Does that make sense?

I've indented nested logic and used {%- to prevent the nesting whitespace from showing up in the final rendered file.

ozdanborne · 2018-04-17T21:15:10Z

master/getting-started/kubernetes/installation/manifests/allow-istio-pilot.yaml

@@ -0,0 +1,19 @@
+# This NetworkPolicy allows pods egress access the to 


Any reason this is in manifests/ instead of manifests/app-layer-policy?

Same for the other few manifests here...

ozdanborne · 2018-04-17T21:39:38Z

_includes/master/manifests/calico-config.yaml

+{%- endif %}
+{%- if include.network == "calico" %}
+  # Configure the {{site.prodname}} backend to use.
+  calico_backend: "bird"


not super important, but this is smushing with typha_service_name in the kdd manifest:

data: # To enable Typha, set this to "calico-typha" *and* set a n # below. We recommend using Typha if you have more than 50 # essential. typha_service_name: "none" # Configure the Calico backend to use. calico_backend: "bird"

never mind, forget i mentioned this. there's a few places i'm seeing this, but we can fix it later.

ozdanborne · 2018-04-17T21:52:42Z

master/getting-started/kubernetes/installation/hosted/canal/canal-etcd.yaml

-  namespace: kube-system
-data:
-  # Configure this with the location of your etcd cluster.
-  etcd_endpoints: "https://127.0.0.1:2379"


interestingly, looks like canal etcd used https instead of http. I wouldn't have thought that https would work...

ozdanborne · 2018-04-17T21:55:03Z

master/getting-started/kubernetes/installation/hosted/canal/canal-etcd.yaml

-kind: DaemonSet
-apiVersion: extensions/v1beta1
-metadata:
-  name: canal-node


looks like the name of the daemonset changes to canal (instead of canal-node) in the new manifests. This would break upgrades for existing users applying our manifests directly...

ozdanborne · 2018-04-17T22:07:26Z

master/getting-started/kubernetes/installation/hosted/canal/canal-etcd.yaml

-      hostNetwork: true
-      tolerations:
-        # Make sure canal node can be scheduled on all nodes.
-        - effect: NoSchedule


looks like we lost this toleration in the new canal manifest. Not sure what our stance is on running kube-controllers on master...

Casey's stance is

¯\_(ツ)_/¯

ozdanborne · 2018-04-17T22:26:29Z

_includes/master/manifests/calico-node.yaml

+            # The default IPv4 pool to create on startup if none exists. Pod IPs will be
+            # chosen from this range. Changing this value after installation will have
+            # no effect. This should fall within `--cluster-cidr`.
+            - name: CALICO_IPV4POOL_CIDR


The pool is still needed when net=none, as felix uses it to identify container traffic. So this needs to be moved into its own if statement where include.network == "calico" || include.network == "none"

ozdanborne · 2018-04-17T22:29:28Z

...ting-started/kubernetes/installation/hosted/kubernetes-datastore/policy-only/1.7/calico.yaml

-            - name: CALICO_IPV4POOL_CIDR
-              value: "192.168.0.0/16"
-            # Enable IPIP
-            - name: CALICO_IPV4POOL_IPIP


looks like we've dropped this field in the new system. which feels right. but just pointing out for the record 👍

ozdanborne · 2018-04-17T22:41:49Z

Ok I've manually gone through all the diffs and pointed out all the major differences I found.

I've also attached the bash script I used to generate local manifests for comparisons, in case anyone else wants to compare them. (uploaded as .txt since github doesn't like .sh. just rename it).
render-comparable-manifests.txt

ozdanborne · 2018-04-17T22:52:10Z

master/getting-started/kubernetes/installation/manifests/app-layer-policy/istio.yaml

+- apiGroups: [""]
+  resources: ["endpoints", "pods", "services"]
+  verbs: ["get", "list", "watch"]
+- apiGroups: [""]


Nit: this block and the previous block could be combined, since they share the exact same verbs and apiGroups:

- apiGroups: [""] resources: ["endpoints", "pods", "services", "namespaces", "nodes", "secrets"] verbs: ["get", "list", "watch"]

ozdanborne · 2018-04-17T22:52:37Z

master/getting-started/kubernetes/installation/manifests/app-layer-policy/istio.yaml

+metadata:
+  name: istio-sidecar-injector-istio-system
+rules:
+- apiGroups: ["*"]


should be [""]

I'd rather not modify the istio.yaml since it's mostly taken from the Istio project directly, so us fixing it up is going to make merges difficult in the future.

ozdanborne · 2018-04-17T22:52:59Z

master/getting-started/kubernetes/installation/manifests/app-layer-policy/istio.yaml

+- apiGroups: [""]
+  resources: ["serviceaccounts"]
+  verbs: ["get", "watch", "list"]
+- apiGroups: [""]


this could be combined with the previous group as well

ozdanborne · 2018-04-17T22:56:39Z

...tting-started/kubernetes/installation/manifests/app-layer-policy/istio-inject-configmap.yaml

+      - name: istio-certs
+        secret:
+          optional: true
+          {% raw %}{{ if eq .Spec.ServiceAccountName "" }}secretName: istio.default{{ else }}secretName: {{ printf "istio.%s" .Spec.ServiceAccountName }}{{ end }}{% endraw %}


I don't think this raw is needed at all. It's currently rendering like this, which doesn't seem right:

secret: optional: true {{ if eq .Spec.ServiceAccountName "" }}secretName: istio.default{{ else }}secretName: {{ printf "istio.%s" .Spec.ServiceAccountName }}{{ end }}

If this is supported my mind is going to explode. Does kubectl support templating of the manifests themselves?

No, kubectl doesn't but the pod that consumes this configmap is itself a kind of templating engine. That's why this configmap has template tags in it that need to be escaped.

ohh, i didn't notice this was just technically a string inside a configmap. phew. Though i kind of wish it did work that way 😄

So we can dismiss the original comment, it's fine as is.

Signed-off-by: Spike Curtis <[email protected]>

* Most upstream changes were buried in calico#1884 which switched from non-templated manifests to templating * projectcalico/calico#1884 * projectcalico/calico#1853 * projectcalico/calico#2069 * projectcalico/calico#2032 * projectcalico/calico#1841 * projectcalico/calico#1770

spikecurtis requested a review from ozdanborne April 12, 2018 23:28

caseydavenport added the hold-merge label Apr 12, 2018

ozdanborne reviewed Apr 13, 2018

View reviewed changes

spikecurtis force-pushed the manifest-templates branch 2 times, most recently from 74740cf to 61bce5c Compare April 17, 2018 20:54

ozdanborne reviewed Apr 17, 2018

View reviewed changes

spikecurtis force-pushed the manifest-templates branch from 61bce5c to 67fc77f Compare April 17, 2018 21:22

ozdanborne reviewed Apr 17, 2018

View reviewed changes

spikecurtis force-pushed the manifest-templates branch from 1c1ffad to e0634b1 Compare April 18, 2018 17:43

Template calico.yaml and canal.yaml manifests

e67a23f

Signed-off-by: Spike Curtis <[email protected]>

spikecurtis force-pushed the manifest-templates branch from e0634b1 to e67a23f Compare April 18, 2018 18:52

ozdanborne approved these changes Apr 18, 2018

View reviewed changes

spikecurtis merged commit eb339b4 into projectcalico:master Apr 18, 2018

spikecurtis deleted the manifest-templates branch April 18, 2018 20:35

caseydavenport changed the title ~~[Do not merge] Manifest templates~~ Manifest templates Apr 18, 2018

This was referenced Aug 15, 2018

Update Calico from v3.1.3 to v3.2.0 poseidon/terraform-render-bootstrap#74

Merged

Update Calico from v3.1.3 to v3.2.1 poseidon/typhoon#278

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Manifest templates #1884

Manifest templates #1884

spikecurtis commented Apr 12, 2018

spikecurtis commented Apr 12, 2018

ozdanborne commented Apr 13, 2018

ozdanborne commented Apr 13, 2018

ozdanborne Apr 13, 2018

spikecurtis commented Apr 13, 2018

ozdanborne Apr 13, 2018 •

edited

Loading

ozdanborne Apr 13, 2018

ozdanborne commented Apr 13, 2018 •

edited

Loading

spikecurtis commented Apr 13, 2018 •

edited

Loading

ozdanborne Apr 13, 2018 •

edited

Loading

spikecurtis Apr 13, 2018

ozdanborne Apr 17, 2018

spikecurtis Apr 17, 2018

ozdanborne Apr 17, 2018

ozdanborne Apr 17, 2018

ozdanborne Apr 17, 2018

ozdanborne Apr 17, 2018

ozdanborne Apr 17, 2018

spikecurtis Apr 17, 2018 •

edited

Loading

ozdanborne Apr 17, 2018 •

edited

Loading

ozdanborne Apr 17, 2018

ozdanborne commented Apr 17, 2018

ozdanborne Apr 17, 2018 •

edited

Loading

ozdanborne Apr 17, 2018

spikecurtis Apr 17, 2018

ozdanborne Apr 17, 2018

ozdanborne Apr 17, 2018

ozdanborne Apr 17, 2018 •

edited

Loading

spikecurtis Apr 17, 2018

ozdanborne Apr 17, 2018

		@@ -0,0 +1,127 @@
		# This ConfigMap is used to configure a self-hosted {{include.prodname}} installation.

		@@ -0,0 +1,19 @@
		# This NetworkPolicy allows pods egress access the to

Manifest templates #1884

Manifest templates #1884

Conversation

spikecurtis commented Apr 12, 2018

Description

Todos

Release Note

spikecurtis commented Apr 12, 2018

ozdanborne commented Apr 13, 2018

ozdanborne commented Apr 13, 2018

Choose a reason for hiding this comment

spikecurtis commented Apr 13, 2018

ozdanborne Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ozdanborne commented Apr 13, 2018 • edited Loading

spikecurtis commented Apr 13, 2018 • edited Loading

ozdanborne Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spikecurtis Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

ozdanborne Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ozdanborne commented Apr 17, 2018

ozdanborne Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ozdanborne Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ozdanborne Apr 13, 2018 •

edited

Loading

ozdanborne commented Apr 13, 2018 •

edited

Loading

spikecurtis commented Apr 13, 2018 •

edited

Loading

ozdanborne Apr 13, 2018 •

edited

Loading

spikecurtis Apr 17, 2018 •

edited

Loading

ozdanborne Apr 17, 2018 •

edited

Loading

ozdanborne Apr 17, 2018 •

edited

Loading

ozdanborne Apr 17, 2018 •

edited

Loading