chart(fix): basicAuth.embeddedUrl in GraphQL endpoint for old scaler compatible

[VietND96]

User description

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

A fix for a breaking in change #2401
Fixes #2407

Motivation and Context

If using official KEDA core <= 2.15.1 (or not using a patched version mentioned here - https://github.com/SeleniumHQ/docker-selenium/blob/trunk/.keda/README.md - which means without separate parameters username and password for basic auth).

Set basicAuth.embeddedUrl to true (default it is false) to put back the basic auth in GraphQL endpoint (e.g http://admin:[email protected]:4444/graphql). Otherwise, scaler will raise "error": "error requesting selenium grid endpoint: selenium grid returned 401"

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

Bug fix, Tests

---

Description

• Added support for basicAuth.embeddedUrl in test scripts and templates to ensure compatibility with older scalers.
• Enhanced KEDA installation and configuration logic in test scripts to handle patched and unpatched scenarios.
• Updated Makefile to include new variables TEST_PATCHED_KEDA and BASIC_AUTH_EMBEDDED_URL for chart testing.
• Refactored URL generation in templates to conditionally include basic authentication credentials.

---

Changes walkthrough 📝

	Relevant files
Tests	chart_test.sh Add basicAuth and KEDA configuration in test script            tests/charts/make/chart_test.sh Added TEST_PATCHED_KEDA and BASIC_AUTH_EMBEDDED_URL variables. Modified Helm command to include basicAuth.embeddedUrl. Enhanced KEDA installation logic with conditional checks. Refactored KEDA image setting logic. +29/-16
Bug fix	_helpers.tpl Update URL generation for basicAuth support                            charts/selenium-grid/templates/_helpers.tpl Adjusted URL generation logic for basicAuth. Ensured embedded URL is used when basicAuth.embeddedUrl is true. +3/-3
Configuration changes	Makefile Integrate basicAuth and KEDA settings in Makefile targets Makefile Added TEST_PATCHED_KEDA and BASIC_AUTH_EMBEDDED_URL to chart test targets. Updated chart test commands to reflect new configurations. +2/-2

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[qodo-merge-pro]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 Security concerns Sensitive information exposure: The changes include handling of basic authentication credentials. While the code doesn't directly expose these credentials, care should be taken to ensure that the basicAuth.embeddedUrl feature doesn't inadvertently expose credentials in logs or other outputs. The security implications of embedding credentials in URLs should be carefully considered and documented.

⚡ Key issues to review Conditional Logic The new conditional logic for KEDA installation and image setting is complex and may need careful review to ensure it works as intended for all scenarios. URL Generation The changes to the URL generation logic for basicAuth need to be verified to ensure they correctly handle all cases, especially when basicAuth.embeddedUrl is true.

[qodo-merge-pro]

PR Code Suggestions ✨

Category	Suggestion	Score
Enhancement	Simplify URL construction using a ternary operator for conditional logic Consider using a more concise conditional statement to set the URL when basicAuth.embeddedUrl is true. charts/selenium-grid/templates/_helpers.tpl [648-651] -{{- $url := printf "%s://%s%s%s" (include "seleniumGrid.server.url.schema" .) (include "seleniumGrid.server.url.host" .) (include "seleniumGrid.server.url.port" .) (include "seleniumGrid.url.subPath" .) -}} -{{- if $.Values.basicAuth.embeddedUrl -}} - {{- $url = printf "%s://%s%s%s%s" (include "seleniumGrid.server.url.schema" .) (include "seleniumGrid.url.basicAuth" .) (include "seleniumGrid.server.url.host" .) (include "seleniumGrid.server.url.port" .) (include "seleniumGrid.url.subPath" .) -}} -{{- end -}} +{{- $auth := ternary (include "seleniumGrid.url.basicAuth" .) "" $.Values.basicAuth.embeddedUrl -}} +{{- $url := printf "%s://%s%s%s%s" (include "seleniumGrid.server.url.schema" .) $auth (include "seleniumGrid.server.url.host" .) (include "seleniumGrid.server.url.port" .) (include "seleniumGrid.url.subPath" .) -}} Apply this suggestion Suggestion importance[1-10]: 9 Why: Using a ternary operator to simplify the URL construction is an effective enhancement that reduces code duplication and improves clarity, making the logic more concise and easier to understand.	9
Maintainability	Extract KEDA image setting logic into a separate function Consider extracting the KEDA image setting logic into a separate function to improve code readability and maintainability. tests/charts/make/chart_test.sh [337-342] -if [ "${TEST_PATCHED_KEDA}" = "true" ]; then - KEDA_SET_IMAGES="--set image.keda.registry=${KEDA_BASED_NAME} --set image.keda.repository=keda --set image.keda.tag=${KEDA_BASED_TAG} \ - --set image.metricsApiServer.registry=${KEDA_BASED_NAME} --set image.metricsApiServer.repository=keda-metrics-apiserver --set image.metricsApiServer.tag=${KEDA_BASED_TAG} \ - --set image.webhooks.registry=${KEDA_BASED_NAME} --set image.webhooks.repository=keda-admission-webhooks --set image.webhooks.tag=${KEDA_BASED_TAG} \ - " -fi +set_keda_images() { + if [ "${TEST_PATCHED_KEDA}" = "true" ]; then + KEDA_SET_IMAGES="--set image.keda.registry=${KEDA_BASED_NAME} --set image.keda.repository=keda --set image.keda.tag=${KEDA_BASED_TAG} \ + --set image.metricsApiServer.registry=${KEDA_BASED_NAME} --set image.metricsApiServer.repository=keda-metrics-apiserver --set image.metricsApiServer.tag=${KEDA_BASED_TAG} \ + --set image.webhooks.registry=${KEDA_BASED_NAME} --set image.webhooks.repository=keda-admission-webhooks --set image.webhooks.tag=${KEDA_BASED_TAG} \ + " + else + KEDA_SET_IMAGES="" + fi +} +set_keda_images + Apply this suggestion Suggestion importance[1-10]: 8 Why: Extracting the KEDA image setting logic into a separate function enhances code readability and maintainability by reducing complexity and improving organization.	8
	Use variables for common flag combinations to reduce duplication Consider using variables for common flag combinations to reduce duplication and improve maintainability. Makefile [922-926] -PLATFORMS=$(PLATFORMS) TEST_EXISTING_KEDA=true RELEASE_NAME=selenium CHART_ENABLE_TRACING=true TEST_PATCHED_KEDA=false \ -SECURE_CONNECTION_SERVER=true SECURE_USE_EXTERNAL_CERT=true SERVICE_TYPE_NODEPORT=true SELENIUM_GRID_PROTOCOL=https SELENIUM_GRID_HOST=$$(hostname -i) SELENIUM_GRID_PORT=31444 \ -SELENIUM_GRID_AUTOSCALING_MIN_REPLICA=1 \ -VERSION=$(TAG_VERSION) VIDEO_TAG=$(FFMPEG_TAG_VERSION)-$(BUILD_DATE) KEDA_BASED_NAME=$(KEDA_BASED_NAME) KEDA_BASED_TAG=$(KEDA_BASED_TAG) NAMESPACE=$(NAMESPACE) BINDING_VERSION=$(BINDING_VERSION) \ -TEMPLATE_OUTPUT_FILENAME="k8s_prefixSelenium_enableTracing_secureServer_externalCerts_nodePort_autoScaling_scaledObject_existingKEDA_subPath.yaml" \ +COMMON_FLAGS := PLATFORMS=$(PLATFORMS) VERSION=$(TAG_VERSION) VIDEO_TAG=$(FFMPEG_TAG_VERSION)-$(BUILD_DATE) KEDA_BASED_NAME=$(KEDA_BASED_NAME) KEDA_BASED_TAG=$(KEDA_BASED_TAG) NAMESPACE=$(NAMESPACE) BINDING_VERSION=$(BINDING_VERSION) +SECURE_FLAGS := SECURE_CONNECTION_SERVER=true SECURE_USE_EXTERNAL_CERT=true SELENIUM_GRID_PROTOCOL=https +chart_test_autoscaling_deployment: + $(COMMON_FLAGS) $(SECURE_FLAGS) \ + TEST_EXISTING_KEDA=true RELEASE_NAME=selenium CHART_ENABLE_TRACING=true TEST_PATCHED_KEDA=false \ + SERVICE_TYPE_NODEPORT=true SELENIUM_GRID_HOST=$$(hostname -i) SELENIUM_GRID_PORT=31444 \ + SELENIUM_GRID_AUTOSCALING_MIN_REPLICA=1 \ + TEMPLATE_OUTPUT_FILENAME="k8s_prefixSelenium_enableTracing_secureServer_externalCerts_nodePort_autoScaling_scaledObject_existingKEDA_subPath.yaml" \ + Apply this suggestion Suggestion importance[1-10]: 8 Why: Introducing variables for common flag combinations reduces duplication and enhances maintainability by making the Makefile cleaner and easier to modify in the future.	8
	Rename variable to better reflect its purpose Consider using a more descriptive variable name for TEST_PATCHED_KEDA. A name like USE_CUSTOM_KEDA_IMAGES might better convey its purpose. tests/charts/make/chart_test.sh [56] -TEST_PATCHED_KEDA=${TEST_PATCHED_KEDA:-"true"} +USE_CUSTOM_KEDA_IMAGES=${USE_CUSTOM_KEDA_IMAGES:-"true"} Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion to rename TEST_PATCHED_KEDA to USE_CUSTOM_KEDA_IMAGES improves code readability by making the variable's purpose clearer, which is beneficial for maintainability.	7

💡 Need additional feedback ? start a PR chat