/test

/test

/test

Why is this change necessary?

If we don't need to enable masquerading, then I assume we don't need to discuss which interfaces you want to apply masquerading to.

If we don't need to enable masquerading [...]

Sorry I was not clear, this is precisely my question: Why do we disable masquerading? You submit a PR to turn it off, but there's no explanation why. What's the objective, here?

Ah sorry I hadn't paid attention to the linked issue, now I see.

Could you please summarise the reason for the change in the commit description? So that we understand the motivation when looking at the Git history in the future.

Ah sorry I hadn't paid attention to the linked issue, now I see.

Could you please summarise the reason for the change in the commit description? So that we understand the motivation when looking at the Git history in the future.

Sure. I thought I should put more why in the issue to discuss and put what change this is in the PR commits. I will change that and I'm waiting for more feedback to push my local commits

more why in the issue to discuss

It makes sense to have the full context in the issue. My recommendation would be to still provide at least a brief in summary in the PR and commit description, because that's where we'll be looking at when trying to understand the reason for the change.

and put what change this is in the PR commits

Ideally, it should be easy to understand what changes by looking at the code. The part that is not present in the diff is why it changes - that's why it's also important to motivate the change in the description 🙂

I will change that and I'm waiting for more feedback to push my local commits

Sure, thanks!

Looks good, thank you! Some formatting suggestions.

Thanks and will push those in my next commit

/test

Please note that you have a merge conflict to address, on the upgrade notes.

/test

I updated the release note.
Should we suggest that existing clusters be migrated? (Is this safe to migrate on a running cluster)?
What about changing the helm charts themselves?

I was bit hesitant to put in the helm values or exsiting clusters for now.

If we have some people deployed their cluster nodes in the AWS public subnets(where it's not a good practice), their worker nodes could have the public EIP, where this could be a breaking change to them since pods IP will be SNAT to their worker nodes public IP when they access the Internet.

I think maybe it's better to keep this way for a while to see if we will receive the complain. Then we could move to something like

if eni=true and enableIPv4Masquerade is empty, then enableIPv4Masquerade = false.

I'm still new to make changes to helm values so I'm more conservative to introduce the changes to exsiting users

@squeed WDYT

We met some problem on public nodes.
We have to use some public ip nodes, and I enabled bpf.masquerade=true before 1.18.

When I upgrade to cilium 1.18, I see errors in cilium console that enableIPv4Masquerade was not set, and I see this issue, so I just removed bpf.masquerade=true, then pods on public nodes cannot access Internet anymore while pods on private nodes work well.
I had to add bpf.masquerade back and enableIPv4Masquerade finally.

May I ask what could be the problem?

I dont think there is any problem and it's the default behaviour change just for enableIPv4Masquerade.

Before the change I checked Cilium and AWS doc, I can see they suggest or assume that the worker nodes are in the private subnets.

Before this change, I can see the users having the confusion since they have enabled the Masqueraded but found some traffic got SNAT(VPC to external) but some not(traffic within the VPC). I think it's more confusing and that's why I proposed this change.