From 0af13234d5a17b6d55a44947a99984f504d48755 Mon Sep 17 00:00:00 2001 From: Matthew Barnes Date: Mon, 18 Nov 2024 08:55:10 -0500 Subject: [PATCH 1/6] database: Generalize in-memory cache iterator Works for any document type now, and just stores a slice rather than the entire cache for that document type. --- internal/database/cache.go | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/internal/database/cache.go b/internal/database/cache.go index 63b6ca620..210973f6e 100644 --- a/internal/database/cache.go +++ b/internal/database/cache.go @@ -24,14 +24,14 @@ type Cache struct { subscription map[string]*SubscriptionDocument } -type operationCacheIterator struct { - operation map[string]*OperationDocument - err error +type cacheIterator struct { + docs []any + err error } -func (iter operationCacheIterator) Items(ctx context.Context) iter.Seq[[]byte] { +func (iter cacheIterator) Items(ctx context.Context) iter.Seq[[]byte] { return func(yield func([]byte) bool) { - for _, doc := range iter.operation { + for _, doc := range iter.docs { // Marshalling the document struct only to immediately unmarshal // it back to a document struct is a little silly but this is to // conform to the DBClientIterator interface. @@ -48,7 +48,7 @@ func (iter operationCacheIterator) Items(ctx context.Context) iter.Seq[[]byte] { } } -func (iter operationCacheIterator) GetError() error { +func (iter cacheIterator) GetError() error { return iter.err } @@ -164,7 +164,11 @@ func (c *Cache) DeleteOperationDoc(ctx context.Context, operationID string) erro } func (c *Cache) ListAllOperationDocs(ctx context.Context) DBClientIterator { - return operationCacheIterator{operation: c.operation} + var iterator cacheIterator + for _, doc := range c.operation { + iterator.docs = append(iterator.docs, doc) + } + return iterator } func (c *Cache) GetSubscriptionDoc(ctx context.Context, subscriptionID string) (*SubscriptionDocument, error) { From 17513a490b570de25c807d81ef2a3f7a45f9e5ee Mon Sep 17 00:00:00 2001 From: Matthew Barnes Date: Tue, 19 Nov 2024 12:48:51 -0500 Subject: [PATCH 2/6] database: Add GetContinuationToken to DBClientIterator A new function NewQueryItemsSinglePageIterator alters the behavior of QueryItemsIterator to stop after the first "page" of items and record the Cosmos DB continuation token. The continuation token can be retrieved from the iterator with GetContinuationToken. A QueryItemsIterator created with NewQueryItemsIterator will never have a continuation token because it iterates until the last item. The in-memory cache also adds a GetContinuationToken method to its iterator implementation to fulfill the interface contract, but it always returns an empty string. --- internal/database/cache.go | 4 ++++ internal/database/database.go | 1 + internal/database/util.go | 26 ++++++++++++++++++++++++-- 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/internal/database/cache.go b/internal/database/cache.go index 210973f6e..d758752e7 100644 --- a/internal/database/cache.go +++ b/internal/database/cache.go @@ -48,6 +48,10 @@ func (iter cacheIterator) Items(ctx context.Context) iter.Seq[[]byte] { } } +func (iter cacheIterator) GetContinuationToken() string { + return "" +} + func (iter cacheIterator) GetError() error { return iter.err } diff --git a/internal/database/database.go b/internal/database/database.go index 85b7c7348..53f6f5ecd 100644 --- a/internal/database/database.go +++ b/internal/database/database.go @@ -51,6 +51,7 @@ func isResponseError(err error, statusCode int) bool { type DBClientIterator interface { Items(ctx context.Context) iter.Seq[[]byte] + GetContinuationToken() string GetError() error } diff --git a/internal/database/util.go b/internal/database/util.go index 2ccbec4e4..df8abf006 100644 --- a/internal/database/util.go +++ b/internal/database/util.go @@ -12,8 +12,10 @@ import ( ) type QueryItemsIterator struct { - pager *runtime.Pager[azcosmos.QueryItemsResponse] - err error + pager *runtime.Pager[azcosmos.QueryItemsResponse] + singlePage bool + continuationToken string + err error } // NewQueryItemsIterator is a failable push iterator for a paged query response. @@ -21,6 +23,13 @@ func NewQueryItemsIterator(pager *runtime.Pager[azcosmos.QueryItemsResponse]) Qu return QueryItemsIterator{pager: pager} } +// NewQueryItemsSinglePageIterator is a failable push iterator for a paged +// query response that stops at the end of the first page and includes a +// continuation token if additional items are available. +func NewQueryItemsSinglePageIterator(pager *runtime.Pager[azcosmos.QueryItemsResponse]) QueryItemsIterator { + return QueryItemsIterator{pager: pager, singlePage: true} +} + // Items returns a push iterator that can be used directly in for/range loops. // If an error occurs during paging, iteration stops and the error is recorded. func (iter QueryItemsIterator) Items(ctx context.Context) iter.Seq[[]byte] { @@ -31,15 +40,28 @@ func (iter QueryItemsIterator) Items(ctx context.Context) iter.Seq[[]byte] { iter.err = err return } + if iter.singlePage && response.ContinuationToken != nil { + iter.continuationToken = *response.ContinuationToken + } for _, item := range response.Items { if !yield(item) { return } } + if iter.singlePage { + return + } } } } +// GetContinuationToken returns a continuation token that can be used to obtain +// the next page of results. This is only set when the iterator was created with +// NewQueryItemsSinglePageIterator and additional items are available. +func (iter QueryItemsIterator) GetContinuationToken() string { + return iter.continuationToken +} + // GetError returns any error that occurred during iteration. Call this after the // for/range loop that calls Items() to check if iteration completed successfully. func (iter QueryItemsIterator) GetError() error { From 256c8dcde047c2727f878f75cbed755f607934bd Mon Sep 17 00:00:00 2001 From: Matthew Barnes Date: Tue, 19 Nov 2024 13:12:00 -0500 Subject: [PATCH 3/6] database: Return a DBClientIterator from ListResourceDocs For CosmosDBClient, the maxItems argument controls the type of iterator returned. A positive maxItems returns a single- page iterator with a possible continuation token, otherwise the iterator continues until the last item. Since the in-memory cache does not have continuation tokens, the maxItems argument is ignored. This also drops the resourceType argument. Callers first need to parse the iterator items into resource documents before checking the resource type. --- frontend/pkg/frontend/frontend.go | 40 ++++++++++++++--------- internal/database/cache.go | 12 +++---- internal/database/database.go | 53 +++++++++++-------------------- 3 files changed, 47 insertions(+), 58 deletions(-) diff --git a/frontend/pkg/frontend/frontend.go b/frontend/pkg/frontend/frontend.go index ea7becf46..700fe1ac4 100644 --- a/frontend/pkg/frontend/frontend.go +++ b/frontend/pkg/frontend/frontend.go @@ -192,17 +192,29 @@ func (f *Frontend) ArmResourceList(writer http.ResponseWriter, request *http.Req return } - documentList, continuationToken, err := f.dbClient.ListResourceDocs(ctx, prefix, &api.ClusterResourceType, pageSizeHint, continuationToken) - if err != nil { - f.logger.Error(err.Error()) - arm.WriteInternalServerError(writer) - return - } + iterator := f.dbClient.ListResourceDocs(ctx, prefix, pageSizeHint, continuationToken) // Build a map of cluster documents by Cluster Service cluster ID. documentMap := make(map[string]*database.ResourceDocument) - for _, doc := range documentList { - documentMap[doc.InternalID.ID()] = doc + for item := range iterator.Items(ctx) { + var doc database.ResourceDocument + + err = json.Unmarshal(item, &doc) + if err != nil { + f.logger.Error(err.Error()) + arm.WriteInternalServerError(writer) + return + } + + if strings.EqualFold(doc.Key.ResourceType.String(), api.ClusterResourceType.String()) { + documentMap[doc.InternalID.ID()] = &doc + } + } + + err = iterator.GetError() + if err != nil { + f.logger.Error(err.Error()) + arm.WriteInternalServerError(writer) } // Build a Cluster Service query that looks for @@ -240,13 +252,11 @@ func (f *Frontend) ArmResourceList(writer http.ResponseWriter, request *http.Req } } - if continuationToken != nil { - err = pagedResponse.SetNextLink(request.Referer(), *continuationToken) - if err != nil { - f.logger.Error(err.Error()) - arm.WriteInternalServerError(writer) - return - } + err = pagedResponse.SetNextLink(request.Referer(), iterator.GetContinuationToken()) + if err != nil { + f.logger.Error(err.Error()) + arm.WriteInternalServerError(writer) + return } _, err = arm.WriteJSONResponse(writer, http.StatusOK, pagedResponse) diff --git a/internal/database/cache.go b/internal/database/cache.go index d758752e7..a3679e397 100644 --- a/internal/database/cache.go +++ b/internal/database/cache.go @@ -9,8 +9,6 @@ import ( "iter" "strings" - azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" - "github.com/Azure/ARO-HCP/internal/api/arm" ) @@ -112,21 +110,19 @@ func (c *Cache) DeleteResourceDoc(ctx context.Context, resourceID *arm.ResourceI return nil } -func (c *Cache) ListResourceDocs(ctx context.Context, prefix *arm.ResourceID, resourceType *azcorearm.ResourceType, pageSizeHint int32, continuationToken *string) ([]*ResourceDocument, *string, error) { - var resourceList []*ResourceDocument +func (c *Cache) ListResourceDocs(ctx context.Context, prefix *arm.ResourceID, maxItems int32, continuationToken *string) DBClientIterator { + var iterator cacheIterator // Make sure key prefix is lowercase. prefixString := strings.ToLower(prefix.String() + "/") for key, doc := range c.resource { if strings.HasPrefix(key, prefixString) { - if resourceType == nil || strings.EqualFold(resourceType.String(), doc.Key.ResourceType.String()) { - resourceList = append(resourceList, doc) - } + iterator.docs = append(iterator.docs, doc) } } - return resourceList, nil, nil + return iterator } func (c *Cache) GetOperationDoc(ctx context.Context, operationID string) (*OperationDocument, error) { diff --git a/internal/database/database.go b/internal/database/database.go index 53f6f5ecd..f7575fdd0 100644 --- a/internal/database/database.go +++ b/internal/database/database.go @@ -13,7 +13,6 @@ import ( "strings" "github.com/Azure/azure-sdk-for-go/sdk/azcore" - azcorearm "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" "github.com/Azure/azure-sdk-for-go/sdk/data/azcosmos" "github.com/Azure/ARO-HCP/internal/api/arm" @@ -72,7 +71,7 @@ type DBClient interface { // DeleteResourceDoc deletes a ResourceDocument from the database given the resourceID // of a Microsoft.RedHatOpenShift/HcpOpenShiftClusters resource or NodePools child resource. DeleteResourceDoc(ctx context.Context, resourceID *arm.ResourceID) error - ListResourceDocs(ctx context.Context, prefix *arm.ResourceID, resourceType *azcorearm.ResourceType, pageSizeHint int32, continuationToken *string) ([]*ResourceDocument, *string, error) + ListResourceDocs(ctx context.Context, prefix *arm.ResourceID, maxItems int32, continuationToken *string) DBClientIterator GetOperationDoc(ctx context.Context, operationID string) (*OperationDocument, error) CreateOperationDoc(ctx context.Context, doc *OperationDocument) error @@ -270,13 +269,23 @@ func (d *CosmosDBClient) DeleteResourceDoc(ctx context.Context, resourceID *arm. return nil } -func (d *CosmosDBClient) ListResourceDocs(ctx context.Context, prefix *arm.ResourceID, resourceType *azcorearm.ResourceType, pageSizeHint int32, continuationToken *string) ([]*ResourceDocument, *string, error) { +// ListResourceDocs searches for resource documents that match the given resource ID prefix. +// maxItems can limit the number of items returned at once. A negative value will cause the +// returned iterator to yield all matching items. A positive value will cause the returned +// iterator to include a continuation token if additional items are available. +func (d *CosmosDBClient) ListResourceDocs(ctx context.Context, prefix *arm.ResourceID, maxItems int32, continuationToken *string) DBClientIterator { // Make sure partition key is lowercase. pk := azcosmos.NewPartitionKeyString(strings.ToLower(prefix.SubscriptionID)) + // XXX The Cosmos DB REST API gives special meaning to -1 for "x-ms-max-item-count" + // but it's not clear if it treats all negative values equivalently. The Go SDK + // passes the PageSizeHint value as provided so normalize negative values to -1 + // to be safe. + maxItems = max(maxItems, -1) + query := "SELECT * FROM c WHERE STARTSWITH(c.key, @prefix, true)" opt := azcosmos.QueryOptions{ - PageSizeHint: pageSizeHint, + PageSizeHint: maxItems, ContinuationToken: continuationToken, QueryParameters: []azcosmos.QueryParameter{ { @@ -286,39 +295,13 @@ func (d *CosmosDBClient) ListResourceDocs(ctx context.Context, prefix *arm.Resou }, } - var response azcosmos.QueryItemsResponse - resourceDocs := make([]*ResourceDocument, 0, pageSizeHint) - - // Loop until we fill the pre-allocated resourceDocs slice, - // or until we run out of items from the resources container. - for opt.PageSizeHint > 0 { - var err error - - response, err = d.resources.NewQueryItemsPager(query, pk, &opt).NextPage(ctx) - if err != nil { - return nil, nil, fmt.Errorf("failed to advance page while querying Resources container for items with a key prefix of '%s': %w", prefix, err) - } - - for _, item := range response.Items { - var doc ResourceDocument - err = json.Unmarshal(item, &doc) - if err != nil { - return nil, nil, fmt.Errorf("failed to unmarshal item while querying Resources container for items with a key prefix of '%s': %w", prefix, err) - } - if resourceType == nil || strings.EqualFold(resourceType.String(), doc.Key.ResourceType.String()) { - resourceDocs = append(resourceDocs, &doc) - } - } - - if response.ContinuationToken == nil { - break - } + pager := d.resources.NewQueryItemsPager(query, pk, &opt) - opt.PageSizeHint = int32(cap(resourceDocs) - len(resourceDocs)) - opt.ContinuationToken = response.ContinuationToken + if maxItems > 0 { + return NewQueryItemsSinglePageIterator(pager) + } else { + return NewQueryItemsIterator(pager) } - - return resourceDocs, response.ContinuationToken, nil } // GetOperationDoc retrieves the asynchronous operation document for the given From 0a734bbe02c53e6f16cb8f422464c668d31b0c6c Mon Sep 17 00:00:00 2001 From: Matthew Barnes Date: Thu, 21 Nov 2024 15:49:27 -0500 Subject: [PATCH 4/6] ocm: Add push iterator types to encapsulate OCM pagination Similar to QueryItemsIterator for Cosmos DB pagination. --- internal/ocm/iterators.go | 120 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+) create mode 100644 internal/ocm/iterators.go diff --git a/internal/ocm/iterators.go b/internal/ocm/iterators.go new file mode 100644 index 000000000..b867c0da3 --- /dev/null +++ b/internal/ocm/iterators.go @@ -0,0 +1,120 @@ +package ocm + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +import ( + "context" + "iter" + "math" + + cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1" +) + +type ClusterListIterator struct { + request *cmv1.ClustersListRequest + err error +} + +// Items returns a push iterator that can be used directly in for/range loops. +// If an error occurs during paging, iteration stops and the error is recorded. +func (iter ClusterListIterator) Items(ctx context.Context) iter.Seq[*cmv1.Cluster] { + return func(yield func(*cmv1.Cluster) bool) { + // Request can be nil to allow for mocking. + if iter.request != nil { + var page int = 0 + var count int = 0 + var total int = math.MaxInt + + for count < total { + page++ + result, err := iter.request.Page(page).SendContext(ctx) + if err != nil { + iter.err = err + return + } + + total = result.Total() + items := result.Items() + + // Safety check to prevent an infinite loop in case + // the result is somehow empty before count = total. + if items == nil || items.Empty() { + return + } + + count += items.Len() + + // XXX ClusterList.Each() lacks a boolean return to + // indicate whether iteration fully completed. + // ClusterList.Slice() may be less efficient but + // is easier to work with. + for _, item := range items.Slice() { + if !yield(item) { + return + } + } + } + } + } +} + +// GetError returns any error that occurred during iteration. Call this after the +// for/range loop that calls Items() to check if iteration completed successfully. +func (iter ClusterListIterator) GetError() error { + return iter.err +} + +type NodePoolListIterator struct { + request *cmv1.NodePoolsListRequest + err error +} + +// Items returns a push iterator that can be used directly in for/range loops. +// If an error occurs during paging, iteration stops and the error is recorded. +func (iter NodePoolListIterator) Items(ctx context.Context) iter.Seq[*cmv1.NodePool] { + return func(yield func(*cmv1.NodePool) bool) { + // Request can be nil to allow for mocking. + if iter.request != nil { + var page int = 0 + var count int = 0 + var total int = math.MaxInt + + for count < total { + page++ + result, err := iter.request.Page(page).SendContext(ctx) + if err != nil { + iter.err = err + return + } + + total = result.Total() + items := result.Items() + + // Safety check to prevent an infinite loop in case + // the result is somehow empty before count = total. + if items == nil || items.Empty() { + return + } + + count += items.Len() + + // XXX NodePoolList.Each() lacks a boolean return to + // indicate whether iteration fully completed. + // NodePoolList.Slice() may be less efficient but + // is easier to work with. + for _, item := range items.Slice() { + if !yield(item) { + return + } + } + } + } + } +} + +// GetError returns any error that occurred during iteration. Call this after the +// for/range loop that calls Items() to check if iteration completed successfully. +func (iter NodePoolListIterator) GetError() error { + return iter.err +} From 53de71c1c380253b0c2b391238b949c11cc67445 Mon Sep 17 00:00:00 2001 From: Matthew Barnes Date: Thu, 21 Nov 2024 16:21:57 -0500 Subject: [PATCH 5/6] ocm: Add ListCSClusters and ListCSNodePools methods --- internal/ocm/mock.go | 8 ++++++++ internal/ocm/ocm.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/internal/ocm/mock.go b/internal/ocm/mock.go index 2081d2517..fdcde0828 100644 --- a/internal/ocm/mock.go +++ b/internal/ocm/mock.go @@ -91,6 +91,10 @@ func (mcsc *MockClusterServiceClient) DeleteCSCluster(ctx context.Context, inter return nil } +func (mcsc *MockClusterServiceClient) ListCSClusters(searchExpression string) ClusterListIterator { + return ClusterListIterator{err: fmt.Errorf("ListCSClusters not implemented")} +} + func (mcsc *MockClusterServiceClient) GetCSNodePool(ctx context.Context, internalID InternalID) (*cmv1.NodePool, error) { nodePool, ok := mcsc.nodePools[internalID] if !ok { @@ -133,3 +137,7 @@ func (mcsc *MockClusterServiceClient) DeleteCSNodePool(ctx context.Context, inte delete(mcsc.nodePools, internalID) return nil } + +func (mcsc *MockClusterServiceClient) ListCSNodePools(clusterInternalID InternalID, searchExpression string) NodePoolListIterator { + return NodePoolListIterator{err: fmt.Errorf("ListCSClusters not implemented")} +} diff --git a/internal/ocm/ocm.go b/internal/ocm/ocm.go index 6c5dc2fea..3f7e121ac 100644 --- a/internal/ocm/ocm.go +++ b/internal/ocm/ocm.go @@ -18,10 +18,12 @@ type ClusterServiceClientSpec interface { PostCSCluster(ctx context.Context, cluster *cmv1.Cluster) (*cmv1.Cluster, error) UpdateCSCluster(ctx context.Context, internalID InternalID, cluster *cmv1.Cluster) (*cmv1.Cluster, error) DeleteCSCluster(ctx context.Context, internalID InternalID) error + ListCSClusters(searchExpression string) ClusterListIterator GetCSNodePool(ctx context.Context, internalID InternalID) (*cmv1.NodePool, error) PostCSNodePool(ctx context.Context, clusterInternalID InternalID, nodePool *cmv1.NodePool) (*cmv1.NodePool, error) UpdateCSNodePool(ctx context.Context, internalID InternalID, nodePool *cmv1.NodePool) (*cmv1.NodePool, error) DeleteCSNodePool(ctx context.Context, internalID InternalID) error + ListCSNodePools(clusterInternalID InternalID, searchExpression string) NodePoolListIterator } // Get the default set of properties for the Cluster Service @@ -153,6 +155,17 @@ func (csc *ClusterServiceClient) DeleteCSCluster(ctx context.Context, internalID return err } +// ListCSClusters prepares a GET request with the given search expression. Call Items() on +// the returned iterator in a for/range loop to execute the request and paginate over results, +// then call GetError() to check for an iteration error. +func (csc *ClusterServiceClient) ListCSClusters(searchExpression string) ClusterListIterator { + clustersListRequest := csc.Conn.ClustersMgmt().V1().Clusters().List() + if searchExpression != "" { + clustersListRequest.Search(searchExpression) + } + return ClusterListIterator{request: clustersListRequest} +} + // GetCSNodePool creates and sends a GET request to fetch a node pool from Clusters Service func (csc *ClusterServiceClient) GetCSNodePool(ctx context.Context, internalID InternalID) (*cmv1.NodePool, error) { client, ok := internalID.GetNodePoolClient(csc.Conn) @@ -213,3 +226,18 @@ func (csc *ClusterServiceClient) DeleteCSNodePool(ctx context.Context, internalI _, err := client.Delete().SendContext(ctx) return err } + +// ListCSNodePools prepares a GET request with the given search expression. Call Items() on +// the returned iterator in a for/range loop to execute the request and paginate over results, +// then call GetError() to check for an iteration error. +func (csc *ClusterServiceClient) ListCSNodePools(clusterInternalID InternalID, searchExpression string) NodePoolListIterator { + client, ok := clusterInternalID.GetClusterClient(csc.Conn) + if !ok { + return NodePoolListIterator{err: fmt.Errorf("OCM path is not a cluster: %s", clusterInternalID)} + } + nodePoolsListRequest := client.NodePools().List() + if searchExpression != "" { + nodePoolsListRequest.Search(searchExpression) + } + return NodePoolListIterator{request: nodePoolsListRequest} +} From f8b56ec949265a0e37e307531be480709cbc8b2f Mon Sep 17 00:00:00 2001 From: Matthew Barnes Date: Thu, 21 Nov 2024 13:57:30 -0500 Subject: [PATCH 6/6] frontend: Generalize ArmResourceList to also handle node pools I was surprised to discover the collection endpoint for node pools is a valid resource ID (according to ParseResourceID), whereas the collection endpoints for clusters (by subscription and by resource group) are not. This required tweaking the static validation middleware, which was blocking node pool collection requests because the parsed resource ID had no resource name. --- frontend/pkg/frontend/frontend.go | 86 ++++++++++++++----- .../frontend/middleware_resourceid_test.go | 11 +++ .../pkg/frontend/middleware_validatestatic.go | 47 +++++----- frontend/pkg/frontend/routes.go | 3 + 4 files changed, 99 insertions(+), 48 deletions(-) diff --git a/frontend/pkg/frontend/frontend.go b/frontend/pkg/frontend/frontend.go index 700fe1ac4..0aafdc974 100644 --- a/frontend/pkg/frontend/frontend.go +++ b/frontend/pkg/frontend/frontend.go @@ -13,6 +13,7 @@ import ( "net" "net/http" "os" + "path" "strconv" "strings" "sync/atomic" @@ -175,6 +176,8 @@ func (f *Frontend) ArmResourceList(writer http.ResponseWriter, request *http.Req subscriptionID := request.PathValue(PathSegmentSubscriptionID) resourceGroupName := request.PathValue(PathSegmentResourceGroupName) + resourceName := request.PathValue(PathSegmentResourceName) + resourceTypeName := path.Base(request.URL.Path) // Even though the bulk of the list content comes from Cluster Service, // we start by querying Cosmos DB because its continuation token meets @@ -185,6 +188,13 @@ func (f *Frontend) ArmResourceList(writer http.ResponseWriter, request *http.Req if resourceGroupName != "" { prefixString += "/resourceGroups/" + resourceGroupName } + if resourceName != "" { + // This is a nested resource request. Build a resource ID for + // the parent cluster. We use this below to get the cluster's + // ResourceDocument from Cosmos DB. + prefixString += "/providers/" + api.ProviderNamespace + prefixString += "/" + api.ClusterResourceTypeName + "/" + resourceName + } prefix, err := arm.ParseResourceID(prefixString) if err != nil { f.logger.Error(err.Error()) @@ -192,11 +202,11 @@ func (f *Frontend) ArmResourceList(writer http.ResponseWriter, request *http.Req return } - iterator := f.dbClient.ListResourceDocs(ctx, prefix, pageSizeHint, continuationToken) + dbIterator := f.dbClient.ListResourceDocs(ctx, prefix, pageSizeHint, continuationToken) // Build a map of cluster documents by Cluster Service cluster ID. documentMap := make(map[string]*database.ResourceDocument) - for item := range iterator.Items(ctx) { + for item := range dbIterator.Items(ctx) { var doc database.ResourceDocument err = json.Unmarshal(item, &doc) @@ -206,12 +216,14 @@ func (f *Frontend) ArmResourceList(writer http.ResponseWriter, request *http.Req return } - if strings.EqualFold(doc.Key.ResourceType.String(), api.ClusterResourceType.String()) { + // FIXME This filtering could be made part of the query expression. It would + // require some reworking (or elimination) of the DBClient interface. + if strings.HasSuffix(strings.ToLower(doc.Key.ResourceType.Type), resourceTypeName) { documentMap[doc.InternalID.ID()] = &doc } } - err = iterator.GetError() + err = dbIterator.GetError() if err != nil { f.logger.Error(err.Error()) arm.WriteInternalServerError(writer) @@ -226,33 +238,61 @@ func (f *Frontend) ArmResourceList(writer http.ResponseWriter, request *http.Req query := fmt.Sprintf("id in (%s)", strings.Join(queryIDs, ", ")) f.logger.Info(fmt.Sprintf("Searching Cluster Service for %q", query)) - listRequest := f.clusterServiceClient.GetConn().ClustersMgmt().V1().Clusters().List().Search(query) + switch resourceTypeName { + case strings.ToLower(api.ClusterResourceTypeName): + csIterator := f.clusterServiceClient.ListCSClusters(query) + + for csCluster := range csIterator.Items(ctx) { + if doc, ok := documentMap[csCluster.ID()]; ok { + value, err := marshalCSCluster(csCluster, doc, versionedInterface) + if err != nil { + f.logger.Error(err.Error()) + arm.WriteInternalServerError(writer) + return + } + pagedResponse.AddValue(value) + } + } + err = csIterator.GetError() + + case strings.ToLower(api.NodePoolResourceTypeName): + var resourceDoc *database.ResourceDocument + + // Fetch the cluster document for the Cluster Service ID. + resourceDoc, err = f.dbClient.GetResourceDoc(ctx, prefix) + if err != nil { + f.logger.Error(err.Error()) + arm.WriteInternalServerError(writer) + return + } + + csIterator := f.clusterServiceClient.ListCSNodePools(resourceDoc.InternalID, query) - // XXX This SHOULD avoid dealing with pagination from Cluster Service. - // As far I can tell, uhc-cluster-service does not impose its own - // limit on the page size. Further testing is needed to verify. - listRequest.Size(len(documentMap)) + for csNodePool := range csIterator.Items(ctx) { + if doc, ok := documentMap[csNodePool.ID()]; ok { + value, err := marshalCSNodePool(csNodePool, doc, versionedInterface) + if err != nil { + f.logger.Error(err.Error()) + arm.WriteInternalServerError(writer) + return + } + pagedResponse.AddValue(value) + } + } + err = csIterator.GetError() - listResponse, err := listRequest.SendContext(ctx) + default: + err = fmt.Errorf("unsupported resource type: %s", resourceTypeName) + } + + // Check for iteration error. if err != nil { f.logger.Error(err.Error()) arm.WriteInternalServerError(writer) return } - for _, csCluster := range listResponse.Items().Slice() { - if doc, ok := documentMap[csCluster.ID()]; ok { - value, err := marshalCSCluster(csCluster, doc, versionedInterface) - if err != nil { - f.logger.Error(err.Error()) - arm.WriteInternalServerError(writer) - return - } - pagedResponse.AddValue(value) - } - } - - err = pagedResponse.SetNextLink(request.Referer(), iterator.GetContinuationToken()) + err = pagedResponse.SetNextLink(request.Referer(), dbIterator.GetContinuationToken()) if err != nil { f.logger.Error(err.Error()) arm.WriteInternalServerError(writer) diff --git a/frontend/pkg/frontend/middleware_resourceid_test.go b/frontend/pkg/frontend/middleware_resourceid_test.go index 91aa4d202..f94b629eb 100644 --- a/frontend/pkg/frontend/middleware_resourceid_test.go +++ b/frontend/pkg/frontend/middleware_resourceid_test.go @@ -61,6 +61,17 @@ func TestMiddlewareResourceID(t *testing.T) { "Microsoft.Resources/tenants", }, }, + { + name: "node pool collection", + path: "/SUBSCRIPTIONS/00000000-0000-0000-0000-000000000000/RESOURCEGROUPS/MyResourceGroup/PROVIDERS/MICROSOFT.REDHATOPENSHIFT/HCPOPENSHIFTCLUSTERS/myCluster/NODEPOOLS", + resourceTypes: []string{ + "MICROSOFT.REDHATOPENSHIFT/HCPOPENSHIFTCLUSTERS/NODEPOOLS", + "MICROSOFT.REDHATOPENSHIFT/HCPOPENSHIFTCLUSTERS", + "Microsoft.Resources/resourceGroups", + "Microsoft.Resources/subscriptions", + "Microsoft.Resources/tenants", + }, + }, { name: "preflight deployment", path: "/SUBSCRIPTIONS/00000000-0000-0000-0000-000000000000/RESOURCEGROUPS/MyResourceGroup/PROVIDERS/MICROSOFT.REDHATOPENSHIFT/DEPLOYMENTS/preflight", diff --git a/frontend/pkg/frontend/middleware_validatestatic.go b/frontend/pkg/frontend/middleware_validatestatic.go index 4f172d898..833e0bac3 100644 --- a/frontend/pkg/frontend/middleware_validatestatic.go +++ b/frontend/pkg/frontend/middleware_validatestatic.go @@ -18,8 +18,6 @@ import ( var rxHCPOpenShiftClusterResourceName = regexp.MustCompile(`^[a-zA-Z][a-zA-Z0-9-]{2,53}$`) var rxNodePoolResourceName = regexp.MustCompile(`^[a-zA-Z][a-zA-Z0-9-]{2,14}$`) -var resourceTypeSubscription = "Microsoft.Resources/subscriptions" - func MiddlewareValidateStatic(w http.ResponseWriter, r *http.Request, next http.HandlerFunc) { // To conform with "OAPI012: Resource IDs must not be case sensitive" // we need to use the original, non-lowercased resource ID components @@ -40,29 +38,28 @@ func MiddlewareValidateStatic(w http.ResponseWriter, r *http.Request, next http. } } - // Skip static validation for subscription resources - if !strings.EqualFold(resource.ResourceType.String(), resourceTypeSubscription) { - switch strings.ToLower(resource.ResourceType.Type) { - case strings.ToLower(api.ClusterResourceType.Type): - if !rxHCPOpenShiftClusterResourceName.MatchString(resource.Name) { - arm.WriteError(w, http.StatusBadRequest, - arm.CloudErrorCodeInvalidResourceName, - resource.String(), - "The Resource '%s/%s' under resource group '%s' does not conform to the naming restriction.", - resource.ResourceType, resource.Name, - resource.ResourceGroupName) - return - } - case strings.ToLower(api.NodePoolResourceType.Type): - if !rxNodePoolResourceName.MatchString(resource.Name) { - arm.WriteError(w, http.StatusBadRequest, - arm.CloudErrorCodeInvalidResourceName, - resource.String(), - "The Resource '%s/%s' under resource group '%s' does not conform to the naming restriction.", - resource.ResourceType, resource.Name, - resource.ResourceGroupName) - return - } + switch strings.ToLower(resource.ResourceType.Type) { + case strings.ToLower(api.ClusterResourceType.Type): + if !rxHCPOpenShiftClusterResourceName.MatchString(resource.Name) { + arm.WriteError(w, http.StatusBadRequest, + arm.CloudErrorCodeInvalidResourceName, + resource.String(), + "The Resource '%s/%s' under resource group '%s' does not conform to the naming restriction.", + resource.ResourceType, resource.Name, + resource.ResourceGroupName) + return + } + case strings.ToLower(api.NodePoolResourceType.Type): + // The collection GET endpoint for nested resources + // parses into a ResourceID with an empty Name field. + if resource.Name != "" && !rxNodePoolResourceName.MatchString(resource.Name) { + arm.WriteError(w, http.StatusBadRequest, + arm.CloudErrorCodeInvalidResourceName, + resource.String(), + "The Resource '%s/%s' under resource group '%s' does not conform to the naming restriction.", + resource.ResourceType, resource.Name, + resource.ResourceGroupName) + return } } } diff --git a/frontend/pkg/frontend/routes.go b/frontend/pkg/frontend/routes.go index 70bc9e34c..371729015 100644 --- a/frontend/pkg/frontend/routes.go +++ b/frontend/pkg/frontend/routes.go @@ -68,6 +68,9 @@ func (f *Frontend) routes() *MiddlewareMux { mux.Handle( MuxPattern(http.MethodGet, PatternSubscriptions, PatternResourceGroups, PatternProviders, api.ClusterResourceTypeName), postMuxMiddleware.HandlerFunc(f.ArmResourceList)) + mux.Handle( + MuxPattern(http.MethodGet, PatternSubscriptions, PatternResourceGroups, PatternProviders, PatternClusters, api.NodePoolResourceTypeName), + postMuxMiddleware.HandlerFunc(f.ArmResourceList)) // Resource ID endpoints // Request context holds an azcorearm.ResourceID