Add support for field defaults

[Julian Gutierrez Oschmann (juli4n)]

Populate default fields during both Create and Update RPC calls so that resources are stored with defaults instead of zero values. This means that a) consumers don't need to be aware of the default value and b) default values can potentially change.

Only ActorTemplate had defaults, but added default functions for all resources so that all RPC handlers are wired up

Note that optional fields with defaults need to be tagged as +k8s:required. This is because they are required when they are stored in the DB. This is different than required from the client perspective.

Part of #1593

[Tim Hockin (thockin)]

In fact we have logic for this in DV:

   // All of our tags are expressed from the perspective of a client of the    
    // API, but the code we generate is for the server. Optional is tricky.    
    //    
    // A field which is marked as optional and does not have a default is    
    // strictly optional. A client is allowed to not set it and the server will    
    // not give it a default value. Code which consumes it must handle that it    
    // might not have any value at all.    
    //    
    // A field which is marked as optional but has a default is optional to    
    // clients, but required to the server. A client is allowed to not set it    
    // but the server will give it a default value. Code which consumes it can    
    // assume that it always has a value.    
    //    
    // One special case must be handled: optional non-pointer fields with    
    // default values. If the default is not the zero value for the type, then    
    // the zero value is used to decide whether to assign the default value,    
    // and so must be out of bounds; we can proceed as above.    
    //    
    // But if the default is the zero value, then the zero value is obviously    
    // valid, and the fact that the field is optional is meaningless - there is    
    // no way to tell the difference between a client not setting it (yielding    
    // the zero value) and a client setting it to the zero value.    
    //    
    // TODO: handle default=ref(...)    
    // TODO: handle manual defaulting    
    if hasDefault, zeroDefault, err := rtv.hasZeroDefault(context); err != nil {                                                                      
        return Validations{}, err    
    } else if hasDefault {    
        if !util.IsNilableType(context.Type) && zeroDefault {    
            return Validations{Comments: []string{"optional value-type fields with zero-value defaults are purely documentation"}}, nil    
        }    
        validations, err := rtv.doRequired(context)    
        if err != nil {    
            return Validations{}, err    
        }    
        for i, fn := range validations.Functions {    
            validations.Functions[i] = fn.WithComment("optional fields with default values are effectively required")    
        }    
        return validations, nil    
    }                                                                                               

Yongrui Lin (@yongruilin) Joe Betz (@jpbetz) 2 things go wrong for us to use DV here:

1. Defaults is a totally different tool and different tag style.
2. We really want to be able to say +default=SNAPSHOT_CONTENT_SCOPE_FULL but validation-gen barfs: "failed to parse default value "SNAPSHOT_CONTENT_SCOPE_FULL": invalid character 'S' looking for beginning of value". We could fix that to just take any such value as a Go symbol name (if JSON parse fails with this specific error)

We could do +default=1 but eww. Here's what we get if we do that:

diff --git cmd/ateapi/internal/controlapi/zz_generated.validation.go cmd/ateapi/internal/controlapi/zz_generated.validation.go
index 94c2e534..93047826 100644
--- cmd/ateapi/internal/controlapi/zz_generated.validation.go
+++ cmd/ateapi/internal/controlapi/zz_generated.validation.go
@@ -5658,6 +5658,7 @@ func Validate_SnapshotsConfig(
 			}
 			// call field-attached validations
 			earlyReturn := false
+			// optional fields with default values are effectively required
 			if e := validate.RequiredValue(ctx, op, fldPath, obj, oldObj).MarkShortCircuit(); len(e) != 0 {
 				errs = append(errs, e...)
 				earlyReturn = true
diff --git pkg/proto/ateapipb/ateapi.pb.go pkg/proto/ateapipb/ateapi.pb.go
index 0317b0ed..b4c344d5 100644
--- pkg/proto/ateapipb/ateapi.pb.go
+++ pkg/proto/ateapipb/ateapi.pb.go
@@ -2452,7 +2452,8 @@ type SnapshotsConfig struct {
 	// on_pause defines the scope of the snapshot captured when an actor
 	// is paused. Defaults to FULL when unset.
 	//
-	// +k8s:required
+	// +k8s:optional
+	// +default=1
 	// +k8s:minimum=1
 	// +k8s:maximum=2 # keep this in sync with the SnapshotContentScope enum
 	OnPause SnapshotContentScope `protobuf:"varint,1,opt,name=on_pause,json=onPause,proto3,enum=ateapi.SnapshotContentScope" json:"on_pause,omitempty"`
diff --git pkg/proto/ateapipb/ateapi.proto pkg/proto/ateapipb/ateapi.proto
index 41b76370..d61f8a9a 100644
--- pkg/proto/ateapipb/ateapi.proto
+++ pkg/proto/ateapipb/ateapi.proto
@@ -850,7 +850,8 @@ message SnapshotsConfig {
   // on_pause defines the scope of the snapshot captured when an actor
   // is paused. Defaults to FULL when unset.
   //
-  // +k8s:required
+  // +k8s:optional
+  // +default=1
   // +k8s:minimum=1
   // +k8s:maximum=2 # keep this in sync with the SnapshotContentScope enum
   SnapshotContentScope on_pause = 1;

The question is whether we think optional or required is actually correct for documentation and tools.

[Yongrui Lin (yongruilin)]

defaulter-gen supports +default=ref(xxx). There was an effort to support the same in DV a while back but I lost the track. kubernetes/kubernetes#136361

[Tim Hockin (thockin)]

This is a really bad default - we should fix it to "/" and make "/readyz" explicit.

[Joe Betz (jpbetz)]

+1. Defaults should be used sparingly. Having "port" default to "type: TCP" is arguably sane if that's what the vast majority of users will want. Having a highly opinionated defaults, defaults that are computed dynamically, or are expected to change across releases tend to be trouble.

[Tim Hockin (thockin)]

Curious why not put these in their respective resource-files?