From 4e632b4d2920979ff985aa751cb46e1d5d1ad1cb Mon Sep 17 00:00:00 2001 From: lakshmimsft Date: Wed, 7 Oct 2026 13:52:45 -0700 Subject: [PATCH] fix(cli): resolve API versions consistently for generic resource operations Signed-off-by: lakshmimsft --- pkg/cli/clients/management.go | 94 ++++++------- pkg/cli/clients/management_test.go | 192 +++++++++++++++++++++++--- pkg/cli/cmd/resource/create/create.go | 5 +- 3 files changed, 220 insertions(+), 71 deletions(-) diff --git a/pkg/cli/clients/management.go b/pkg/cli/clients/management.go index f12ac2aac05..4488554cdfc 100644 --- a/pkg/cli/clients/management.go +++ b/pkg/cli/clients/management.go @@ -58,13 +58,13 @@ var _ ApplicationsManagementClient = (*UCPApplicationsManagementClient)(nil) // ListResourcesOfType lists all resources of a given type in the configured scope. func (amc *UCPApplicationsManagementClient) ListResourcesOfType(ctx context.Context, resourceType string) ([]generated.GenericResource, error) { - apiVersions, err := amc.getApiVersionsForResourceType(ctx, resourceType) + apiVersion, err := amc.getAPIVersionForResourceType(ctx, resourceType) if err != nil { return nil, fmt.Errorf("failed to get API versions for resource type %q: %w", resourceType, err) } results := []generated.GenericResource{} - client, err := amc.getGenericClient(amc.RootScope, resourceType, apiVersions, false) + client, err := amc.getGenericClient(amc.RootScope, resourceType, apiVersion, false) if err != nil { return nil, err } @@ -132,7 +132,7 @@ func (amc *UCPApplicationsManagementClient) ListResourcesOfTypeInEnvironment(ctx // GetResource retrieves a resource by its type and name (or id). func (amc *UCPApplicationsManagementClient) GetResource(ctx context.Context, resourceType string, resourceNameOrID string) (generated.GenericResource, error) { - apiVersions, err := amc.getApiVersionsForResourceType(ctx, resourceType) + apiVersion, err := amc.getAPIVersionForResourceType(ctx, resourceType) if err != nil { return generated.GenericResource{}, err } @@ -142,7 +142,7 @@ func (amc *UCPApplicationsManagementClient) GetResource(ctx context.Context, res return generated.GenericResource{}, err } - client, err := amc.getGenericClient(scope, resourceType, apiVersions, false) + client, err := amc.getGenericClient(scope, resourceType, apiVersion, false) if err != nil { return generated.GenericResource{}, err } @@ -157,12 +157,17 @@ func (amc *UCPApplicationsManagementClient) GetResource(ctx context.Context, res // CreateOrUpdateResource creates or updates a resource using its type name (or id). func (amc *UCPApplicationsManagementClient) CreateOrUpdateResource(ctx context.Context, resourceType string, resourceNameOrID string, resource *generated.GenericResource) (generated.GenericResource, error) { + apiVersion, err := amc.getAPIVersionForResourceType(ctx, resourceType) + if err != nil { + return generated.GenericResource{}, err + } + scope, name, err := amc.extractScopeAndName(resourceNameOrID) if err != nil { return generated.GenericResource{}, err } - client, err := amc.createGenericClient(scope, resourceType) + client, err := amc.getGenericClient(scope, resourceType, apiVersion, false) if err != nil { return generated.GenericResource{}, err } @@ -182,7 +187,7 @@ func (amc *UCPApplicationsManagementClient) CreateOrUpdateResource(ctx context.C // DeleteResource deletes a resource by its type and name (or id). func (amc *UCPApplicationsManagementClient) DeleteResource(ctx context.Context, resourceType string, resourceNameOrID string, force bool) (bool, error) { - apiVersions, err := amc.getApiVersionsForResourceType(ctx, resourceType) + apiVersion, err := amc.getAPIVersionForResourceType(ctx, resourceType) if err != nil { return false, err } @@ -193,7 +198,7 @@ func (amc *UCPApplicationsManagementClient) DeleteResource(ctx context.Context, } var client genericResourceClient - client, err = amc.getGenericClient(scope, resourceType, apiVersions, force) + client, err = amc.getGenericClient(scope, resourceType, apiVersion, force) if err != nil { return false, err } @@ -829,16 +834,16 @@ func (amc *UCPApplicationsManagementClient) ListResourcesInResourceGroup(ctx con // classify a 404 from this method as "the resource group does not exist", which only the // GetResourceGroup check above can establish. Letting a per-resource-type 404 reach the caller // as a 404 would turn an enumeration failure into a silent no-op that reports success while - // the group and its contents survive. getApiVersionsForResourceType converts a provider 404 + // the group and its contents survive. getAPIVersionForResourceType converts a provider 404 // for the same reason. for _, resourceType := range resourceTypesList { // Create a client scoped to this resource group - apiVersions, err := amc.getApiVersionsForResourceType(ctx, resourceType) + apiVersion, err := amc.getAPIVersionForResourceType(ctx, resourceType) if err != nil { return nil, NewResourceEnumerationError(resourceType, "failed to get API versions for resource type %q: %v", resourceType, err) } - client, err := amc.getGenericClient(groupScope, resourceType, apiVersions, false) + client, err := amc.getGenericClient(groupScope, resourceType, apiVersion, false) if err != nil { return nil, NewResourceEnumerationError(resourceType, "failed to create client for resource type %q: %v", resourceType, err) } @@ -898,12 +903,12 @@ func (amc *UCPApplicationsManagementClient) ListResourcesOfTypeInResourceGroup(c groupScope := fmt.Sprintf("/planes/radius/%s/resourceGroups/%s", planeName, resourceGroupName) // Get API versions for the resource type - apiVersions, err := amc.getApiVersionsForResourceType(ctx, resourceType) + apiVersion, err := amc.getAPIVersionForResourceType(ctx, resourceType) if err != nil { return nil, err } - client, err := amc.getGenericClient(groupScope, resourceType, apiVersions, false) + client, err := amc.getGenericClient(groupScope, resourceType, apiVersion, false) if err != nil { return nil, err } @@ -1383,29 +1388,6 @@ func (amc *UCPApplicationsManagementClient) createRadiusCoreEnvironmentClient(sc return &scopedRadiusCoreEnvironmentsClient{inner: inner, scope: strings.TrimPrefix(scope, resources.SegmentSeparator)}, nil } -func (amc *UCPApplicationsManagementClient) createGenericClient(scope string, resourceType string, apiVersion ...string) (genericResourceClient, error) { - // Radius.Core resources require a specific API version, matching getGenericClient. Without this - // the default below is used, and the server rejects the request for the resource type. - if isRadiusCoreType(resourceType) { - apiVersion = []string{radiusCoreAPIVersion} - } - - if amc.genericResourceClientFactory == nil { - clientOptions := *amc.ClientOptions - if len(apiVersion) != 0 { - // If an API version is provided, set it in the client options. - // Otherwise, the default API version (2023-10-01-preview) will be used. - // Note: If multiple API versions are supported for Applications.Core resource types in the future, - // update this logic to select the appropriate client. - clientOptions.APIVersion = apiVersion[0] - } - // Generated client doesn't like the leading '/' in the scope. - return generated.NewGenericResourcesClient(resourceType, strings.TrimPrefix(scope, resources.SegmentSeparator), &aztoken.AnonymousCredential{}, &clientOptions) - } - - return amc.genericResourceClientFactory(scope, resourceType) -} - func (amc *UCPApplicationsManagementClient) createResourceGroupClient() (resourceGroupClient, error) { if amc.resourceGroupClientFactory == nil { return ucpv20231001.NewResourceGroupsClient(&aztoken.AnonymousCredential{}, amc.ClientOptions) @@ -1522,15 +1504,18 @@ func (amc *UCPApplicationsManagementClient) captureResponse(ctx context.Context, return amc.capture(ctx, response) } -// getApiVersionsForResourceType retrieves the API versions for a given resource type in the configured scope. -func (amc *UCPApplicationsManagementClient) getApiVersionsForResourceType(ctx context.Context, resourceType string) ([]string, error) { +// getAPIVersionForResourceType resolves the API version to use for a given resource type in the +// configured scope. It prefers the resource provider's declared default API version and falls back +// to the lowest advertised version when no default is set. It returns an empty version when the +// resource type advertises none, which leaves the caller on the generated client's default. +func (amc *UCPApplicationsManagementClient) getAPIVersionForResourceType(ctx context.Context, resourceType string) (string, error) { provider, _, _ := strings.Cut(resourceType, "/") summary, err := amc.GetResourceProviderSummary(ctx, "local", provider) if err != nil { if clientv2.Is404Error(err) { - return nil, fmt.Errorf("resource provider %q not found in the configured scope", provider) + return "", fmt.Errorf("resource provider %q not found in the configured scope", provider) } - return nil, err + return "", err } resType := strings.Split(resourceType, "/")[1] @@ -1545,24 +1530,39 @@ func (amc *UCPApplicationsManagementClient) getApiVersionsForResourceType(ctx co } } if !ok { - return nil, fmt.Errorf("resource type %q not found in the resource provider %q", resType, provider) + return "", fmt.Errorf("resource type %q not found in the resource provider %q", resType, provider) } - return maps.Keys(resourceTypeSummary.APIVersions), nil + // Prefer the API version the resource provider declares as its default. Resource types may + // advertise several versions, and map iteration order is not stable, so falling straight + // through to an arbitrary key would pick a different version from one call to the next. + if resourceTypeSummary.DefaultAPIVersion != nil && *resourceTypeSummary.DefaultAPIVersion != "" { + return *resourceTypeSummary.DefaultAPIVersion, nil + } + + apiVersions := maps.Keys(resourceTypeSummary.APIVersions) + if len(apiVersions) == 0 { + return "", nil + } + + // No default is declared, so sort to keep the choice stable across invocations. + slices.Sort(apiVersions) + + return apiVersions[0], nil } -// getGenericClient returns a generic resource client for the specified scope and resource type. -// If apiVersions is empty, it uses the default version (2023-10-01-preview), else uses any version supported by the resource type. +// getGenericClient returns a generic resource client for the specified scope and resource type +// using the supplied API version. // When no genericResourceClientFactory is configured and force is true, a per-call policy is added // that appends force=true to the request URL query string. This is used for force-deleting resources // that are in a non-terminal provisioning state. Factory-based configurations (used in tests) bypass // the force policy since mock clients do not exercise the HTTP pipeline. -func (amc *UCPApplicationsManagementClient) getGenericClient(scope, resourceType string, apiVersions []string, force bool) (client genericResourceClient, err error) { +func (amc *UCPApplicationsManagementClient) getGenericClient(scope, resourceType string, apiVersion string, force bool) (client genericResourceClient, err error) { // Radius.Core resources require a specific API version. // Eventually version 2023-10-01-preview will be removed along with Applications.Core resources. // Then we will not need this special case. if isRadiusCoreType(resourceType) { - apiVersions = []string{radiusCoreAPIVersion} + apiVersion = radiusCoreAPIVersion } if amc.genericResourceClientFactory != nil { @@ -1575,8 +1575,8 @@ func (amc *UCPApplicationsManagementClient) getGenericClient(scope, resourceType clientOptions = withForceDeletePolicy(clientOptions) } - if len(apiVersions) != 0 { - clientOptions.APIVersion = apiVersions[0] + if apiVersion != "" { + clientOptions.APIVersion = apiVersion } return generated.NewGenericResourcesClient(resourceType, strings.TrimPrefix(scope, resources.SegmentSeparator), &aztoken.AnonymousCredential{}, &clientOptions) diff --git a/pkg/cli/clients/management_test.go b/pkg/cli/clients/management_test.go index d0ac22f8e8d..6cbdfcbf061 100644 --- a/pkg/cli/clients/management_test.go +++ b/pkg/cli/clients/management_test.go @@ -745,8 +745,29 @@ func Test_Resource(t *testing.T) { }) t.Run("CreateOrUpdateResource", func(t *testing.T) { - mock := NewMockgenericResourceClient(gomock.NewController(t)) + ctrl := gomock.NewController(t) + mock := NewMockgenericResourceClient(ctrl) + resourceProviderMock := NewMockresourceProviderClient(ctrl) client := createClient(mock) + client.resourceProviderClientFactory = func() (resourceProviderClient, error) { + return resourceProviderMock, nil + } + expectedResourceSummary := ucp.ResourceProviderSummary{ + Name: new("Applications.Test"), + ResourceTypes: map[string]*ucp.ResourceProviderSummaryResourceType{ + "testResource": { + APIVersions: map[string]*ucp.ResourceTypeSummaryResultAPIVersion{ + version: {}, + }, + }, + }, + Locations: map[string]*ucp.ResourceProviderSummaryLocation{ + "east": {}, + }, + } + resourceProviderMock.EXPECT(). + GetProviderSummary(gomock.Any(), "local", "Applications.Test", gomock.Any()). + Return(ucp.ResourceProvidersClientGetProviderSummaryResponse{ResourceProviderSummary: expectedResourceSummary}, nil) mock.EXPECT(). BeginCreateOrUpdate(gomock.Any(), testResourceName, expectedResource, gomock.Any()). @@ -894,49 +915,173 @@ func Test_ForceDeletePolicy(t *testing.T) { }) } -// Radius.Core resources are only served at 2025-08-01-preview. The generic client otherwise falls -// back to the default 2023-10-01-preview, which the server rejects for these types, so every write -// path has to pin the version the same way the read paths do. -func Test_CreateOrUpdateResource_RadiusCoreAPIVersion(t *testing.T) { +func Test_CreateOrUpdateResource_APIVersion(t *testing.T) { t.Parallel() - for _, tt := range []struct { - name string - resourceType string - expectedVersion string + tests := []struct { + name string + resourceType string + providerName string + resourceTypeName string + // advertisedVersions are the API versions the resource type advertises. Defaults to a + // single entry of `version` when nil. + advertisedVersions []string + // defaultAPIVersion is advertised as the resource type's declared default when non-empty. + defaultAPIVersion string + expectedAPIVersion string + lookupErr error }{ - {"radius core pins the supported version", "Radius.Core/terraformSettings", "2025-08-01-preview"}, - {"radius core is matched case-insensitively", "radius.core/environments", "2025-08-01-preview"}, - {"applications core keeps the default", "Applications.Core/extenders", "2023-10-01-preview"}, - } { + { + name: "uses the API version advertised by the resource provider", + resourceType: "Applications.Test/testResource", + providerName: "Applications.Test", + resourceTypeName: "testResource", + // Must come from the provider summary, not the 2023-10-01-preview client default. + expectedAPIVersion: version, + }, + { + // Radius.Compute is outside the Radius.Core namespace, so it must resolve + // dynamically rather than fall through to the Radius.Core pin below. + name: "resolves non-Core Radius types from the resource provider", + resourceType: "Radius.Compute/containers", + providerName: "Radius.Compute", + resourceTypeName: "containers", + expectedAPIVersion: version, + }, + { + name: "pins Radius.Core resources to their required API version", + resourceType: "Radius.Core/environments", + providerName: "Radius.Core", + resourceTypeName: "environments", + expectedAPIVersion: "2025-08-01-preview", + }, + { + // Resource type names are case-insensitive, so the pin must survive a lowercase + // namespace all the way through CreateOrUpdateResource, not just in isRadiusCoreType. + name: "pins Radius.Core resources matched case-insensitively", + resourceType: "radius.core/environments", + providerName: "radius.core", + resourceTypeName: "environments", + expectedAPIVersion: "2025-08-01-preview", + }, + { + // Guards the default-preference branch: the declared default must win even though it + // is neither the lowest nor the highest advertised version. + name: "prefers the declared default API version over the advertised versions", + resourceType: "Applications.Test/testResource", + providerName: "Applications.Test", + resourceTypeName: "testResource", + advertisedVersions: []string{"2023-05-01", "2024-01-01", "2025-01-01"}, + defaultAPIVersion: "2024-01-01", + expectedAPIVersion: "2024-01-01", + }, + { + // Guards the deterministic-sort branch: with no declared default and several + // advertised versions, the lowest must be chosen. Map iteration order is random, so an + // unsorted implementation fails this case for most of the advertised versions. + name: "falls back to the lowest advertised version when no default is declared", + resourceType: "Applications.Test/testResource", + providerName: "Applications.Test", + resourceTypeName: "testResource", + advertisedVersions: []string{ + "2025-08-01-preview", + "2023-10-01-preview", + "2024-01-01", + "2026-01-01", + "2023-05-01", + }, + expectedAPIVersion: "2023-05-01", + }, + { + name: "does not send a request when the API version lookup fails", + resourceType: "Applications.Test/testResource", + providerName: "Applications.Test", + resourceTypeName: "testResource", + lookupErr: errors.New("provider summary unavailable"), + }, + } + + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - var capturedURLs []string + var putAPIVersions []string transport := &mockTransport{ do: func(req *http.Request) (*http.Response, error) { - capturedURLs = append(capturedURLs, req.URL.String()) + if req.Method == http.MethodPut { + putAPIVersions = append(putAPIVersions, req.URL.Query().Get("api-version")) + } header := http.Header{} header.Set("Content-Type", "application/json") return &http.Response{ StatusCode: http.StatusOK, Header: header, - Body: io.NopCloser(strings.NewReader(`{"id": "` + testScope + `/providers/` + tt.resourceType + `/myresource", "properties": {"provisioningState": "Succeeded"}}`)), + Body: io.NopCloser(strings.NewReader(`{"status": "Succeeded"}`)), Request: req, }, nil }, } + ctrl := gomock.NewController(t) + rpClient := NewMockresourceProviderClient(ctrl) + if tt.lookupErr != nil { + rpClient.EXPECT(). + GetProviderSummary(gomock.Any(), "local", tt.providerName, gomock.Any()). + Return(ucp.ResourceProvidersClientGetProviderSummaryResponse{}, tt.lookupErr) + } else { + advertised := tt.advertisedVersions + if advertised == nil { + advertised = []string{version} + } + + apiVersions := map[string]*ucp.ResourceTypeSummaryResultAPIVersion{} + for _, advertisedVersion := range advertised { + apiVersions[advertisedVersion] = &ucp.ResourceTypeSummaryResultAPIVersion{} + } + + resourceTypeSummary := &ucp.ResourceProviderSummaryResourceType{ + APIVersions: apiVersions, + } + if tt.defaultAPIVersion != "" { + resourceTypeSummary.DefaultAPIVersion = new(tt.defaultAPIVersion) + } + + rpClient.EXPECT(). + GetProviderSummary(gomock.Any(), "local", tt.providerName, gomock.Any()). + Return(ucp.ResourceProvidersClientGetProviderSummaryResponse{ + ResourceProviderSummary: ucp.ResourceProviderSummary{ + Name: new(tt.providerName), + ResourceTypes: map[string]*ucp.ResourceProviderSummaryResourceType{ + tt.resourceTypeName: resourceTypeSummary, + }, + }, + }, nil) + } + client := &UCPApplicationsManagementClient{ - RootScope: testScope, - ClientOptions: &arm.ClientOptions{Transport: transport}, + RootScope: testScope, + ClientOptions: &arm.ClientOptions{ + Transport: transport, + }, + resourceProviderClientFactory: func() (resourceProviderClient, error) { + return rpClient, nil + }, } - _, err := client.CreateOrUpdateResource(t.Context(), tt.resourceType, "myresource", &generated.GenericResource{}) - require.NoError(t, err) + resourceID := testScope + "/providers/" + tt.resourceType + "/myresource" + _, err := client.CreateOrUpdateResource(t.Context(), tt.resourceType, resourceID, &generated.GenericResource{}) - require.NotEmpty(t, capturedURLs) - require.Contains(t, capturedURLs[0], "api-version="+tt.expectedVersion) + if tt.lookupErr != nil { + require.ErrorContains(t, err, "provider summary unavailable") + require.Empty(t, putAPIVersions, "no PUT should be sent when the API version cannot be resolved") + return + } + + require.NoError(t, err) + require.NotEmpty(t, putAPIVersions, "expected at least one PUT request") + for _, actual := range putAPIVersions { + require.Equal(t, tt.expectedAPIVersion, actual, "unexpected api-version on PUT request") + } }) } } @@ -2083,7 +2228,8 @@ func Test_ListResourcesInResourceGroup(t *testing.T) { ResourceProviderSummary: *summariesWithErrors[0].Value[0], }, nil) - // Second provider has empty API versions + // Second provider has empty API versions, so it falls back to the generated client's + // default API version rather than failing the whole enumeration. mockRP.EXPECT(). GetProviderSummary(gomock.Any(), "local", "Applications.TestNoVersion", gomock.Any()). Return(ucp.ResourceProvidersClientGetProviderSummaryResponse{ diff --git a/pkg/cli/cmd/resource/create/create.go b/pkg/cli/cmd/resource/create/create.go index 23db0377dac..e2cdf72f6fb 100644 --- a/pkg/cli/cmd/resource/create/create.go +++ b/pkg/cli/cmd/resource/create/create.go @@ -48,7 +48,10 @@ Resources are the primary entities that make up applications. Input can be passed via the -f flag to specify a file name.`, Example: ` # Create a resource (from file) -rad resource create 'Applications.Core/containers' mycontainer -f /path/to/input.json`, +rad resource create 'Applications.Core/containers' mycontainer -f /path/to/input.json + +# Create a resource of any registered resource type +rad resource create 'Radius.Compute/containers' mycontainer -f /path/to/input.json`, Args: cobra.ExactArgs(2), RunE: framework.RunCommand(runner), }