MGMT-15356: Ensure filenames are distinct between openshift and manifest directories. #5382

paul-maidment · 2023-07-25T17:16:44Z

In MGMT-15336 it was reported that it is possible to upload a file with the same name to the openshift and manifests directories when creating or updating a manifest.

Because of the way in which manifest files are later merged into a single directory, this leads to a scenario where the user may not be aware that a manifest uploaded to "openshift" has overwritten a manifest that was uploaded to "manifests".

To prevent any confusion, we should ensure that filenames are distinct between the two directories when attempting to create or upload a manifest.

This PR implements this functionality.

List all the issues related to this PR

What environments does this code impact?

Automation (CI, tools, etc)
Cloud
Operator Managed Deployments
[] None

How was this code tested?

assisted-test-infra environment
dev-scripts environment
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
[] No tests needed

Checklist

Title and description added to both, commit and PR.
Relevant issues have been associated (see CONTRIBUTING guide)
This change does not require a documentation update (docstring, docs, README, etc)
Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?
Is there a bug required (and linked) for this change?
Should this PR be backported?

openshift-ci-robot · 2023-07-25T17:16:47Z

openshift-ci · 2023-07-25T17:16:50Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2023-07-25T17:16:51Z

avishayt · 2023-07-26T07:25:35Z

internal/manifests/manifests.go

@@ -229,6 +234,15 @@ func (m *Manifests) UpdateClusterManifestInternal(ctx context.Context, params op
 		return nil, err
 	}

