Update kruize UI to Deployment#68
Open
shreyabiradar07 wants to merge 8 commits intokruize:mvp_demofrom
Open
Conversation
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Reviewer's GuideConvert the Kruize UI nginx component from a standalone Pod to an apps/v1 Deployment and update tests to validate the new resource shape and deployed UI image, ensuring UI ownership is decoupled from the Kruize CR. Sequence diagram for reconciliation creating Kruize UI nginx DeploymentsequenceDiagram
participant User
participant API as KubernetesAPI
participant CR as KruizeCR
participant KC as KruizeController
participant GEN as KruizeResourceGenerator
participant DEP as Deployment_kruize_ui_nginx
User->>API: Apply KruizeCR
API-->>CR: Persist KruizeCR
API->>KC: Trigger reconcile for KruizeCR
KC->>GEN: NamespacedResources()
GEN-->>KC: Service, ConfigMap, Deployment_kruize_ui_nginx
KC->>API: CreateOrUpdate Service
KC->>API: CreateOrUpdate ConfigMap
KC->>API: CreateOrUpdate Deployment_kruize_ui_nginx
API-->>DEP: Ensure Deployment exists
DEP-->>API: Manage ReplicaSet and Pods
API-->>User: Kruize UI nginx Pod running under Deployment ownerReference
Class diagram for updated KruizeResourceGenerator UI nginx resourcesclassDiagram
class KruizeResourceGenerator {
string Namespace
string Autotune_ui_image
NamespacedResources() []client_Object
KubernetesNamespacedResources() []client_Object
kruizeServiceMonitor() *monitoringv1_ServiceMonitor
nginxConfigMap() *corev1_ConfigMap
kruizeUINginxService() *corev1_Service
kruizeUINginxDeployment() *appsv1_Deployment
deletePartitionCronJob() *batchv1_CronJob
}
class corev1_Service {
}
class corev1_ConfigMap {
}
class appsv1_Deployment {
int32 replicas
DeploymentStrategy strategy
LabelSelector selector
PodTemplateSpec template
}
class corev1_PodTemplateSpec {
}
class corev1_PodSpec {
Container[] containers
Volume[] volumes
}
class corev1_Container {
string name
string image
PullPolicy imagePullPolicy
EnvVar[] env
VolumeMount[] volumeMounts
SecurityContext securityContext
}
class corev1_Volume {
string name
VolumeSource volumeSource
}
class corev1_VolumeSource {
ConfigMapVolumeSource configMap
EmptyDirVolumeSource emptyDir
}
class corev1_ConfigMapVolumeSource {
LocalObjectReference localObjectReference
}
class corev1_EmptyDirVolumeSource {
}
class corev1_SecurityContext {
bool allowPrivilegeEscalation
Capabilities capabilities
SeccompProfile seccompProfile
}
KruizeResourceGenerator --> corev1_Service : creates
KruizeResourceGenerator --> corev1_ConfigMap : creates
KruizeResourceGenerator --> appsv1_Deployment : creates
appsv1_Deployment o-- corev1_PodTemplateSpec : template
corev1_PodTemplateSpec o-- corev1_PodSpec : spec
corev1_PodSpec o-- corev1_Container : containers
corev1_PodSpec o-- corev1_Volume : volumes
corev1_Volume o-- corev1_VolumeSource : source
corev1_VolumeSource o-- corev1_ConfigMapVolumeSource : configMap
corev1_VolumeSource o-- corev1_EmptyDirVolumeSource : emptyDir
corev1_Container o-- corev1_SecurityContext : securityContext
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new Deployment spec drops several previously-set environment variables (e.g., KRUIZE_API_URL, REACT_APP_KRUIZE_API_URL, KRUIZE_UI_API_URL, API_URL); if the UI still relies on these, consider keeping them to avoid breaking existing behavior.
- The Pod’s security context and emptyDir volumes for cache/pid/tmp were removed in the Deployment; if these were originally added for security or nginx runtime stability, consider porting them over to the new Pod template instead of omitting them.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new Deployment spec drops several previously-set environment variables (e.g., KRUIZE_API_URL, REACT_APP_KRUIZE_API_URL, KRUIZE_UI_API_URL, API_URL); if the UI still relies on these, consider keeping them to avoid breaking existing behavior.
- The Pod’s security context and emptyDir volumes for cache/pid/tmp were removed in the Deployment; if these were originally added for security or nginx runtime stability, consider porting them over to the new Pod template instead of omitting them.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- When converting the UI pod to a Deployment, the container security context lost
RunAsNonRootandRunAsUsersettings; consider preserving these fields to avoid silently weakening the workload’s security posture compared to the original Pod. - The e2e check for UI readiness assumes
.status.readyReplicasequals the string"1"; this can be brittle if replica counts change or the field is temporarily empty—consider asserting thatreadyReplicasis non-zero or matching the spec replicas instead of a hard-coded string.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When converting the UI pod to a Deployment, the container security context lost `RunAsNonRoot` and `RunAsUser` settings; consider preserving these fields to avoid silently weakening the workload’s security posture compared to the original Pod.
- The e2e check for UI readiness assumes `.status.readyReplicas` equals the string `"1"`; this can be brittle if replica counts change or the field is temporarily empty—consider asserting that `readyReplicas` is non-zero or matching the spec replicas instead of a hard-coded string.
## Individual Comments
### Comment 1
<location> `internal/utils/kruize_generator.go:708-715` </location>
<code_context>
- {Name: "nginx-pid", MountPath: "/var/run"},
- {Name: "nginx-tmp", MountPath: "/tmp"},
},
- SecurityContext: &corev1.SecurityContext{
- AllowPrivilegeEscalation: boolPtr(false),
- RunAsNonRoot: boolPtr(true),
- RunAsUser: int64Ptr(101),
- Capabilities: &corev1.Capabilities{
- Drop: []corev1.Capability{"ALL"},
- },
- SeccompProfile: &corev1.SeccompProfile{
- Type: corev1.SeccompProfileTypeRuntimeDefault,
+ Volumes: []corev1.Volume{
</code_context>
<issue_to_address>
**🚨 issue (security):** Dropping RunAsNonRoot/RunAsUser weakens the previous security posture
These fields previously enforced non-root execution (`RunAsNonRoot: true`, `RunAsUser: 101`) but are now removed from the pod template. If nginx is expected to run as UID 101, please retain equivalent settings (pod or container level) to prevent it from unintentionally running as root in some environments.
</issue_to_address>
### Comment 2
<location> `test/e2e/e2e_test.go:299-308` </location>
<code_context>
+ By("verifying deployed Kruize UI image")
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the UI image verification by asserting on mismatches when KRUIZE_UI_IMAGE is set
When `kruizeUIImage` is set, the test only logs a warning if the deployed UI image differs, so CI won’t fail on a miswired or regressed image. Please make a mismatch a failing assertion (e.g. `Expect(deployedUIImage).To(Equal(kruizeUIImage))`), or gate that behavior behind a flag that CI can enable, so `KRUIZE_UI_IMAGE` is actually validated end-to-end.
Suggested implementation:
```golang
// If custom Kruize UI image was specified, verify it matches and fail on mismatch
if kruizeUIImage != "" {
fmt.Fprintf(GinkgoWriter, "Validating deployed UI image against KRUIZE_UI_IMAGE: %s\n", kruizeUIImage)
ExpectWithOffset(1, deployedUIImage).To(
Equal(kruizeUIImage),
fmt.Sprintf("Deployed UI image %s does not match KRUIZE_UI_IMAGE %s", deployedUIImage, kruizeUIImage),
)
fmt.Fprintf(GinkgoWriter, "✓ Deployed UI image matches specified KRUIZE_UI_IMAGE: %s\n", kruizeUIImage)
```
The above change makes a mismatch between the deployed UI image and `kruizeUIImage` a hard assertion failure whenever `kruizeUIImage` is set, which aligns with the CI requirement you described.
Because I can only see part of the block, you should:
1. Remove any existing `else` branch below this `if kruizeUIImage != "" { ... }` that only logs a warning on mismatch. With the new `Expect`, such an `else` is no longer needed and will cause compilation issues if it still references the old `if deployedUIImage == kruizeUIImage` structure.
2. If you need the “warning-only” behavior for local runs, consider guarding this strict check behind a boolean flag (e.g. an env var) and wrapping the `Expect` in another `if` so CI can enable strict verification while developers can opt out locally.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
…shift Signed-off-by: Shreya Biradar <shbirada@ibm.com>
internal/utils/kruize_generator.go
Outdated
Comment on lines
+722
to
+725
| {Name: "KRUIZE_API_URL", Value: "http://kruize:8080"}, | ||
| {Name: "REACT_APP_KRUIZE_API_URL", Value: "http://kruize:8080"}, | ||
| {Name: "KRUIZE_UI_API_URL", Value: "http://kruize:8080"}, | ||
| {Name: "API_URL", Value: "http://kruize:8080"}, |
Member
There was a problem hiding this comment.
Are these env vars needed?
Contributor
Author
There was a problem hiding this comment.
Thanks for pointing, removed them
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR converts Kruize UI from kind
PodtoDeploymentto ensure UI pod ownerReference is UI deployment and not Kruize CR.Docker image:
quay.io/shbirada/kruize_operator:kruize_ui_deploymentSummary by Sourcery
Convert the Kruize UI nginx component from a standalone Pod to a Deployment and add CI automation to keep Kruize image versions in sync with upstream releases.
New Features:
Enhancements:
CI:
Summary by Sourcery
Convert the Kruize UI nginx component from a standalone Pod to a Deployment and update tests accordingly.
New Features:
Enhancements:
Tests: