Skip to content

Fix bug in subscriptions file save#1436

Closed
saschagrunert wants to merge 1 commit into
containers:mainfrom
saschagrunert:subscriptions-path
Closed

Fix bug in subscriptions file save#1436
saschagrunert wants to merge 1 commit into
containers:mainfrom
saschagrunert:subscriptions-path

Conversation

@saschagrunert
Copy link
Copy Markdown
Member

@saschagrunert saschagrunert commented Apr 26, 2023

We have to keep the filepath.Dir since it has been added on purpose.

Follow-up on #1421

We have to keep the `filepath.Dir` since it has been added on purpose.

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Apr 26, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: saschagrunert
Once this PR has been reviewed and has the lgtm label, please assign rhatdan for approval. For more information see the Kubernetes Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

@saschagrunert saschagrunert mentioned this pull request Apr 26, 2023
@Luap99
Copy link
Copy Markdown
Member

Luap99 commented Apr 26, 2023

I also found it just now, WDYT about:

diff --git a/pkg/subscriptions/subscriptions.go b/pkg/subscriptions/subscriptions.go
index c21f5ecc..b751f487 100644
--- a/pkg/subscriptions/subscriptions.go
+++ b/pkg/subscriptions/subscriptions.go
@@ -27,9 +27,10 @@ var (
        UserOverrideMountsFile = filepath.Join(os.Getenv("HOME"), ".config/containers/mounts.conf")
 )
 
-// subscriptionData stores the name of the file and the content read from it
+// subscriptionData stores the relative name of the file and the content read from it
 type subscriptionData struct {
-       name    string
+       // relPath is the relative path to the file
+       relPath string
        data    []byte
        mode    os.FileMode
        dirMode os.FileMode
@@ -37,10 +38,13 @@ type subscriptionData struct {
 
 // saveTo saves subscription data to given directory
 func (s subscriptionData) saveTo(dir string) error {
-       if err := umask.MkdirAllIgnoreUmask(dir, s.dirMode); err != nil {
+       // We need to join the path here and create all parent directories, only
+       // creating dir is not good enough as relPath could also contain directories.
+       path := filepath.Join(dir, s.relPath)
+       if err := umask.MkdirAllIgnoreUmask(filepath.Dir(path), s.dirMode); err != nil {
                return fmt.Errorf("create subscription directory: %w", err)
        }
-       if err := umask.WriteFileIgnoreUmask(filepath.Join(dir, s.name), s.data, s.mode); err != nil {
+       if err := umask.WriteFileIgnoreUmask(path, s.data, s.mode); err != nil {
                return fmt.Errorf("write subscription data: %w", err)
        }
        return nil
@@ -96,7 +100,7 @@ func readFileOrDir(root, name string, parentMode os.FileMode) ([]subscriptionDat
                return nil, err
        }
        return []subscriptionData{{
-               name:    name,
+               relPath: name,
                data:    bytes,
                mode:    s.Mode(),
                dirMode: parentMode,

Because name is not really a name but a relative path instead. This should prevent others from making the same mistake again.

@Luap99
Copy link
Copy Markdown
Member

Luap99 commented Apr 26, 2023

I created #1437 with my diff + added a test. I think this makes it more clear.

@saschagrunert saschagrunert deleted the subscriptions-path branch April 26, 2023 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants