Return error if reference-like property is not labelled (#1629)

This reduces the probability of us accidentally missing references,
which it turns out we had already done because as soon as this
was turned on, a number of errors were returned due to references
we missed.

hack/generated/controllers/crd_vmss_test.go

@@ -113,6 +113,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{
@@ -162,12 +167,11 @@ func newVMSS(
 										Name: "myipconfiguration",
 										Properties: &compute.VirtualMachineScaleSetIPConfigurationProperties{
 											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/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"
@@ -30,15 +31,23 @@ func AddCrossResourceReferences(configuration *config.Configuration, idFactory a
 
 			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", typeName, prop.PropertyName()))
 				}
 
 				return isReference
@@ -64,6 +73,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 +89,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,
 	}
 
@@ -166,31 +180,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"), "VirtualMachineScaleSetIPConfiguration"),
+			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"), "VirtualMachineScaleSetNetworkConfiguration"),
+			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,
 	}
 }