Copilot commented on code in PR #809: URL: https://github.com/apache/ranger/pull/809#discussion_r2700657259
########## dev-support/ranger-docker/scripts/opensearch/ranger-opensearch-setup.sh: ########## @@ -0,0 +1,37 @@ +#!/bin/bash + +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Wait for Kerberos keytabs (enabled by default) +if [ "${KERBEROS_ENABLED}" != "false" ] +then + echo "Kerberos is enabled, waiting for keytabs..." + ${RANGER_SCRIPTS}/wait_for_keytab.sh opensearch.keytab Review Comment: The setup script only waits for opensearch.keytab but not for HTTP.keytab, even though both are needed based on the JAAS configuration. The script should also wait for HTTP.keytab to ensure both required keytabs are available before proceeding with the setup. ```suggestion ${RANGER_SCRIPTS}/wait_for_keytab.sh opensearch.keytab ${RANGER_SCRIPTS}/wait_for_keytab.sh HTTP.keytab ``` ########## dev-support/ranger-docker/scripts/opensearch/ranger-opensearch.sh: ########## @@ -0,0 +1,37 @@ +#!/bin/bash + +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +if [ ! -e ${OPENSEARCH_HOME}/.setupDone ] +then + if "${RANGER_SCRIPTS}"/ranger-opensearch-setup.sh; + then + touch "${OPENSEARCH_HOME}"/.setupDone + else + echo "OpenSearch Setup Script didn't complete proper execution." Review Comment: The setup script failure only prints an error message but does not exit or prevent the container from continuing. This could lead to OpenSearch starting with an incomplete setup. Consider adding an exit statement after the error message to prevent the container from continuing with a failed setup. ```suggestion echo "OpenSearch Setup Script didn't complete proper execution." >&2 exit 1 ``` ########## dev-support/ranger-docker/scripts/opensearch/opensearch.yml: ########## @@ -0,0 +1,49 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# OpenSearch Configuration +cluster.name: ranger-opensearch-cluster +node.name: ranger-opensearch.rangernw + +# Network settings +network.host: 0.0.0.0 +http.port: 9200 +transport.port: 9300 + +# Discovery settings +discovery.type: single-node + +# Path settings +path.data: /opt/opensearch/data +path.logs: /opt/opensearch/logs + +# Memory settings +bootstrap.memory_lock: false + +# Disable OpenSearch Security Plugin. +# This can be enabled with Ranger Plugin. +plugins.security.disabled: true + +# Allow HTTP methods +http.cors.enabled: true +http.cors.allow-origin: "*" +http.cors.allow-methods: OPTIONS, HEAD, GET, POST, PUT, DELETE Review Comment: The CORS configuration allows all origins with "*" and permits all common HTTP methods. With the security plugin disabled, this creates a significant security risk as any website could make requests to this OpenSearch instance. Consider restricting allow-origin to specific domains or removing CORS configuration if cross-origin requests are not needed in this development/testing environment. ```suggestion http.cors.allow-origin: "http://localhost"; http.cors.allow-methods: OPTIONS, HEAD, GET, POST ``` ########## dev-support/ranger-docker/docker-compose.ranger-opensearch.yml: ########## @@ -0,0 +1,40 @@ +services: + ranger-opensearch: + build: + context: . + dockerfile: Dockerfile.ranger-opensearch + args: + - RANGER_BASE_IMAGE=${RANGER_BASE_IMAGE} + - RANGER_BASE_VERSION=${RANGER_BASE_VERSION} + - OPENSEARCH_VERSION=${OPENSEARCH_VERSION} + - KERBEROS_ENABLED=${KERBEROS_ENABLED} + image: ranger-opensearch + container_name: ranger-opensearch + hostname: ranger-opensearch.rangernw + volumes: + - ./dist/keytabs/ranger-opensearch:/etc/keytabs + - opensearch-data:/opt/opensearch/data + - opensearch-logs:/opt/opensearch/logs + stdin_open: true + tty: true + networks: + - ranger + ports: + - "9200:9200" + - "9300:9300" + depends_on: + ranger-kdc: + condition: service_started + environment: + - OPENSEARCH_VERSION + - KERBEROS_ENABLED=true Review Comment: The environment variable KERBEROS_ENABLED is hardcoded to "true" here, which overrides the value from .env file and the build argument. This prevents running OpenSearch without Kerberos even if KERBEROS_ENABLED is set to false in the .env file. Consider using KERBEROS_ENABLED=${KERBEROS_ENABLED} to allow the value from .env to be passed through, consistent with the build args section where it's already parameterized. ```suggestion - KERBEROS_ENABLED=${KERBEROS_ENABLED} ``` ########## dev-support/ranger-docker/scripts/opensearch/opensearch-jaas.conf: ########## @@ -0,0 +1,18 @@ +Client { + com.sun.security.auth.module.Krb5LoginModule required + useKeyTab=true + storeKey=true + useTicketCache=false + keyTab="/etc/keytabs/opensearch.keytab" + principal="opensearch/[email protected]"; +}; + +Server { + com.sun.security.auth.module.Krb5LoginModule required + useKeyTab=true + storeKey=true + useTicketCache=false + keyTab="/etc/keytabs/opensearch.keytab" Review Comment: The Server section of the JAAS configuration uses opensearch.keytab for the HTTP principal, but based on the KDC setup, the HTTP principal should have its own keytab file at /etc/keytabs/HTTP.keytab. The keyTab path in the Server section should be changed from "/etc/keytabs/opensearch.keytab" to "/etc/keytabs/HTTP.keytab" to match the keytab created for the HTTP principal. ```suggestion keyTab="/etc/keytabs/HTTP.keytab" ``` ########## dev-support/ranger-docker/docker-compose.ranger-opensearch.yml: ########## @@ -0,0 +1,40 @@ +services: + ranger-opensearch: + build: + context: . + dockerfile: Dockerfile.ranger-opensearch + args: + - RANGER_BASE_IMAGE=${RANGER_BASE_IMAGE} + - RANGER_BASE_VERSION=${RANGER_BASE_VERSION} + - OPENSEARCH_VERSION=${OPENSEARCH_VERSION} + - KERBEROS_ENABLED=${KERBEROS_ENABLED} + image: ranger-opensearch + container_name: ranger-opensearch + hostname: ranger-opensearch.rangernw + volumes: + - ./dist/keytabs/ranger-opensearch:/etc/keytabs + - opensearch-data:/opt/opensearch/data + - opensearch-logs:/opt/opensearch/logs + stdin_open: true + tty: true + networks: + - ranger + ports: + - "9200:9200" + - "9300:9300" + depends_on: + ranger-kdc: + condition: service_started Review Comment: The OpenSearch service only depends on ranger-kdc but not on the ranger service itself. Most other services like ranger-kafka, ranger-knox, etc. depend on both ranger and ranger-kdc (or ranger-zk). If OpenSearch is intended to integrate with Ranger for authorization (as mentioned in the PR description), it should likely also depend on the ranger service to ensure Ranger is available when OpenSearch starts. ```suggestion condition: service_started ranger: condition: service_started ``` ########## dev-support/ranger-docker/scripts/opensearch/opensearch.yml: ########## @@ -0,0 +1,49 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# OpenSearch Configuration +cluster.name: ranger-opensearch-cluster +node.name: ranger-opensearch.rangernw + +# Network settings +network.host: 0.0.0.0 +http.port: 9200 +transport.port: 9300 + +# Discovery settings +discovery.type: single-node + +# Path settings +path.data: /opt/opensearch/data +path.logs: /opt/opensearch/logs + +# Memory settings +bootstrap.memory_lock: false + +# Disable OpenSearch Security Plugin. +# This can be enabled with Ranger Plugin. Review Comment: The comment states that the OpenSearch Security Plugin is disabled and can be enabled with Ranger Plugin, but there is no actual Ranger plugin installation or configuration for OpenSearch in the Dockerfile or setup scripts. This suggests incomplete implementation. Either the comment should be updated to clarify the current status, or the Ranger OpenSearch plugin installation should be added if it exists. ```suggestion # Disable OpenSearch Security Plugin. Ranger OpenSearch plugin integration # is not configured in this image. ``` ########## dev-support/ranger-docker/scripts/opensearch/ranger-opensearch.sh: ########## @@ -0,0 +1,37 @@ +#!/bin/bash + +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +if [ ! -e ${OPENSEARCH_HOME}/.setupDone ] Review Comment: The variable OPENSEARCH_HOME should be quoted in the conditional check to prevent word splitting issues if the path contains spaces or special characters. Change to: if [ ! -e "${OPENSEARCH_HOME}/.setupDone" ] ```suggestion if [ ! -e "${OPENSEARCH_HOME}/.setupDone" ] ``` ########## dev-support/ranger-docker/scripts/opensearch/opensearch.yml: ########## @@ -0,0 +1,49 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# OpenSearch Configuration +cluster.name: ranger-opensearch-cluster +node.name: ranger-opensearch.rangernw + +# Network settings +network.host: 0.0.0.0 Review Comment: Setting network.host to 0.0.0.0 binds OpenSearch to all network interfaces, which could be a security concern. For a Docker environment where the container should only be accessible through Docker networking, consider using the specific hostname or _site_ binding. This is particularly important when the security plugin is disabled. ```suggestion network.host: ranger-opensearch.rangernw ``` ########## dev-support/ranger-docker/docker-compose.ranger-opensearch.yml: ########## @@ -0,0 +1,40 @@ +services: + ranger-opensearch: + build: + context: . + dockerfile: Dockerfile.ranger-opensearch + args: + - RANGER_BASE_IMAGE=${RANGER_BASE_IMAGE} + - RANGER_BASE_VERSION=${RANGER_BASE_VERSION} + - OPENSEARCH_VERSION=${OPENSEARCH_VERSION} + - KERBEROS_ENABLED=${KERBEROS_ENABLED} + image: ranger-opensearch + container_name: ranger-opensearch + hostname: ranger-opensearch.rangernw + volumes: + - ./dist/keytabs/ranger-opensearch:/etc/keytabs + - opensearch-data:/opt/opensearch/data + - opensearch-logs:/opt/opensearch/logs + stdin_open: true + tty: true + networks: + - ranger + ports: + - "9200:9200" + - "9300:9300" + depends_on: + ranger-kdc: + condition: service_started + environment: + - OPENSEARCH_VERSION Review Comment: The OPENSEARCH_VERSION variable is passed as a build argument but is also set in the environment section of the service. In the environment section, it should be explicitly set like KERBEROS_ENABLED or removed if not needed at runtime. Compare with other services like ranger-knox where KNOX_VERSION is set explicitly in the environment section without relying on the .env file default. ```suggestion - OPENSEARCH_VERSION=${OPENSEARCH_VERSION} ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
