-
Notifications
You must be signed in to change notification settings - Fork 46
Update cilium to v1.17.1 #510
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
Conversation
18e9676
to
b9a3ecd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm, do we also want to allow/support TLS interception?
charts/internal/cilium/values.yaml
Outdated
# -- Configure iptables--random-fully. Disabled by default. View https://github.com/cilium/cilium/issues/13037 for more information. | ||
iptablesRandomFully: false | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to change the default for our scenario? Reading through the issue also with context from other areas in mind, I do not see any reason why you would not want to use iptablesRandomFully
. Whether we enable it here or adapt the values can be up for discussion.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, makes sense. I set it to true.
{{- if .Values.global.ipam.multiPoolPreAllocation }} | ||
ipam-multi-pool-pre-allocation: {{ .Values.global.ipam.multiPoolPreAllocation | quote }} | ||
{{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is multiPoolPreAllocation
in our values file? I do not find it. Should we add it to have a proper default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default is empty. I don't think it makes sense to add it with an empty string as default.
@@ -650,7 +674,7 @@ data: | |||
dnsproxy-socket-linger-timeout: "10" | |||
tofqdns-dns-reject-response-code: "refused" | |||
tofqdns-enable-dns-compression: "true" | |||
tofqdns-endpoint-max-ip-per-hostname: "50" | |||
tofqdns-endpoint-max-ip-per-hostname: "1000" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand why we use here the literal value while in other cases we took over the templated from. Could you please explain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was already a literal. Only the value changed.
@@ -151,6 +151,13 @@ rules: | |||
- watch | |||
- delete | |||
- patch | |||
- apiGroups: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are secrets
missing in the section # to check apiserver connectivity
? If I diff the versions, it seems like this should have been added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
secrets
would be needed if .Values.tls.secretSync.enabled
would be true.
That would be part of TLS interception part, we want to add later.
/test pull-extension-networking-cilium-e2e-kind |
1 similar comment
/test pull-extension-networking-cilium-e2e-kind |
0089fe9
to
2f1cf32
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
How to categorize this PR?
/area networking
/kind enhancement
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note: