Make all ContourConfig CRD fields optional #4451
Conversation
| // +optional | ||
| // +kubebuilder:default=false | ||
| EnableExternalNameService bool `json:"enableExternalNameService"` | ||
| EnableExternalNameService *bool `json:"enableExternalNameService,omitempty"` |
There was a problem hiding this comment.
I ended up making all boolean fields pointers. The reasoning is that, if we ever wanted to change the internal default for one of these to true, then if this weren't a pointer we'd have no way to distinguish between "user didn't provide a value for the field" and "user explicitly specified false". However, we do generally follow a pattern of boolean config fields always defaulting to false, so I'm definitely open to leaving these as non-pointers that always must default to false (it makes them easier to work with in code, fewer derefs). Looking for input.
More generally, my thinking with primitive types is - if the zero value is a valid option for the field, then the field probably should be a pointer to distinguish between "not specified" and "explicitly specified as the zero value". If the zero value is invalid, then the field doesn't need a pointer since we can assume anything with the zero value was not specified.
|
|
||
| // Health defines the endpoints Contour uses to serve health checks. | ||
| // | ||
| // Contour's default is { address: "0.0.0.0", port: 8000 }. |
There was a problem hiding this comment.
Since HealthConfig is a shared type, these defaults have to be documented at this level, which is a little inconsistent with other non-shared struct fields, but I think it's the best we can do? Applies to a couple other ones too, like Metrics and TLS.
| cases := map[string]struct { | ||
| getServeContext func(ctx *serveContext) *serveContext | ||
| getContourConfiguration func(cfg contour_api_v1alpha1.ContourConfigurationSpec) contour_api_v1alpha1.ContourConfigurationSpec | ||
| }{ |
There was a problem hiding this comment.
I switched this test to use the pattern of "have a base config, then each test adds its own modification to that config" which I think works really well here for clarity & maintaintability and saves us ~600 lines of copy-pasted config.
Codecov Report
@@ Coverage Diff @@
## main #4451 +/- ##
==========================================
- Coverage 76.07% 73.93% -2.14%
==========================================
Files 138 137 -1
Lines 12332 12444 +112
==========================================
- Hits 9382 9201 -181
- Misses 2701 3044 +343
+ Partials 249 199 -50
|
|
I was able to get the provisioner updated to generate a ContourConfiguration instead of a ConfigMap, based on this PR (the provisioner work is still WIP but wasn't working prior to the ContourConfig changes): https://github.com/skriss/contour/tree/gw-provisioner-contourconfig |
|
This one should be ready for official review, ad-hoc testing looks good as well as all the E2E's and the WIP branch for the provisioner that makes use of this (and was previously not working). I'll tackle the dropping of the kubebuilder enum constraints in a separate PR. |
skriss
added
the
release-note/minor
Removes all kubebuilder defaults from ContourConfig fields and switches them to pointers with the omitempty JSON tag to make them all fully optional. Defaults are then applied internally by Contour. Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
Signed-off-by: Steve Kriss <krisss@vmware.com>
|
rebased. |
TODOs