Prevent edit, clone, and upgrade of placeholder package revisions (#487)

* Issue #994 - prevent edit, clone, and upgrade of placeholder package revisions

- validate clone, edit, and upgrade operations and reject if they use
  placeholder/main package revisions as source/upstream
  - placeholder package revision detection:
    revision == -1 && workspaceName == repo branch
- verify in e2e tests

https://github.com/nephio-project/nephio/issues/994

* Add points in relevant guide docs for create-with-placeholder validation failures

* lint fixes

* Coverage improvements

* Incorporate dosubot's documentation suggestions

* Address Copilot comments

* nitpicky refactor to retrigger tests

* address review comments

* Additional comment fixes + coverage increases

* coverage increases

* Add pkgerrors.Wrap'ing according to comment

Author: JamesMcDermott

controllers/packagevariants/pkg/controllers/packagevariant/packagevariant_controller_test.go

@@ -1615,19 +1615,19 @@ func TestIsValidUpstram(t *testing.T) {
 }
 
 func TestGetUpstreamPr(t *testing.T) {
-	pvReconcier := PackageVariantReconciler{}
+	pvReconciler := PackageVariantReconciler{}
 	upstream := api.Upstream{}
 	prList := porchapi.PackageRevisionList{}
 
-	_, err := pvReconcier.getUpstreamPR(&upstream, &prList)
+	_, err := pvReconciler.getUpstreamPR(&upstream, &prList)
 	assert.True(t, strings.HasPrefix(err.Error(), "could not find upstream package revision"))
 
 	upstream.Repo = "my-repo"
 	upstream.Package = "my-package"
 	upstream.WorkspaceName = "my-workspace"
 
 	prList.Items = append(prList.Items, porchapi.PackageRevision{})
-	_, err = pvReconcier.getUpstreamPR(&upstream, &prList)
+	_, err = pvReconciler.getUpstreamPR(&upstream, &prList)
 	assert.True(t, strings.HasPrefix(err.Error(), "could not find upstream package revision"))
 
 	prList.Items = append(prList.Items, porchapi.PackageRevision{
@@ -1638,7 +1638,7 @@ func TestGetUpstreamPr(t *testing.T) {
 			Revision:       1,
 		},
 	})
-	_, err = pvReconcier.getUpstreamPR(&upstream, &prList)
+	_, err = pvReconciler.getUpstreamPR(&upstream, &prList)
 	assert.True(t, strings.HasPrefix(err.Error(), "could not find upstream package revision"))
 
 	prList.Items = append(prList.Items, porchapi.PackageRevision{
@@ -1649,10 +1649,10 @@ func TestGetUpstreamPr(t *testing.T) {
 			Revision:       1,
 		},
 	})
-	_, err = pvReconcier.getUpstreamPR(&upstream, &prList)
+	_, err = pvReconciler.getUpstreamPR(&upstream, &prList)
 	assert.True(t, strings.HasPrefix(err.Error(), "could not find upstream package revision"))
 
-	_, err = pvReconcier.getUpstreamPR(&upstream, &prList)
+	_, err = pvReconciler.getUpstreamPR(&upstream, &prList)
 	assert.True(t, strings.HasPrefix(err.Error(), "could not find upstream package revision"))
 	prList.Items = append(prList.Items, porchapi.PackageRevision{
 		Spec: porchapi.PackageRevisionSpec{
@@ -1662,14 +1662,14 @@ func TestGetUpstreamPr(t *testing.T) {
 			Revision:       1,
 		},
 	})
-	_, err = pvReconcier.getUpstreamPR(&upstream, &prList)
+	_, err = pvReconciler.getUpstreamPR(&upstream, &prList)
 	assert.True(t, err == nil)
 
 	upstream.WorkspaceName = ""
-	_, err = pvReconcier.getUpstreamPR(&upstream, &prList)
+	_, err = pvReconciler.getUpstreamPR(&upstream, &prList)
 	assert.True(t, strings.HasPrefix(err.Error(), "could not find upstream package revision"))
 
-	_, err = pvReconcier.getUpstreamPR(&upstream, &prList)
+	_, err = pvReconciler.getUpstreamPR(&upstream, &prList)
 	assert.True(t, strings.HasPrefix(err.Error(), "could not find upstream package revision"))
 
 	prList.Items = append(prList.Items, porchapi.PackageRevision{
@@ -1679,11 +1679,11 @@ func TestGetUpstreamPr(t *testing.T) {
 			WorkspaceName:  "main",
 		},
 	})
-	_, err = pvReconcier.getUpstreamPR(&upstream, &prList)
+	_, err = pvReconciler.getUpstreamPR(&upstream, &prList)
 	assert.True(t, err == nil)
 
 	upstream.Revision = -1
-	_, err = pvReconcier.getUpstreamPR(&upstream, &prList)
+	_, err = pvReconciler.getUpstreamPR(&upstream, &prList)
 	assert.True(t, err == nil)
 
 	upstream.Repo = "my-repo2"
@@ -1713,7 +1713,7 @@ func TestGetUpstreamPr(t *testing.T) {
 			Revision:       -1,
 		},
 	})
-	_, err = pvReconcier.getUpstreamPR(&upstream, &prList)
+	_, err = pvReconciler.getUpstreamPR(&upstream, &prList)
 	assert.True(t, err != nil)
 
 	prList.Items = append(prList.Items, porchapi.PackageRevision{
@@ -1724,7 +1724,7 @@ func TestGetUpstreamPr(t *testing.T) {
 			Revision:       -1,
 		},
 	})
-	_, err = pvReconcier.getUpstreamPR(&upstream, &prList)
+	_, err = pvReconciler.getUpstreamPR(&upstream, &prList)
 	assert.True(t, err == nil)
 }
 

