-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
chart: optimize spire image for offline environment #27621
Conversation
6ce7385
to
b9e9d08
Compare
Heads up @mhofstetter this will have user-facing impact due to the change in the format for Helm values. Hence I've updated |
Thanks for reminding I will add 'Upgrade Notes' in the documentation/operations/upgrade.rst, right ? |
d274cb2
to
4694793
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.
@weizhoublue thanks for the contribution! The change request itself looks reasonable.
But as@joestringer already pointed out - i have some questions about the breaking change (vs deprecation period) and the respective communication in the upgrade notes for the respective SIGs.
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
a73234e
to
98ce438
Compare
/test |
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 - thanks again!
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.
Thanks for the submission! I had one question on the templates for something I don't understand, plus a request to make the image variables definitions to be lined up the same way as other external dependencies in the tree, using install/kubernetes/Makefile.values
.
command: | ||
- /bin/sh | ||
- -c | ||
- | | ||
{{- tpl (.Files.Get "files/spire/wait-for-spire.bash") . | nindent 14 }} | ||
containers: | ||
- name: spire-agent | ||
{{- if eq (typeOf .Values.authentication.mutual.spire.install.agent.image) "string" }} |
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 PR descriptions making this an "image" and "tag" value, similar to other components?
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
the common way of cilium images use "tag" and "repository" things to define the image
I think it conforms to best practices, especially for offline environment, the administrator could just set helm option --xx. repository=offline.io/cilium , and is not necessary to care about the "tag" for cilium v1.4 or v1.5, and just keep the best matched and recommended tag in the chart
Therefore, the original definition of spire image does not conform what other cilium image does, and that is what this PR want to optimize
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.
and why it is compatible with the string type here , is owing to comment
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.
Sorry, some of the confusion may be because I added the release note. I will say though that these settings should conform to the same configuration format that is exposed for components like hubble-ui.
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.
Docs look good and my earlier comments were resolved. I didn't take a fresh look at the Helm format, delegating review to @cilium/helm for that.
/test |
the CI failed owing to some connectivity and ipam tests, which seems to have no bussiness with PR. |
Commit 84e93d63b865a59f366420a06e37c7293e8be146 does not contain "Signed-off-by". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
84e93d6
to
6e19f8d
Compare
/test |
|
@weizhoublue Hi, Could you rebase the PR as there is a conflict? Thanks! |
Signed-off-by: weizhou.lan@daocloud.io <weizhou.lan@daocloud.io>
a404cae
to
0aad3db
Compare
sure, thanks for reminding |
/test |
Can we please get this one backported to 1.14? Edit: After some internal discussions, I now see why it's challenging to backport this without introducing a breaking change via 1.14 patch version. Also, affected enterprises that must override this image often already have a pipeline to implement a search-replace as a workaround until Cilium 1.15. |
optimize the chart values for spire
align the image definition map with other images, such as cilium operator and hubble, which decouple the repository and tag, and could better deploy in an offline environment
prevent hardcode the busybox image in the yaml, which could help deploy cilium in an offline K8S cluster