✨ [POC] Optional fields struct linter

CONTRIBUTING.md

@@ -252,6 +252,71 @@ There may, at times, need to be exceptions where breaking changes are allowed in
 discretion of the project's maintainers, and must be carefully considered before merging. An example of an allowed
 breaking change might be a fix for a behavioral bug that was released in an initial minor version (such as `v0.3.0`).
 
+
+## API conventions
+
+In general we adhere to the [Kubernetes API conventions](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required).
+We have a small set of exceptions due to the fact that we're using CRDs while the API conventions predates the introduction 
+of CRDs and the way our controllers only patch actual changes to the status instead of updating the entire status in every 
+reconciliation.
+
+### Optional vs. Required
+
+Status fields must be optional, because our controllers are patching selected fields instead of updating the entire status 
+in every reconciliation.
+
+Optional fields have the following properties:
+* In general an optional field must have both an `+optional` marker and an `omitempty` JSON tag.
+* If the semantic difference between nil and the zero value is important for your code, the field must also be a pointer.
+  * Note: This doesn't apply to map or slice types as they already have a built-in `nil` value.
+  * Example: When using ClusterClass, the semantic difference is important when you have a field in a template which will 
+    have instance-specific different values in derived objects. Because in this case it's possible to set the field to `nil`
+    in the template and then the value can be set in derived objects without being overwritten by the cluster topology controller.
+
+* Exceptions:
+  * Fields in root objects should be kept as scaffolded by kubebuilder, e.g.:
+    ```golang
+    type Machine struct {
+      metav1.TypeMeta   `json:",inline"`
+      metav1.ObjectMeta `json:"metadata,omitempty"`
+
+      Spec   MachineSpec   `json:"spec,omitempty"`
+      Status MachineStatus `json:"status,omitempty"`
+    }
+    type MachineList struct {
+      metav1.TypeMeta `json:",inline"`
+      metav1.ListMeta `json:"metadata,omitempty"`
+      Items           []Machine `json:"items"`
+    }
+    ```
+  * Top-level fields in `status` must always have the `+optional` annotation. If we want the field to be always visible even if it 
+    has the zero value, it must **not** have the `omitempty` JSON tag, e.g.:
+    * Replica counters like `availableReplicas` in the `MachineDeployment`
+    * Flags expressing progress in the object lifecycle like `infrastructureReady` in `Machine`
+
+### CRD additionalPrinterColumns
+
+All our CRD objects should have the following `additionalPrinterColumns` order (if the respective field exists in the CRD):
+* Namespace (added automatically)
+* Name (added automatically)
+* Cluster
+* Other fields
+* Replica-related fields
+* Phase
+* Age (mandatory field for all CRDs)
+* Version
+* Other fields for -o wide (fields with priority `1` are only shown with `-o wide` and not per default)
+
+Examples:
+```bash
+$ kubectl get kubeadmcontrolplane
+NAMESPACE            NAME                               INITIALIZED   API SERVER AVAILABLE   REPLICAS   READY   UPDATED   UNAVAILABLE   AGE     VERSION
+quick-start-d5ufye   quick-start-ntysk0-control-plane   true          true                   1          1       1                       2m44s   v1.22.0
+$ kubectl get machinedeployment
+NAMESPACE            NAME                      CLUSTER              REPLICAS   READY   UPDATED   UNAVAILABLE   PHASE       AGE     VERSION
+quick-start-d5ufye   quick-start-ntysk0-md-0   quick-start-ntysk0   1                  1         1             ScalingUp   3m28s   v1.22.0
+```
+
 ## Google Doc Viewing Permissions
 
 To gain viewing permissions to google docs in this project, please join either the

Makefile

@@ -82,6 +82,7 @@ CONTROLLER_GEN := $(abspath $(TOOLS_BIN_DIR)/controller-gen)
 GOTESTSUM := $(abspath $(TOOLS_BIN_DIR)/gotestsum)
 GOLANGCI_LINT := $(abspath $(TOOLS_BIN_DIR)/golangci-lint)
 CONVERSION_GEN := $(abspath $(TOOLS_BIN_DIR)/conversion-gen)
