Skip to content

doc: add early warning for policy-default-local-cluster#40108

Merged
joestringer merged 1 commit intocilium:mainfrom
MrFreezeex:upgrade-policy-default-local-cluster
Jun 24, 2025
Merged

doc: add early warning for policy-default-local-cluster#40108
joestringer merged 1 commit intocilium:mainfrom
MrFreezeex:upgrade-policy-default-local-cluster

Conversation

@MrFreezeex
Copy link
Copy Markdown
Member

@MrFreezeex MrFreezeex commented Jun 18, 2025 •
edited
Loading

Add an early warning about defaulting policy-default-local-cluster to true in 1.19 which will result in a breaking change. Also include some migration guidance with cli command dedicated for that.

policy: clustermesh: policy-default-local-cluster will be set by default in 1.19 see the upgrade guide for guidance on how to prepare your migration if you are using ClusterMesh and have network policies

I asked about this on slack (https://cilium.slack.com/archives/C2B917YHE/p1749648584305359) if there was any objection to default it in 1.19 and no one raised something so far.

Related to #36194 / #36194 (comment)

@MrFreezeex MrFreezeex requested review from a team as code owners June 18, 2025 14:52
@MrFreezeex MrFreezeex requested review from giorio94 and qmonnet June 18, 2025 14:52
@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 Jun 18, 2025
@MrFreezeex MrFreezeex added the area/clustermesh Relates to multi-cluster routing functionality in Cilium. label Jun 18, 2025
@MrFreezeex MrFreezeex force-pushed the upgrade-policy-default-local-cluster branch from 1a6bd59 to c4dca90 Compare June 18, 2025 14:55
@MrFreezeex
Copy link
Copy Markdown
Member Author

MrFreezeex commented Jun 18, 2025

I am not actually sure about the release note, technically it could even be misc but maybe major would be a thing too to highlight the future breaking change 🤔 (although probably not a major change and even the actual breaking change would be in 1.19 when we will change the default value of the option 🤷‍♂️)

@MrFreezeex MrFreezeex force-pushed the upgrade-policy-default-local-cluster branch from c4dca90 to 630cfdb Compare June 18, 2025 15:06
@MrFreezeex MrFreezeex marked this pull request as draft June 18, 2025 15:09
@MrFreezeex MrFreezeex force-pushed the upgrade-policy-default-local-cluster branch from 630cfdb to 42cd6ce Compare June 18, 2025 15:26
@MrFreezeex MrFreezeex marked this pull request as ready for review June 18, 2025 15:29
@MrFreezeex
Copy link
Copy Markdown
Member Author

MrFreezeex commented Jun 18, 2025

/test

joestringer
joestringer requested changes Jun 18, 2025
View reviewed changes
Copy link
Copy Markdown
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.

Some small grammar suggestions and standardizing the language. Most notable question is what the process is for someone to actually change the policies. I ran the recommended command but in my dev cluster I guess my simplistic policies did not require any conversion. It wasn't super obvious to me what we're instructing the user about with the new setting.

I also considered where the right place is for these instructions, and in the longer term the clustermesh policy doc should just define what the desired semantics are, so that should be the primary message communicated in that section. For the upgrade / migration instructions I would suggest moving that over to the upgrade guide.

@joestringer joestringer added release-note/misc This PR makes changes that have no direct user impact. release-blocker/1.18 This issue will prevent the release of the next version of Cilium. labels Jun 18, 2025
@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 Jun 18, 2025
@joestringer
Copy link
Copy Markdown
Member

joestringer commented Jun 18, 2025

This information seems relevant to users upgrading to v1.18, so I've marked it as a release-blocker for v1.18. We've still got plenty of time to get the PR in so you don't have to worry too much about that, this just helps us to track and prioritize PRs for the coming weeks.

@joestringer
Copy link
Copy Markdown
Member

joestringer commented Jun 19, 2025

I am not actually sure about the release note, technically it could even be misc but maybe major would be a thing too to highlight the future breaking change 🤔

Typically we'd probably set the original PR as a major (or at least minor) change. I guess the way we prepare release notes isn't exactly structured to clearly identify "breaking changes" up front. That said, I'm not sure I fully understand what happens right now - will this change the setting by default for existing users? My read of the doc was no, which means it's technically not a breaking change yet until we change the default. That said, the main thing I would want to make sure of is that if someone reads the upgrade guide, it is clear to them that there is a potential breaking change for clustermesh + policy and then they can decide whether to take action (+ take action based on the instructions provided by this PR). If the current structure of the upgrade guide doesn't adequately highlight this breaking change, we could introduce a new title to make this more clear.

@giorio94
Copy link
Copy Markdown
Member

giorio94 commented Jun 19, 2025 •
edited
Loading

I am not actually sure about the release note, technically it could even be misc but maybe major would be a thing too to highlight the future breaking change 🤔

Typically we'd probably set the original PR as a major (or at least minor) change. I guess the way we prepare release notes isn't exactly structured to clearly identify "breaking changes" up front. That said, I'm not sure I fully understand what happens right now - will this change the setting by default for existing users? My read of the doc was no, which means it's technically not a breaking change yet until we change the default. That said, the main thing I would want to make sure of is that if someone reads the upgrade guide, it is clear to them that there is a potential breaking change for clustermesh + policy and then they can decide whether to take action (+ take action based on the instructions provided by this PR). If the current structure of the upgrade guide doesn't adequately highlight this breaking change, we could introduce a new title to make this more clear.

I agree with Joe that the best is probably marking the original PR as a minor change, to let users be aware of the new option. Nothing changes in the default behavior at the moment (for v1.18), so I don't think there's the need to highlight it as a breaking change.

(Edit: I've double-checked, and #39338 is already marked release-note/minor).

giorio94
giorio94 reviewed Jun 19, 2025
View reviewed changes
@MrFreezeex
Copy link
Copy Markdown
Member Author

MrFreezeex commented Jun 19, 2025 •
edited
Loading

That said, I'm not sure I fully understand what happens right now - will this change the setting by default for existing users? My read of the doc was no, which means it's technically not a breaking change yet until we change the default. That said, the main thing I would want to make sure of is that if someone reads the upgrade guide, it is clear to them that there is a potential breaking change for clustermesh + policy and then they can decide whether to take action (+ take action based on the instructions provided by this PR). If the current structure of the upgrade guide doesn't adequately highlight this breaking change, we could introduce a new title to make this more clear.

Yes this is opt-in for 1.18 so no breaking change / no change whatsoever as of now but users can start to actively change their network policies to explicitly target some clusters so that its behavior won't change whatever to what value policy-default-local-cluster is set to. For instance they can explicitly set that they want all clusters via an exist expression on the cluster labels key as shown in the doc which would essentially force the existing behavior on an individual policy (but from your other comment I reckon it's not extra clear while it should be).

@qmonnet qmonnet removed their request for review June 19, 2025 09:15
@MrFreezeex MrFreezeex force-pushed the upgrade-policy-default-local-cluster branch 5 times, most recently from ffde6f2 to b3171a5 Compare June 20, 2025 13:37
@MrFreezeex MrFreezeex force-pushed the upgrade-policy-default-local-cluster branch 3 times, most recently from ea2284a to 8974a85 Compare June 20, 2025 13:56
@joestringer joestringer added the area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. label Jun 20, 2025
giorio94
giorio94 reviewed Jun 23, 2025
View reviewed changes
@MrFreezeex MrFreezeex force-pushed the upgrade-policy-default-local-cluster branch from 8974a85 to 17cf5df Compare June 23, 2025 08:30
joestringer
joestringer requested changes Jun 24, 2025
View reviewed changes
Copy link
Copy Markdown
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.

I made some minor wording suggestions for use of plural and tone (avoid use of we in favor of just instructing the user what to do).

Other than that, the structure looks good and the text seems fairly straightforward to follow, including three common policy transition cases. This feels like it should be comprehensive enough for users to upgrade.

@github-project-automation github-project-automation bot moved this from Proposed to Active in Release blockers Jun 24, 2025
@joestringer joestringer added this to the 1.18 milestone Jun 24, 2025
giorio94
giorio94 requested changes Jun 24, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Thanks! I'm still not totally convinced about the upgrade note -- I've tried adding one suggestion inline, let me know if it makes sense to you. Looks good to me besides that, pending Joe's comments.

@MrFreezeex MrFreezeex force-pushed the upgrade-policy-default-local-cluster branch 2 times, most recently from fb05e75 to 58f9985 Compare June 24, 2025 08:14
@MrFreezeex
doc: add early warning for policy-default-local-cluster
78e04fd 
Add an early warning about defaulting `policy-default-local-cluster` to
true in 1.19 which will result in a breaking change. Also include some
migration guidance with cli command dedicated for that.

Signed-off-by: Arthur Outhenin-Chalandre <[email protected]>
@MrFreezeex MrFreezeex force-pushed the upgrade-policy-default-local-cluster branch from 58f9985 to 78e04fd Compare June 24, 2025 08:20
@MrFreezeex
Copy link
Copy Markdown
Member Author

MrFreezeex commented Jun 24, 2025 •
edited
Loading

Thanks a lot for your suggestions/reviews it improved greatly the proposed docs 🙏 ❤️. Let me know if you find anything else!

giorio94
giorio94 approved these changes Jun 24, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

joestringer
joestringer approved these changes Jun 24, 2025
View reviewed changes
Copy link
Copy Markdown
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.

LGTM, thanks! I think I'll bookmark this as a good example of how to communicate a breaking change to users in advance while expecting them to take action to mitigate.

@joestringer
Copy link
Copy Markdown
Member

joestringer commented Jun 24, 2025

/test

@joestringer joestringer enabled auto-merge June 24, 2025 16:03
@joestringer joestringer added this pull request to the merge queue Jun 24, 2025
Merged via the queue into cilium:main with commit 260af0e Jun 24, 2025
73 of 74 checks passed
@github-project-automation github-project-automation bot moved this from Active to Done in Release blockers Jun 24, 2025
@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 Jun 24, 2025
@MrFreezeex MrFreezeex mentioned this pull request Jul 7, 2025
Closed
@MrFreezeex MrFreezeex mentioned this pull request Jul 20, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joestringer joestringer joestringer approved these changes

@giorio94 giorio94 giorio94 approved these changes

Assignees

No one assigned

Labels

area/clustermesh Relates to multi-cluster routing functionality in Cilium. area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-blocker/1.18 This issue will prevent the release of the next version of Cilium. release-note/misc This PR makes changes that have no direct user impact.

Projects

Release blockers
Archived in project

Milestone

1.18

Development

Successfully merging this pull request may close these issues.

3 participants

@MrFreezeex @joestringer @giorio94