Fix declarative defaults using aliases #246

alexzielenski · 2023-08-17T00:46:00Z

#239 seems to have gone stale so I took a stab at fixing this myself.

One distinction this approach taken here has over the original attempt is that it is able to construct arbitrary interleavings of aliases and pointers from any default convertible to the same base type.

Based on #247 since it fixes bug resolving local package names

alexzielenski

Annotated some changes to give context to reviewer

alexzielenski · 2023-08-17T00:47:58Z

examples/defaulter-gen/generators/defaulter.go

 	var defaultString string
 	if len(defaultMap) == 1 {
 		defaultString = defaultMap[0]
+	} else if len(defaultMap) > 1 {


Moved this line up from below since defaultMap is not ever mutated

alexzielenski · 2023-08-17T00:50:04Z

examples/defaulter-gen/generators/defaulter.go

-		klog.Fatalf("Found more than one default tag for %v", t.Kind)
-	} else if len(defaultMap) == 0 {
+
+	if len(defaultString) == 0 {


Changed len(defaultMap) == 0 -> len(defaultString) == 0` since that seemed to be the original intention to allow nested default to work. Nested default is only used if default map is empty, so the original condition here would block their use.

alexzielenski · 2023-08-17T00:52:42Z

examples/defaulter-gen/generators/defaulter.go


-	t, depth := resolveTypeAndDepth(t)
-	if depth > 0 && defaultString == "" {
+	baseT, depth := resolveTypeAndDepth(t)


Renamed t->baseT since getNestedDefault would always be invoked on the primitive base type like string, int, struct, etc and always yield no nested default string unless base type was struct with default defined. But that case is already unusable due to mustEnforceDefault enforcing all structs use {} or nothing as their default value.

It seems the original intention of the code was to call getNestedDefault on the input argument. This would allow a default declared on an alias to be respected. I've refactored t->baseT usages elsewhere where the base primitive element type was desired.

examples/defaulter-gen/generators/defaulter.go

alexzielenski · 2023-08-17T01:00:11Z

/cc @sttts

apelisse · 2023-08-17T23:29:22Z

That's great, I still need to continue reviewing, I just want to mention that these defaults won't be included in the OpenAPI. The code that parses the default flags into their OpenAPI components lives in kube-openapi.

apelisse · 2023-08-17T23:51:18Z

examples/defaulter-gen/_output_tests/marker/type.go

+	// Type-level default is not respected unless a pointer
+	AliasDefault DefaultedValueItem `json:",omitempty"`
+
+	// Type-level default is not respected unless a pointer
+	AliasPointerDefault *DefaultedValueItem `json:",omitempty"`


My memory on this is blurry, but shouldn't we accept the defaults if it's omitempty, even if it's not a pointer?

That's what I would've expected but the source seems explicitly written to only read the default from the struct/alias type if the type is pointer (depth > 0):

gengo/examples/defaulter-gen/generators/defaulter.go

Lines 518 to 528 in ab3349d

func populateDefaultValue(node *callNode, t *types.Type, tags string, commentLines []string) *callNode {

defaultMap := extractDefaultTag(commentLines)

var defaultString string

if len(defaultMap) == 1 {

defaultString = defaultMap[0]

}

t, depth := resolveTypeAndDepth(t)

if depth > 0 && defaultString == "" {

defaultString = getNestedDefault(t)

}

That's probably a bug to be honest.

examples/defaulter-gen/_output_tests/marker/type.go

examples/defaulter-gen/generators/defaulter.go

apelisse

Thanks
/lgtm

apelisse · 2023-08-22T21:44:22Z

examples/defaulter-gen/_output_tests/marker/type.go

+	// +default=ref(SomeDefault)
+	AliasOverride Item
+
+	// Type-level default is not respected unless a pointer


Not sure if that makes sense or not.

apelisse

/lgtm

examples/defaulter-gen/generators/defaulter.go

struct was getting very large also adds omitempty tag for non-pointer fields. Openapi-gen would throw error. probably a bug that it is accepted in defaulter-gen test: import package of types that are used for casting cleanup: go mod tidy

also unconditionally cast when using references since we dont know their source type otherwise if you have a `string` field, and provide a `type Value string`-valued default, you get an error trying to assign an alias to a string cleanup and commenting

apelisse

/lgtm

apelisse · 2023-08-24T21:53:19Z

pull-gengo-test seems to have errors but the job is still succeeding, is this expected?

alexzielenski · 2023-08-24T22:06:33Z

pull-gengo-test seems to have errors but the job is still succeeding, is this expected?

yes those are expected errors from the import boss test

alexzielenski · 2023-08-25T06:31:42Z

Tested against current k8s master branch with only one minor package naming change due to #247 (configv1alpha1 "pkg/controller/apis/config/v1alpha1" is no longer being added to import tracker since it is the local package):

diff --git a/pkg/controller/apis/config/v1alpha1/zz_generated.defaults.go b/pkg/controller/apis/config/v1alpha1/zz_generated.defaults.go
index bd27200c5a4..b441808f2ec 100644
--- a/pkg/controller/apis/config/v1alpha1/zz_generated.defaults.go
+++ b/pkg/controller/apis/config/v1alpha1/zz_generated.defaults.go
@@ -23,7 +23,7 @@ package v1alpha1
 
 import (
 	runtime "k8s.io/apimachinery/pkg/runtime"
-	cloudproviderconfigv1alpha1 "k8s.io/cloud-provider/config/v1alpha1"
+	configv1alpha1 "k8s.io/cloud-provider/config/v1alpha1"
 	v1alpha1 "k8s.io/kube-controller-manager/config/v1alpha1"
 )
 
@@ -39,5 +39,5 @@ func RegisterDefaults(scheme *runtime.Scheme) error {
 
 func SetObjectDefaults_KubeControllerManagerConfiguration(in *v1alpha1.KubeControllerManagerConfiguration) {
 	SetDefaults_KubeControllerManagerConfiguration(in)
-	cloudproviderconfigv1alpha1.SetDefaults_KubeCloudSharedConfiguration(&in.KubeCloudShared)
+	configv1alpha1.SetDefaults_KubeCloudSharedConfiguration(&in.KubeCloudShared)
 }

examples/defaulter-gen/generators/defaulter.go

apelisse

/lgtm

sttts · 2023-08-29T15:13:30Z

/approve

k8s-ci-robot · 2023-08-29T15:13:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, apelisse, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~examples/defaulter-gen/OWNERS~~ [sttts]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 17, 2023

k8s-ci-robot requested review from lavalamp and smarterclayton August 17, 2023 00:46

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 17, 2023

alexzielenski commented Aug 17, 2023

View reviewed changes

k8s-ci-robot requested a review from sttts August 17, 2023 01:00

alexzielenski force-pushed the default-alias-fix branch 4 times, most recently from c4967f5 to 7b0b2a6 Compare August 17, 2023 01:24

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 17, 2023

alexzielenski force-pushed the default-alias-fix branch 3 times, most recently from 14834e1 to cafd10d Compare August 17, 2023 01:48

alexzielenski mentioned this pull request Aug 18, 2023

OpenAPI Defaults not accurate kubernetes/kubernetes#119766

Open

79 tasks

apelisse reviewed Aug 21, 2023

View reviewed changes

alexzielenski force-pushed the default-alias-fix branch from 4e747b6 to 22fc851 Compare August 22, 2023 00:27

apelisse reviewed Aug 22, 2023

View reviewed changes

k8s-ci-robot assigned apelisse Aug 22, 2023

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 22, 2023

alexzielenski force-pushed the default-alias-fix branch from 22fc851 to a57114b Compare August 23, 2023 19:30

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 23, 2023

alexzielenski force-pushed the default-alias-fix branch 3 times, most recently from e625bcb to 5d46758 Compare August 24, 2023 00:23

alexzielenski added 2 commits August 24, 2023 11:46

cleanup: silence lint error about omitempty tag reused

47d5695

test: add *int32 and *int64 test cases

08c1fba

alexzielenski force-pushed the default-alias-fix branch from 5d46758 to ca53d90 Compare August 24, 2023 18:51

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2023

apelisse reviewed Aug 24, 2023

View reviewed changes

examples/defaulter-gen/generators/defaulter.go Outdated Show resolved Hide resolved

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2023

alexzielenski added 2 commits August 24, 2023 14:11

alexzielenski force-pushed the default-alias-fix branch from ca53d90 to b5ba24c Compare August 24, 2023 21:12

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2023

apelisse reviewed Aug 24, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2023

sttts reviewed Aug 25, 2023

View reviewed changes

examples/defaulter-gen/generators/defaulter.go Outdated Show resolved Hide resolved

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 25, 2023

alexzielenski force-pushed the default-alias-fix branch from 1da33ff to 2d1998f Compare August 25, 2023 17:54

rename depth, add comments

be9cf52

alexzielenski force-pushed the default-alias-fix branch from 2d1998f to be9cf52 Compare August 25, 2023 17:55

apelisse reviewed Aug 28, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 28, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 29, 2023

k8s-ci-robot merged commit 9cce18d into kubernetes:master Aug 29, 2023

This was referenced Sep 5, 2023

Fix declarative defaults for aliases #239

Closed

generated defaults lose type information for aliases #235

Closed

alexzielenski mentioned this pull request Oct 16, 2023

Declarative Validation kubernetes/enhancements#4153

Closed

4 tasks

aaron-prindle mentioned this pull request Jun 13, 2025

declarative defaults need to handle const values kubernetes/kubernetes#104207

Open

	func populateDefaultValue(node callNode, t types.Type, tags string, commentLines []string) *callNode {
	defaultMap := extractDefaultTag(commentLines)
	var defaultString string
	if len(defaultMap) == 1 {
	defaultString = defaultMap[0]
	}

	t, depth := resolveTypeAndDepth(t)
	if depth > 0 && defaultString == "" {
	defaultString = getNestedDefault(t)
	}

Fix declarative defaults using aliases #246

Fix declarative defaults using aliases #246

Uh oh!

Conversation

alexzielenski commented Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexzielenski left a comment

Choose a reason for hiding this comment

Uh oh!

alexzielenski Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

alexzielenski Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexzielenski Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexzielenski commented Aug 17, 2023

Uh oh!

apelisse commented Aug 17, 2023

Uh oh!

apelisse Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

alexzielenski Aug 21, 2023

Choose a reason for hiding this comment

Uh oh!

apelisse Aug 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

apelisse left a comment

Choose a reason for hiding this comment

Uh oh!

apelisse Aug 22, 2023

Choose a reason for hiding this comment

Uh oh!

apelisse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

apelisse left a comment

Choose a reason for hiding this comment

Uh oh!

apelisse commented Aug 24, 2023

Uh oh!

alexzielenski commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexzielenski commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

apelisse left a comment

Choose a reason for hiding this comment

Uh oh!

sttts commented Aug 29, 2023

Uh oh!

k8s-ci-robot commented Aug 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alexzielenski commented Aug 17, 2023 •

edited

Loading

alexzielenski Aug 17, 2023 •

edited

Loading

alexzielenski Aug 17, 2023 •

edited

Loading

alexzielenski commented Aug 24, 2023 •

edited

Loading

alexzielenski commented Aug 25, 2023 •

edited

Loading