Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

opa run: always read .tar.gz file provided in argument as a bundle #5879

Closed
anderseknert opened this issue Apr 28, 2023 · 10 comments · Fixed by #6112
Closed

opa run: always read .tar.gz file provided in argument as a bundle #5879

anderseknert opened this issue Apr 28, 2023 · 10 comments · Fixed by #6112
Assignees
Labels

Comments

@anderseknert
Copy link
Member

When running opa run -s bundle.tar.gz, OPA currently seems to extract the tar ball as a regular directory, ignoring the .manifest file included in the bundle. This has rightly caused some confusion and does not seem like it would ever be what the user wanted or intended.

We should fix this so that when a tar ball is provided in the list of files in the args, it is always read as a bundle, no matter if the bundle is local or remote.

@yogisinha
Copy link
Contributor

@anderseknert can I take this ?

@anderseknert
Copy link
Member Author

@yogisinha of course!

@yogisinha
Copy link
Contributor

Hi @anderseknert ,
when I run opa run -s bundle.tar.gz, I get the following output and don't get the "bundle loaded successfully message"

"addrs":[":8181"],"diagnostic-addrs":[],"level":"info","msg":"Initializing server. OPA is running on a public (0.0.0.0) network interface. Unless you intend to expose OPA outside of the host, binding to the localhost interface (--addr localhost:8181) is recommended. See https://www.openpolicyagent.org/docs/latest/security/#interface-binding","time":"2023-06-17T16:25:53-04:00"}

Whereas loading it from remote loads it successfully. something like this: opa run -s http://localhost:8080/bundle.tar.gz
In case of loading it from remote there are 2 Plugins getting triggered. their names are "bundle" and "discovery" (Start method in plugins/plugins.go file)
In case of local load, only "discovery" is getting triggered. So do we have to make the "bundle" plugin load too in case of local load ? Just wanted to clarify that. Thanks.

@anderseknert
Copy link
Member Author

As far as I remember, the bundle plugin only deals with remote bundles provided from configuration. I don't think you need that for this purpose. What we'll want to ensure here is that a bundle loader is used to load the .tar.gz file. As it currently stands, it seems to be loaded as a regular directory, which ignores things like .manifest files, wasm, etc

yogisinha added a commit to yogisinha/opa that referenced this issue Jul 1, 2023
yogisinha added a commit to yogisinha/opa that referenced this issue Jul 21, 2023
yogisinha added a commit to yogisinha/opa that referenced this issue Jul 24, 2023
yogisinha added a commit to yogisinha/opa that referenced this issue Jul 24, 2023
Fixes open-policy-agent#5879

Signed-off-by: Yogesh Sinha <sinhayogi@gmail.com>
yogisinha added a commit to yogisinha/opa that referenced this issue Jul 24, 2023
Fixes open-policy-agent#5879

Signed-off-by: Yogesh Sinha <sinhayogi@gmail.com>
yogisinha added a commit to yogisinha/opa that referenced this issue Jul 25, 2023
Fixes open-policy-agent#5879

Signed-off-by: Yogesh Sinha <sinhayogi@gmail.com>
yogisinha added a commit to yogisinha/opa that referenced this issue Jul 26, 2023
Fixes open-policy-agent#5879

Signed-off-by: Yogesh Sinha <sinhayogi@gmail.com>
@ashutosh-narkar
Copy link
Member

When running opa run -s bundle.tar.gz, OPA currently seems to extract the tar ball as a regular directory

So when you do opa run -s bundle.tar.gz, the Filtered method is called. This calls loadKnownTypes. That method looks like below:

switch filepath.Ext(path) {
case ".json":
    return loadJSON(path, bs, m)
case ".rego":
    return loadRego(path, bs, m, opts)
case ".yaml", ".yml":
    return loadYAML(path, bs, m)
default:
    if strings.HasSuffix(path, ".tar.gz") {
	r, err := loadBundleFile(path, bs, m)
	if err != nil {
		err = fmt.Errorf("bundle %s: %w", path, err)
	}
	return r, err
    }
}
return nil, unrecognizedFile(path)

loadBundleFile in the default case will load bundle.tar.gz as a bundle. @anderseknert @yogisinha am I missing something here?

@yogisinha
Copy link
Contributor

Hi @ashutosh-narkar , you are right for just one bundle given on cmd line.
But if you give 2 or more bundles it will fail with following error

opa run -s bundle1.tar.gz bundle2.tar.gz
error: load error: 1 error occurred during loading: bundle3.tar.gz: merge error
exit status 1

With my change in it will load it and if the cmd given in non bundle mode, it will still load all the file ending with .tar.gz in bundle mode which was the requirement for this issue.

Refer #6112

@anderseknert
Copy link
Member Author

No, even with a single argument, the behavior differs between opa run -s bundle.tar.gz and opa run -s -b bundle.tar.gz. This is described well in the issue linked in the description, which also provides a repo for reproducing.

Another user reported the same issue (or so I suspect) in the OPA Slack some week back.

@yogisinha if that's the case, that seems like a good thing to fix too! But since that wasn't the original issue, perhaps we should create one about that and link your PR to that instead?

@yogisinha
Copy link
Contributor

yogisinha commented Jul 29, 2023

