diff --git a/hack/crossplane/apis/sql/v1api20201101preview/servers_database_types_gen.go b/hack/crossplane/apis/sql/v1api20201101preview/servers_database_types_gen.go index ebc38236bd1..86439614635 100644 --- a/hack/crossplane/apis/sql/v1api20201101preview/servers_database_types_gen.go +++ b/hack/crossplane/apis/sql/v1api20201101preview/servers_database_types_gen.go @@ -15,7 +15,7 @@ import ( // +kubebuilder:subresource:status // +kubebuilder:storageversion // Generator information: -// - Generated from: /sql/resource-manager/Microsoft.Sql/preview/2020-11-01-preview/Databases_legacy.json +// - Generated from: /sql/resource-manager/Microsoft.Sql/preview/2020-11-01-preview/Databases.json // - ARM URI: /subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Sql/servers/{serverName}/databases/{databaseName} type Servers_Database struct { metav1.TypeMeta `json:",inline"` @@ -26,7 +26,7 @@ type Servers_Database struct { // +kubebuilder:object:root=true // Generator information: -// - Generated from: /sql/resource-manager/Microsoft.Sql/preview/2020-11-01-preview/Databases_legacy.json +// - Generated from: /sql/resource-manager/Microsoft.Sql/preview/2020-11-01-preview/Databases.json // - ARM URI: /subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Sql/servers/{serverName}/databases/{databaseName} type Servers_DatabaseList struct { metav1.TypeMeta `json:",inline"` diff --git a/v2/azure-arm.yaml b/v2/azure-arm.yaml index 957bec6506e..0b6163b076e 100644 --- a/v2/azure-arm.yaml +++ b/v2/azure-arm.yaml @@ -129,6 +129,22 @@ typeFilters: group: costmanagement because: QueryFilter is a self-referential, infinitely recursive type. We can't easily unroll it and controller-gen doesn't support recursive types +# +# When loading resources, it's possible for us to end up with a name collision if two resources +# specify similar URLs (we derive the names from the structure of the ARM URL specified for the +# resource). This is an extremely rare event. +# Because this happens so early in the operation of our pipeline, our existing mechanisms for +# disambiguation and renaming can't be applied, so we special case them here. +# +# This is configuration of last resort; in most cases, you should use the objectModelConfiguration +# to rename things as it is much more flexible. +# +typeLoaderRenames: + - group: insights + name: DiagnosticSetting + scope: Location + renameTo: SubscriptionDiagnosticSetting + # Exclusions for packages that currently produce types including AnyType. # TODO: get rid of these, either by chasing the teams generating # weird json or by handling them differently in the generator. diff --git a/v2/tools/generator/internal/codegen/pipeline/load_types.go b/v2/tools/generator/internal/codegen/pipeline/load_types.go index 0c4c421ec10..1c9cf694b7e 100644 --- a/v2/tools/generator/internal/codegen/pipeline/load_types.go +++ b/v2/tools/generator/internal/codegen/pipeline/load_types.go @@ -11,7 +11,7 @@ import ( "os" "path/filepath" "regexp" - "sort" + "slices" "strings" "sync" "time" @@ -91,8 +91,7 @@ func LoadTypes( // add on ARM Type, URI, and supported operations resourceType = resourceType.WithARMType(resourceInfo.ARMType).WithARMURI(resourceInfo.ARMURI) - scope := categorizeResourceScope(resourceInfo.ARMURI) - resourceType = resourceType.WithScope(scope) + resourceType = resourceType.WithScope(resourceInfo.Scope) resourceType = resourceType.WithSupportedOperations(resourceInfo.SupportedOperations) resourceDefinition := astmodel.MakeTypeDefinition(resourceName, resourceType) @@ -125,31 +124,6 @@ func LoadTypes( }) } -var ( - resourceGroupScopeRegex = regexp.MustCompile(`(?i)^/subscriptions/[^/]+/resourcegroups/[^/]+/.*`) - locationScopeRegex = regexp.MustCompile(`(?i)^/subscriptions/[^/]+/.*`) -) - -func categorizeResourceScope(armURI string) astmodel.ResourceScope { - // this is a bit of a hack, eventually we should have better scope support. - // at the moment we assume that a resource is an extension if it can be applied to - // any scope or if armURI contains more than 1 provider: - if strings.HasPrefix(armURI, "/{scope}/") || strings.Count(armURI, "providers") > 1 { - return astmodel.ResourceScopeExtension - } - - if resourceGroupScopeRegex.MatchString(armURI) { - return astmodel.ResourceScopeResourceGroup - } - - if locationScopeRegex.MatchString(armURI) { - return astmodel.ResourceScopeLocation - } - - // TODO: Not currently possible to generate a resource with scope Location, we should fix that - return astmodel.ResourceScopeTenant -} - var requiredSpecFields = astmodel.NewObjectType().WithProperties( astmodel.NewPropertyDefinition(astmodel.NameProperty, "name", astmodel.StringType)) @@ -356,7 +330,7 @@ func loadSwaggerData( "loaded", countLoaded, "total", len(schemas)) - return mergeSwaggerTypesByGroup(idFactory, typesByGroup) + return mergeSwaggerTypesByGroup(idFactory, typesByGroup, config) } var ( @@ -376,14 +350,22 @@ func logInfoSparse(log logr.Logger, message string, keysAndValues ...interface{} } } -func mergeSwaggerTypesByGroup(idFactory astmodel.IdentifierFactory, m map[astmodel.LocalPackageReference][]typesFromFile) (jsonast.SwaggerTypes, error) { +func mergeSwaggerTypesByGroup( + idFactory astmodel.IdentifierFactory, + m map[astmodel.LocalPackageReference][]typesFromFile, + cfg *config.Configuration, +) (jsonast.SwaggerTypes, error) { result := jsonast.SwaggerTypes{ ResourceDefinitions: make(jsonast.ResourceDefinitionSet), OtherDefinitions: make(astmodel.TypeDefinitionSet), } for pkg, group := range m { - merged := mergeTypesForPackage(idFactory, group) + merged, err := mergeTypesForPackage(group, idFactory, cfg) + if err != nil { + return result, errors.Wrapf(err, "merging swagger types for %s", pkg) + } + for rn, rt := range merged.ResourceDefinitions { if _, ok := result.ResourceDefinitions[rn]; ok { panic("duplicate resource generated") @@ -392,7 +374,7 @@ func mergeSwaggerTypesByGroup(idFactory astmodel.IdentifierFactory, m map[astmod result.ResourceDefinitions[rn] = rt } - err := result.OtherDefinitions.AddTypesAllowDuplicates(merged.OtherDefinitions) + err = result.OtherDefinitions.AddTypesAllowDuplicates(merged.OtherDefinitions) if err != nil { return result, errors.Wrapf(err, "when combining swagger types for %s", pkg) } @@ -406,17 +388,20 @@ type typesFromFile struct { filePath string } -type typesFromFilesSorter struct{ x []typesFromFile } - -var _ sort.Interface = typesFromFilesSorter{} - -func (s typesFromFilesSorter) Len() int { return len(s.x) } -func (s typesFromFilesSorter) Swap(i, j int) { s.x[i], s.x[j] = s.x[j], s.x[i] } -func (s typesFromFilesSorter) Less(i, j int) bool { return s.x[i].filePath < s.x[j].filePath } - -// mergeTypesForPackage merges the types for a single package from multiple files -func mergeTypesForPackage(idFactory astmodel.IdentifierFactory, typesFromFiles []typesFromFile) jsonast.SwaggerTypes { - sort.Sort(typesFromFilesSorter{typesFromFiles}) +// mergeTypesForPackage merges the types for a single package from multiple files into our master set. +// typesFromFiles is a slice of typesFromFile, each of which contains the types for a single file. +// idFactory is used to generate new identifiers for types that collide. +func mergeTypesForPackage( + typesFromFiles []typesFromFile, + idFactory astmodel.IdentifierFactory, + cfg *config.Configuration, +) (*jsonast.SwaggerTypes, error) { + // Sort into order by filePath so we're deterministic + slices.SortFunc( + typesFromFiles, + func(left typesFromFile, right typesFromFile) int { + return strings.Compare(left.filePath, right.filePath) + }) typeNameCounts := make(map[astmodel.InternalTypeName]int) for _, typesFromFile := range typesFromFiles { @@ -452,7 +437,7 @@ func mergeTypesForPackage(idFactory astmodel.IdentifierFactory, typesFromFiles [ } } - mergedResult := jsonast.SwaggerTypes{ + mergedResult := &jsonast.SwaggerTypes{ ResourceDefinitions: make(jsonast.ResourceDefinitionSet), OtherDefinitions: make(astmodel.TypeDefinitionSet), } @@ -467,17 +452,94 @@ func mergeTypesForPackage(idFactory astmodel.IdentifierFactory, typesFromFiles [ // but they might be structurally equal } + defs := mergedResult.OtherDefinitions for rn, rt := range typesFromFile.ResourceDefinitions { - if foundRT, ok := mergedResult.ResourceDefinitions[rn]; ok && - !(astmodel.TypeEquals(foundRT.SpecType, rt.SpecType) && astmodel.TypeEquals(foundRT.StatusType, rt.StatusType)) { - panic(fmt.Sprintf("While merging file %s: duplicate resource types generated", typesFromFile.filePath)) + if foundRT, ok := mergedResult.ResourceDefinitions[rn]; ok { + // We have two resources with the same name, if they have exact same structure, they're the same + scopesEqual := foundRT.Scope == rt.Scope + specsEqual := structurallyIdentical(foundRT.SpecType, defs, rt.SpecType, defs) + statusesEqual := structurallyIdentical(foundRT.StatusType, defs, rt.StatusType, defs) + if scopesEqual && specsEqual && statusesEqual { + // They're the same resource, we're good. + continue + } + + // Check for renames and apply them if available + delete(mergedResult.ResourceDefinitions, rn) + newNameA, renamedA := tryRename(rn, rt, cfg) + newNameB, renamedB := tryRename(rn, foundRT, cfg) + + if _, collides := mergedResult.ResourceDefinitions[newNameA]; collides { + err := errors.Errorf( + "merging file %s: renaming %s to %s resulted in a collision with an existing type", + typesFromFile.filePath, + rn.Name(), + newNameA.Name()) + return nil, err + } + + if _, collides := mergedResult.ResourceDefinitions[newNameB]; collides { + err := errors.Errorf( + "merging file %s: renaming %s to %s resulted in a collision with an existing type", + typesFromFile.filePath, + rn.Name(), + newNameB.Name()) + return nil, err + } + + if newNameA != newNameB && (renamedA || renamedB) { + // One or other or both was successfully renamed, we can keep going + mergedResult.ResourceDefinitions[newNameA] = rt + mergedResult.ResourceDefinitions[newNameB] = foundRT + continue + } + + // Names are still the same. We have a collision. + err := errors.Errorf( + "merging file %s: duplicate resource types generated with name %s", + typesFromFile.filePath, + rn.Name()) + return nil, err } mergedResult.ResourceDefinitions[rn] = rt } } - return mergedResult + return mergedResult, nil +} + +func tryRename( + name astmodel.InternalTypeName, + rsrc jsonast.ResourceDefinition, + cfg *config.Configuration, +) (astmodel.InternalTypeName, bool) { + if cfg == nil { + // No renames configured + return name, false + } + + for _, ren := range cfg.TypeLoaderRenames { + if !ren.AppliesToType(name) { + // Doesn't apply to us, not our name + continue + } + + if ren.Scope != nil && + !strings.EqualFold(*ren.Scope, string(rsrc.Scope)) { + // Doesn't apply to us, not our scope + continue + } + + if ren.RenameTo == nil { + // No rename configured + return name, false + } + + return name.WithName(*ren.RenameTo), true + } + + return name, false } type typeAndSource struct { @@ -596,6 +658,7 @@ func applyRenames( ARMURI: rt.ARMURI, ARMType: rt.ARMType, SupportedOperations: rt.SupportedOperations, + Scope: rt.Scope, } } diff --git a/v2/tools/generator/internal/config/configuration.go b/v2/tools/generator/internal/config/configuration.go index b0e0895f586..407f98becfb 100644 --- a/v2/tools/generator/internal/config/configuration.go +++ b/v2/tools/generator/internal/config/configuration.go @@ -47,6 +47,8 @@ type Configuration struct { AnyTypePackages []string `yaml:"anyTypePackages"` // Filters used to control which types are created from the JSON schema TypeFilters []*TypeFilter `yaml:"typeFilters"` + // Renamers used to resolve naming collisions during loading + TypeLoaderRenames []*TypeLoaderRename `yaml:"typeLoaderRenames"` // Transformers used to remap types Transformers []*TypeTransformer `yaml:"typeTransformers"` // RootURL is the root URL for ASOv2 repo, paths are appended to this to generate resource links. diff --git a/v2/tools/generator/internal/config/type_loader_rename.go b/v2/tools/generator/internal/config/type_loader_rename.go new file mode 100644 index 00000000000..4f48d0892fb --- /dev/null +++ b/v2/tools/generator/internal/config/type_loader_rename.go @@ -0,0 +1,14 @@ +/* + * Copyright (c) Microsoft Corporation. + * Licensed under the MIT license. + */ + +package config + +// A TypeLoaderRename is used to resolve a naming conflict that happens during loading of types +// right at the start of the code generator. +type TypeLoaderRename struct { + TypeMatcher `yaml:",inline"` + Scope *string `yaml:"scope,omitempty"` + RenameTo *string `yaml:"renameTo,omitempty"` +} diff --git a/v2/tools/generator/internal/jsonast/swagger_type_extractor.go b/v2/tools/generator/internal/jsonast/swagger_type_extractor.go index 9cd79f11899..1510ac1a35f 100644 --- a/v2/tools/generator/internal/jsonast/swagger_type_extractor.go +++ b/v2/tools/generator/internal/jsonast/swagger_type_extractor.go @@ -70,6 +70,7 @@ type ResourceDefinition struct { ARMType string // e.g. Microsoft.XYZ/resourceThings ARMURI string SupportedOperations set.Set[astmodel.ResourceOperation] + Scope astmodel.ResourceScope // TODO: use ARMURI for generating Resource URIs (only used for documentation & ownership at the moment) } @@ -278,6 +279,7 @@ func (extractor *SwaggerTypeExtractor) extractOneResourceType( ARMType: armType, ARMURI: operationPath, SupportedOperations: supportedOperations, + Scope: categorizeResourceScope(operationPath), } } @@ -953,3 +955,39 @@ func makeSchemaFromSimpleSchemaParam(param schemaAndValidations) *spec.Schema { return schema } + +var ( + resourceGroupScopeRegex = regexp.MustCompile(`(?i)^/subscriptions/[^/]+/resourcegroups/[^/]+/.*`) + locationScopeRegex = regexp.MustCompile(`(?i)^/subscriptions/[^/]+/.*`) + extensionScopePrefixes = []string{ + "/{scope}/", + "/{resourceUri}/", + } +) + +func categorizeResourceScope(armURI string) astmodel.ResourceScope { + // this is a bit of a hack, eventually we should have better scope support. + // at the moment we assume that a resource is an extension if it has a specific prefix + for _, prefix := range extensionScopePrefixes { + // Case-insensitive prefix check, as befits a heuristic + if strings.EqualFold(prefix, armURI[:len(prefix)]) { + return astmodel.ResourceScopeExtension + } + } + + // Assume that a resource is an extension if armURI contains more than 1 provider: + if strings.Count(armURI, "providers") > 1 { + return astmodel.ResourceScopeExtension + } + + if resourceGroupScopeRegex.MatchString(armURI) { + return astmodel.ResourceScopeResourceGroup + } + + if locationScopeRegex.MatchString(armURI) { + return astmodel.ResourceScopeLocation + } + + // TODO: Not currently possible to generate a resource with scope Location, we should fix that + return astmodel.ResourceScopeTenant +} diff --git a/v2/tools/generator/internal/jsonast/swagger_type_extractor_test.go b/v2/tools/generator/internal/jsonast/swagger_type_extractor_test.go index a2e04eb773d..eb1d22858f9 100644 --- a/v2/tools/generator/internal/jsonast/swagger_type_extractor_test.go +++ b/v2/tools/generator/internal/jsonast/swagger_type_extractor_test.go @@ -393,3 +393,40 @@ func makeResourceGroupParameter() spec.Parameter { }, } } + +func TestCategorizeResourceScope(t *testing.T) { + t.Parallel() + + cases := map[string]struct { + path string + expected astmodel.ResourceScope + }{ + "virtual machine": { + path: "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Compute/virtualMachines/{vmName}/extensions/{vmExtensionName}", + expected: astmodel.ResourceScopeResourceGroup, + }, + "role assignment": { + path: "/{scope}/providers/Microsoft.Authorization/roleAssignments/{roleAssignmentName}", + expected: astmodel.ResourceScopeExtension, + }, + "diagnostic setting": { + path: "/subscriptions/{subscriptionId}/providers/Microsoft.Insights/diagnosticSettings/{name}", + expected: astmodel.ResourceScopeLocation, + }, + "diagnostic setting extension": { + path: "/{resourceUri}/providers/Microsoft.Insights/diagnosticSettings/{name}", + expected: astmodel.ResourceScopeExtension, + }, + } + + for name, c := range cases { + c := c + t.Run(name, func(t *testing.T) { + t.Parallel() + g := NewGomegaWithT(t) + + scope := categorizeResourceScope(c.path) + g.Expect(scope).To(Equal(c.expected)) + }) + } +}