Updates the folder structure for build artifacts #1749

DarshitChanpura · 2022-04-07T15:39:34Z

Description

Updates the folder structure created during build to follow the suggested structure.

Category - Maintenance

"Before"

OpenSearch
│
└─── plugins
│     │
      └─── opensearch-security
│          │   
           └─── securityconfig
|               │   
                └─── *.yml

After:

OpenSearch
│
└─── config
│     │
      └─── opensearch-security
│          │   
           └─── *.yml

Issues Resolved

Resolves Standardize file locations for supporting files #1673

Check List

Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

peternied · 2022-04-07T16:08:40Z

Looks like the classpath isn't resolving correctly:

fatal error in thread [main], exiting
java.lang.NoClassDefFoundError: io/netty/util/internal/PlatformDependent
	at org.opensearch.security.ssl.OpenSearchSecuritySSLPlugin.<clinit>(OpenSearchSecuritySSLPlugin.java:94)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:[49](https://github.com/opensearch-project/security/runs/5871493875?check_suite_focus=true#step:11:49)9)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480)
	at org.opensearch.plugins.PluginsService.loadPlugin(PluginsService.java:776)
	at org.opensearch.plugins.PluginsService.loadBundle(PluginsService.java:725)
	at org.opensearch.plugins.PluginsService.loadBundles(PluginsService.java:[52](https://github.com/opensearch-project/security/runs/5871493875?check_suite_focus=true#step:11:52)7)
	at org.opensearch.plugins.PluginsService.<init>(PluginsService.java:190)
	at org.opensearch.node.Node.<init>(Node.java:406)
	at org.opensearch.node.Node.<init>(Node.java:329)
	at org.opensearch.bootstrap.Bootstrap$5.<init>(Bootstrap.java:242)
	at org.opensearch.bootstrap.Bootstrap.setup(Bootstrap.java:242)
	at org.opensearch.bootstrap.Bootstrap.init(Bootstrap.java:412)
	at org.opensearch.bootstrap.OpenSearch.init(OpenSearch.java:178)
	at org.opensearch.bootstrap.OpenSearch.execute(OpenSearch.java:169)
	at org.opensearch.cli.EnvironmentAwareCommand.execute(EnvironmentAwareCommand.java:100)
	at org.opensearch.cli.Command.mainWithoutErrorHandling(Command.java:138)
	at org.opensearch.cli.Command.main(Command.java:101)
	at org.opensearch.bootstrap.OpenSearch.main(OpenSearch.java:135)
	at org.opensearch.bootstrap.OpenSearch.main(OpenSearch.java:101)
Caused by: java.lang.ClassNotFoundException: io.netty.util.internal.PlatformDependent
	at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:445)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:[58](https://github.com/opensearch-project/security/runs/5871493875?check_suite_focus=true#step:11:58)7)
	at java.base/java.net.FactoryURLClassLoader.loadClass(URLClassLoader.java:8[72](https://github.com/opensearch-project/security/runs/5871493875?check_suite_focus=true#step:11:72))
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:520)
	... 22 more

peternied · 2022-04-08T22:56:45Z

@DarshitChanpura ~~Lets close this pull request, it looks like the build is broken and the tools/bin issue is going to be a problem~~

[Edit 4/13] I change my mind! We should do this, but don't make the changes to the tools folder.

codecov-commenter · 2022-04-14T02:56:47Z

Codecov Report

Merging #1749 (2b789f8) into main (e213e9c) will increase coverage by 0.02%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               main    #1749      +/-   ##
============================================
+ Coverage     60.79%   60.82%   +0.02%     
  Complexity     3186     3186              
============================================
  Files           253      253              
  Lines         17931    17931              
  Branches       3204     3204              
============================================
+ Hits          10902    10906       +4     
+ Misses         5453     5451       -2     
+ Partials       1576     1574       -2

Impacted Files	Coverage Δ
...ecurity/configuration/ConfigurationRepository.java	`71.97% <0.00%> (ø)`
...opensearch/security/tools/AuditConfigMigrater.java	`0.00% <0.00%> (ø)`
...security/auditlog/sink/ExternalOpenSearchSink.java	`59.25% <0.00%> (-2.47%)`	⬇️
...earch/security/ssl/util/SSLConnectionTestUtil.java	`95.45% <0.00%> (+2.27%)`	⬆️
...urity/ssl/transport/SecuritySSLNettyTransport.java	`66.66% <0.00%> (+4.30%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e213e9c...2b789f8. Read the comment docs.

davidlago · 2022-04-14T10:02:45Z

is it intentional that the log4j change is part of this PR?

DarshitChanpura · 2022-04-14T16:42:03Z

is it intentional that the log4j change is part of this PR?

Nope...I have fixed that change now

DarshitChanpura · 2022-04-14T18:00:38Z

White-source check failed with this check: https://github.com/opensearch-project/security/pull/1749/checks?check_run_id=6027602013
should we fix it in this branch?

davidlago · 2022-04-14T18:04:15Z

I'd bump it in a separate one (did whitesource auto-create a PR for this?) and then rebase this one

peternied

@DarshitChanpura do you have a before/after layout for the plugin so we can confirm its the expected layout?

There are many more places that need adjustment, from a search in this codebase (which is hard to parse), I see some of the following:

security/tools/install_demo_configuration.sh

Line 420 in 74618eb

    
           echo $SUDO_CMD \""$OPENSEARCH_PLUGINS_DIR/opensearch-security/tools/securityadmin.sh"\" -cd \""$OPENSEARCH_PLUGINS_DIR/opensearch-security/securityconfig"\" -icl -key \""$OPENSEARCH_CONF_DIR/kirk-key.pem"\" -cert \""$OPENSEARCH_CONF_DIR/kirk.pem"\" -cacert \""$OPENSEARCH_CONF_DIR/root-ca.pem"\" -nhnv | $SUDO_CMD tee -a securityadmin_demo.sh > /dev/null

security/src/test/java/org/opensearch/security/test/helper/cluster/ClusterHelper.java

Line 76 in 54a920b

    
           System.setProperty("security.default_init.dir", new File("./securityconfig").getAbsolutePath());

Also, please create an issue in the documentation website to update all the callouts as well, search on that codebase https://github.com/opensearch-project/documentation-website/search?q=securityconfig

DarshitChanpura · 2022-04-14T18:39:45Z

@DarshitChanpura do you have a before/after layout for the plugin so we can confirm its the expected layout?

There are many more places that need adjustment, from a search in this codebase (which is hard to parse), I see some of the following:

security/tools/install_demo_configuration.sh

Line 420 in 74618eb

echo $SUDO_CMD \""$OPENSEARCH_PLUGINS_DIR/opensearch-security/tools/securityadmin.sh"\" -cd \""$OPENSEARCH_PLUGINS_DIR/opensearch-security/securityconfig"\" -icl -key \""$OPENSEARCH_CONF_DIR/kirk-key.pem"\" -cert \""$OPENSEARCH_CONF_DIR/kirk.pem"\" -cacert \""$OPENSEARCH_CONF_DIR/root-ca.pem"\" -nhnv | $SUDO_CMD tee -a securityadmin_demo.sh > /dev/null

security/src/test/java/org/opensearch/security/test/helper/cluster/ClusterHelper.java

Line 76 in 54a920b

System.setProperty("security.default_init.dir", new File("./securityconfig").getAbsolutePath());

We have a folder structure mismatch when following different installation formats:

When we install using ./opensearch-plugin install: (The "After")

OpenSearch
│
└─── config
│     │
      └─── opensearch-security
│          │   
           └─── *.yml

When we install using Dev guide: (The "Before")

OpenSearch
│
└─── plugins
│     │
      └─── opensearch-security
│          │   
           └─── config (folder renamed from `securityconfig`)
|               │   
                └─── *.yml

As you can see in the "after", all the config files moved to a new location under core's config/opensearch-security folder.

I'll update the changes according to scenario 1 and push again.

DarshitChanpura · 2022-04-14T19:34:21Z

The CVE was introduced with this PR (#1735), alongwith two other CVEs.
And whitesource didn't catch it because whitesource check didn't run: https://github.com/opensearch-project/security/runs/5959469645

We should create a PR to fix all 3

peternied · 2022-04-14T20:25:38Z

The CVE was introduced with this PR( #1735), alongwith two other CVEs. And whitesource didn't catch it because whitesource check didn't run: https://github.com/opensearch-project/security/runs/5959469645

We should create a PR to fix all 3

We can do that out of band of this PR, since this PR did not add any new dependencies. We don't need to block this change if we are following up separately.

If this PR did add the items that were flagged by whitesource then it would be a case to prevent the PR from merging until it was resolved.

…vant changes Signed-off-by: Darshit Chanpura <[email protected]>

davidlago · 2022-04-15T12:51:40Z

I agree with @peternied, let's go ahead and merge... I just saw the auto-opened issue by whitesource #1765

DarshitChanpura · 2022-04-15T15:17:57Z

@cliu123 can I get a second review on this?

…vant changes (opensearch-project#1749) Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura requested a review from a team April 7, 2022 15:39

DarshitChanpura marked this pull request as draft April 7, 2022 15:41

DarshitChanpura marked this pull request as ready for review April 14, 2022 02:33

DarshitChanpura force-pushed the standardize-file-locations branch from 707eb1a to cc3b8c3 Compare April 14, 2022 16:41

peternied requested changes Apr 14, 2022

View reviewed changes

DarshitChanpura force-pushed the standardize-file-locations branch from cc3b8c3 to 7b7c6e5 Compare April 15, 2022 01:42

Renames securityconfig folder to config in bundle step and makes rele…

2b789f8

…vant changes Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura force-pushed the standardize-file-locations branch from 7b7c6e5 to 2b789f8 Compare April 15, 2022 02:11

davidlago approved these changes Apr 15, 2022

View reviewed changes

cliu123 approved these changes Apr 15, 2022

View reviewed changes

DarshitChanpura merged commit b44d7ea into opensearch-project:main Apr 15, 2022

davidlago mentioned this pull request Apr 19, 2022

Update PA directories from plugins to root opensearch-project/opensearch-build#1987

Merged

1 task

minhthong582000 mentioned this pull request Sep 26, 2022

[BUG][Security Plugin Configuration] opensearch-project/ansible-playbook#80

Closed

wuychn pushed a commit to ochprince/security that referenced this pull request Mar 16, 2023

Renames securityconfig folder to config in bundle step and makes rele…

7e08236

…vant changes (opensearch-project#1749) Signed-off-by: Darshit Chanpura <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates the folder structure for build artifacts #1749

Updates the folder structure for build artifacts #1749

DarshitChanpura commented Apr 7, 2022 •

edited

Loading

peternied commented Apr 7, 2022

peternied commented Apr 8, 2022 •

edited

Loading

codecov-commenter commented Apr 14, 2022 •

edited

Loading

davidlago commented Apr 14, 2022

DarshitChanpura commented Apr 14, 2022

DarshitChanpura commented Apr 14, 2022

davidlago commented Apr 14, 2022

peternied left a comment •

edited

Loading

DarshitChanpura commented Apr 14, 2022 •

edited

Loading

DarshitChanpura commented Apr 14, 2022 •

edited

Loading

peternied commented Apr 14, 2022 •

edited by DarshitChanpura

Loading

davidlago commented Apr 15, 2022

DarshitChanpura commented Apr 15, 2022

Updates the folder structure for build artifacts #1749

Updates the folder structure for build artifacts #1749

Conversation

DarshitChanpura commented Apr 7, 2022 • edited Loading

Description

Issues Resolved

Check List

peternied commented Apr 7, 2022

peternied commented Apr 8, 2022 • edited Loading

codecov-commenter commented Apr 14, 2022 • edited Loading

Codecov Report

davidlago commented Apr 14, 2022

DarshitChanpura commented Apr 14, 2022

DarshitChanpura commented Apr 14, 2022

davidlago commented Apr 14, 2022

peternied left a comment • edited Loading

Choose a reason for hiding this comment

DarshitChanpura commented Apr 14, 2022 • edited Loading

DarshitChanpura commented Apr 14, 2022 • edited Loading

peternied commented Apr 14, 2022 • edited by DarshitChanpura Loading

davidlago commented Apr 15, 2022

DarshitChanpura commented Apr 15, 2022

DarshitChanpura commented Apr 7, 2022 •

edited

Loading

peternied commented Apr 8, 2022 •

edited

Loading

codecov-commenter commented Apr 14, 2022 •

edited

Loading

peternied left a comment •

edited

Loading

DarshitChanpura commented Apr 14, 2022 •

edited

Loading

DarshitChanpura commented Apr 14, 2022 •

edited

Loading

peternied commented Apr 14, 2022 •

edited by DarshitChanpura

Loading