Yes, @anderseknert, In #6112 these issues are fixed. I changed LoadPaths method after some discussion/clarifications with @charlieegan3 . I am waiting for him to take a look and give me feedback.

So after my change the behavior will be following. lets suppose bundle_test is a folder containing rego and data files

  1. opa run -s -b bundle1.tar.gz bundle2.tar.gz bundle_test // all will be loaded in bundle mode because we gave -b flag
  2. opa run -s bundle1.tar.gz bundle2.tar.gz bundle_test // only bundle1.tar.gz bundle2.tar.gz will be loaded in bundle mode and budnle_test will be loaded in non-bundle mode
  3. opa run bundle1.tar.gz bundle2.tar.gz bundle_test // only bundle1.tar.gz bundle2.tar.gz will be loaded in bundle mode and budnle_test will be loaded in non-bundle mode. And OPA will be in interactive mode.

@anderseknert
Copy link
Member Author

Great! I'm still on vacation for a few more days, but I'll take a look once I'm back :) Would be great to see some tests to verify the behavior in that PR.

yogisinha added a commit to yogisinha/opa that referenced this issue Jul 31, 2023
Fixes open-policy-agent#5879

Signed-off-by: Yogesh Sinha <sinhayogi@gmail.com>
@yogisinha
Copy link
Contributor

Hi @anderseknert , I have added a test method.

yogisinha added a commit to yogisinha/opa that referenced this issue Aug 8, 2023
Fixes open-policy-agent#5879

Signed-off-by: Yogesh Sinha <sinhayogi@gmail.com>
yogisinha added a commit to yogisinha/opa that referenced this issue Aug 10, 2023
…ctorings.

Fixes open-policy-agent#5879

Signed-off-by: Yogesh Sinha <sinhayogi@gmail.com>
yogisinha added a commit to yogisinha/opa that referenced this issue Aug 17, 2023
yogisinha added a commit to yogisinha/opa that referenced this issue Aug 17, 2023
Fixes open-policy-agent#5879

Signed-off-by: Yogesh Sinha <sinhayogi@gmail.com>
yogisinha added a commit to yogisinha/opa that referenced this issue Aug 17, 2023
yogisinha added a commit to yogisinha/opa that referenced this issue Aug 17, 2023
Fixes open-policy-agent#5879

Signed-off-by: Yogesh Sinha <sinhayogi@gmail.com>
yogisinha added a commit to yogisinha/opa that referenced this issue Aug 17, 2023
Fixes open-policy-agent#5879

Signed-off-by: Yogesh Sinha <sinhayogi@gmail.com>
yogisinha added a commit to yogisinha/opa that referenced this issue Aug 17, 2023
Fixes open-policy-agent#5879

Signed-off-by: Yogesh Sinha <sinhayogi@gmail.com>
yogisinha added a commit to yogisinha/opa that referenced this issue Aug 17, 2023
…ctorings.

Fixes open-policy-agent#5879

Signed-off-by: Yogesh Sinha <sinhayogi@gmail.com>
yogisinha added a commit to yogisinha/opa that referenced this issue Aug 29, 2023
Fixes open-policy-agent#5879

Signed-off-by: Yogesh Sinha <sinhayogi@gmail.com>
yogisinha added a commit to yogisinha/opa that referenced this issue Aug 30, 2023
Squashing all the commits for following issue:
Fix for the issue when OPA doesnot load tarball on cmd line as a bundle.

Fixes open-policy-agent#5879

Signed-off-by: Yogesh Sinha <sinhayogi@gmail.com>
ashutosh-narkar pushed a commit to yogisinha/opa that referenced this issue Aug 30, 2023
ashutosh-narkar pushed a commit to yogisinha/opa that referenced this issue Aug 30, 2023
Fixes open-policy-agent#5879

Signed-off-by: Yogesh Sinha <sinhayogi@gmail.com>
ashutosh-narkar pushed a commit to yogisinha/opa that referenced this issue Aug 30, 2023
ashutosh-narkar pushed a commit to yogisinha/opa that referenced this issue Aug 30, 2023
Squashing all the commits for following issue:
Fix for the issue when OPA doesnot load tarball on cmd line as a bundle.

Fixes open-policy-agent#5879

Signed-off-by: Yogesh Sinha <sinhayogi@gmail.com>
yogisinha added a commit to yogisinha/opa that referenced this issue Aug 30, 2023
Squashing all the commits for following issue:
Fix for the issue when OPA doesnot load tarball on cmd line as a bundle.

Fixes open-policy-agent#5879

Signed-off-by: Yogesh Sinha <sinhayogi@gmail.com>
ashutosh-narkar pushed a commit to yogisinha/opa that referenced this issue Aug 30, 2023
Squashing all the commits for following issue:
Fix for the issue when OPA doesnot load tarball on cmd line as a bundle.

Fixes open-policy-agent#5879

Signed-off-by: Yogesh Sinha <sinhayogi@gmail.com>
Open Policy Agent automation moved this from Backlog to Done Aug 30, 2023
ashutosh-narkar pushed a commit that referenced this issue Aug 30, 2023
Squashing all the commits for following issue:
Fix for the issue when OPA doesnot load tarball on cmd line as a bundle.

Fixes #5879

Signed-off-by: Yogesh Sinha <sinhayogi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment