Properly support enum Names in resources (#326)

### Naming changes

Don't convert all resource Spec `Name` properties to `string`, and instead allow the following cases:

- An enum with one value generates no `AzureName` property in the CRD as it cannot be changed. An `AzureName()` function is provided for `ConvertToArm` to invoke, which returns the fixed value.
  This only triggers for one resource currently:
    - Web `HostingEnvironmentsMultiRolePool` which must have a name of `default`

- An enum with multiple values generates an `AzureName` property with an enum value. An `AzureName()` function is provided for `ConvertToArm` which casts the enum value to string.
  Example resources for this are:
    - Keyvault `VaultsAccessPolicies` which allows names of `add`, `remove`, and `replace`
    - API Management `ServiceIdentityProvider` which allows names of `aad`, `aadB2C`, `facebook`, `google`, `microsoft`, and `twitter`

All Spec `Name` properties are still converted to `string` for the corresponding ARM type, as we need to insert a full ARM ID including owning resource IDs, etc.

### Serialization changes

Split `ArmTransformer` interface into two parts `FromArmConverter`/`ToArmConverter` and make it so that Status types only implement `FromArmConverter`.

## Other cases

There are other cases to be solved; some services (like Storage) require a Name ending in `default` (which is actually more correct, as the Name is a full URI including parent resources). At the schema level this is enforced via a regex like `^.*/default$`. We can recognize these cases and handle them; but that will be a different PR—we don’t yet support type validation properly.

Taskfile.yml

@@ -50,6 +50,12 @@ tasks:
     cmds:
     - go test ./... -tags=noexit
 
+  generator:update-golden-tests:
+    desc: Update {{.GENERATOR_APP}} golden test outputs.
+    dir: "{{.GENERATOR_ROOT}}"
+    cmds:
+    - go test ./pkg/codegen -run ^TestGolden$ -update
+
   generator:format-code:
     desc: Ensure all code is correctly formatted.
     dir: "{{.GENERATOR_ROOT}}"

hack/generated/apis/microsoft.resources/v20200601/resourcegroup_types.go

@@ -62,6 +62,10 @@ type ResourceGroupStatus struct {
 
 var _ genruntime.ArmTransformer = &ResourceGroupStatus{}
 
+func (status *ResourceGroupStatus) CreateEmptyArmValue() interface{} {
+	return ResourceGroupStatusArm{}
+}
+
 // ConvertToArm converts from a Kubernetes CRD object to an ARM object
 func (status *ResourceGroupStatus) ConvertToArm(name string) (interface{}, error) {
 	if status == nil {
@@ -113,6 +117,10 @@ type ResourceGroupStatusProperties struct {
 
 var _ genruntime.ArmTransformer = &ResourceGroupStatusProperties{}
 
+func (p *ResourceGroupStatusProperties) CreateEmptyArmValue() interface{} {
+	return ResourceGroupStatusPropertiesArm{}
+}
+
 // ConvertToArm converts from a Kubernetes CRD object to an ARM object
 func (p *ResourceGroupStatusProperties) ConvertToArm(name string) (interface{}, error) {
 	if p == nil {
@@ -151,6 +159,10 @@ type ResourceGroupSpec struct {
 
 var _ genruntime.ArmTransformer = &ResourceGroupSpec{}
 
+func (spec *ResourceGroupSpec) CreateEmptyArmValue() interface{} {
+	return ResourceGroupSpecArm{}
+}
+
 // ConvertToArm converts from a Kubernetes CRD object to an ARM object
 func (spec *ResourceGroupSpec) ConvertToArm(name string) (interface{}, error) {
 	if spec == nil {

hack/generated/controllers/generic_controller.go

@@ -424,7 +424,7 @@ func (gr *GenericReconciler) MonitorDeployment(ctx context.Context, action Recon
 		return ctrl.Result{}, err
 	}
 
-	var status genruntime.ArmTransformer
+	var status genruntime.FromArmConverter
 	err = gr.Patch(ctx, data, func(ctx context.Context, mutData *ReconcileMetadata) error {
 
 		deployment, err = gr.ARMClient.GetDeployment(ctx, deployment.Id)
@@ -560,7 +560,7 @@ func (gr *GenericReconciler) constructArmResource(ctx context.Context, data *Rec
 	return resource, nil
 }
 
-func (gr *GenericReconciler) getStatus(ctx context.Context, id string, data *ReconcileMetadata) (genruntime.ArmTransformer, error) {
+func (gr *GenericReconciler) getStatus(ctx context.Context, id string, data *ReconcileMetadata) (genruntime.FromArmConverter, error) {
 	deployableSpec, err := reflecthelpers.ConvertResourceToDeployableResource(ctx, gr.ResourceResolver, data.metaObj)
 	if err != nil {
 		return nil, err

hack/generated/controllers/reconcile_metadata.go

@@ -180,7 +180,7 @@ func (r *ReconcileMetadata) SpecSignature() (string, error) {
 
 func (r *ReconcileMetadata) Update(
 	deployment *armclient.Deployment,
-	status genruntime.ArmTransformer) error {
+	status genruntime.FromArmConverter) error {
 
 	controllerutil.AddFinalizer(r.metaObj, GenericControllerFinalizer)
 

hack/generated/pkg/genruntime/arm_transformer.go

@@ -9,15 +9,23 @@ import (
 	"strings"
 )
 
-// TODO: Consider ArmSpecTransformer and ArmTransformer, so we don't have to pass owningName/name through all the calls
+type ToArmConverter interface {
+	// name is the "a/b/c" name for the owner. For example this would be "VNet1" when deploying subnet1
+	// or myaccount when creating Batch pool1
+	ConvertToArm(name string) (interface{}, error)
+}
 
+type FromArmConverter interface {
+	CreateEmptyArmValue() interface{}
+	PopulateFromArm(owner KnownResourceReference, input interface{}) error
+}
+
+// TODO: Consider ArmSpecTransformer and ArmTransformer, so we don't have to pass owningName/name through all the calls
 // ArmTransformer is a type which can be converted to/from an Arm object shape.
 // Each CRD resource must implement these methods.
 type ArmTransformer interface {
-	// owningName is the "a/b/c" name for the owner. For example this would be "VNet1" when deploying subnet1
-	// or myaccount when creating Batch pool1
-	ConvertToArm(name string) (interface{}, error)
-	PopulateFromArm(owner KnownResourceReference, input interface{}) error
+	ToArmConverter
+	FromArmConverter
 }
 
 // CombineArmNames creates a "fully qualified" resource name for use

hack/generated/pkg/reflecthelpers/reflect_helpers.go

@@ -87,11 +87,7 @@ func NewEmptyArmResourceStatus(metaObject genruntime.MetaObject) (genruntime.Arm
 	}
 
 	// TODO: Do we actually want to return a ptr here, not a value?
-	// No need to actually pass name here (we're going to populate this entity from ARM anyway)
-	armStatus, err := kubeStatus.ConvertToArm("")
-	if err != nil {
-		return nil, err
-	}
+	armStatus := kubeStatus.CreateEmptyArmValue()
 
 	// TODO: Some reflect hackery here to make sure that this is a ptr not a value
 	armStatusPtr := NewPtrFromValue(armStatus)
@@ -104,9 +100,9 @@ func NewEmptyArmResourceStatus(metaObject genruntime.MetaObject) (genruntime.Arm
 	return castArmStatus, nil
 }
 
-// NewEmptyStatus creates a new empty Status object (which implements ArmTransformer) from
+// NewEmptyStatus creates a new empty Status object (which implements FromArmConverter) from
 // a genruntime.MetaObject.
-func NewEmptyStatus(metaObject genruntime.MetaObject) (genruntime.ArmTransformer, error) {
+func NewEmptyStatus(metaObject genruntime.MetaObject) (genruntime.FromArmConverter, error) {
 	t := reflect.TypeOf(metaObject).Elem()
 
 	statusField, ok := t.FieldByName("Status")
@@ -115,7 +111,7 @@ func NewEmptyStatus(metaObject genruntime.MetaObject) (genruntime.ArmTransformer
 	}
 
 	statusPtr := reflect.New(statusField.Type)
-	status, ok := statusPtr.Interface().(genruntime.ArmTransformer)
+	status, ok := statusPtr.Interface().(genruntime.FromArmConverter)
 	if !ok {
 		return nil, errors.Errorf("status did not implement genruntime.ArmTransformer")
 	}

hack/generator/pkg/astbuilder/builder.go

@@ -35,31 +35,55 @@ func CheckErrorAndReturn(otherReturns ...ast.Expr) ast.Stmt {
 	}
 }
 
-// NewQualifiedStruct creates a new assignment statement where a struct is constructed and stored in a variable of the given name.
+// NewVariableQualified creates a new declaration statement where a variable is declared
+// with its default value.
+//
 // For example:
+//     var <varName> <packageRef>.<structName>
+//
+// Note that it does *not* do:
 //     <varName> := <packageRef>.<structName>{}
-func NewQualifiedStruct(varName *ast.Ident, qualifier *ast.Ident, structName *ast.Ident) ast.Stmt {
-	return SimpleAssignment(
-		varName,
-		token.DEFINE,
-		&ast.CompositeLit{
-			Type: &ast.SelectorExpr{
-				X:   qualifier,
-				Sel: structName,
+//
+// …as that does not work for enum types.
+func NewVariableQualified(varName *ast.Ident, qualifier *ast.Ident, structName *ast.Ident) ast.Stmt {
+	return &ast.DeclStmt{
+		Decl: &ast.GenDecl{
+			Tok: token.VAR,
+			Specs: []ast.Spec{
+				&ast.TypeSpec{
+					Name: varName,
+					Type: &ast.SelectorExpr{
+						X:   qualifier,
+						Sel: structName,
+					},
+				},
 			},
-		})
+		},
+	}
 }
 
-// NewStruct creates a new assignment statement where a struct is constructed and stored in a variable of the given name.
+// NewVariable creates a new declaration statement where a variable is declared
+// with its default value.
+//
 // For example:
+//     var <varName> <structName>
+//
+// Note that it does *not* do:
 //     <varName> := <structName>{}
-func NewStruct(varName *ast.Ident, structName *ast.Ident) ast.Stmt {
-	return SimpleAssignment(
-		varName,
-		token.DEFINE,
-		&ast.CompositeLit{
-			Type: structName,
-		})
+//
+// …as that does not work for enum types.
+func NewVariable(varName *ast.Ident, structName *ast.Ident) ast.Stmt {
+	return &ast.DeclStmt{
+		Decl: &ast.GenDecl{
+			Tok: token.VAR,
+			Specs: []ast.Spec{
+				&ast.TypeSpec{
+					Name: varName,
+					Type: structName,
+				},
+			},
+		},
+	}
 }
 
 // LocalVariableDeclaration performs a local variable declaration for use within a method like:

hack/generator/pkg/astbuilder/calls.go

@@ -10,7 +10,7 @@ import "go/ast"
 // CallFunc() creates an expression to call a function with specified arguments, generating code
 // like:
 // <funcName>(<arguments>...)
-func CallFunc(funcName *ast.Ident, arguments ...ast.Expr) ast.Expr {
+func CallFunc(funcName ast.Expr, arguments ...ast.Expr) ast.Expr {
 	return &ast.CallExpr{
 		Fun:  funcName,
 		Args: arguments,

hack/generator/pkg/astmodel/armconversion/arm_conversion_function.go

@@ -7,8 +7,9 @@ package armconversion
 
 import (
 	"fmt"
-	"github.com/Azure/k8s-infra/hack/generator/pkg/astmodel"
 	"go/ast"
+
+	"github.com/Azure/k8s-infra/hack/generator/pkg/astmodel"
 )
 
 type ConversionDirection string
@@ -26,7 +27,7 @@ type ArmConversionFunction struct {
 	armType     *astmodel.ObjectType
 	idFactory   astmodel.IdentifierFactory
 	direction   ConversionDirection
-	isResource  bool
+	isSpecType  bool
 }
 
 var _ astmodel.Function = &ArmConversionFunction{}

hack/generator/pkg/astmodel/armconversion/convert_from_arm_function_builder.go

@@ -41,7 +41,7 @@ func newConvertFromArmFunctionBuilder(
 			receiverTypeExpr:      receiver.AsType(codeGenerationContext),
 			armTypeIdent:          ast.NewIdent(c.armTypeName.Name()),
 			idFactory:             c.idFactory,
-			isResource:            c.isResource,
+			isSpecType:            c.isSpecType,
 			codeGenerationContext: codeGenerationContext,
 		},
 	}
@@ -134,15 +134,52 @@ func (builder *convertFromArmBuilder) namePropertyHandler(
 	toProp *astmodel.PropertyDefinition,
 	fromType *astmodel.ObjectType) []ast.Stmt {
 
-	if !toProp.Equals(GetAzureNameProperty(builder.idFactory)) || !builder.isResource {
+	if !builder.isSpecType || !toProp.HasName(astmodel.AzureNameProperty) {
+		return nil
+	}
+
+	if typeName, ok := toProp.PropertyType().(astmodel.TypeName); ok {
+		// we are assigning to (presumably) an enum-typed AzureName property, (no way to check that here)
+		// we will cast the result of ExtractKubernetesResourceNameFromArmName
+		// to the target type:
+
+		// Check to make sure that the ARM object has a "Name" property (which matches our "AzureName")
+		fromProp, ok := fromType.Property("Name")
+		if !ok {
+			panic("ARM resource missing property 'Name'")
+		}
+
+		result := astbuilder.SimpleAssignment(
+			&ast.SelectorExpr{
+				X:   builder.receiverIdent,
+				Sel: ast.NewIdent(string(toProp.PropertyName())),
+			},
+			token.ASSIGN,
+			astbuilder.CallFunc(
+				// "calling" enum name is equivalent to casting
+				ast.NewIdent(typeName.Name()),
+				astbuilder.CallQualifiedFuncByName(
+					astmodel.GenRuntimePackageName,
+					"ExtractKubernetesResourceNameFromArmName",
+					&ast.SelectorExpr{
+						X:   builder.typedInputIdent,
+						Sel: ast.NewIdent(string(fromProp.PropertyName())),
+					})))
+
+		return []ast.Stmt{result}
+	}
+
+	// otherwise check if we are writing to a string-typed AzureName property
+	if !toProp.Equals(GetAzureNameProperty(builder.idFactory)) {
 		return nil
 	}
 
 	// Check to make sure that the ARM object has a "Name" property (which matches our "AzureName")
 	fromProp, ok := fromType.Property("Name")
 	if !ok {
-		panic("Arm resource missing property 'Name'")
+		panic("ARM resource missing property 'Name'")
 	}
+
 	result := astbuilder.SimpleAssignment(
 		&ast.SelectorExpr{
 			X:   builder.receiverIdent,
@@ -158,14 +195,13 @@ func (builder *convertFromArmBuilder) namePropertyHandler(
 			}))
 
 	return []ast.Stmt{result}
-
 }
 
 func (builder *convertFromArmBuilder) ownerPropertyHandler(
 	toProp *astmodel.PropertyDefinition,
 	_ *astmodel.ObjectType) []ast.Stmt {
 
-	if toProp.PropertyName() != builder.idFactory.CreatePropertyName(astmodel.OwnerProperty, astmodel.Exported) || !builder.isResource {
+	if toProp.PropertyName() != builder.idFactory.CreatePropertyName(astmodel.OwnerProperty, astmodel.Exported) || !builder.isSpecType {
 		return nil
 	}
 
@@ -525,21 +561,21 @@ func (builder *convertFromArmBuilder) convertComplexTypeNameProperty(
 
 	ownerName := builder.idFactory.CreateIdentifier(astmodel.OwnerProperty, astmodel.NotExported)
 
-	newStruct := astbuilder.NewStruct(propertyLocalVar, ast.NewIdent(destinationType.Name()))
+	newVariable := astbuilder.NewVariable(propertyLocalVar, ast.NewIdent(destinationType.Name()))
 	if !destinationType.PackageReference.Equals(builder.codeGenerationContext.CurrentPackage()) {
 		// struct name has to be qualified
 		packageName, err := builder.codeGenerationContext.GetImportedPackageName(destinationType.PackageReference)
 		if err != nil {
 			panic(err)
 		}
 
-		newStruct = astbuilder.NewQualifiedStruct(
+		newVariable = astbuilder.NewVariableQualified(
 			propertyLocalVar,
 			ast.NewIdent(packageName),
 			ast.NewIdent(destinationType.Name()))
 	}
 
-	results = append(results, newStruct)
+	results = append(results, newVariable)
 	results = append(
 		results,
 		astbuilder.SimpleAssignment(