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

chart: optimize spire image for offline environment #27621

Merged
merged 1 commit into from Aug 30, 2023

Conversation

weizhoublue
Copy link
Contributor

@weizhoublue weizhoublue commented Aug 22, 2023

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

Change the Helm values configuration for SPIRE to match other images in the Helm charts

@weizhoublue weizhoublue requested review from a team as code owners August 22, 2023 09:25
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 22, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Aug 22, 2023
@mhofstetter mhofstetter added kind/enhancement This would improve or streamline existing functionality. release-note/misc This PR makes changes that have no direct user impact. area/servicemesh GH issues or PRs regarding servicemesh feature/authentication labels Aug 22, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 22, 2023
@joestringer joestringer added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. and removed release-note/misc This PR makes changes that have no direct user impact. labels Aug 23, 2023
@joestringer
Copy link
Member

Heads up @mhofstetter this will have user-facing impact due to the change in the format for Helm values. Hence I've updated release-note/minor. This change should also be documented in the upgrade guide.

@weizhoublue
Copy link
Contributor Author

weizhoublue commented Aug 23, 2023

Heads up @mhofstetter this will have user-facing impact due to the change in the format for Helm values. Hence I've updated release-note/minor. This change should also be documented in the upgrade guide.

Thanks for reminding

I will add 'Upgrade Notes' in the documentation/operations/upgrade.rst, right ?

Copy link
Member

@mhofstetter mhofstetter left a 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.

Documentation/operations/upgrade.rst Outdated Show resolved Hide resolved
install/kubernetes/cilium/values.yaml.tmpl Show resolved Hide resolved
Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

LGTM

@weizhoublue weizhoublue force-pushed the pr/welan/chart branch 2 times, most recently from a73234e to 98ce438 Compare August 24, 2023 07:19
@mhofstetter
Copy link
Member

/test

Copy link
Member

@mhofstetter mhofstetter left a comment

Choose a reason for hiding this comment

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

LGTM - thanks again!

Copy link
Member

@joestringer joestringer left a 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" }}
Copy link
Contributor

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?

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
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

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Member

@joestringer joestringer left a 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.

@joestringer
Copy link
Member

/test

@weizhoublue
Copy link
Contributor Author

weizhoublue commented Aug 27, 2023

the CI failed owing to some connectivity and ipam tests, which seems to have no bussiness with PR.
Should I retry to rebase main branch

@maintainer-s-little-helper
Copy link

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

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Aug 27, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Aug 27, 2023
@joestringer
Copy link
Member

/test

@joestringer
Copy link
Member

the CI failed owing to some connectivity and ipam tests, which seems to have no bussiness with PR.

Known failures #27672 and #27318.

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 28, 2023
@aditighag
Copy link
Member

@weizhoublue Hi, Could you rebase the PR as there is a conflict? Thanks!

@aditighag aditighag added dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Aug 29, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 29, 2023
Signed-off-by: weizhou.lan@daocloud.io <weizhou.lan@daocloud.io>
@weizhoublue
Copy link
Contributor Author

@weizhoublue Hi, Could you rebase the PR as there is a conflict? Thanks!

sure, thanks for reminding

@mhofstetter mhofstetter removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 30, 2023
@mhofstetter
Copy link
Member

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Aug 30, 2023
@aditighag aditighag merged commit f40d7a7 into cilium:main Aug 30, 2023
60 checks passed
@PhilipSchmid
Copy link
Contributor

PhilipSchmid commented Nov 7, 2023

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.

@sayboras sayboras added needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/servicemesh GH issues or PRs regarding servicemesh dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. feature/authentication kind/community-contribution This was a contribution made by a community member. kind/enhancement This would improve or streamline existing functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants