The committers listed above are authorized under a signed CLA.

• ✅ login: slickwilli / name: Matt Williams (432ba52, 8582de4, 6348c29, e8a5b11, a1f0dea, 2f59e05, a7b4ab9, 8240c69)

😊 Welcome @slickwilli! This is either your first contribution to the Istio istio repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

Hi @slickwilli. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

/ok-to-test

Any input on this would be great, currently this is causing us issues with running Windows server on ambient, thanks

Hi @slickwilli,
My apologies, I started thinking about this but got sidetracked onto a bunch of other things.

Rather than arbitrarily configurable ranges, which opens up possibilities for misconfiguration, could we instead offer an alternative range option which we've chosen ahead of time? That way we know it will parse and is reasonable in size. The user just flips alt range to "true" if they are on a platform that requires it.

@ilrudie No worries. Thanks for taking a look!

While I’m in favor of preventing misconfiguration, I’m hesitant to enforce predefined alternative range(s), as end-users may have specific reasons for choosing certain range(s). Would additional validation on the supplied range(s) be sufficient? For example, we could enforce minimum usable space requirements or validate against other relevant criteria.

Additionally, appending OVERRIDE to the environment variable names might help clarify their intended use.

That being said, I think 100.64.0.0/10 would be an effective alternative range.

yeah, there's definitely some options. I'll try to capture my view of the tradeoffs here:

• full yolo, users configure what they think is good and we do our best with that (mostly how it sits now)

  • If they supply a range thats invalid: I think istiod won't come up (maybe not what we want, maybe right?)
  • If they supply a range that's in conflict: probably undefined behavior today (we could do better by making sure kube addresses are always preferred over SE addresses but hypothetically at least a user might accidentally overlap their pod range and this might weird/awkward to figure out)
  • If they supply a range that is too small: we start up but they run into IP exhaustion later. This could be partially mitigated by better ip garbage collection (presently I think we only garbage collect when the controller initializes and records the currently assigned addresses).

• full walled garden, we provide a toggle for alt range and the user chooses one of our ranges

  • Possible both ranges are not suitable for some user out there?
  • (anything else?)
  • What should we do when there are addresses from the default range assigned but the alt-range is toggled on? Re-assign those? Nothing, user can clean status if they want?