+STRUCT_VERIFIER := $(abspath $(TOOLS_BIN_DIR)/struct-verifier)
 ENVSUBST := $(abspath $(TOOLS_BIN_DIR)/envsubst)
 
 # clusterctl.
@@ -204,6 +205,9 @@ $(GOTESTSUM): $(TOOLS_DIR)/go.mod # Build gotestsum from tools folder.
 $(CONVERSION_GEN): $(TOOLS_DIR)/go.mod
 	cd $(TOOLS_DIR); go build -tags=tools -o $(BIN_DIR)/conversion-gen k8s.io/code-generator/cmd/conversion-gen
 
+$(STRUCT_VERIFIER): $(TOOLS_DIR)/go.mod
+	cd $(TOOLS_DIR); go build -tags=tools -o $(BIN_DIR)/struct-verifier sigs.k8s.io/cluster-api/hack/tools/struct-verifier
+
 $(GO_APIDIFF): $(TOOLS_DIR)/go.mod
 	cd $(TOOLS_DIR) && go build -tags=tools -o $(GO_APIDIFF_BIN) github.com/joelanford/go-apidiff
 
@@ -223,6 +227,7 @@ kustomize: $(KUSTOMIZE) ## Build a local copy of kustomize.
 setup-envtest: $(SETUP_ENVTEST) ## Build a local copy of setup-envtest.
 controller-gen: $(CONTROLLER_GEN) ## Build a local copy of controller-gen.
 conversion-gen: $(CONVERSION_GEN) ## Build a local copy of conversion-gen.
+struct-verifier: $(STRUCT_VERIFIER) ## Build a local copy of struct-verifier.
 gotestsum: $(GOTESTSUM) ## Build a local copy of gotestsum.
 
 .PHONY: e2e-framework

api/v1beta1/cluster_types.go

