Update ContourConfig definitions #4292

youngnick · 2022-01-24T01:58:26Z

Remove enum defnitions, replace with godoc for allowed values and documentation of constants for string alias types
Simplify defaults
Update defaulting logic for TLS Ciphersuites
Update servecontext_test.go tests to reduce boilerplate
Update API Reference docs to list available constant values for string alias types

Here I'm proposing swapping everything to optional, in line with my comments in #4062. While working on that, I've cleaned up a few other things I've found, as listed above.

codecov · 2022-01-24T05:00:35Z

Codecov Report

Merging #4292 (987eb82) into main (f33ae54) will decrease coverage by 0.55%.
The diff coverage is 78.57%.

❗ Current head 987eb82 differs from pull request most recent head 961f9fd. Consider uploading reports for the commit 961f9fd to get more accurate results

@@            Coverage Diff             @@
##             main    #4292      +/-   ##
==========================================
- Coverage   78.34%   77.78%   -0.56%     
==========================================
  Files         113      112       -1     
  Lines       10196    10030     -166     
==========================================
- Hits         7988     7802     -186     
- Misses       2025     2045      +20     
  Partials      183      183

Impacted Files	Coverage Δ
cmd/contour/serve.go	`12.54% <0.00%> (-0.40%)`	⬇️
pkg/config/ciphersuites.go	`100.00% <ø> (ø)`
cmd/contour/servecontext.go	`82.87% <86.66%> (+0.15%)`	⬆️
internal/envoy/v3/stats.go	`100.00% <100.00%> (ø)`
internal/featuretests/v3/envoy.go	`100.00% <100.00%> (ø)`
internal/featuretests/v3/featuretests.go	`87.11% <100.00%> (ø)`
internal/xdscache/v3/listener.go	`90.53% <100.00%> (-0.12%)`	⬇️
internal/debug/dot.go	`0.00% <0.00%> (-54.55%)`	⬇️
internal/dag/accessors.go	`94.59% <0.00%> (-3.98%)`	⬇️
internal/status/gatewaystatus.go	`71.73% <0.00%> (-3.27%)`	⬇️
... and 29 more

youngnick · 2022-01-24T05:13:19Z

With the current spec as I write this, applying a blank ContourConfiguration object:

apiVersion: projectcontour.io/v1alpha1
kind: ContourConfiguration
metadata:
  namespace: projectcontour
  name: blank2
spec: {}

will get you this:

apiVersion: projectcontour.io/v1alpha1
kind: ContourConfiguration
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"projectcontour.io/v1alpha1","kind":"ContourConfiguration","metadata":{"annotations":{},"name":"blank2","namespace":"projectcontour"},"spec":{}}
  creationTimestamp: "2022-01-24T05:07:57Z"
  generation: 1
  name: blank2
  namespace: projectcontour
  resourceVersion: "461305830"
  uid: 791c3e8e-8d65-4788-9ed2-1b9e2259badb
spec:
  debug:
    kubernetesLogLevel: 0
    logLevel: info
  enableExternalNameService: false
  envoy:
    cluster:
      dnsLookupFamily: auto
    defaultHTTPVersions:
    - HTTP/1.1
    - HTTP/2
    health:
      address: 0.0.0.0
      port: 8002
    http:
      accessLog: /dev/stdout
      address: 0.0.0.0
      port: 8080
    https:
      accessLog: /dev/stdout
      address: 0.0.0.0
      port: 8443
    listener:
      tls:
        minimumProtocolVersion: "1.2"
      useProxyProtocol: false
    logging:
      accessLogFormat: envoy
    metrics:
      address: 0.0.0.0
      port: 8002
    network:
      adminPort: 9001
    service:
      name: envoy
      namespace: projectcontour
  health:
    address: 0.0.0.0
    port: 8002
  httpproxy:
    disablePermitInsecure: false
  metrics:
    address: 0.0.0.0
    port: 8002
  xdsServer:
    address: 0.0.0.0
    port: 8001
    tls:
      caFile: /certs/ca.crt
      certFile: /certs/tls.crt
      insecure: false
      keyFile: /certs/tls.key
    type: contour

skriss

Added a few specific comments but probably due for an in-person discussion

skriss · 2022-01-24T20:50:39Z

apis/projectcontour/v1alpha1/contourconfig.go

 	// Defines the XDSServer to use for `contour serve`.
-	// +kubebuilder:validation:Enum=contour;envoy
+	//
+	// Values: `contour` (default), `envoy`
+	//
+	// Other values will produce an error.
+	// +kubebuilder:default=contour
+	// +optional
 	Type XDSServerType `json:"type"`


This is a good example of one where we'll likely want to switch the default at some point (#2134), how would that work? Are we saying that that requires a major version bump of the API to do?

skriss · 2022-01-24T20:51:34Z

apis/projectcontour/v1alpha1/contourconfig.go