docs/content/en/docs/4_tutorials_and_how-tos/working_with_package_revisions/_index.md

@@ -163,6 +163,13 @@ Common issues when working with PackageRevisions and their solutions:
 - Verify service account has proper roles for PackageRevision operations
 - Ensure repository authentication is configured correctly
 
+**Errors about "placeholder package revision"?**
+
+- Clone, edit/copy, and upgrade operations cannot be performed on or using placeholder PackageRevisions
+  - (identified by revision number -1 and workspace name matching the repository's Git branch, typically 'main')
+- Use published PackageRevisions with specific version numbers in these operations
+- For more information on placeholder PackageRevisions, see [Core Concepts]({{% relref "/docs/2_concepts#core-concepts" %}})
+
 **Pipeline functions failing?**
 
 - Check function image availability and version

docs/content/en/docs/4_tutorials_and_how-tos/working_with_package_revisions/cloning-packages.md

@@ -269,6 +269,12 @@ Common issues when cloning PackageRevisions and how to resolve them.
 - Check the exact name including repository, package, and workspace
 - Ensure you have permission to read the source PackageRevision
 
+**Clone fails with "upstream revision may not be the placeholder package revision":**
+
+- Using the upstream package's placeholder PackageRevision as the upstream PackageRevision is not supported
+- See [Core Concepts]({{% relref "/docs/2_concepts#core-concepts" %}}) for the rules which identify a placeholder PackageRevision
+- Choose a different PackageRevision from the same package to use as the upstream PackageRevision
+
 **Clone fails with "workspace already exists"?**
 
 - The workspace name must be unique within the package in the target repository

docs/content/en/docs/4_tutorials_and_how-tos/working_with_package_revisions/copying-packages.md

@@ -272,6 +272,12 @@ Common issues when copying PackageRevisions and how to resolve them.
 - Ensure you have permission to read the source PackageRevision
 - Ensure the source is in the same repository (copy only works within the same repository)
 
+**Copy fails with "source revision may not be the placeholder package revision":**
+
+- Using the package's placeholder PackageRevision as the source PackageRevision is not supported
+- See [Core Concepts]({{% relref "/docs/2_concepts#core-concepts" %}}) for the rules which identify a placeholder PackageRevision
+- Choose a different PackageRevision from the same package to use as the source PackageRevision
+
 **Copied PackageRevision has unexpected content:**
 
 - The copy includes all resources from the source at the time of copying

docs/content/en/docs/4_tutorials_and_how-tos/working_with_package_revisions/upgrading-packages.md

@@ -329,6 +329,7 @@ For more details, run `porchctl rpkg upgrade --help`.
 
 *   **Separate Repositories:** For better organization and access control, keep blueprint packages and deployment packages in separate Git repositories.
 *   **Understand Your Strategy:** Before upgrading, be certain which merge strategy fits your use case to avoid accidentally losing important local customizations. When in doubt, the default `resource-merge` is the safest and most intelligent option.
+*   **Use Fixed Revisions:** Do not upgrade packages' placeholder/main package revisions or attempt to use them as upstream for an upgrade - this is not supported. Select defined package revisions instead - see [Core Concepts]({{% relref "/docs/2_concepts#core-concepts" %}}) for the rules which identify a placeholder package revision
 
 ### Cleanup
 

docs/content/en/docs/5_architecture_and_components/engine/functionality/validation-business-rules.md

@@ -209,9 +209,7 @@ CreatePackageRevision
 
 ## Clone Task Validation
 
-The Engine validates clone tasks to prevent creating duplicate packages:
-
-### Clone Constraint Check
+The Engine validates clone tasks to prevent creating invalid packages:
 
 ```
 Clone Task Validation
@@ -226,9 +224,19 @@ Clone Task Validation
         ↓
   Continue
         ↓
+  Resolve Repository Containing Proposed Upstream
+        ↓
+  Upstream is Placeholder? ──Yes──> Reject
+        │
+        No
+        ↓
   Allow Clone
 ```
 
+### Clone Constraint Check: Package Uniqueness In Repository
+
+The package name must be unique in the repository to avoid creating duplicate packages.
+
 **Validation process:**
 1. **List all revisions** in the repository
 2. **Check for existing package** with same name
@@ -243,6 +251,24 @@ Clone Task Validation
 - Existing packages should use `edit` or `copy` for new revisions
 - Prevents accidental overwriting of existing packages
 
+### Clone Constraint Check: Exclude Placeholder Package Revision
+
+The upstream package revision cannot be a placeholder package revision (identified by `Revision == -1` and `WorkspaceName` matching the Git repository's branch)
+
+**Validation process:**
+1. **Resolve repository details** for the upstream package revision
+2. **Check if placeholder** (revision == -1 and WorkspaceName matches repo Git branch)
+3. **Reject if placeholder** package revision
+4. **Allow if not placeholder**
+
+**Error message:**
+- "upstream revision may not be the placeholder package revision {repo}/{name}"
+
+**Rationale:**
+- Placeholder package revisions represent the main/branch-HEAD state
+      - they are not fixed revisions, making them unsuitable for cloning
+- Operations would fail in later lifecycle stages
+
 ### Clone vs Edit/Copy
 
 **When to use clone:**
@@ -255,9 +281,72 @@ Clone Task Validation
 - Package already exists in repository
 - Iterating on existing package
 
+## Edit Task Validation
+
+The Engine validates edit tasks to ensure source revisions meet requirements:
+
+```
+Edit Task Validation
+        ↓
+  Fetch Source Revision
+        ↓
+  Check Same Package? ──No──> Reject
+        │
+       Yes
+        ↓
+  Check if Placeholder? ──Yes──> Reject
+        │
+        No
+        ↓
+  Check Published? ──No──> Reject
+        │
+       Yes
+        ↓
+  Allow Edit
+```
+
+### Edit Constraint Check: Source Package Revision Validation
+
+The new package revision must be from the same package as the source package revision since we are iterating on the existing package.
+
+**Validation process:**
+1. **Fetch source revision** from specified reference
+2. **Verify same package** (same repository and package name)
+3. **Check if published** (only published revisions can be edited)
+4. **Allow if all checks pass**
+
+**Error messages:**
+- "source revision must be from same package {repo}/{package}"
+- "source revision must be published"
+
+**Rationale:**
+- Edit creates new revisions from existing packages in the same repository
+- Placeholder package revisions represent unstable main branch state
+- Only published revisions provide stable source for editing
+
+### Edit Constraint Check: Exclude Placeholder Package Revision
+
+The source package revision cannot be a placeholder package revision (identified by `Revision == -1` and `WorkspaceName` matching the Git repository's branch)
+
+
+**Validation process:**
+1. **Fetch source revision** from specified reference
+2. **Resolve repository details** for the source package revision
+3. **Check if placeholder** (Revision == -1 and WorkspaceName matches repo Git branch)
+4. **Reject if placeholder** package revision
+5. **Allow if not placeholder**
+
+**Error message:**
+- "source revision may not be the placeholder package revision {repo}/{name}"
+
+**Rationale:**
+- Placeholder package revisions are not fixed revisions
+- Editing from unstable main/branch-HEAD state is not supported
+- Prevents creating revisions from non-deterministic sources
+
 ## Upgrade Task Validation
 
-The Engine validates upgrade tasks to ensure source revisions are published:
+The Engine validates upgrade tasks to ensure the three source revisions meet requirements:
 
 ### Upgrade Source Validation
 
@@ -280,6 +369,18 @@ Upgrade Task Validation
         ↓
   Continue
         ↓
+  Resolve Repository Containing Proposed New Upstream
+        ↓
+  Upstream is Placeholder? ──Yes──> Reject
+        │
+        No
+        ↓
+  Resolve Repository Containing Local Revision
+        ↓
+  Local Revision is Placeholder? ──Yes──> Reject
+        │
+        No
+        ↓
   Allow Upgrade
 ```
 
@@ -304,15 +405,42 @@ Upgrade Task Validation
 ### Upgrade Source Requirements
 
 **Required sources:**
-- **OldUpstream**: The upstream version currently used
-- **NewUpstream**: The upstream version to upgrade to
+- **OldUpstream**: The package's current upstream version
+- **NewUpstream**: The upstream version to which to upgrade
 - **LocalPackageRevision**: The local package revision to upgrade
 
 **All sources must be:**
 - Published lifecycle state
 - Accessible in repository
 - Valid package revisions
 
+### Placeholder Package Revision Check
+
+Neither the target upstream package revision (the new revision being upgraded to) nor the package revision specified for upgrade can be a placeholder package revision (identified by `Revision == -1` and `WorkspaceName` matching the repository's Git branch)
+
+**Validation process:**
+1. **Fetch target upstream revision** (NewUpstream)
+2. **Resolve repository details** for the target upstream package revision
+3. **Check if placeholder** (revision == -1 and workspaceName matches repo Git branch)
+4. **Reject if target upstream is placeholder**
+5. **Fetch local package revision** to be upgraded
+6. **Resolve repository details** for the local package revision
+7. **Check if placeholder** (revision == -1 and workspaceName matches repo Git branch)
+8. **Reject if local revision is placeholder**
+9. **Allow if neither is placeholder**
+
+**Error messages:**
+- "target upstream revision may not be the placeholder package revision {repo}/{name}"
+- "the placeholder package revision {repo}/{name} may not be upgraded"
+
+**Rationale:**
+- Upgrade performs three-way merge requiring stable source revisions
+- Placeholder package revisions represent unstable main branch state
+- Using placeholder revisions would produce non-deterministic upgrade results
+- Prevents upgrading to non-fixed revision states
+      - Validation on clone and edit operations precludes possibility of old upstream being a placeholder
+
+
 ## Package Path Overlap Validation
 
 The Engine validates package paths to prevent nested packages:

pkg/repository/util.go

@@ -15,13 +15,16 @@
 package repository
 
 import (
+	"context"
 	"fmt"
 	"strconv"
 	"strings"
 
 	kptfilev1 "github.com/kptdev/kpt/pkg/api/kptfile/v1"
 	porchapi "github.com/nephio-project/porch/api/porch/v1alpha1"
+	configapi "github.com/nephio-project/porch/api/porchconfig/v1alpha1"
 	"github.com/nephio-project/porch/pkg/util"
+	pkgerrors "github.com/pkg/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -188,3 +191,21 @@ func PathsOverlap(path1, path2 string) bool {
 	}
 	return false
 }
+
+func PackageRevisionIsPlaceholder(ctx context.Context, namespace string, referenceResolver ReferenceResolver, packageRevision PackageRevision) (bool, error) {
+	if packageRevision.Key().Revision != -1 {
+		return false, nil
+	}
+
+	var upstreamRepo configapi.Repository
+	err := referenceResolver.ResolveReference(ctx, namespace, packageRevision.Key().RKey().Name, &upstreamRepo)
+	if err != nil {
+		return false, pkgerrors.Wrapf(err, "failed to resolve repository reference for %q when checking placeholder revision: %v", packageRevision.Key().RKey().Name, err)
+	}
+
+	if upstreamRepo.Spec.Git != nil && packageRevision.Key().WorkspaceName == upstreamRepo.Spec.Git.Branch {
+		return true, nil
+	}
+
+	return false, nil
+}