@@ -49,7 +49,7 @@ type ClusterSpec struct {
 
 	// ControlPlaneEndpoint represents the endpoint used to communicate with the control plane.
 	// +optional
-	ControlPlaneEndpoint APIEndpoint `json:"controlPlaneEndpoint"`
+	ControlPlaneEndpoint APIEndpoint `json:"controlPlaneEndpoint,omitempty"`
 
 	// ControlPlaneRef is an optional reference to a provider-specific resource that holds
 	// the details for provisioning the Control Plane for a Cluster.
@@ -84,7 +84,7 @@ type Topology struct {
 
 	// ControlPlane describes the cluster control plane.
 	// +optional
-	ControlPlane ControlPlaneTopology `json:"controlPlane"`
+	ControlPlane ControlPlaneTopology `json:"controlPlane,omitempty"`
 
 	// Workers encapsulates the different constructs that form the worker nodes
 	// for the cluster.
@@ -99,6 +99,7 @@ type ControlPlaneTopology struct {
 	//
 	// This field is supported if and only if the control plane provider template
 	// referenced in the ClusterClass is Machine based.
+	// +optional
 	Metadata ObjectMeta `json:"metadata,omitempty"`
 
 	// Replicas is the number of control plane nodes.
@@ -112,6 +113,7 @@ type ControlPlaneTopology struct {
 // WorkersTopology represents the different sets of worker nodes in the cluster.
 type WorkersTopology struct {
 	// MachineDeployments is a list of machine deployments in the cluster.
+	// +optional
 	MachineDeployments []MachineDeploymentTopology `json:"machineDeployments,omitempty"`
 }
 
@@ -120,6 +122,7 @@ type WorkersTopology struct {
 type MachineDeploymentTopology struct {
 	// Metadata is the metadata applied to the machines of the MachineDeployment.
 	// At runtime this metadata is merged with the corresponding metadata from the ClusterClass.
+	// +optional
 	Metadata ObjectMeta `json:"metadata,omitempty"`
 
 	// Class is the name of the MachineDeploymentClass used to create the set of worker nodes.
@@ -189,6 +192,7 @@ func (n *NetworkRanges) String() string {
 // ClusterStatus defines the observed state of Cluster.
 type ClusterStatus struct {
 	// FailureDomains is a slice of failure domain objects synced from the infrastructure provider.
+	// +optional
 	FailureDomains FailureDomains `json:"failureDomains,omitempty"`
 
 	// FailureReason indicates that there is a fatal problem reconciling the
@@ -213,7 +217,7 @@ type ClusterStatus struct {
 
 	// ControlPlaneReady defines if the control plane is ready.
 	// +optional
-	ControlPlaneReady bool `json:"controlPlaneReady,omitempty"`
+	ControlPlaneReady bool `json:"controlPlaneReady"`
 
 	// Conditions defines current service state of the cluster.
 	// +optional
@@ -279,8 +283,9 @@ func (v APIEndpoint) String() string {
 // +kubebuilder:resource:path=clusters,shortName=cl,scope=Namespaced,categories=cluster-api
 // +kubebuilder:storageversion
 // +kubebuilder:subresource:status
-// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="Time duration since creation of Cluster"
 // +kubebuilder:printcolumn:name="Phase",type="string",JSONPath=".status.phase",description="Cluster status such as Pending/Provisioning/Provisioned/Deleting/Failed"
+// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="Time duration since creation of Cluster"
+// +kubebuilder:printcolumn:name="Version",type="string",JSONPath=".spec.topology.version",description="Kubernetes version associated with this Cluster"
 
 // Cluster is the Schema for the clusters API.
 type Cluster struct {
@@ -427,7 +432,7 @@ func (in FailureDomains) GetIDs() []*string {
 type FailureDomainSpec struct {
 	// ControlPlane determines if this failure domain is suitable for use by control plane machines.
 	// +optional
-	ControlPlane bool `json:"controlPlane"`
+	ControlPlane bool `json:"controlPlane,omitempty"`
 
 	// Attributes is a free form map of attributes an infrastructure provider might use or require.
 	// +optional

api/v1beta1/clusterclass_types.go

@@ -41,10 +41,12 @@ type ClusterClassSpec struct {
 	// for the underlying provider.
 	// The underlying provider is responsible for the implementation
 	// of the template to an infrastructure cluster.
+	// +optional
 	Infrastructure LocalObjectTemplate `json:"infrastructure,omitempty"`
 
 	// ControlPlane is a reference to a local struct that holds the details
 	// for provisioning the Control Plane for the Cluster.
+	// +optional
 	ControlPlane ControlPlaneClass `json:"controlPlane,omitempty"`
 
 	// Workers describes the worker nodes for the cluster.
@@ -61,6 +63,7 @@ type ControlPlaneClass struct {
 	//
 	// This field is supported if and only if the control plane provider template
 	// referenced is Machine based.
+	// +optional
 	Metadata ObjectMeta `json:"metadata,omitempty"`
 
 	// LocalObjectTemplate contains the reference to the control plane provider.
@@ -80,6 +83,7 @@ type ControlPlaneClass struct {
 type WorkersClass struct {
 	// MachineDeployments is a list of machine deployment classes that can be used to create
 	// a set of worker nodes.
+	// +optional
 	MachineDeployments []MachineDeploymentClass `json:"machineDeployments,omitempty"`
 }
 
@@ -101,6 +105,7 @@ type MachineDeploymentClass struct {
 type MachineDeploymentClassTemplate struct {
 	// Metadata is the metadata applied to the machines of the MachineDeployment.
 	// At runtime this metadata is merged with the corresponding metadata from the topology.
+	// +optional
 	Metadata ObjectMeta `json:"metadata,omitempty"`
 
 	// Bootstrap contains the bootstrap template reference to be used

api/v1beta1/condition_types.go

@@ -56,11 +56,9 @@ type Condition struct {
 	// Type of condition in CamelCase or in foo.example.com/CamelCase.
 	// Many .condition.type values are consistent across resources like Available, but because arbitrary conditions
 	// can be useful (see .node.status.conditions), the ability to deconflict is important.
-	// +required
 	Type ConditionType `json:"type"`
 
 	// Status of the condition, one of True, False, Unknown.
-	// +required
 	Status corev1.ConditionStatus `json:"status"`
 
 	// Severity provides an explicit classification of Reason code, so the users or machines can immediately
@@ -72,8 +70,7 @@ type Condition struct {
 	// Last time the condition transitioned from one status to another.
 	// This should be when the underlying condition changed. If that is not known, then using the time when
 	// the API field changed is acceptable.
-	// +required
-	LastTransitionTime metav1.Time `json:"lastTransitionTime,omitempty"`
+	LastTransitionTime metav1.Time `json:"lastTransitionTime"`
 
 	// The reason for the condition's last transition in CamelCase.
 	// The specific API may choose whether or not this field is considered a guaranteed API.

api/v1beta1/machine_types.go

@@ -237,11 +237,11 @@ type Bootstrap struct {
 // +kubebuilder:subresource:status
 // +kubebuilder:storageversion
 // +kubebuilder:printcolumn:name="Cluster",type="string",JSONPath=".spec.clusterName",description="Cluster"
-// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="Time duration since creation of Machine"
+// +kubebuilder:printcolumn:name="NodeName",type="string",JSONPath=".status.nodeRef.name",description="Node name associated with this machine"
 // +kubebuilder:printcolumn:name="ProviderID",type="string",JSONPath=".spec.providerID",description="Provider ID"
 // +kubebuilder:printcolumn:name="Phase",type="string",JSONPath=".status.phase",description="Machine status such as Terminating/Pending/Running/Failed etc"
+// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="Time duration since creation of Machine"
 // +kubebuilder:printcolumn:name="Version",type="string",JSONPath=".spec.version",description="Kubernetes version associated with this Machine"
-// +kubebuilder:printcolumn:name="NodeName",type="string",JSONPath=".status.nodeRef.name",description="Node name associated with this machine",priority=1
 
 // Machine is the Schema for the machines API.
 type Machine struct {

api/v1beta1/machinedeployment_types.go

@@ -104,6 +104,7 @@ type MachineDeploymentSpec struct {
 	// process failed deployments and a condition with a ProgressDeadlineExceeded
 	// reason will be surfaced in the deployment status. Note that progress will
 	// not be estimated during the time a deployment is paused. Defaults to 600s.
+	// +optional
 	ProgressDeadlineSeconds *int32 `json:"progressDeadlineSeconds,omitempty"`
 }
 
@@ -190,29 +191,44 @@ type MachineDeploymentStatus struct {
 	// Total number of non-terminated machines targeted by this deployment
 	// (their labels match the selector).
 	// +optional
-	Replicas int32 `json:"replicas,omitempty"`
+	Replicas int32 `json:"replicas"`
 
 	// Total number of non-terminated machines targeted by this deployment
 	// that have the desired template spec.
 	// +optional
-	UpdatedReplicas int32 `json:"updatedReplicas,omitempty"`
+	UpdatedReplicas int32 `json:"updatedReplicas"`
+
+	// Note: shows up in status
+	// +optional
+	TestReplicas int32 `json:"testReplicas"`
+
+	// +optional
+	TestReplicasOmitEmpty int32 `json:"testReplicasOmitEmpty,omitempty"`
+
+	// Note: shows up in status
+	// +optional
+	TestReplicasPointer *int32 `json:"testReplicasPointer"`
+
+	// Note: shows up in status
+	// +optional
+	TestReplicasPointerOmitEmpty *int32 `json:"testReplicasPointerOmitEmpty,omitempty"`
 
 	// Total number of ready machines targeted by this deployment.
 	// +optional
-	ReadyReplicas int32 `json:"readyReplicas,omitempty"`
+	ReadyReplicas int32 `json:"readyReplicas"`
 
 	// Total number of available machines (ready for at least minReadySeconds)
 	// targeted by this deployment.
 	// +optional
-	AvailableReplicas int32 `json:"availableReplicas,omitempty"`
+	AvailableReplicas int32 `json:"availableReplicas"`
 
 	// Total number of unavailable machines targeted by this deployment.
 	// This is the total number of machines that are still required for
 	// the deployment to have 100% available capacity. They may either
 	// be machines that are running but not yet available or machines
 	// that still have not been created.
 	// +optional
-	UnavailableReplicas int32 `json:"unavailableReplicas,omitempty"`
+	UnavailableReplicas int32 `json:"unavailableReplicas"`
 
 	// Phase represents the current phase of a MachineDeployment (ScalingUp, ScalingDown, Running, Failed, or Unknown).
 	// +optional
@@ -271,12 +287,13 @@ func (md *MachineDeploymentStatus) GetTypedPhase() MachineDeploymentPhase {
 // +kubebuilder:subresource:status
 // +kubebuilder:subresource:scale:specpath=.spec.replicas,statuspath=.status.replicas,selectorpath=.status.selector
 // +kubebuilder:printcolumn:name="Cluster",type="string",JSONPath=".spec.clusterName",description="Cluster"
-// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="Time duration since creation of MachineDeployment"
-// +kubebuilder:printcolumn:name="Phase",type="string",JSONPath=".status.phase",description="MachineDeployment status such as ScalingUp/ScalingDown/Running/Failed/Unknown"
 // +kubebuilder:printcolumn:name="Replicas",type="integer",JSONPath=".status.replicas",description="Total number of non-terminated machines targeted by this MachineDeployment"
 // +kubebuilder:printcolumn:name="Ready",type="integer",JSONPath=".status.readyReplicas",description="Total number of ready machines targeted by this MachineDeployment"
 // +kubebuilder:printcolumn:name="Updated",type=integer,JSONPath=".status.updatedReplicas",description="Total number of non-terminated machines targeted by this deployment that have the desired template spec"
 // +kubebuilder:printcolumn:name="Unavailable",type=integer,JSONPath=".status.unavailableReplicas",description="Total number of unavailable machines targeted by this MachineDeployment"
+// +kubebuilder:printcolumn:name="Phase",type="string",JSONPath=".status.phase",description="MachineDeployment status such as ScalingUp/ScalingDown/Running/Failed/Unknown"
+// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="Time duration since creation of MachineDeployment"
+// +kubebuilder:printcolumn:name="Version",type="string",JSONPath=".spec.template.spec.version",description="Kubernetes version associated with this MachineDeployment"
 
 // MachineDeployment is the Schema for the machinedeployments API.
 type MachineDeployment struct {

api/v1beta1/machinedeployment_types_test.go

@@ -0,0 +1,73 @@
+/*
+Copyright 2021 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1beta1
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+var testScheme = runtime.NewScheme()
+
+func init() {
+	_ = clientgoscheme.AddToScheme(testScheme)
+	_ = apiextensionsv1.AddToScheme(testScheme)
+	_ = AddToScheme(testScheme)
+}
+
+func TestList(t *testing.T) {
+	cfg := ctrl.GetConfigOrDie()
+
+	c, err := client.New(cfg, client.Options{
+		Scheme: testScheme,
+	})
+	if err != nil {
+		panic(err)
+	}
+
+	ctx := context.Background()
+
+	list := &MachineDeploymentList{}
+	if err := c.List(ctx, list); err != nil {
+		panic(err)
+	}
+
+	md := &MachineDeployment{}
+	if err := c.Get(ctx, client.ObjectKey{Namespace: "quick-start-hr8mgd", Name: "quick-start-bcd1q5-md-0"}, md); err != nil {
+		panic(err)
+	}
+
+	md.Status.TestReplicasPointer = nil
+	md.Status.TestReplicasPointerOmitEmpty = nil
+	if err := c.Status().Update(ctx, md); err != nil {
+		panic(err)
+	}
+
+	mdfinal := &MachineDeployment{}
+	if err := c.Get(ctx, client.ObjectKey{Namespace: "quick-start-hr8mgd", Name: "quick-start-bcd1q5-md-0"}, mdfinal); err != nil {
+		panic(err)
+	}
+
+	fmt.Println(list)
+}