From 54d67c8cfd64e65ffe2755aeda9ae5e38d1afbb4 Mon Sep 17 00:00:00 2001 From: lakshmimsft Date: Thu, 19 Dec 2024 16:01:10 -0800 Subject: [PATCH] update per comments Signed-off-by: lakshmimsft --- pkg/cli/cmd/resourceprovider/create/create.go | 3 +- pkg/cli/cmd/utils.go | 20 --- pkg/cli/connections/factory.go | 38 +---- pkg/cli/manifest/registermanifest.go | 140 --------------- pkg/cli/manifest/testclientfactory.go | 160 ++++++++++++++++++ pkg/cli/workspaces/connection.go | 17 +- pkg/ucp/config.go | 1 + pkg/ucp/initializer/service.go | 28 ++- test/rp/rptest.go | 2 +- 9 files changed, 196 insertions(+), 213 deletions(-) create mode 100644 pkg/cli/manifest/testclientfactory.go diff --git a/pkg/cli/cmd/resourceprovider/create/create.go b/pkg/cli/cmd/resourceprovider/create/create.go index 6b913d2c59..5534a8912a 100644 --- a/pkg/cli/cmd/resourceprovider/create/create.go +++ b/pkg/cli/cmd/resourceprovider/create/create.go @@ -21,7 +21,6 @@ import ( aztoken "github.com/radius-project/radius/pkg/azure/tokencredentials" "github.com/radius-project/radius/pkg/cli" - "github.com/radius-project/radius/pkg/cli/cmd" "github.com/radius-project/radius/pkg/cli/cmd/commonflags" "github.com/radius-project/radius/pkg/cli/cmd/resourceprovider/common" "github.com/radius-project/radius/pkg/cli/framework" @@ -148,7 +147,7 @@ func (r *Runner) Run(ctx context.Context) error { } func (r *Runner) initializeClientFactory(ctx context.Context, workspace *workspaces.Workspace) error { - connection, err := cmd.GetConnection(ctx, workspace) + connection, err := workspace.Connect(ctx) if err != nil { return err } diff --git a/pkg/cli/cmd/utils.go b/pkg/cli/cmd/utils.go index 58541acd4f..e13b1243a2 100644 --- a/pkg/cli/cmd/utils.go +++ b/pkg/cli/cmd/utils.go @@ -18,16 +18,13 @@ package cmd import ( "context" - "errors" "fmt" "github.com/radius-project/radius/pkg/cli/aws" "github.com/radius-project/radius/pkg/cli/azure" "github.com/radius-project/radius/pkg/cli/clients" "github.com/radius-project/radius/pkg/cli/clierrors" - "github.com/radius-project/radius/pkg/cli/workspaces" corerp "github.com/radius-project/radius/pkg/corerp/api/v20231001preview" - "github.com/radius-project/radius/pkg/sdk" "github.com/radius-project/radius/pkg/to" ) @@ -97,20 +94,3 @@ func CheckIfRecipeExists(ctx context.Context, client clients.ApplicationsManagem return envResource, recipeProperties, nil } - -// GetConnection from Workspace. -func GetConnection(ctx context.Context, workspace *workspaces.Workspace) (sdk.Connection, error) { - connection, err := workspace.Connect() - if err != nil { - return nil, err - } - - err = sdk.TestConnection(ctx, connection) - if errors.Is(err, &sdk.ErrRadiusNotInstalled{}) { - return nil, clierrors.MessageWithCause(err, "Could not connect to Radius.") - } else if err != nil { - return nil, err - } - - return connection, nil -} diff --git a/pkg/cli/connections/factory.go b/pkg/cli/connections/factory.go index 93b34f0142..cd7bda56dd 100644 --- a/pkg/cli/connections/factory.go +++ b/pkg/cli/connections/factory.go @@ -18,13 +18,11 @@ package connections import ( "context" - "errors" "fmt" aztoken "github.com/radius-project/radius/pkg/azure/tokencredentials" "github.com/radius-project/radius/pkg/cli/clients" "github.com/radius-project/radius/pkg/cli/clients_new/generated" - "github.com/radius-project/radius/pkg/cli/clierrors" cli_credential "github.com/radius-project/radius/pkg/cli/credential" "github.com/radius-project/radius/pkg/cli/deployment" "github.com/radius-project/radius/pkg/cli/kubernetes" @@ -55,18 +53,11 @@ type impl struct { // CreateDeploymentClient connects to a workspace, tests the connection, creates a deployment client and an operations // client, and returns them along with the resource group name. It returns an error if any of the steps fail. func (i *impl) CreateDeploymentClient(ctx context.Context, workspace workspaces.Workspace) (clients.DeploymentClient, error) { - connection, err := workspace.Connect() + connection, err := workspace.Connect(ctx) if err != nil { return nil, err } - err = sdk.TestConnection(ctx, connection) - if errors.Is(err, &sdk.ErrRadiusNotInstalled{}) { - return nil, clierrors.MessageWithCause(err, "Could not connect to Radius.") - } else if err != nil { - return nil, err - } - armClientOptions := sdk.NewClientOptions(connection) dc, err := sdkclients.NewResourceDeploymentsClient(&sdkclients.Options{ Cred: &aztoken.AnonymousCredential{}, @@ -102,18 +93,11 @@ func (i *impl) CreateDeploymentClient(ctx context.Context, workspace workspaces. // CreateDiagnosticsClient creates a DiagnosticsClient by connecting to a workspace, testing the connection, and creating // clients for applications, containers, environments, and gateways. If an error occurs, it is returned. func (i *impl) CreateDiagnosticsClient(ctx context.Context, workspace workspaces.Workspace) (clients.DiagnosticsClient, error) { - connection, err := workspace.Connect() + connection, err := workspace.Connect(ctx) if err != nil { return nil, err } - err = sdk.TestConnection(ctx, connection) - if errors.Is(err, &sdk.ErrRadiusNotInstalled{}) { - return nil, clierrors.MessageWithCause(err, "Could not connect to Radius.") - } else if err != nil { - return nil, err - } - connectionConfig, err := workspace.ConnectionConfig() if err != nil { return nil, err @@ -168,18 +152,11 @@ func (i *impl) CreateDiagnosticsClient(ctx context.Context, workspace workspaces // CreateApplicationsManagementClient connects to the workspace, tests the connection, and returns a // UCPApplicationsManagementClient if successful, or an error if unsuccessful. func (*impl) CreateApplicationsManagementClient(ctx context.Context, workspace workspaces.Workspace) (clients.ApplicationsManagementClient, error) { - connection, err := workspace.Connect() + connection, err := workspace.Connect(ctx) if err != nil { return nil, err } - err = sdk.TestConnection(ctx, connection) - if errors.Is(err, &sdk.ErrRadiusNotInstalled{}) { - return nil, clierrors.MessageWithCause(err, "Could not connect to Radius.") - } else if err != nil { - return nil, err - } - return &clients.UCPApplicationsManagementClient{ RootScope: workspace.Scope, ClientOptions: sdk.NewClientOptions(connection), @@ -192,18 +169,11 @@ func (*impl) CreateApplicationsManagementClient(ctx context.Context, workspace w // CreateCredentialManagementClient establishes a connection to a workspace, tests the connection, creates Azure and AWS // credential clients, and returns a UCPCredentialManagementClient. An error is returned if any of the steps fail. func (*impl) CreateCredentialManagementClient(ctx context.Context, workspace workspaces.Workspace) (cli_credential.CredentialManagementClient, error) { - connection, err := workspace.Connect() + connection, err := workspace.Connect(ctx) if err != nil { return nil, err } - err = sdk.TestConnection(ctx, connection) - if errors.Is(err, &sdk.ErrRadiusNotInstalled{}) { - return nil, clierrors.MessageWithCause(err, "Could not connect to Radius.") - } else if err != nil { - return nil, err - } - clientOptions := sdk.NewClientOptions(connection) azureCredentialClient, err := v20231001preview.NewAzureCredentialsClient(&aztoken.AnonymousCredential{}, clientOptions) diff --git a/pkg/cli/manifest/registermanifest.go b/pkg/cli/manifest/registermanifest.go index 0e97ee5290..1ceca54f72 100644 --- a/pkg/cli/manifest/registermanifest.go +++ b/pkg/cli/manifest/registermanifest.go @@ -14,18 +14,12 @@ package manifest import ( "context" "fmt" - "net/http" "os" "path/filepath" - armpolicy "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/policy" - azfake "github.com/Azure/azure-sdk-for-go/sdk/azcore/fake" - - "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" v1 "github.com/radius-project/radius/pkg/armrpc/api/v1" "github.com/radius-project/radius/pkg/to" "github.com/radius-project/radius/pkg/ucp/api/v20231001preview" - ucpfake "github.com/radius-project/radius/pkg/ucp/api/v20231001preview/fake" ) // RegisterFile registers a manifest file @@ -167,137 +161,3 @@ func logIfEnabled(logger func(format string, args ...any), format string, args . logger(format, args...) } } - -func NewTestClientFactory() (*v20231001preview.ClientFactory, error) { - // Create fake servers for each client - resourceProvidersServer := ucpfake.ResourceProvidersServer{ - BeginCreateOrUpdate: func( - ctx context.Context, - planeName string, - resourceProviderName string, - resource v20231001preview.ResourceProviderResource, - options *v20231001preview.ResourceProvidersClientBeginCreateOrUpdateOptions, - ) (resp azfake.PollerResponder[v20231001preview.ResourceProvidersClientCreateOrUpdateResponse], errResp azfake.ErrorResponder) { - // Simulate successful creation - result := v20231001preview.ResourceProvidersClientCreateOrUpdateResponse{ - ResourceProviderResource: resource, - } - resp.AddNonTerminalResponse(http.StatusCreated, nil) - resp.SetTerminalResponse(http.StatusOK, result, nil) - - return - }, - Get: func( - ctx context.Context, - planeName string, - resourceProviderName string, - options *v20231001preview.ResourceProvidersClientGetOptions, // Add this parameter - ) (resp azfake.Responder[v20231001preview.ResourceProvidersClientGetResponse], errResp azfake.ErrorResponder) { - response := v20231001preview.ResourceProvidersClientGetResponse{ - ResourceProviderResource: v20231001preview.ResourceProviderResource{ - Name: to.Ptr(resourceProviderName), - }, - } - resp.SetResponse(http.StatusOK, response, nil) - return - }, - } - - // Create other fake servers similarly - resourceTypesServer := ucpfake.ResourceTypesServer{ - BeginCreateOrUpdate: func( - ctx context.Context, - planeName string, - resourceProviderName string, - resourceTypeName string, - resource v20231001preview.ResourceTypeResource, - options *v20231001preview.ResourceTypesClientBeginCreateOrUpdateOptions, - ) (resp azfake.PollerResponder[v20231001preview.ResourceTypesClientCreateOrUpdateResponse], errResp azfake.ErrorResponder) { - result := v20231001preview.ResourceTypesClientCreateOrUpdateResponse{ - ResourceTypeResource: resource, - } - - resp.AddNonTerminalResponse(http.StatusCreated, nil) - resp.SetTerminalResponse(http.StatusOK, result, nil) - - return - }, - Get: func( - ctx context.Context, - planeName string, - resourceProviderName string, - resourceTypeName string, - options *v20231001preview.ResourceTypesClientGetOptions, - ) (resp azfake.Responder[v20231001preview.ResourceTypesClientGetResponse], errResp azfake.ErrorResponder) { - response := v20231001preview.ResourceTypesClientGetResponse{ - ResourceTypeResource: v20231001preview.ResourceTypeResource{ - Name: to.Ptr(resourceTypeName), - }, - } - resp.SetResponse(http.StatusOK, response, nil) - return - }, - } - - apiVersionsServer := ucpfake.APIVersionsServer{ - BeginCreateOrUpdate: func( - ctx context.Context, - planeName string, - resourceProviderName string, - resourceTypeName string, - apiVersionName string, // Added missing parameter - resource v20231001preview.APIVersionResource, - options *v20231001preview.APIVersionsClientBeginCreateOrUpdateOptions, - ) (resp azfake.PollerResponder[v20231001preview.APIVersionsClientCreateOrUpdateResponse], errResp azfake.ErrorResponder) { - // Simulate successful creation - result := v20231001preview.APIVersionsClientCreateOrUpdateResponse{ - APIVersionResource: resource, - } - resp.AddNonTerminalResponse(http.StatusCreated, nil) - resp.SetTerminalResponse(http.StatusOK, result, nil) - return - }, - } - - locationsServer := ucpfake.LocationsServer{ - BeginCreateOrUpdate: func( - ctx context.Context, - planeName string, - resourceProviderName string, - locationName string, - resource v20231001preview.LocationResource, - options *v20231001preview.LocationsClientBeginCreateOrUpdateOptions, - ) (resp azfake.PollerResponder[v20231001preview.LocationsClientCreateOrUpdateResponse], errResp azfake.ErrorResponder) { - // Simulate successful creation - result := v20231001preview.LocationsClientCreateOrUpdateResponse{ - LocationResource: resource, - } - resp.AddNonTerminalResponse(http.StatusCreated, nil) - resp.SetTerminalResponse(http.StatusOK, result, nil) - - return - }, - } - - serverFactory := ucpfake.ServerFactory{ - ResourceProvidersServer: resourceProvidersServer, - ResourceTypesServer: resourceTypesServer, - APIVersionsServer: apiVersionsServer, - LocationsServer: locationsServer, - } - - serverFactoryTransport := ucpfake.NewServerFactoryTransport(&serverFactory) - - clientOptions := &armpolicy.ClientOptions{ - ClientOptions: policy.ClientOptions{ - Transport: serverFactoryTransport, - }, - } - - clientFactory, err := v20231001preview.NewClientFactory(&azfake.TokenCredential{}, clientOptions) - if err != nil { - return nil, err - } - - return clientFactory, err -} diff --git a/pkg/cli/manifest/testclientfactory.go b/pkg/cli/manifest/testclientfactory.go new file mode 100644 index 0000000000..d5c1962e49 --- /dev/null +++ b/pkg/cli/manifest/testclientfactory.go @@ -0,0 +1,160 @@ +/* +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package manifest + +import ( + "context" + "net/http" + + armpolicy "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm/policy" + azfake "github.com/Azure/azure-sdk-for-go/sdk/azcore/fake" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" + "github.com/radius-project/radius/pkg/to" + "github.com/radius-project/radius/pkg/ucp/api/v20231001preview" + ucpfake "github.com/radius-project/radius/pkg/ucp/api/v20231001preview/fake" +) + +// NewTestClientFactory creates a new client factory for testing purposes. +func NewTestClientFactory() (*v20231001preview.ClientFactory, error) { + // Create fake servers for each client + resourceProvidersServer := ucpfake.ResourceProvidersServer{ + BeginCreateOrUpdate: func( + ctx context.Context, + planeName string, + resourceProviderName string, + resource v20231001preview.ResourceProviderResource, + options *v20231001preview.ResourceProvidersClientBeginCreateOrUpdateOptions, + ) (resp azfake.PollerResponder[v20231001preview.ResourceProvidersClientCreateOrUpdateResponse], errResp azfake.ErrorResponder) { + // Simulate successful creation + result := v20231001preview.ResourceProvidersClientCreateOrUpdateResponse{ + ResourceProviderResource: resource, + } + resp.AddNonTerminalResponse(http.StatusCreated, nil) + resp.SetTerminalResponse(http.StatusOK, result, nil) + + return + }, + Get: func( + ctx context.Context, + planeName string, + resourceProviderName string, + options *v20231001preview.ResourceProvidersClientGetOptions, // Add this parameter + ) (resp azfake.Responder[v20231001preview.ResourceProvidersClientGetResponse], errResp azfake.ErrorResponder) { + response := v20231001preview.ResourceProvidersClientGetResponse{ + ResourceProviderResource: v20231001preview.ResourceProviderResource{ + Name: to.Ptr(resourceProviderName), + }, + } + resp.SetResponse(http.StatusOK, response, nil) + return + }, + } + + // Create other fake servers similarly + resourceTypesServer := ucpfake.ResourceTypesServer{ + BeginCreateOrUpdate: func( + ctx context.Context, + planeName string, + resourceProviderName string, + resourceTypeName string, + resource v20231001preview.ResourceTypeResource, + options *v20231001preview.ResourceTypesClientBeginCreateOrUpdateOptions, + ) (resp azfake.PollerResponder[v20231001preview.ResourceTypesClientCreateOrUpdateResponse], errResp azfake.ErrorResponder) { + result := v20231001preview.ResourceTypesClientCreateOrUpdateResponse{ + ResourceTypeResource: resource, + } + + resp.AddNonTerminalResponse(http.StatusCreated, nil) + resp.SetTerminalResponse(http.StatusOK, result, nil) + + return + }, + Get: func( + ctx context.Context, + planeName string, + resourceProviderName string, + resourceTypeName string, + options *v20231001preview.ResourceTypesClientGetOptions, + ) (resp azfake.Responder[v20231001preview.ResourceTypesClientGetResponse], errResp azfake.ErrorResponder) { + response := v20231001preview.ResourceTypesClientGetResponse{ + ResourceTypeResource: v20231001preview.ResourceTypeResource{ + Name: to.Ptr(resourceTypeName), + }, + } + resp.SetResponse(http.StatusOK, response, nil) + return + }, + } + + apiVersionsServer := ucpfake.APIVersionsServer{ + BeginCreateOrUpdate: func( + ctx context.Context, + planeName string, + resourceProviderName string, + resourceTypeName string, + apiVersionName string, // Added missing parameter + resource v20231001preview.APIVersionResource, + options *v20231001preview.APIVersionsClientBeginCreateOrUpdateOptions, + ) (resp azfake.PollerResponder[v20231001preview.APIVersionsClientCreateOrUpdateResponse], errResp azfake.ErrorResponder) { + // Simulate successful creation + result := v20231001preview.APIVersionsClientCreateOrUpdateResponse{ + APIVersionResource: resource, + } + resp.AddNonTerminalResponse(http.StatusCreated, nil) + resp.SetTerminalResponse(http.StatusOK, result, nil) + return + }, + } + + locationsServer := ucpfake.LocationsServer{ + BeginCreateOrUpdate: func( + ctx context.Context, + planeName string, + resourceProviderName string, + locationName string, + resource v20231001preview.LocationResource, + options *v20231001preview.LocationsClientBeginCreateOrUpdateOptions, + ) (resp azfake.PollerResponder[v20231001preview.LocationsClientCreateOrUpdateResponse], errResp azfake.ErrorResponder) { + // Simulate successful creation + result := v20231001preview.LocationsClientCreateOrUpdateResponse{ + LocationResource: resource, + } + resp.AddNonTerminalResponse(http.StatusCreated, nil) + resp.SetTerminalResponse(http.StatusOK, result, nil) + + return + }, + } + + serverFactory := ucpfake.ServerFactory{ + ResourceProvidersServer: resourceProvidersServer, + ResourceTypesServer: resourceTypesServer, + APIVersionsServer: apiVersionsServer, + LocationsServer: locationsServer, + } + + serverFactoryTransport := ucpfake.NewServerFactoryTransport(&serverFactory) + + clientOptions := &armpolicy.ClientOptions{ + ClientOptions: policy.ClientOptions{ + Transport: serverFactoryTransport, + }, + } + + clientFactory, err := v20231001preview.NewClientFactory(&azfake.TokenCredential{}, clientOptions) + if err != nil { + return nil, err + } + + return clientFactory, err +} diff --git a/pkg/cli/workspaces/connection.go b/pkg/cli/workspaces/connection.go index 12addf527c..d4f40b2292 100644 --- a/pkg/cli/workspaces/connection.go +++ b/pkg/cli/workspaces/connection.go @@ -17,11 +17,14 @@ limitations under the License. package workspaces import ( + "context" + "errors" "fmt" "net/url" "strings" "github.com/mitchellh/mapstructure" + clierrors "github.com/radius-project/radius/pkg/cli/clierrors" "github.com/radius-project/radius/pkg/cli/kubernetes" "github.com/radius-project/radius/pkg/sdk" ) @@ -94,12 +97,24 @@ func (ws Workspace) ConnectionConfig() (ConnectionConfig, error) { // Connect attempts to create a connection to the workspace using the connection configuration and returns the // connection and an error if one occurs. -func (ws Workspace) Connect() (sdk.Connection, error) { +func (ws Workspace) Connect(ctx context.Context) (sdk.Connection, error) { connectionConfig, err := ws.ConnectionConfig() if err != nil { return nil, err } + connection, err := connectionConfig.Connect() + if err != nil { + return nil, err + } + + err = sdk.TestConnection(ctx, connection) + if errors.Is(err, &sdk.ErrRadiusNotInstalled{}) { + return nil, clierrors.MessageWithCause(err, "Could not connect to Radius.") + } else if err != nil { + return nil, err + } + return connectionConfig.Connect() } diff --git a/pkg/ucp/config.go b/pkg/ucp/config.go index dbb6088033..b145427e16 100644 --- a/pkg/ucp/config.go +++ b/pkg/ucp/config.go @@ -126,6 +126,7 @@ type Plane struct { Properties PlaneProperties `json:"properties" yaml:"properties"` } +// PlaneProperties represents the properties of a plane resource. type PlaneProperties struct { // ResourceProviders is a map of resource provider namespaces to their respective addresses. // diff --git a/pkg/ucp/initializer/service.go b/pkg/ucp/initializer/service.go index 3d48dad2d7..7c3e3250b8 100644 --- a/pkg/ucp/initializer/service.go +++ b/pkg/ucp/initializer/service.go @@ -81,6 +81,18 @@ func waitForServer(ctx context.Context, host, port string, retryInterval time.Du func (w *Service) Run(ctx context.Context) error { logger := ucplog.FromContextOrDiscard(ctx) + manifestDir := w.options.Config.Initialization.ManifestDirectory + if manifestDir == "" { + logger.Info("No manifest directory specified, initialization is complete") + return nil + } + + if _, err := os.Stat(manifestDir); os.IsNotExist(err) { + return fmt.Errorf("manifest directory does not exist: %w", err) + } else if err != nil { + return fmt.Errorf("error checking manifest directory: %w", err) + } + if w.options.UCP == nil || w.options.UCP.Endpoint() == "" { return fmt.Errorf("connection to UCP is not set") } @@ -95,28 +107,14 @@ func (w *Service) Run(ctx context.Context) error { if err != nil { return fmt.Errorf("failed to split host and port: %w", err) } - logger.Info("Parsed Host and Port", "host", hostName, "port", port) // Attempt to connect to the server err = waitForServer(ctx, hostName, port, 500*time.Millisecond, 500*time.Millisecond, 5*time.Second) if err != nil { - logger.Error(err, "Server is not available for manifest registration") - return nil + return fmt.Errorf("failed to connect to server : %w", err) } // Server is up, proceed to register manifests - manifestDir := w.options.Config.Initialization.ManifestDirectory - if manifestDir == "" { - logger.Info("No manifest directory specified") - return nil - } - - if _, err := os.Stat(manifestDir); os.IsNotExist(err) { - return fmt.Errorf("manifest directory does not exist: %w", err) - } else if err != nil { - return fmt.Errorf("error checking manifest directory: %w", err) - } - clientOptions := sdk.NewClientOptions(w.options.UCP) clientFactory, err := v20231001preview.NewClientFactory(&aztoken.AnonymousCredential{}, clientOptions) diff --git a/test/rp/rptest.go b/test/rp/rptest.go index 21dc72d435..5a05099002 100644 --- a/test/rp/rptest.go +++ b/test/rp/rptest.go @@ -174,7 +174,7 @@ func NewRPTestOptions(t *testing.T) RPTestOptions { client, err := connections.DefaultFactory.CreateApplicationsManagementClient(ctx, *workspace) require.NoError(t, err, "failed to create ApplicationsManagementClient") - connection, err := workspace.Connect() + connection, err := workspace.Connect(ctx) require.NoError(t, err, "failed to connect to workspace") customAction, err := clientv2.NewCustomActionClient("", &clientv2.Options{