Merge branch 'master' into more-flattening

hack/generated/controllers/crd_vmss_test.go

@@ -111,6 +111,11 @@ func newVMSS(
 	upgradePolicyMode := compute.UpgradePolicyModeAutomatic
 	adminUsername := "adminUser"
 
+	inboundNATPoolRef := genruntime.ResourceReference{
+		// TODO: It is the most awkward thing in the world that this is not a fully fledged resource
+		ARMID: *loadBalancer.Status.InboundNatPools[0].Id,
+	}
+
 	return &compute.VirtualMachineScaleSet{
 		ObjectMeta: tc.MakeObjectMetaWithName(tc.Namer.GenerateName("vmss")),
 		Spec: compute.VirtualMachineScaleSets_Spec{
@@ -158,12 +163,11 @@ func newVMSS(
 								{
 									Name: "myipconfiguration",
 									Subnet: &compute.ApiEntityReference{
-										Id: subnet.Status.Id,
+										Reference: tc.MakeReferencePtrFromResource(subnet),
 									},
 									LoadBalancerInboundNatPools: []compute.SubResource{
 										{
-											// TODO: It is the most awkward thing in the world that this is not a fully fledged resource
-											Id: loadBalancer.Status.InboundNatPools[0].Id,
+											Reference: &inboundNATPoolRef,
 										},
 									},
 								},

hack/generated/pkg/testcommon/kube_per_test_context.go

@@ -89,6 +89,11 @@ func (tc KubePerTestContext) MakeReferenceFromResource(resource controllerutil.O
 	}
 }
 
+func (tc KubePerTestContext) MakeReferencePtrFromResource(resource controllerutil.Object) *genruntime.ResourceReference {
+	result := tc.MakeReferenceFromResource(resource)
+	return &result
+}
+
 func (tc KubePerTestContext) NewTestResourceGroup() *resources.ResourceGroup {
 	return &resources.ResourceGroup{
 		ObjectMeta: tc.MakeObjectMeta("rg"),

hack/generator/pkg/astbuilder/builder.go

@@ -25,6 +25,21 @@ func CheckErrorAndReturn(otherReturns ...dst.Expr) dst.Stmt {
 	return CheckErrorAndSingleStatement(retStmt)
 }
 
+// CheckErrorAndWrap checks if the err is non-nil, and if it is returns it, wrapped with additional information
+//
+// if err != nil {
+//      return errors.Wrapf(err, <message>, <args>)
+// }
+//
+func CheckErrorAndWrap(errorsPackage string, message string, args ...dst.Expr) dst.Stmt {
+	wrap := CallQualifiedFunc(
+		errorsPackage,
+		"Wrap",
+		Expressions(dst.NewIdent("err"), StringLiteral(message), args)...)
+
+	return CheckErrorAndSingleStatement(Returns(wrap))
+}
+
 // CheckErrorAndSingleStatement checks if the err is non-nil, and if it is executes the provided statement.
 //
 // 	if err != nil {

hack/generator/pkg/codegen/pipeline/add_cross_resource_references.go

@@ -11,6 +11,7 @@ import (
 	"strings"
 
 	"github.com/pkg/errors"
+	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/klog/v2"
 
 	"github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel"
@@ -27,18 +28,25 @@ func AddCrossResourceReferences(configuration *config.Configuration, idFactory a
 		"addCrossResourceReferences",
 		"Replace cross-resource references with genruntime.ResourceReference",
 		func(ctx context.Context, definitions astmodel.Types) (astmodel.Types, error) {
-
 			result := make(astmodel.Types)
 			knownReferences := newKnownReferencesMap(configuration)
+
+			var isCrossResourceReferenceErrs []error
+
 			isCrossResourceReference := func(typeName astmodel.TypeName, prop *astmodel.PropertyDefinition) bool {
 				ref := referencePair{
 					typeName: typeName,
 					propName: prop.PropertyName(),
 				}
-				_, isReference := knownReferences[ref]
-
-				if DoesPropertyLookLikeARMReference(prop) && !isReference {
-					klog.V(0).Infof("\"%s.%s\" looks like a resource reference but was not labelled as one", typeName, prop.PropertyName())
+				isReference, found := knownReferences[ref]
+
+				if DoesPropertyLookLikeARMReference(prop) && !found {
+					// This is an error for now to ensure that we don't accidentally miss adding references.
+					// If/when we move to using an upstream marker for cross resource refs, we can remove this and just
+					// trust the Swagger.
+					isCrossResourceReferenceErrs = append(
+						isCrossResourceReferenceErrs,
+						errors.Errorf("\"%s.%s\" looks like a resource reference but was not labelled as one. It might need to be manually added to `newKnownReferencesMap`", typeName, prop.PropertyName()))
 				}
 
 				return isReference
@@ -64,6 +72,11 @@ func AddCrossResourceReferences(configuration *config.Configuration, idFactory a
 				// TODO: Properties collapsing work for this.
 			}
 
+			err := kerrors.NewAggregate(isCrossResourceReferenceErrs)
+			if err != nil {
+				return nil, err
+			}
+
 			return result, nil
 		})
 }
@@ -75,15 +88,15 @@ type referencePair struct {
 
 type crossResourceReferenceChecker func(typeName astmodel.TypeName, prop *astmodel.PropertyDefinition) bool
 
-type crossResourceReferenceTypeVisitor struct {
+type CrossResourceReferenceTypeVisitor struct {
 	astmodel.TypeVisitor
 	// referenceChecker is a function describing what a cross resource reference looks like. It is overridable so that
 	// we can use a more simplistic criteria for tests.
 	isPropertyAnARMReference crossResourceReferenceChecker
 }
 
-func MakeCrossResourceReferenceTypeVisitor(idFactory astmodel.IdentifierFactory, referenceChecker crossResourceReferenceChecker) crossResourceReferenceTypeVisitor {
-	visitor := crossResourceReferenceTypeVisitor{
+func MakeCrossResourceReferenceTypeVisitor(idFactory astmodel.IdentifierFactory, referenceChecker crossResourceReferenceChecker) CrossResourceReferenceTypeVisitor {
+	visitor := CrossResourceReferenceTypeVisitor{
 		isPropertyAnARMReference: referenceChecker,
 	}
 
@@ -137,7 +150,6 @@ func DoesPropertyLookLikeARMReference(prop *astmodel.PropertyDefinition) bool {
 }
 
 func makeResourceReferenceProperty(idFactory astmodel.IdentifierFactory, existing *astmodel.PropertyDefinition) *astmodel.PropertyDefinition {
-
 	var referencePropertyName string
 	// Special case for "Id" and properties that end in "Id", which are quite common in the specs. This is primarily
 	// because it's awkward to have a field called "Id" not just be a string and instead but a complex type describing
@@ -166,31 +178,73 @@ func makeResourceReferenceProperty(idFactory astmodel.IdentifierFactory, existin
 }
 
 // TODO: This will go away in favor of a cleaner solution in the future, as obviously this isn't great
-func newKnownReferencesMap(configuration *config.Configuration) map[referencePair]struct{} {
-	return map[referencePair]struct{}{
+// Set the value to false to eliminate a reference which has incorrectly been flagged
+func newKnownReferencesMap(configuration *config.Configuration) map[referencePair]bool {
+	return map[referencePair]bool{
+		// Batch
 		{
 			typeName: astmodel.MakeTypeName(configuration.MakeLocalPackageReference("microsoft.batch", "v1alpha1api20210101"), "KeyVaultReference"),
 			propName: "Id",
-		}: {},
+		}: true,
+		// Document DB
 		{
 			typeName: astmodel.MakeTypeName(configuration.MakeLocalPackageReference("microsoft.documentdb", "v1alpha1api20210515"), "VirtualNetworkRule"),
 			propName: "Id",
-		}: {},
+		}: true,
+		// Storage
 		{
 			typeName: astmodel.MakeTypeName(configuration.MakeLocalPackageReference("microsoft.storage", "v1alpha1api20210401"), "VirtualNetworkRule"),
 			propName: "Id",
-		}: {},
+		}: true,
+		// Service bus
+		{
+			typeName: astmodel.MakeTypeName(configuration.MakeLocalPackageReference("microsoft.servicebus", "v1alpha1api20210101preview"), "PrivateEndpoint"),
+			propName: "Id",
+		}: true,
+		// Network
 		{
 			typeName: astmodel.MakeTypeName(configuration.MakeLocalPackageReference("microsoft.network", "v1alpha1api20201101"), "SubResource"),
 			propName: "Id",
-		}: {},
+		}: true,
+		// Compute
 		{
 			typeName: astmodel.MakeTypeName(configuration.MakeLocalPackageReference("microsoft.compute", "v1alpha1api20200930"), "SourceVault"),
 			propName: "Id",
-		}: {},
+		}: true,
 		{
 			typeName: astmodel.MakeTypeName(configuration.MakeLocalPackageReference("microsoft.compute", "v1alpha1api20200930"), "ImageDiskReference"),
 			propName: "Id",
-		}: {},
+		}: true,
+		{
+			typeName: astmodel.MakeTypeName(configuration.MakeLocalPackageReference("microsoft.compute", "v1alpha1api20201201"), "DiskEncryptionSetParameters"),
+			propName: "Id",
+		}: true,
+		// This is an Id that I believe refers to itself.
+		// It's never supplied in a PUT I don't think, and is only returned in a GET because the
+		// IPConfiguration is actually an ARM resource that can only be created by issuing a PUT VMSS.
+		{
+			typeName: astmodel.MakeTypeName(configuration.MakeLocalPackageReference("microsoft.compute", "v1alpha1api20201201"), "VirtualMachineScaleSets_Spec_Properties_VirtualMachineProfile_NetworkProfile_NetworkInterfaceConfigurations_Properties_IpConfigurations"),
+			propName: "Id",
+		}: false,
+		{
+			typeName: astmodel.MakeTypeName(configuration.MakeLocalPackageReference("microsoft.compute", "v1alpha1api20201201"), "ApiEntityReference"),
+			propName: "Id",
+		}: true,
+		{
+			typeName: astmodel.MakeTypeName(configuration.MakeLocalPackageReference("microsoft.compute", "v1alpha1api20201201"), "ImageReference"),
+			propName: "Id",
+		}: true,
+		// This is an Id that I believe refers to itself.
+		// It's never supplied in a PUT I don't think, and is only returned in a GET because the
+		// IPConfiguration is actually an ARM resource that can only be created by issuing a PUT VMSS.
+		{
+			typeName: astmodel.MakeTypeName(configuration.MakeLocalPackageReference("microsoft.compute", "v1alpha1api20201201"), "VirtualMachineScaleSets_Spec_Properties_VirtualMachineProfile_NetworkProfile_NetworkInterfaceConfigurations"),
+			propName: "Id",
+		}: false,
+		// When SubResource is used directly in a property, it's meant as a reference. When it's inherited from, the Id is for self
+		{
+			typeName: astmodel.MakeTypeName(configuration.MakeLocalPackageReference("microsoft.compute", "v1alpha1api20201201"), "SubResource"),
+			propName: "Id",
+		}: true,
 	}
 }

hack/generator/pkg/codegen/pipeline/inject_property_assignment_functions.go

@@ -14,6 +14,7 @@ import (
 	"github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel"
 	"github.com/Azure/azure-service-operator/hack/generator/pkg/codegen/storage"
 	"github.com/Azure/azure-service-operator/hack/generator/pkg/conversions"
+	"github.com/Azure/azure-service-operator/hack/generator/pkg/functions"
 )
 
 // injectPropertyAssignmentFunctionsStageId is the unique identifier for this pipeline stage
@@ -102,13 +103,13 @@ func (f propertyAssignmentFunctionsFactory) injectBetween(
 	// Create conversion functions
 	conversionContext := conversions.NewPropertyConversionContext(f.types, f.idFactory)
 
-	assignFromFn, err := conversions.NewPropertyAssignmentFromFunction(upstreamDef, downstreamDef, f.idFactory, conversionContext)
+	assignFromFn, err := functions.NewPropertyAssignmentFromFunction(upstreamDef, downstreamDef, f.idFactory, conversionContext)
 	upstreamName := upstreamDef.Name()
 	if err != nil {
 		return astmodel.TypeDefinition{}, errors.Wrapf(err, "creating AssignFrom() function for %q", upstreamName)
 	}
 
-	assignToFn, err := conversions.NewPropertyAssignmentToFunction(upstreamDef, downstreamDef, f.idFactory, conversionContext)
+	assignToFn, err := functions.NewPropertyAssignmentToFunction(upstreamDef, downstreamDef, f.idFactory, conversionContext)
 	if err != nil {
 		return astmodel.TypeDefinition{}, errors.Wrapf(err, "creating AssignTo() function for %q", upstreamName)
 	}

hack/generator/pkg/conversions/direction.go

@@ -5,12 +5,76 @@
 
 package conversions
 
+import (
+	"github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel"
+)
+
 // Direction specifies the direction of conversion we're implementing with this function
-type Direction int
+type Direction interface {
+	// SelectString returns one of the provided strings, depending on the direction of conversion
+	SelectString(from string, to string) string
+	// SelectType returns one of the provided types, depending on the direction of conversion
+	SelectType(from astmodel.Type, to astmodel.Type) astmodel.Type
+	// WhenFrom will run the specified function only if the direction is "From", returning the current direction for chaining
+	WhenFrom(fn func()) Direction
+	// WhenTo will run the specified function only if the direction is "To", returning the current direction for chaining
+	WhenTo(fn func()) Direction
+}
 
-const (
+var (
 	// ConvertFrom indicates the conversion is from the passed 'other', populating the receiver with properties from the other
-	ConvertFrom = Direction(1)
+	ConvertFrom Direction = &ConvertFromDirection{}
 	// ConvertTo indicates the conversion is to the passed 'other', populating the other with properties from the receiver
-	ConvertTo = Direction(2)
+	ConvertTo Direction = &ConvertToDirection{}
 )
+
+type ConvertFromDirection struct{}
+
+var _ Direction = &ConvertFromDirection{}
+
+// SelectString returns the string for conversion FROM
+func (dir *ConvertFromDirection) SelectString(fromString string, _ string) string {
+	return fromString
+}
+
+// SelectType returns the type for conversion FROM
+func (dir *ConvertFromDirection) SelectType(fromType astmodel.Type, _ astmodel.Type) astmodel.Type {
+	return fromType
+}
+
+// WhenFrom will run the supplied function, returning this FROM direction for chaining
+func (dir *ConvertFromDirection) WhenFrom(fn func()) Direction {
+	fn()
+	return dir
+}
+
+// WhenTo will skip the supplied function, returning this FROM direction for chaining
+func (dir *ConvertFromDirection) WhenTo(_ func()) Direction {
+	// Nothing
+	return dir
+}
+
+type ConvertToDirection struct{}
+
+var _ Direction = &ConvertToDirection{}
+
+// SelectString returns the string for conversion TO
+func (dir *ConvertToDirection) SelectString(_ string, toValue string) string {
+	return toValue
+}
+
+// SelectType returns the type for conversion TO
+func (dir *ConvertToDirection) SelectType(_ astmodel.Type, toType astmodel.Type) astmodel.Type {
+	return toType
+}
+
+// WhenFrom will skip the supplied function, returning this TO direction for chaining
+func (dir *ConvertToDirection) WhenFrom(_ func()) Direction {
+	return dir
+}
+
+// WhenTo will run the supplied function, returning this TO direction for chaining
+func (dir *ConvertToDirection) WhenTo(fn func()) Direction {
+	fn()
+	return dir
+}

hack/generator/pkg/conversions/property_conversions.go

@@ -145,6 +145,13 @@ func CreateTypeConversion(
 	return nil, err
 }
 
+func NameOfPropertyAssignmentFunction(name astmodel.TypeName, direction Direction, idFactory astmodel.IdentifierFactory) string {
+	nameOfOtherType := idFactory.CreateIdentifier(name.Name(), astmodel.Exported)
+	return direction.SelectString(
+		"AssignPropertiesFrom"+nameOfOtherType,
+		"AssignPropertiesTo"+nameOfOtherType)
+}
+
 // assignToOptional will generate a conversion where the destination is optional, if the
 // underlying type of the destination is compatible with the source.
 //
@@ -853,7 +860,7 @@ func assignObjectFromObject(
 			actualReader = astbuilder.AddrOf(reader)
 		}
 
-		functionName := nameOfPropertyAssignmentFunction(sourceName, conversionContext.direction, conversionContext.idFactory)
+		functionName := NameOfPropertyAssignmentFunction(sourceName, conversionContext.direction, conversionContext.idFactory)
 
 		var conversion dst.Stmt
 		if destinationName.PackageReference.Equals(generationContext.CurrentPackage()) {