@bimmlerd thanks for the comments! Do you think a higher default (say 255) would help here? I would expect regex to be smaller than the actual DNS for most cases. It would make sense to limit the pattern length given DNS name itself has a hard limit.
Regarding upgrade path, user can set the flag along with the upgrade to make sure existing policies don't fail. Although a breaking change, do you see this being a hard blocker?

Do you think a higher default (say 255) would help here?

I'm uncertain. It seems possible to construct contrived examples of matchPatterns which are longer but still valid - and we can't break those without making this a breaking change which would need a bit more consideration.

Regarding upgrade path, user can set the flag along with the upgrade to make sure existing policies don't fail.

Or we could make the matchPattern validation flag default to 0, which would allow any length and warn on very long patterns, i.e. issue a deprecation. Then flip the flag to opt-out or remove the flag outright in a future release. I'd prefer this, because then the users which don't know about this feature/don't read the upgrade guide carefully don't end up with potentially broken policies, which would be bad.

Or we could make the matchPattern validation flag default to 0

This makes sense, I have defaulted to 0 and run the validation only if value is non-zero. I guess documenting it as an available option should also work for those who are looking for this specifically to improve performance.

Yeah upgrade is the tricky aspect. So generally speaking it should be fine to set a maxLength of 255 for matchPatterns in the CRDs as well, for the same reason that matchNames can only be 255: RFC 1035 Section 2.3.4 states this as the maximum number of octets for a name. It doesn't make sense that a matchPattern would be longer if the actual FQDN is shorter than the matchpattern.

Then yeah having a configurable limit seems fine. Then again, we should really be able to convince ourselves there's a future where that setting will be configured at a lower value and will support the majority of users. Otherwise we're just adding yet one more flag that will be rarely used.

The other angle on upgrade is this suggestion from the original issue, we could potentially do something like this (doesn't need to be done in this PR):

Extend the preflight check to validate whether any existing CNPs or CCNPs have matchpattern / matchnames that exceed the default limits. If yes, highlight the statements. Instruct the user to configure the above Cilium flag to raise the limits to match the policies they use in their environment.

I don't recall where I came up with the idea of limiting matchPatterns to 63 characters. If my original motivation there is that Cilium might take a long time to process such rules, then maybe instead of putting a low limit like 63, Cilium should just export some metric or something that would allow users to identify when the worst case policy calculation takes a long time due to FQDN rules. Or warn on "bad" patterns - long patterns like @bimmlerd suggested, or maybe if there's a matchPattern with excessive wildcards (we could pick a threshold, say 5 or more * characters or something).

it should be fine to set a maxLength of 255 for matchPatterns in the CRDs as well

Didn't want to do this as this won't allow users with specific needs to configure it higher.

Then again, we should really be able to convince ourselves there's a future where that setting will be configured at a lower value and will support the majority of users. Otherwise we're just adding yet one more flag that will be rarely used.

To ensure this we could have warnings that inform users to use shorter DNS matchPatterns if length is > 255 to improve performance. Having this knob definitely allows cluster admins to expose cilium functionality while also ensuring bad configurations don't affect overall performance. Not sure about how to do warnings though, let me have a look.

@bimmlerd @joestringer do let me know if anything else makes more sense here. I have already disabled the matchPatterns check by default (value 0).

it should be fine to set a maxLength of 255 for matchPatterns in the CRDs as well

Didn't want to do this as this won't allow users with specific needs to configure it higher.

Could you expand more on the use case? If DNS names can't be longer than that, and matchPatterns inherently have a wildcard pattern which expands to one or more characters, then I don't understand how it's possible to use a higher value. Am I missing something?

Then again, we should really be able to convince ourselves there's a future where that setting will be configured at a lower value and will support the majority of users. Otherwise we're just adding yet one more flag that will be rarely used.

To ensure this we could have warnings that inform users to use shorter DNS matchPatterns if length is > 255 to improve performance. Having this knob definitely allows cluster admins to expose cilium functionality while also ensuring bad configurations don't affect overall performance. Not sure about how to do warnings though, let me have a look.

Bear with me, as I'm developing my thinking on this topic as we discuss it. Here's where I'm currently thinking as a direction: We should provide the tooling for interested admins to measure the impact and take action. So metrics totally make sense for this. The more I think about it, the less I'm convinced that cluster admins are going to be able to come up with some reasonable configuration for this flag that realistically limits the performance impact. If there is a specific pattern that causes high performance impact it would be nice to know, but then the solution is to check the policy and change it. Probably a pattern as simple as *.*.*.*.* could have high impact due to the number of wildcards, yet the actual length of the pattern is not that long. (Note I didn't try this out at all, just speculating. Maybe it takes 10 wildcard patterns to trigger bad behaviour, not sure. I think the original idea / concern about performance of regex matching came from reports from oss-fuzz).

The other thing I'm thinking is.. if we are going to impose a limit, then it is nice to have a (hidden) configuration for this option so that in the worst case it is easy to tweak if the new limit causes problems. But if the default is 0 then I'm not sure whether any user will actually configure the option. We have hundreds of flags in Cilium, so if users aren't specifically asking for a new flag for this configuration then maybe we don't need to be exporting an option to them.

I don't recall where I came up with the idea of limiting matchPatterns to 63 characters.

Some context here: K8s limits individual label strings to 63 characters. If we ever take a DNS name and try to put it into a k8s API field with that limit, it could cause problems. I don't know exactly whether we ever do that or how that limit might come up, so I couldn't argue to set that limit at this point. Just highlighting this context that I remembered after that post.

@rudrakhp ping?

@youngnick will get back to this, have been busy elsewhere. From what I gather as per the comments:

If DNS names can't be longer than that, and matchPatterns inherently have a wildcard pattern which expands to one or more characters, then I don't understand how it's possible to use a higher value.

@joestringer patterns convoluted enough can lead to the patterns themselves exceeding the 255 limit, although practically that shouldn't be the case. If everyone is of the same opinion I can have a hard limit of 255 for regex as well.

The more I think about it, the less I'm convinced that cluster admins are going to be able to come up with some reasonable configuration for this flag that realistically limits the performance impact. If there is a specific pattern that causes high performance impact it would be nice to know, but then the solution is to check the policy and change it.

Ok this makes sense, length of the regular expression will seldom affect the performance, it's more about how complex the final expression is.
Limiting the scope of this PR, will just have two hard limits on DNS name and patterns. PR for emitting performance metrics for bad patterns can be a good follow up WDYT?

Limiting the scope of this PR, will just have two hard limits on DNS name and patterns. PR for emitting performance metrics for bad patterns can be a good follow up WDYT?

Yeah, I think that would be fine. 👍

/test

Many tests were failing, looks like due to ImagePullBackOff issue with cilium-envoy, which I believe was a transient failure on main earlier this week. I clicked the button to rebase the PR to tip of main so we can re-run the testsuite.

/test

/test

/test