proxy without mgrpxy for Kubernetes#11421
Conversation
|
👋 Hello! Thanks for contributing to our project. You can see the progress at the end of this page and at https://github.com/uyuni-project/uyuni/pull/11421/checks If you are unsure the failing tests are related to your code, you can check the "reference jobs". These are jobs that run on a scheduled time with code from master. If they fail for the same reason as your build, it means the tests or the infrastructure are broken. If they do not fail, but yours do, it means it is related to your code. Reference tests: KNOWN ISSUES Sometimes the build can fail when pulling new jar files from download.opensuse.org . This is a known limitation. Given this happens rarely, when it does, all you need to do is rerun the test. Sorry for the inconvenience. For more tips on troubleshooting, see the troubleshooting guide. Happy hacking! |
|
PR based on #11408 |
f32e65a to
7d4d32b
Compare
81370ce to
1efa0e2
Compare
0ccdeb5 to
c5f93c9
Compare
64de591 to
e1d2e47
Compare
The server running on Kubernetes knows nothing about TSL certificates. Proxy configuration won't generate any certificate in such a case.
In order to deploy to the server running on Kubernetes, the maven/deploy.sh script need adjustments: * properly handle commands as arrays * add the executor command and parameters for kubectl The helm chart also required having a unique component for the server pods to make it easy to lookup using label selector without getting the setup pod too.
A few changes are needed in order to deploy the proxy helm chart on RKE2 with selinux enabled and externally-managed TLS certificates: * In order to call the proxy helm chart as a subchart in another one, the configuration files generated by the server are now expected as global values. * The subPath mounts are leading to selinux denials: only folders can be mounted. To work this around, the config and secrets are all mounted in /etc/uyuni using projected volumes. The files to mount on folders that are not empty need an initContainer trick to copy the existing content over, otherwise it would have been hidden. * The TLS CA and certificate are expected in separate configmap and secret mounted directly in the container. The uyuni-configure code takes care of this case and will be simplified later on. * Having the server_version in the config doesn't make sense: it is now extracted directly from the server API as it doesn't require authentication.
These files date from the very first introduction of the containers and are no longer used or valid.
ee429ef to
992e13a
Compare
There was a problem hiding this comment.
Pull request overview
This pull request modifies the Uyuni proxy configuration system to support deployment on Kubernetes without SSL certificate management by mgr-proxy. The changes introduce a "no SSL" mode in the UI for servers running on Kubernetes, while maintaining the existing functionality for Podman deployments. Additionally, the PR refactors TFTP handling, renaming proxy-tftpd-image to shared-tftpd-image for reuse across server and proxy components, and significantly overhauls the proxy Helm chart structure.
Changes:
- Added a "Skip SSL configuration" option in the proxy container configuration UI that appears only when the server is running on Kubernetes (determined by
ConfigDefaults.isSsl()) - Backend Java code conditionally generates SSL certificates and configuration based on the SSL mode, supporting
no-ssl,create-ssl, anduse-sslmodes - Renamed and refactored TFTP container image from
proxy-tftpd-imagetoshared-tftpd-imagefor use by both server and proxy deployments - Completely restructured proxy Helm chart with improved configuration management, separate TFTP deployment, and better Kubernetes resource organization
- Enhanced
deploy.shscript with kubectl mode support for deploying to Kubernetes clusters - Removed TFTP-related environment variables and configuration from server setup scripts
Reviewed changes
Copilot reviewed 62 out of 65 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| web/html/src/manager/proxy/container-config.tsx | Added noSSL prop to conditionally show SSL configuration options and set appropriate default SSL mode |
| web/html/src/manager/proxy/container-config.renderer.tsx | Passes noSSL parameter from backend to React component |
| java/core/src/main/java/com/suse/manager/webui/controllers/ProxyController.java | Determines if server is running without SSL using ConfigDefaults.isSsl() |
| java/core/src/main/java/com/suse/manager/webui/utils/gson/ProxyContainerConfigJson.java | Added validation for NO_SSL mode |
| java/core/src/main/java/com/redhat/rhn/manager/system/proxycontainerconfig/*.java | Conditionally generates SSL certificates and configuration based on SSL availability |
| containers/shared-tftpd-image/* | Renamed from proxy-tftpd-image; updated configuration script to conditionally handle CA certificates |
| containers/proxy-helm/* | Major restructuring with new values schema, separate TFTP deployment, improved service definitions, and comprehensive unit tests |
| containers/server-helm/* | Added TFTP deployment configuration with hostNetwork option |
| java/scripts/maven/deploy.sh | Added kubectl deployment mode with namespace support |
| testsuite/* | Removed TFTP service checks and environment variable from server setup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| deploy_execute mv "${TARGET_DIR}/WEB-INF/lib/branding-${BRANDING_VERSION}.jar" /usr/share/rhn/lib/java-branding.jar | ||
| deploy_execute ln -sf /usr/share/rhn/lib/java-branding.jar ${TARGET_DIR}/WEB-INF/lib/java-branding.jar | ||
|
|
||
| print "Linking rhn jar..." | ||
| deploy_execute "mv ${TARGET_DIR}/WEB-INF/lib/core-${SPACEWALK_JAVA_VERSION}.jar /usr/share/rhn/lib/rhn.jar" | ||
| deploy_execute "ln -sf /usr/share/rhn/lib/rhn.jar ${TARGET_DIR}/WEB-INF/lib/rhn.jar" | ||
| deploy_execute mv ${TARGET_DIR}/WEB-INF/lib/core-${SPACEWALK_JAVA_VERSION}.jar /usr/share/rhn/lib/rhn.jar | ||
| deploy_execute ln -sf /usr/share/rhn/lib/rhn.jar "${TARGET_DIR}/WEB-INF/lib/rhn.jar" | ||
|
|
||
| print "Linking jars for Taskomatic..." | ||
| deploy_execute "ln -sf ${TARGET_DIR}/WEB-INF/lib/*.jar /usr/share/spacewalk/taskomatic" | ||
| deploy_execute ln -sf "${TARGET_DIR}/WEB-INF/lib/*.jar" /usr/share/spacewalk/taskomatic |
There was a problem hiding this comment.
Several arguments to deploy_execute are not properly quoted. For example, on line 327 ${TARGET_DIR}/WEB-INF/lib/branding-${BRANDING_VERSION}.jar should be quoted to handle paths with spaces. Similarly, on lines 331, 332, 335, and throughout the script, file paths and variables should be quoted. This could cause failures if paths contain spaces or special characters.
| report-db-port=${REPORT_DB_PORT} | ||
| report-db-user=${REPORT_DB_USER} | ||
| report-db-password=${REPORT_DB_PASS} | ||
| enable-tftp=${MANAGER_ENABLE_TFTP} | ||
| product_name=${PRODUCT_NAME} |
There was a problem hiding this comment.
The TFTP environment variable MANAGER_ENABLE_TFTP has been removed, but this might break backward compatibility if there are existing scripts or documentation that rely on this environment variable. Consider documenting this breaking change or maintaining backward compatibility by ignoring the variable if present.
| And service "salt-master" is active on "server" | ||
| And service "taskomatic" is enabled on "server" | ||
| And service "taskomatic" is active on "server" |
There was a problem hiding this comment.
Removed tests for TFTP socket being enabled and active. While this aligns with moving TFTP to a separate container, ensure that TFTP functionality is adequately tested in the new architecture, perhaps in a separate test suite for the TFTP container.
| EXECUTOR_POD=`kubectl get pod -n ${DEPLOY_NAMESPACE} -l 'app.kubernetes.io/part-of=uyuni,app.kubernetes.io/component=server' -o name` | ||
| if test $? -ne 0; then | ||
| print_error "Error: failed to find pod to work with." | ||
| fi | ||
| EXECUTOR_PARAMETERS=("exec" "-n" ${DEPLOY_NAMESPACE} "-ti" ${EXECUTOR_POD} "-c" "uyuni" "--") |
There was a problem hiding this comment.
The kubectl pod selection command on line 291 is missing proper quoting for variables used in the command. The ${DEPLOY_NAMESPACE} variable should be quoted. Additionally, the error handling on line 292-294 checks the exit code but still continues execution - it should include an exit 1 after the error message.
| selector: | ||
| app.kubernetes.io/component: proxy | ||
| app.kubernetes.io/part-of: uyuni |
There was a problem hiding this comment.
The TFTP service selector uses app.kubernetes.io/component: proxy but the TFTP deployment in tftp.yaml uses app.kubernetes.io/component: proxy-tftp. This mismatch means the service won't route traffic to the TFTP pod. The selector should be changed to app.kubernetes.io/component: proxy-tftp to match the deployment labels.
| deploy_execute() { | ||
| local cmd_to_run="$1" | ||
| # Execute the command with parameters as a proper array | ||
| "$EXECUTOR_COMMAND" "${EXECUTOR_PARAMETERS[@]}" "$cmd_to_run" | ||
| "$EXECUTOR_COMMAND" "${EXECUTOR_PARAMETERS[@]}" $@ | ||
| } |
There was a problem hiding this comment.
The deploy_execute function now passes all arguments without proper quoting. This could cause issues if any arguments contain spaces or special characters. For example, a file path with spaces will be split into multiple arguments. Consider using "$@" instead of $@ to preserve argument boundaries.
| server_version = get_server_version(config["server"]) | ||
| # Only check version for SUSE Multi-Linux Manager, not Uyuni | ||
| matcher = re.fullmatch(r"([0-9]+\.)[0-9]+\.[0-9]+", server_version) | ||
| if matcher: | ||
| major_version = matcher.group(1) | ||
| container_version = subprocess.run( | ||
| ["rpm", "-q", "--queryformat", "%{version}", "spacewalk-proxy-common"], | ||
| stdout=subprocess.PIPE, | ||
| universal_newlines=True, | ||
| check=False, | ||
| ).stdout | ||
| if not container_version.startswith(major_version): | ||
| logging.critical( | ||
| "Proxy container image version (%s) doesn't match server major version (%s)", | ||
| container_version, | ||
| major_version, | ||
| ) | ||
| sys.exit(1) |
There was a problem hiding this comment.
The version check has been moved after certificate configuration. If the version check fails and exits the script, the SSL certificates will have been configured but the proxy won't be functional. Consider keeping the version check before any configuration changes, or make it non-fatal in Kubernetes environments where version mismatches might be acceptable during rolling updates.
| {xhr.responseJSON.messages.map((line: string) => ( | ||
| <p key={line}>{line}</p> | ||
| ))} |
There was a problem hiding this comment.
The error handling assumes xhr.responseJSON.messages is always an array. If the API returns a different error format (e.g., without the messages field or with a string instead of an array), this will throw a TypeError. Add a check to ensure xhr.responseJSON.messages exists and is an array before calling .map() on it.
Running and accessing a TFTP server on Kubernetes requires either a load balancer or setting the pod to use hostNetwork. In order to minimize the side effects of the hostNetwork to only the TFTP service, the TFTP server needs to move to its own pod on both the server and the proxy. Note that not all load balancers will work with TFTP: serviceLB doesn't while MetalLB does. The proxy server can also work for the server with adjusted parameters to the command. This allows the TFTP pod of the server to not have to share a volume with the server pod. This way it can be scheduled on different nodes later on if required.
|
I have reverted the tftpd image rename |
|
|
||
| print_detailed " Syncing from remote $temp_dir to $dest_dir..." | ||
| deploy_execute "rsync -a ${rsync_params} ${temp_dir}/ ${dest_dir}" | ||
| deploy_execute rsync -a ${rsync_params} ${temp_dir}/ ${dest_dir} |
There was a problem hiding this comment.
shouldn't we use quote here? like "${rsync_params}".
Valid for all the others var in the files
| deploy_execute() { | ||
| local cmd_to_run="$1" | ||
| # Execute the command with parameters as a proper array | ||
| "$EXECUTOR_COMMAND" "${EXECUTOR_PARAMETERS[@]}" "$cmd_to_run" | ||
| "$EXECUTOR_COMMAND" "${EXECUTOR_PARAMETERS[@]}" $@ | ||
| } |
aaannz
left a comment
There was a problem hiding this comment.
Personally I think this PR should have been 2 or more PRs
That aside, I think we can ignore the deploy.sh copilot remarks. They have a merit, but it is a code just for our deployment on dev machine. Would be great if Copilot could create an issue with found problems.
From my POV this is LGTM
What does this PR change?
A server running on Kubernetes will have no clue on how to generate the SSL certificates for the proxies. This PR removes this option in the proxy configuration UI and adds an option to skip SSL certificates completely. The UI is unchanged for servers running on podman.
GUI diff
No difference on podman.
Documentation
No documentation needed: A bigger documentation for Kubernetes will be needed once bits are more stable.
DONE
Test coverage
No tests: No unit tests in that area yet
DONE
Links
Issue(s): https://github.com/SUSE/spacewalk/issues/29411
Changelogs
Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository
If you don't need a changelog check, please mark this checkbox:
If you uncheck the checkbox after the PR is created, you will need to re-run
changelog_test(see below)Re-run a test
If you need to re-run a test, please mark the related checkbox, it will be unchecked automatically once it has re-run:
Before you merge
Check How to branch and merge properly!