diff --git a/hack/generated/controllers/crd_vmss_test.go b/hack/generated/controllers/crd_vmss_test.go index a56ab2ab705..e85b4b184f3 100644 --- a/hack/generated/controllers/crd_vmss_test.go +++ b/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, }, }, }, diff --git a/hack/generated/pkg/testcommon/kube_per_test_context.go b/hack/generated/pkg/testcommon/kube_per_test_context.go index 5ec08325a80..7cf7126fe49 100644 --- a/hack/generated/pkg/testcommon/kube_per_test_context.go +++ b/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"), diff --git a/hack/generator/pkg/codegen/pipeline/add_cross_resource_references.go b/hack/generator/pkg/codegen/pipeline/add_cross_resource_references.go index 98c41533608..93e10963a31 100644 --- a/hack/generator/pkg/codegen/pipeline/add_cross_resource_references.go +++ b/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, } }