+	if srcFolder != destFolder && srcFileName != destFileName {


We need to check this if the filename changes, even if the folder name doesn't.

State before: manifests/foo openshift/bar
Rename openshift/bar to openshift/foo

I renamed the file in the same directory, but it's no longer distinct.

I agree there is a problem here, discussing in comment below as this supercedes this point somewhat

avishayt · 2023-07-26T07:28:07Z

internal/manifests/manifests.go

+	if srcFolder != destFolder && srcFileName != destFileName {
+		// We will be performing a delete of the srcFolder/srcFilename, which will result in the files being distinct across folders.
+		// so only perform this check if both the folder and filename are changing.
+		err = m.validateFileDistinct(ctx, params.ClusterID, destFolder, destFileName)


Does this break the ability to rename directories?

Let's say I want to rename openshift/foo to manifests/foo. We call m.validateFileDistinct(ctx, clusterID, "manifests", "foo"). This checks the existence of the openshift/foo object, which of course exists, so the call fails.

Agreed, this needs more thought.

srcfolder srcfilename destfolder destfilename distinctness check needed?

openshift foo manifests foo No - openshift/foo will be deleted

openshift foo openshift bar Yes, need to check for manifests/bar

openshift foo manifests bar Yes, need to check for openshift/bar

manifests foo manifests bar Yes, need to check for openshift/bar

manifests foo manifests foo No - paths will not change

So seems that the only time we need the check is when srcFilename != destFilename

Which agrees with your comment above We need to check this if the filename changes, even if the folder name doesn't.

codecov · 2023-08-09T08:30:04Z

Codecov Report

Merging #5382 (5d8c8c6) into master (4ed8229) will increase coverage by 0.02%.
Report is 30 commits behind head on master.
The diff coverage is 90.90%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5382      +/-   ##
==========================================
+ Coverage   67.67%   67.70%   +0.02%     
==========================================
  Files         226      227       +1     
  Lines       33361    33492     +131     
==========================================
+ Hits        22578    22676      +98     
- Misses       8747     8777      +30     
- Partials     2036     2039       +3

Files Changed	Coverage Δ
internal/manifests/manifests.go	`74.92% <90.90%> (+1.20%)`	⬆️

... and 18 files with indirect coverage changes

avishayt · 2023-08-10T08:02:11Z

internal/manifests/manifests.go

@@ -229,6 +234,15 @@ func (m *Manifests) UpdateClusterManifestInternal(ctx context.Context, params op
 		return nil, err
 	}

+	if srcFileName != destFileName {
+		// We will be performing a delete of the srcFolder/srcFilename, which will result in the files being distinct across folders.
+		// so only perform this check if both the folder and filename are changing.


This comment is no longer accurate

directories. In MGMT-15336 it was reported that it is possible to upload a file with the same name to the openshift and manifests directories when creating or updating a manifest. Because of the way in which manifest files are later merged into a single directory, this leads to a scenario where the user may not be aware that a manifest uploaded to "openshift" has overwritten a manifest that was uploaded to "manifests". To prevent any confusion, we should ensure that filenames are distinct between the two directories when attempting to create or upload a manifest. This PR implements this functionality.

openshift-ci · 2023-08-10T10:01:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: avishayt, paul-maidment

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [avishayt,paul-maidment]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2023-08-10T10:22:17Z

/retest-required

Remaining retests: 0 against base HEAD 59ea056 and 2 for PR HEAD 5d8c8c6 in total

paul-maidment · 2023-08-10T17:15:11Z

/retest

paul-maidment · 2023-08-10T23:37:07Z

/retest

openshift-ci-robot · 2023-08-13T06:52:42Z

/retest-required

Remaining retests: 0 against base HEAD ead6675 and 1 for PR HEAD 5d8c8c6 in total

openshift-ci-robot · 2023-08-13T16:34:14Z

/retest-required

Remaining retests: 0 against base HEAD 338964f and 0 for PR HEAD 5d8c8c6 in total

openshift-ci · 2023-08-13T20:39:57Z

@paul-maidment: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

…est (openshift#5382) directories. In MGMT-15336 it was reported that it is possible to upload a file with the same name to the openshift and manifests directories when creating or updating a manifest. Because of the way in which manifest files are later merged into a single directory, this leads to a scenario where the user may not be aware that a manifest uploaded to "openshift" has overwritten a manifest that was uploaded to "manifests". To prevent any confusion, we should ensure that filenames are distinct between the two directories when attempting to create or upload a manifest. This PR implements this functionality.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 25, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 25, 2023

openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 25, 2023

avishayt reviewed Jul 26, 2023

View reviewed changes

paul-maidment force-pushed the MGMT-15356 branch 2 times, most recently from 98367bb to b0bc248 Compare July 26, 2023 21:16

paul-maidment marked this pull request as ready for review August 9, 2023 08:04

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 9, 2023

openshift-ci bot requested review from nmagnezi and vrutkovs August 9, 2023 08:05

avishayt reviewed Aug 10, 2023

View reviewed changes

paul-maidment force-pushed the MGMT-15356 branch from b0bc248 to 5d8c8c6 Compare August 10, 2023 08:17

avishayt approved these changes Aug 10, 2023

View reviewed changes

openshift-ci bot assigned avishayt Aug 10, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2023

openshift-merge-robot merged commit 519b488 into openshift:master Aug 13, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MGMT-15356: Ensure filenames are distinct between openshift and manifest directories. #5382

MGMT-15356: Ensure filenames are distinct between openshift and manifest directories. #5382

paul-maidment commented Jul 25, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Jul 25, 2023 •

edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci bot commented Jul 25, 2023

openshift-ci-robot commented Jul 25, 2023 •

edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

avishayt Jul 26, 2023

paul-maidment Jul 26, 2023

avishayt Jul 26, 2023

paul-maidment Jul 26, 2023

paul-maidment Jul 26, 2023 •

edited

paul-maidment Jul 26, 2023

codecov bot commented Aug 9, 2023 •

edited

avishayt Aug 10, 2023

openshift-ci bot commented Aug 10, 2023

openshift-ci-robot commented Aug 10, 2023

paul-maidment commented Aug 10, 2023

paul-maidment commented Aug 10, 2023

openshift-ci-robot commented Aug 13, 2023

openshift-ci-robot commented Aug 13, 2023

openshift-ci bot commented Aug 13, 2023

srcfolder	srcfilename	destfolder	destfilename	distinctness check needed?
openshift	foo	manifests	foo	No - openshift/foo will be deleted
openshift	foo	openshift	bar	Yes, need to check for manifests/bar
openshift	foo	manifests	bar	Yes, need to check for openshift/bar
manifests	foo	manifests	bar	Yes, need to check for openshift/bar
manifests	foo	manifests	foo	No - paths will not change

MGMT-15356: Ensure filenames are distinct between openshift and manifest directories. #5382

MGMT-15356: Ensure filenames are distinct between openshift and manifest directories. #5382

Conversation

paul-maidment commented Jul 25, 2023 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Jul 25, 2023 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci bot commented Jul 25, 2023

openshift-ci-robot commented Jul 25, 2023 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

avishayt Jul 26, 2023

Choose a reason for hiding this comment

paul-maidment Jul 26, 2023

Choose a reason for hiding this comment

avishayt Jul 26, 2023

Choose a reason for hiding this comment

paul-maidment Jul 26, 2023

Choose a reason for hiding this comment

paul-maidment Jul 26, 2023 • edited

Choose a reason for hiding this comment

paul-maidment Jul 26, 2023

Choose a reason for hiding this comment

codecov bot commented Aug 9, 2023 • edited

Codecov Report

avishayt Aug 10, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Aug 10, 2023

openshift-ci-robot commented Aug 10, 2023

paul-maidment commented Aug 10, 2023

paul-maidment commented Aug 10, 2023

openshift-ci-robot commented Aug 13, 2023

openshift-ci-robot commented Aug 13, 2023

openshift-ci bot commented Aug 13, 2023

paul-maidment commented Jul 25, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Jul 25, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Jul 25, 2023 •

edited by openshift-ci bot

paul-maidment Jul 26, 2023 •

edited

codecov bot commented Aug 9, 2023 •

edited