• middle ground with more validation

  • Now we have other error/warn states which need to do something. we need to decide what that something should be (prevent starting istiod, warn, prevent starting auto-allocation but allow istiod to start, something I didn't think of)

I imagine these ranges won't be touched by 99.99% of Istio users, and those who are looking to use them are probably pretty advanced users. For example, we only ran into this issue while running Windows VMs via KubeVirt and hit a limitation in the Windows networking stack with the default range (definitely not a common use case, lol).

I’m aligned on guarding against the sharper edge cases:

• We should probably document what happens if someone switches the range mid-flight. We hit that ourselves and just deleted/recreated the offending SEs, but it could trip people up.
• If the override range is invalid or too narrow (or whatever else, like common overlaps), maybe we log a warning and fall back to the default rather than failing Istiod outright? That feels like a safer middle ground and better UX?

That said, this is my first Istio PR, so happy to defer to whatever direction you all feel is most appropriate.

Hi @slickwilli, I brought this up in the combined WG call today to help unblock. The consensus among the folks who joined in on the discussion is:

• We like having ranges being built in to Istio instead of allowing arbitrary user values
• We want a flexible API for future proofing

To that end, the proposal would be to define the current ranges as default and a second set of ranges which we would call something like alt1. We will need to document what these ranges are.

We can then have an env var which specifies a csv list of ranges to use. If the list is empty, unspecified, invalid, anything else which renders it unusable: we just use the default ranges so we never fail to start the controller due to bad user input. If the list can be parsed we will use the first valid entry from the list as our range (in the future we might want to allow combining ranges so a csv list is a useful bit of future proofing).

@ilrudie what if the second set of ranges that is deemed as "alternate" doesn't fit a user's use case? what then? We back to this original PR....how can you possibly define an alternate CIDR to use that will fit every single person's use case?

This is a HIGHLY advanced feature....anybody using this will likely know what they are doing and are accepting the risk in doing so. By limiting this to a CIDR(s) determined by whatever group of folks, all you are doing is kicking the bucket down the road until those ranges don't work for the next user.

My 2 cents.

Hi @jbilliau-rcd,

We did consider the possibility that both the default and alt1 ranges might be unsuitable for some unlucky folks (hence the name alt1, allowing for alt2...N in the future if need arises). We thought it to be sufficiently unlikely that this was a step forward which is likely to solve for most uses.

Using a list (as opposed to a binary "enable alt range") does allow the flexibility to enable free-form CIDR in the same API as well if we see a need for having that, so it's not ruled out so much as not the first step.

It's not feasible to predict why an end user might want to change this subnet or what specific configuration they intend to use, Istio shouldn't need to account for that. Validating a proper subnet is straightforward compared to the complexity introduced by the proposed logic. The suggested approach introduces unnecessary ambiguity, making it unclear which subnet will be used under varying conditions. This complexity not only risks confusion for the user but may also fail to solve the underlying issue given the unpredictable nature of the inputs involved, especially now that kubevirt opened the door to Istio in front of virtually every operating system in existence. If 99% of users will never change this, why would the desired state for those 1% of users not be the goal if it's less arduous to implement in addition?

There hasn't been any movement on this PR in a while and we need this functionality to move forward with a kubevirt/Istio project, otherwise we are hosed. @ilrudie where do we stand currently? How can we get this moving forward?

It's unclear if @slickwilli is still looking to work on this. If they don't have time anymore I suppose someone else would need to take over.

TBD:

• if we're going to choose an alternate range to try to avoid input mistakes, what range?
• if we're going to allow arbitrary range(s), what validation do we do and what to we do when the input isn't usable?
  • based on the discussion in WG, it seems a sensible choice might be
    • zero validation
    • when given input we can't parse, log it and start with the defaults
• how to communicate that this is an experimental/advanced setting and you shouldn't use it in nearly any Istio installation?

He's still working on it (he's a co-worker of mine), just hasn't done anything further since, while we've gone back and forth on certain points regarding this, no specific decisions have been made so there isn't any further work for him to do (yet).

To respond to your points:

if we're going to choose an alternate range to try to avoid input mistakes, what range?

Well, that's kind of our point; what alternate range would we pick? and what if it doesn't work for someone else for any other reason? are we just going to keep adding endless alternate ranges? Seems the better fit is to make this an advanced, experimental setting, and let the users use what they want. 99.9% of users won't likely touch this anyway, so trying to add in guardrails isn't worth it imo.

if we're going to allow arbitrary range(s), what validation do we do and what to we do when the input isn't usable?
based on the discussion in WG, it seems a sensible choice might be
zero validation
when given input we can't parse, log it and start with the defaults

Either/or....if the latter, sure, take whatever has been given, and if it can't parse, log error, default to defaults, and move on.

how to communicate that this is an experimental/advanced setting and you shouldn't use it in nearly any Istio installation?

Same way I assume any other experimental/alpha features are communicated. Examples are here and here

So if you agree, let's move forward. If not, we just need to know what changes to make to get this PR approved and pushed through. Appreciate your time on this!

Yep, sorry I just wasn't sure what the status of this discussion was.. A quick parse of the provided ranges and falling back to the defaults is simple enough and provides the most flexibility

/test integ-ambient-dual

If we're all good with arbitrary ranges (/cc @louiscryan if he's around), then we'll need to make sure we manually cherry-pick this to the 1.27 branch

Awesome, thanks for pushing this forward guys!

@zirain @grnmeira @kfaseela Looks like this merged before feature freeze date but we missed a cherry0pick

/cherrypick release-1.27

it will be part of beta.1

@zirain: new pull request created: #57189