-
Notifications
You must be signed in to change notification settings - Fork 54
[release-4.18] snc: Add logic to create /Users top level directory for OCP #1049
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
[release-4.18] snc: Add logic to create /Users top level directory for OCP #1049
Conversation
… from host" This reverts commit a890014. Since OCP moves to consume bootc images for node this is not working anymore, in next commit workable solution is added.
Since OCP now moved to use bootc where top level directory is immutable and `chattr -i /` doesn't work as it worked before so only option is to create a custom-os image and deploy that as part of day-2 operation. More details : crc-org#1041 (comment)
Reviewer's Guide by SourceryThis pull request adds logic to create a Sequence diagram for creating /Users directorysequenceDiagram
participant SNC Script
participant Podman
participant OCP Registry
participant OCP API
participant Master Node
SNC Script->>Podman: Build Containerfile from RHCOS Image
Podman->>Podman: ln -sf var/Users /Users && mkdir /var/Users
Podman->>OCP Registry: Push Custom Image
SNC Script->>OCP API: Apply MachineConfig with custom image URL
OCP API->>Master Node: Update MachineConfig
Master Node->>Master Node: Create /Users directory and symlink
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @openshift-cherrypick-robot. Thanks for your PR. I'm waiting for a crc-org member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @openshift-cherrypick-robot - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Hardcoded password found for kubeadmin user. (link)
Overall Comments:
- Consider extracting the custom-os image creation logic into a separate function for better readability.
- The added
sleep 60
might be insufficient in some environments; consider using a more robust method to ensure the MachineConfig is applied before proceeding.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🔴 Security: 1 blocking issue
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
while retry ${OC} get mcp master -ojsonpath='{.status.conditions[?(@.type!="Updated")].status}' | grep True; do | ||
echo "Machine config still in updating/degrading state" | ||
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (performance): Consider introducing a delay in the while loop to avoid busy looping.
Adding a short sleep (e.g., 'sleep 5') inside the loop could prevent high CPU usage during the waiting period if the machine config pool takes time to update.
while retry ${OC} get mcp master -ojsonpath='{.status.conditions[?(@.type!="Updated")].status}' | grep True; do | |
echo "Machine config still in updating/degrading state" | |
done | |
while retry ${OC} get mcp master -ojsonpath='{.status.conditions[?(@.type!="Updated")].status}' | grep True; do | |
echo "Machine config still in updating/degrading state" | |
sleep 5 | |
done |
RUN ln -sf var/Users /Users && mkdir /var/Users | ||
EOF | ||
podman build --from ${RHCOS_IMAGE} --authfile ${OPENSHIFT_PULL_SECRET_PATH} -t default-route-openshift-image-registry.apps-crc.testing/openshift-machine-config-operator/rhcos:latest --file ${INSTALL_DIR}/Containerfile . | ||
retry ${OC} login -u kubeadmin -p $(cat ${INSTALL_DIR}/auth/kubeadmin-password) --insecure-skip-tls-verify=true api.${SNC_PRODUCT_NAME}.${BASE_DOMAIN}:6443 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 issue (security): Hardcoded password found for kubeadmin user.
The password for the kubeadmin user is being read directly from a file and used in a command. This is a security risk as the password could be exposed.
/ok-to-test |
@openshift-cherrypick-robot: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This is an automated cherry-pick of #1047
/assign praveenkumar
Summary by Sourcery
Add logic to create /Users directory for OpenShift Container Platform (OCP) in single-node cluster setup
New Features:
Enhancements:
Chores: