cmd/contour: optional CRDs

[nsimons]

Allow disabling certain informers for CRDs by passing a command line flag to the serve command. This makes the corresponding CRD effectively optional.

I'd like some feedback on the terminology. Would the audience of the documents necessarily know what an informer is? Also feature is quite generic, but I tried to explain the intended meaning in this context. I'm open to other suggestions.

I tried to look for a good place to write some tests, but I didn't find much related to the flags. I have tested manually with the CRDs present/missing in my cluster.

Fixes #4684

Signed-off-by: Niklas Simons niklas.simons@est.tech

[github-actions]

Hi @nsimons! Welcome to our community and thank you for opening your first Pull Request. Someone will review it soon. Thank you for committing to making Contour better. You can also join us on our mailing list and in our channel in the Kubernetes Slack Workspace

[codecov]

Codecov Report

Merging #5080 (ee16813) into main (b461e76) will decrease coverage by 0.03%.
The diff coverage is 10.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5080      +/-   ##
==========================================
- Coverage   78.01%   77.99%   -0.03%     
==========================================
  Files         138      138              
  Lines       17509    17516       +7     
==========================================
+ Hits        13660    13661       +1     
- Misses       3583     3589       +6     
  Partials      266      266              

Impacted Files	Coverage Δ
cmd/contour/servecontext.go	80.83% <ø> (ø)
cmd/contour/serve.go	22.16% <10.00%> (-0.08%)	⬇️

[tsaarni]

This looks good to me 👍

Regarding terminology: "informer" to me is terminology from client-go, while "watch" could be more of an API level term used in this context?

In theory, disabled feature could be something else than CRD as well, like in Kubernetes --feature-gates option which could be seen conceptually similar to this feature. However, the ask here is of course for optional CRDs specifically (for the capability of skipping install of the CRDs), and the only features that can be disabled now are CRDs.

Some reasoning about potentially disabled features: I was commenting previously #4684 (comment) that the features included in this PR seem to me the only ones that make sense to be disabled currently because

• ContourDeployement CRD needs to be installed only if executing contour gateway-provisioner. Executing it without the CRD does not make much sense either, so users make their choice by executing the provisioner.
• The Gateway API CRDs (multiple CRDs) need to be installed only if Gateway API is configured. Optionality exists there via the config. So, no need for e.g. --disable-feature=gatewayapi

Reasoning to add a new command line option (like done by this PR), instead of having it in config, could be that this information is needed before ContourConfig CR is looked up.

There might be one small corner case now, if user would add both -disable-feature=contourconfigurations and --contour-config-name=myconfig where the latter would trigger initial lookup for the CR (code here) but user then asked for ignore updates to the CR.

Leaving for others to comment!

[nsimons]

Thanks for the feedback @tsaarni!

"watch" could be better, it's a more familiar term since that is also used e.g. with kubectl when watching for changes in objects. I'll leave this one open for now, if there are other opinions.

Nice catch with the corner case, disabling the feature while trying to use it would not make sense. Validation added.

[tsaarni]

LGTM 👍

[nsimons]

@skriss as you were involved in the original issue, would you like to review this as well?

[skriss]

I have just removed the ContourConfiguration informer entirely in #5133, since the informer wasn't being used and we don't have plans to use it any time soon. So, ContourConfiguration should now be optional in that if you don't use the --contour-config-name flag, then the CRD is not required to be present. I think because of that we can drop the special handling for that resource.

Even though this will now just apply to ExtensionService, I think it is reasonable to have a way to not require alpha resources to be installed, so still 👍 for going ahead with this.

[nsimons]

Thanks @skriss. Fixed the comments, I also double-checked that ContourConfiguration is now indeed optional by default.

[tsaarni]

Thank you @nsimons! Ignoring coverage targets for this one due to the nature of the change and merging since requested changes were made.