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

Update weave template to match source for 2.8.1 #8013

Merged
merged 1 commit into from
Sep 28, 2021
Merged

Update weave template to match source for 2.8.1 #8013

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
44 changes: 27 additions & 17 deletions roles/network_plugin/weave/templates/weave-net.yml.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,28 +27,28 @@ items:
- list
- watch
- apiGroups:
- networking.k8s.io
- extensions
Copy link
Contributor

Choose a reason for hiding this comment

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

I compared this change with the original manifest of weave-net by downloading it from https://cloud.weave.works/k8s/net?k8s-version=1.23

This apiGroups is networking.k8s.io, not extensions on the original manifest.
Why here is changed?
I guess this adds apiGroups 'extentions` for networkpolicies API, but I am not sure the reason.

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 based my changes off of https://github.com/weaveworks/weave/blob/v2.8.1/prog/weave-kube/weave-daemonset-k8s-1.11.yaml I didn't even know the file you linked to was an option.

A quick look shows some of the differences are ordering (which you have commented on below) but some are actual changes like this.

Later on the networking.k8s.io is included so this is partly a change and partly an ordering change.

Would you prefer to go off the file you linked to? There are a bunch of additional timestamped annotations in your version that appear to be set based on the time the file is downloaded. Not sure if it makes sense to include those.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for your explanation, I see that.
I confirmed this change is valid by comparing https://github.com/weaveworks/weave/blob/v2.8.1/prog/weave-kube/weave-daemonset-k8s-1.11.yaml
I am fine to apply the above as the original for updating the manifest.

/lgtm

resources:
- networkpolicies
verbs:
- get
- list
- watch
- apiGroups:
- ''
- 'networking.k8s.io'
resources:
- nodes/status
- networkpolicies
verbs:
- patch
- update
- get
- list
- watch
- apiGroups:
- policy
resourceNames:
- privileged
- ''
resources:
- podsecuritypolicies
- nodes/status
verbs:
- use
- patch
- update
- apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
Expand All @@ -67,16 +67,16 @@ items:
kind: Role
metadata:
name: weave-net
namespace: kube-system
labels:
name: weave-net
namespace: kube-system
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this changes the order of the namespace?
The previous order is the same as the original manifest.

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 kept the ordering from the original file and just added in the various Ansible bits from kubespray. I'm happy to change the ordering to make the PR have less changes.

Same applies to your additional comments on ordering below.

rules:
- apiGroups:
- ''
resourceNames:
- weave-net
resources:
- configmaps
resourceNames:
- weave-net
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto: Changing the order which was the same as the original one.

verbs:
- get
- update
Expand All @@ -90,9 +90,9 @@ items:
kind: RoleBinding
metadata:
name: weave-net
namespace: kube-system
labels:
name: weave-net
namespace: kube-system
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto: Changing the order which was the same as the original one.

roleRef:
kind: Role
name: weave-net
Expand All @@ -109,16 +109,16 @@ items:
name: weave-net
namespace: kube-system
spec:
minReadySeconds: 5
# Wait 5 seconds to let pod connect before rolling next pod
Copy link
Contributor

Choose a reason for hiding this comment

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

The original manifest doesn't contain this comment, and the order of minReadySecounds was the same as the old one.

selector:
matchLabels:
name: weave-net
minReadySeconds: 5
template:
metadata:
labels:
name: weave-net
spec:
priorityClassName: system-node-critical
initContainers:
- name: weave-init
image: {{ weave_kube_image_repo }}:{{ weave_kube_image_tag }}
Expand Down Expand Up @@ -217,6 +217,9 @@ items:
- name: dbus
mountPath: /host/var/lib/dbus
readOnly: true
- mountPath: /host/etc/machine-id
name: cni-machine-id
readOnly: true
- name: xtables-lock
mountPath: /run/xtables.lock
readOnly: false
Expand Down Expand Up @@ -246,7 +249,10 @@ items:
seLinuxOptions: {}
serviceAccountName: weave-net
tolerations:
- operator: Exists
- effect: NoSchedule
operator: Exists
- effect: NoExecute
operator: Exists
volumes:
- name: weavedb
hostPath:
Expand All @@ -260,6 +266,9 @@ items:
- name: cni-conf
hostPath:
path: /etc
- name: cni-machine-id
hostPath:
path: /etc/machine-id
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't here specify type: FileOrCreate which is specified on the original one?

- name: dbus
hostPath:
path: /var/lib/dbus
Expand All @@ -270,6 +279,7 @@ items:
hostPath:
path: /run/xtables.lock
type: FileOrCreate
priorityClassName: system-node-critical
updateStrategy:
rollingUpdate:
maxUnavailable: {{ serial | default('20%') }}
Expand Down