@@ -24,8 +23,8 @@ import (
 type ContourConfigurationSpec struct {
 	// XDSServer contains parameters for the xDS server.
 	// +optional
-	// +kubebuilder:default={type: "contour", address: "0.0.0.0", port: 8001, tls: { caFile: "/certs/ca.crt", certFile: "/certs/tls.crt", keyFile: "/certs/tls.key", insecure: false}}
-	XDSServer XDSServerConfig `json:"xdsServer"`
+	// +kubebuilder:default={type: "contour"}


We should add a good comment at the top of this file explaining how the defaulting works since it is very non-obvious (i.e. specifying one field at this level triggers the lower-level defaults to also fill in).

skriss · 2022-01-24T21:13:52Z

apis/projectcontour/v1alpha1/contourconfig.go

+	//
+	// Other values will produce an error.
+	// The default includes both values.
+	// +kubebuilder:default="HTTP/1.1";"HTTP/2"


Seems like one that'll change over time, how do we handle?

skriss · 2022-01-24T21:14:24Z

apis/projectcontour/v1alpha1/contourconfig.go


 	// TLS holds various configurable Envoy TLS listener values.
-	TLS EnvoyTLS `json:"tls"`
+	// +optional
+	// +kubebuilder:default={minimumProtocolVersion: "1.2"}


another that'll change over time, how to handle?

This one is probably worth looking at the docstring again and making sure we explain that you can choose 1.1, but we don't turn it on by default.

Surely all valid options should be described but I guess the change over time here would be deprecation of 1.2. and setting 1.3 as default.

skriss · 2022-01-24T21:14:43Z

apis/projectcontour/v1alpha1/contourconfig.go

+	// * `1.3`
+	//
+	// Other values will produce an error.
+	// +kubebuilder:default="1.2"


another that'll change over time

youngnick · 2022-01-25T03:18:23Z

In our maintainer's meeting today, we discussed this, @skriss is going to summarize on #4062. I'll leave this as is for now.

sunjayBhatia · 2022-01-25T16:21:55Z

In our maintainer's meeting today, we discussed this, @skriss is going to summarize on #4062. I'll leave this as is for now.

also a TODO item related to this is to write an e2e test to validate kubebuilder defaults, this will ensure if defaults change in a PR, we have a signal and can write a release note etc.

trick will be to make sure new fields are added to this test over time, to ensure we maintain coverage

sunjayBhatia · 2022-01-25T16:30:23Z

apis/projectcontour/v1alpha1/contourconfig.go

-	// +kubebuilder:default={address: "0.0.0.0", port: 8000}
-	Metrics MetricsConfig `json:"metrics"`
+	// +kubebuilder:default={port: 8002}
+	Metrics *MetricsConfig `json:"metrics"`
 }

 // XDSServerType is the type of xDS server implementation.
 type XDSServerType string


for this sort of thing I also see +enum as an annotation (e.g. here), which is described here, not sure if we care to do this or not, might make things nice for downstream consumers? (but also not clear to me if this locks you into the same thing as kubebuilder enum, that adding values is a major change)

The argument against using Enums is that Clients can (and should) assume that they know all possible values of the enum, so adding or removing fields can break clients unless it's made very clear that the enum could be changed. Making the type an aliased string leaves the door open for other values than the list at any particular version.

It feels to me like this applies no matter how the enum is specified.

Does this really apply here? I think so, but I suppose that a lot of these fields are unlikely to have further values added. I think XDSServerType is a good example of one where we could conceivably add further values (like maybe incremental or something) as we add further functionality and want to have a chance to get it in people's hands before it's as stable as the rest.

I agree that having to document this stuff manually sucks, but this feels to me like another time when the rule has been written in upstream maintainer pain.

sunjayBhatia · 2022-01-25T18:27:30Z

apis/projectcontour/v1alpha1/contourconfig.go

@@ -539,7 +673,7 @@ type ContourConfigurationStatus struct {
 	// +patchStrategy=merge
 	// +listType=map
 	// +listMapKey=type
-	Conditions []contour_api_v1.DetailedCondition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`
+	Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`


just wanted to open a thread on this one, to see what the reasoning behind this change was? the standard metav1.Condition list may be sufficient?

Ah, thanks for catching this, I had intended to put a comment in here.

I think DetailedCondition was a mistake on my part. Unfixable in the v1 APIs, but we aren't really using the Errors and Warnings fields in DetailedCondition, so I think we should move towards using the upstream struct instead.

This could (and probably should) be a separate PR though.

youngnick · 2022-01-27T04:23:14Z

With conversation still ongoing in #4062, I'll leave this alone for now. Once we have more consensus there, I'll come back here and update.

github-actions · 2022-02-11T00:13:12Z

Marking this PR stale since there has been no activity for 14 days. It will be closed if there is no activity for another 30 days.

github-actions · 2022-03-01T00:17:21Z

Marking this PR stale since there has been no activity for 14 days. It will be closed if there is no activity for another 30 days.

github-actions · 2022-03-16T00:16:45Z

Marking this PR stale since there has been no activity for 14 days. It will be closed if there is no activity for another 30 days.

* Remove enum defnitions, replace with godoc for allowed values * Simplify defaults Signed-off-by: Nick Young <ynick@vmware.com>

* Update defaulting logic for TLS Ciphersuites * Update servecontext_test.go tests to reduce boilerplate * Update API Reference docs to list available constant values for string alias types * Added default value documentation to fields that have a Kubebuilder default Signed-off-by: Nick Young <ynick@vmware.com>

Signed-off-by: Nick Young <ynick@vmware.com>

youngnick · 2022-03-22T03:49:41Z

TODO list here, from #4062

Remove all the kubebuilder defaults from the ContourConfig CRD. Not validation, just default values. Everything should still be optional though.
Replace them with Contour-managed defaults that are documented in the relevant godoc, so that it shows up on the website and in kubectl describe.
Provide an example YAML for ContourConfig with everything commented out.

Signed-off-by: Nick Young <ynick@vmware.com>

github-actions · 2022-04-07T00:20:09Z

Marking this PR stale since there has been no activity for 14 days. It will be closed if there is no activity for another 30 days.

youngnick · 2022-04-08T00:04:35Z

Superseded by #4451

youngnick added the release-note/small A small change that needs one line of explanation in the release notes. label Jan 24, 2022

youngnick force-pushed the update-contourconfig branch from c6ff0af to 64531e7 Compare January 24, 2022 06:08

skriss reviewed Jan 24, 2022

View reviewed changes

youngnick force-pushed the update-contourconfig branch from 64531e7 to 987eb82 Compare January 25, 2022 04:48

sunjayBhatia mentioned this pull request Jan 25, 2022

test/e2e: Add test for validating ContourConfiguration CRD kubebuilder defaults #4295

Closed

sunjayBhatia reviewed Jan 25, 2022

View reviewed changes

youngnick mentioned this pull request Jan 28, 2022

experimenting with reorganizing builder test cases #4227

Closed

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 11, 2022

youngnick removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 11, 2022

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 1, 2022

youngnick removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 1, 2022

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 16, 2022

Nick Young added 5 commits March 22, 2022 03:25

Update ContourConfig definitions

45ce63b

* Remove enum defnitions, replace with godoc for allowed values * Simplify defaults Signed-off-by: Nick Young <ynick@vmware.com>

Fix UK to US spelling

71aeab8

Signed-off-by: Nick Young <ynick@vmware.com>

Run make generate and fix spelling

c31de86

Signed-off-by: Nick Young <ynick@vmware.com>

Add changelog

2dba758

Signed-off-by: Nick Young <ynick@vmware.com>

youngnick force-pushed the update-contourconfig branch from 987eb82 to fa2a997 Compare March 22, 2022 03:33

CRD fields with defaults must be optional

961f9fd

Signed-off-by: Nick Young <ynick@vmware.com>

youngnick force-pushed the update-contourconfig branch from fa2a997 to 961f9fd Compare March 22, 2022 04:15

github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 23, 2022

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 7, 2022

youngnick closed this Apr 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update ContourConfig definitions #4292

Update ContourConfig definitions #4292

youngnick commented Jan 24, 2022

codecov bot commented Jan 24, 2022 •

edited

Loading

youngnick commented Jan 24, 2022

skriss left a comment

skriss Jan 24, 2022

skriss Jan 24, 2022

skriss Jan 24, 2022

skriss Jan 24, 2022

youngnick Jan 24, 2022

tsaarni Feb 14, 2022

skriss Jan 24, 2022

youngnick commented Jan 25, 2022

sunjayBhatia commented Jan 25, 2022

sunjayBhatia Jan 25, 2022 •

edited

Loading

youngnick Jan 27, 2022

sunjayBhatia Jan 25, 2022

youngnick Jan 27, 2022

youngnick Jan 27, 2022

youngnick commented Jan 27, 2022

github-actions bot commented Feb 11, 2022

github-actions bot commented Mar 1, 2022

github-actions bot commented Mar 16, 2022

youngnick commented Mar 22, 2022

github-actions bot commented Apr 7, 2022

youngnick commented Apr 8, 2022

Update ContourConfig definitions #4292

Update ContourConfig definitions #4292

Conversation

youngnick commented Jan 24, 2022

codecov bot commented Jan 24, 2022 • edited Loading

Codecov Report

youngnick commented Jan 24, 2022

skriss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngnick commented Jan 25, 2022

sunjayBhatia commented Jan 25, 2022

sunjayBhatia Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngnick commented Jan 27, 2022

github-actions bot commented Feb 11, 2022

github-actions bot commented Mar 1, 2022

github-actions bot commented Mar 16, 2022

youngnick commented Mar 22, 2022

github-actions bot commented Apr 7, 2022

youngnick commented Apr 8, 2022

codecov bot commented Jan 24, 2022 •

edited

Loading

sunjayBhatia Jan 25, 2022 •

edited

Loading