review reconciliation

Signed-off-by: Jordan Keister <jordan@nimblewidget.com>
operator-framework · Oct 18, 2024 · ea1ea0e · ea1ea0e
1 parent dd938d5
commit ea1ea0e
Show file tree

Hide file tree

Showing 11 changed files with 70 additions and 72 deletions.
diff --git a/api/core/v1alpha1/clustercatalog_types.go b/api/core/v1alpha1/clustercatalog_types.go
@@ -147,17 +147,21 @@ type ClusterCatalogStatus struct {
 	//  type: Image
 	// +optional
 	ResolvedSource *ResolvedCatalogSource `json:"resolvedSource,omitempty"`
-	// catalogURLs contains the URLs that can be used to access the catalog.
+	// urls contains the URLs that can be used to access the catalog.
 	// +optional
-	URLs *CatalogURLs `json:"urls,omitempty"`
+	URLs *ClusterCatalogURLs `json:"urls,omitempty"`
 	// lastUnpacked represents the time when the
 	// ClusterCatalog object was last unpacked successfully.
 	// +optional
 	LastUnpacked metav1.Time `json:"lastUnpacked,omitempty"`
 }
 
-type CatalogURLs struct {
-	// base is a required cluster-internal URL from which on-cluster components can access the API endpoint for this catalog
+// ClusterCatalogURLs contains the URLs that can be used to access the catalog.
+type ClusterCatalogURLs struct {
+	// base is a required cluster-internal URL which provides API access for this ClusterCatalog.
+	// A suffix API access path can be added to retrieve catalog data for the ClusterCatalog.
+	// Currently, a 'v1' API access provides complete FBC retrival via the path "/api/v1/all", with the general form `{base}/api/v1/all`.
+	// +kubebuilder:validation:Required
 	Base string `json:"base"`
 }
 

diff --git a/api/core/v1alpha1/zz_generated.deepcopy.go b/api/core/v1alpha1/zz_generated.deepcopy.go
diff --git a/cmd/manager/main.go b/cmd/manager/main.go
@@ -258,7 +258,7 @@ func main() {
 		os.Exit(1)
 	}
 
-	localStorage = storage.LocalDir{RootDir: storeDir, RootURL: baseStorageURL}
+	localStorage = storage.LocalDirV1{RootDir: storeDir, RootURL: baseStorageURL}
 
 	// Config for the the catalogd web server
 	catalogServerConfig := serverutil.CatalogServerConfig{

diff --git a/config/base/crd/bases/olm.operatorframework.io_clustercatalogs.yaml b/config/base/crd/bases/olm.operatorframework.io_clustercatalogs.yaml
@@ -274,12 +274,14 @@ spec:
                 - message: source type 'Image' requires image field
                   rule: self.type == 'Image' && has(self.image)
               urls:
-                description: catalogURLs contains the URLs that can be used to access
-                  the catalog.
+                description: urls contains the URLs that can be used to access the
+                  catalog.
                 properties:
                   base:
-                    description: base is a required cluster-internal URL from which
-                      on-cluster components can access the API endpoint for this catalog
+                    description: |-
+                      base is a required cluster-internal URL which provides API access for this ClusterCatalog.
+                      A suffix API access path can be added to retrieve catalog data for the ClusterCatalog.
+                      Currently, a 'v1' API access provides complete FBC retrival via the path "/api/v1/all", with the general form `{base}/api/v1/all`.
                     type: string
                 required:
                 - base

diff --git a/docs/fetching-catalog-contents.md b/docs/fetching-catalog-contents.md
@@ -1,5 +1,5 @@
 # `ClusterCatalog` Interface
-`catalogd` serves catalog content via a catalog-specific, versioned HTTP(S) endpoint. Clients access catalog information via this API endpoint and a versioned reference of the desired format. Current support includes only a complete catalog download, indicated by the path "v1/all", for example if `status.urls.base` is `https://catalogd-service.olmv1-system.svc/catalogs/operatorhub/api` then `https://catalogd-service.olmv1-system.svc/catalogs/operatorhubio/api/vi/all` would receive the complete FBC for the catalog `operatorhubio`.
+`catalogd` serves catalog content via a catalog-specific, versioned HTTP(S) endpoint. Clients access catalog information via this API endpoint and a versioned reference of the desired format. Current support includes only a complete catalog download, indicated by the path "api/v1/all", for example if `status.urls.base` is `https://catalogd-service.olmv1-system.svc/catalogs/operatorhubio` then `https://catalogd-service.olmv1-system.svc/catalogs/operatorhubio/api/vi/all` would receive the complete FBC for the catalog `operatorhubio`.
 
 
 ## Response Format
@@ -80,13 +80,14 @@ For example purposes we make the following assumption:
 For local development, consider skipping TLS verification, such as `curl -k`, or reference external material
 on self-signed certificate verification.
 
-`ClusterCatalog` CRs have a status.baseURL field which identifies the catalog-specific API to access the catalog content:
+`ClusterCatalog` CRs have a `status.urls.base` field which identifies the catalog-specific API to access the catalog content:
 
 ```yaml
   status:
     .
     .
-    baseURL: https://catalogd-service.olmv1-system.svc/catalogs/operatorhubio/api/
+    urls:
+        base: https://catalogd-service.olmv1-system.svc/catalogs/operatorhubio
     resolvedSource:
       image:
         ref: quay.io/operatorhubio/catalog@sha256:e53267559addc85227c2a7901ca54b980bc900276fc24d3f4db0549cb38ecf76
@@ -96,9 +97,9 @@ on self-signed certificate verification.
 ## On cluster
 
 When making a request for the complete contents of the `operatorhubio` `ClusterCatalog` from within
-the cluster, clients would combine `status.baseURL` with the desired API service and issue an HTTP GET request for the URL.
+the cluster, clients would combine `status.urls.base` with the desired API service and issue an HTTP GET request for the URL.
 
-For example, to receive the complete catalog data for the `operatorhubio` catalog indicated above, the client would append the service point designator `v1/all`, like:
+For example, to receive the complete catalog data for the `operatorhubio` catalog indicated above, the client would append the service point designator `api/v1/all`, like:
 
 `https://catalogd-service.olmv1-system.svc/catalogs/operatorhubio/api/v1/all`.
 

diff --git a/internal/controllers/core/clustercatalog_controller.go b/internal/controllers/core/clustercatalog_controller.go
@@ -159,7 +159,7 @@ func (r *ClusterCatalogReconciler) SetupWithManager(mgr ctrl.Manager) error {
 func (r *ClusterCatalogReconciler) reconcile(ctx context.Context, catalog *v1alpha1.ClusterCatalog) (ctrl.Result, error) {
 	l := log.FromContext(ctx)
 	// Check if the catalog availability is set to disabled, if true then
-	// unset content URL, delete it from the cache and set appropriate status
+	// unset base URL, delete it from the cache and set appropriate status
 	if catalog.Spec.Availability == v1alpha1.AvailabilityDisabled {
 		// Delete the catalog from local cache
 		err := r.deleteCatalogCache(ctx, catalog)
@@ -244,10 +244,10 @@ func (r *ClusterCatalogReconciler) reconcile(ctx context.Context, catalog *v1alp
 			updateStatusProgressing(&catalog.Status, catalog.GetGeneration(), storageErr)
 			return ctrl.Result{}, storageErr
 		}
-		contentURL := r.Storage.ContentURL(catalog.Name)
+		baseURL := r.Storage.BaseURL(catalog.Name)
 
 		updateStatusProgressing(&catalog.Status, catalog.GetGeneration(), nil)
-		updateStatusServing(&catalog.Status, *unpackResult, contentURL, catalog.GetGeneration())
+		updateStatusServing(&catalog.Status, *unpackResult, baseURL, catalog.GetGeneration())
 	default:
 		panic(fmt.Sprintf("unknown unpack state %q", unpackResult.State))
 	}
@@ -271,7 +271,7 @@ func (r *ClusterCatalogReconciler) getCurrentState(catalog *v1alpha1.ClusterCata
 	// Set expected status based on what we see in the stored catalog
 	clearUnknownConditions(expectedStatus)
 	if hasStoredCatalog && r.Storage.ContentExists(catalog.Name) {
-		updateStatusServing(expectedStatus, storedCatalog.unpackResult, r.Storage.ContentURL(catalog.Name), storedCatalog.observedGeneration)
+		updateStatusServing(expectedStatus, storedCatalog.unpackResult, r.Storage.BaseURL(catalog.Name), storedCatalog.observedGeneration)
 		updateStatusProgressing(expectedStatus, storedCatalog.observedGeneration, nil)
 	}
 
@@ -323,12 +323,12 @@ func updateStatusProgressing(status *v1alpha1.ClusterCatalogStatus, generation i
 	meta.SetStatusCondition(&status.Conditions, progressingCond)
 }
 
-func updateStatusServing(status *v1alpha1.ClusterCatalogStatus, result source.Result, contentURL string, generation int64) {
+func updateStatusServing(status *v1alpha1.ClusterCatalogStatus, result source.Result, baseURL string, generation int64) {
 	status.ResolvedSource = result.ResolvedSource
 	if status.URLs == nil {
-		status.URLs = &v1alpha1.CatalogURLs{}
+		status.URLs = &v1alpha1.ClusterCatalogURLs{}
 	}
-	status.URLs.Base = contentURL
+	status.URLs.Base = baseURL
 	status.LastUnpacked = metav1.NewTime(result.UnpackTime)
 	meta.SetStatusCondition(&status.Conditions, metav1.Condition{
 		Type:               v1alpha1.TypeServing,
@@ -352,10 +352,7 @@ func updateStatusCatalogDisabled(status *v1alpha1.ClusterCatalogStatus, generati
 
 func updateStatusNotServing(status *v1alpha1.ClusterCatalogStatus, generation int64) {
 	status.ResolvedSource = nil
-	if status.URLs == nil {
-		status.URLs = &v1alpha1.CatalogURLs{}
-	}
-	status.URLs.Base = ""
+	status.URLs = nil
 	status.LastUnpacked = metav1.Time{}
 	meta.SetStatusCondition(&status.Conditions, metav1.Condition{
 		Type:               v1alpha1.TypeServing,

diff --git a/internal/controllers/core/clustercatalog_controller_test.go b/internal/controllers/core/clustercatalog_controller_test.go
@@ -70,7 +70,7 @@ func (m MockStore) Delete(_ string) error {
 	return nil
 }
 
-func (m MockStore) ContentURL(_ string) string {
+func (m MockStore) BaseURL(_ string) string {
 	return "URL"
 }
 
@@ -261,7 +261,7 @@ func TestCatalogdControllerReconcile(t *testing.T) {
 					},
 				},
 				Status: catalogdv1alpha1.ClusterCatalogStatus{
-					URLs: &catalogdv1alpha1.CatalogURLs{Base: "URL"},
+					URLs: &catalogdv1alpha1.ClusterCatalogURLs{Base: "URL"},
 					Conditions: []metav1.Condition{
 						{
 							Type:   catalogdv1alpha1.TypeServing,
@@ -393,7 +393,6 @@ func TestCatalogdControllerReconcile(t *testing.T) {
 					},
 				},
 				Status: catalogdv1alpha1.ClusterCatalogStatus{
-					URLs:         &catalogdv1alpha1.CatalogURLs{},
 					LastUnpacked: metav1.Time{},
 					ResolvedSource: &catalogdv1alpha1.ResolvedCatalogSource{
 						Type: catalogdv1alpha1.SourceTypeImage,
@@ -430,7 +429,6 @@ func TestCatalogdControllerReconcile(t *testing.T) {
 					},
 				},
 				Status: catalogdv1alpha1.ClusterCatalogStatus{
-					URLs: &catalogdv1alpha1.CatalogURLs{},
 					Conditions: []metav1.Condition{
 						{
 							Type:   catalogdv1alpha1.TypeServing,
@@ -473,7 +471,7 @@ func TestCatalogdControllerReconcile(t *testing.T) {
 					},
 				},
 				Status: catalogdv1alpha1.ClusterCatalogStatus{
-					URLs: &catalogdv1alpha1.CatalogURLs{},
+					URLs: &catalogdv1alpha1.ClusterCatalogURLs{},
 					Conditions: []metav1.Condition{
 						{
 							Type:   catalogdv1alpha1.TypeProgressing,
@@ -503,7 +501,7 @@ func TestCatalogdControllerReconcile(t *testing.T) {
 					},
 				},
 				Status: catalogdv1alpha1.ClusterCatalogStatus{
-					URLs: &catalogdv1alpha1.CatalogURLs{},
+					URLs: &catalogdv1alpha1.ClusterCatalogURLs{},
 					Conditions: []metav1.Condition{
 						{
 							Type:   catalogdv1alpha1.TypeProgressing,
@@ -573,7 +571,6 @@ func TestCatalogdControllerReconcile(t *testing.T) {
 					},
 				},
 				Status: catalogdv1alpha1.ClusterCatalogStatus{
-					URLs: &catalogdv1alpha1.CatalogURLs{},
 					Conditions: []metav1.Condition{
 						{
 							Type:   catalogdv1alpha1.TypeProgressing,
@@ -590,7 +587,7 @@ func TestCatalogdControllerReconcile(t *testing.T) {
 			},
 		},
 		{
-			name: "catalog availability set to disabled, ContentURL should get unset",
+			name: "catalog availability set to disabled, status.urls.base should get unset",
 			source: &MockSource{
 				result: &source.Result{
 					State: source.StateUnpacked,
@@ -612,7 +609,7 @@ func TestCatalogdControllerReconcile(t *testing.T) {
 					Availability: "Disabled",
 				},
 				Status: catalogdv1alpha1.ClusterCatalogStatus{
-					URLs:         &catalogdv1alpha1.CatalogURLs{Base: "URL"},
+					URLs:         &catalogdv1alpha1.ClusterCatalogURLs{Base: "URL"},
 					LastUnpacked: metav1.Time{},
 					ResolvedSource: &catalogdv1alpha1.ResolvedCatalogSource{
 						Type: catalogdv1alpha1.SourceTypeImage,
@@ -648,7 +645,6 @@ func TestCatalogdControllerReconcile(t *testing.T) {
 					Availability: "Disabled",
 				},
 				Status: catalogdv1alpha1.ClusterCatalogStatus{
-					URLs: &catalogdv1alpha1.CatalogURLs{},
 					Conditions: []metav1.Condition{
 						{
 							Type:   catalogdv1alpha1.TypeServing,
@@ -688,7 +684,7 @@ func TestCatalogdControllerReconcile(t *testing.T) {
 					Availability: "Disabled",
 				},
 				Status: catalogdv1alpha1.ClusterCatalogStatus{
-					URLs:         &catalogdv1alpha1.CatalogURLs{Base: "URL"},
+					URLs:         &catalogdv1alpha1.ClusterCatalogURLs{Base: "URL"},
 					LastUnpacked: metav1.Time{},
 					ResolvedSource: &catalogdv1alpha1.ResolvedCatalogSource{
 						Type: catalogdv1alpha1.SourceTypeImage,
@@ -725,7 +721,6 @@ func TestCatalogdControllerReconcile(t *testing.T) {
 					Availability: "Disabled",
 				},
 				Status: catalogdv1alpha1.ClusterCatalogStatus{
-					URLs: &catalogdv1alpha1.CatalogURLs{},
 					Conditions: []metav1.Condition{
 						{
 							Type:   catalogdv1alpha1.TypeServing,
@@ -848,7 +843,7 @@ func TestPollingReconcilerUnpack(t *testing.T) {
 	successfulObservedGeneration := int64(2)
 	successfulUnpackStatus := func(mods ...func(status *catalogdv1alpha1.ClusterCatalogStatus)) catalogdv1alpha1.ClusterCatalogStatus {
 		s := catalogdv1alpha1.ClusterCatalogStatus{
-			URLs: &catalogdv1alpha1.CatalogURLs{Base: "URL"},
+			URLs: &catalogdv1alpha1.ClusterCatalogURLs{Base: "URL"},
 			Conditions: []metav1.Condition{
 				{
 					Type:               catalogdv1alpha1.TypeProgressing,

diff --git a/internal/storage/localdir.go b/internal/storage/localdir.go
@@ -14,12 +14,12 @@ import (
 	"github.com/operator-framework/operator-registry/alpha/declcfg"
 )
 
-// LocalDir is a storage Instance. When Storing a new FBC contained in
+// LocalDirV1 is a storage Instance. When Storing a new FBC contained in
 // fs.FS, the content is first written to a temporary file, after which
 // it is copied to its final destination in RootDir/catalogName/. This is
 // done so that clients accessing the content stored in RootDir/catalogName have
 // atomic view of the content for a catalog.
-type LocalDir struct {
+type LocalDirV1 struct {
 	RootDir string
 	RootURL *url.URL
 }
@@ -29,7 +29,7 @@ const (
 	v1ApiData = "all"
 )
 
-func (s LocalDir) Store(ctx context.Context, catalog string, fsys fs.FS) error {
+func (s LocalDirV1) Store(ctx context.Context, catalog string, fsys fs.FS) error {
 	fbcDir := filepath.Join(s.RootDir, catalog, v1ApiPath)
 	if err := os.MkdirAll(fbcDir, 0700); err != nil {
 		return err
@@ -52,15 +52,15 @@ func (s LocalDir) Store(ctx context.Context, catalog string, fsys fs.FS) error {
 	return os.Rename(tempFile.Name(), fbcFile)
 }
 
-func (s LocalDir) Delete(catalog string) error {
+func (s LocalDirV1) Delete(catalog string) error {
 	return os.RemoveAll(filepath.Join(s.RootDir, catalog))
 }
 
-func (s LocalDir) ContentURL(catalog string) string {
-	return s.RootURL.JoinPath(catalog, "api").String()
+func (s LocalDirV1) BaseURL(catalog string) string {
+	return s.RootURL.JoinPath(catalog).String()
 }
 
-func (s LocalDir) StorageServerHandler() http.Handler {
+func (s LocalDirV1) StorageServerHandler() http.Handler {
 	mux := http.NewServeMux()
 	fsHandler := http.FileServer(http.FS(&filesOnlyFilesystem{os.DirFS(s.RootDir)}))
 	spHandler := http.StripPrefix(s.RootURL.Path, fsHandler)
@@ -74,7 +74,7 @@ func (s LocalDir) StorageServerHandler() http.Handler {
 	return mux
 }
 
-func (s LocalDir) ContentExists(catalog string) bool {
+func (s LocalDirV1) ContentExists(catalog string) bool {
 	file, err := os.Stat(filepath.Join(s.RootDir, catalog, v1ApiPath, v1ApiData))
 	if err != nil {
 		return false