Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1794,20 +1794,41 @@ done`, endpoint, manifests.KASConnectionCheckerConfigMapName, manifests.KASConne
deployment.Spec.Template.ObjectMeta.Labels = map[string]string{
"app": manifests.KASConnectionCheckerName,
}
deployment.Spec.Template.ObjectMeta.Annotations = map[string]string{
"openshift.io/required-scc": "restricted-v2",
}
// No openshift.io/required-scc annotation: kube-system is exempt from SCC
// admission, so the annotation would be inert. Worse, if that exemption ever
// changed, restricted-v2 (MustRunAsRange) would reject the explicit UID below
// because it falls outside the namespace uid-range, breaking the checker.
// Set to nil so the annotation is also cleared from pre-existing deployments.
deployment.Spec.Template.ObjectMeta.Annotations = nil
Comment thread
coderabbitai[bot] marked this conversation as resolved.

deployment.Spec.Template.Spec.ServiceAccountName = manifests.KASConnectionCheckerName
deployment.Spec.Template.Spec.PriorityClassName = "system-node-critical"
automount := true
deployment.Spec.Template.Spec.AutomountServiceAccountToken = &automount

// kube-system is exempt from both SCC and Pod Security admission, so nothing
// assigns a UID for us and the cli image would otherwise run as root. The UID
// must be numeric: RunAsNonRoot alone would fail admission at the kubelet
// because the image declares no user. Same approach as konnectivity-agent,
// which also runs in kube-system.
deployment.Spec.Template.Spec.SecurityContext = &corev1.PodSecurityContext{
RunAsUser: ptr.To[int64](1000),
}

deployment.Spec.Template.Spec.Containers = []corev1.Container{
{
Name: "connection-checker",
Image: cliImage,
Command: []string{"/bin/sh", "-c", checkScript},
SecurityContext: &corev1.SecurityContext{
AllowPrivilegeEscalation: ptr.To(false),
ReadOnlyRootFilesystem: ptr.To(true),
RunAsNonRoot: ptr.To(true),
Capabilities: &corev1.Capabilities{
Drop: []corev1.Capability{"ALL"},
},
},
TerminationMessagePolicy: corev1.TerminationMessageFallbackToLogsOnError,
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("5m"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3156,8 +3156,40 @@ func verifyKASCheckerTolerations(t *testing.T, dep *appsv1.Deployment) {

func verifyKASCheckerAnnotations(t *testing.T, dep *appsv1.Deployment) {
t.Helper()
if dep.Spec.Template.ObjectMeta.Annotations["openshift.io/required-scc"] != "restricted-v2" {
t.Errorf("Expected openshift.io/required-scc annotation 'restricted-v2', got %s", dep.Spec.Template.ObjectMeta.Annotations["openshift.io/required-scc"])
// kube-system is exempt from SCC admission, so the annotation is inert there and
// would reject the explicit non-root UID if that exemption ever changed.
if got, ok := dep.Spec.Template.ObjectMeta.Annotations["openshift.io/required-scc"]; ok {
t.Errorf("openshift.io/required-scc annotation should not be set, got %s", got)
}
}

func verifyKASCheckerSecurityContext(t *testing.T, dep *appsv1.Deployment, container corev1.Container) {
t.Helper()
// A numeric UID is required: nothing assigns one in kube-system, and RunAsNonRoot
// alone would fail at the kubelet because the cli image declares no user.
podSecurityContext := dep.Spec.Template.Spec.SecurityContext
if podSecurityContext == nil || podSecurityContext.RunAsUser == nil {
t.Fatal("Pod SecurityContext should set RunAsUser")
}
if *podSecurityContext.RunAsUser != 1000 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (consistency): this pod-level RunAsUser check hand-rolls the nil-guard + manual deref, but the container-level checks just below use ptr.Deref. Could we use ptr.Deref here too for the value comparison to match? We'd keep the SecurityContext == nil Fatal since that's a nil struct pointer ptr.Deref can't guard.
One tradeoff: that collapses the "RunAsUser unset" case into the value check (reported as got 0) rather than a distinct Fatal. If you'd rather keep "never set" as its own explicit failure, current code is fine — just flagging the inconsistency.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to keep the explicit nil check here:

The container-level checks are all *bool, and there ptr.Deref's default is the assertion: unset RunAsNonRoot means "not enabled" (fail), unset AllowPrivilegeEscalation means "escalation allowed" (fail). nil collapses into a correct verdict with nothing lost.

RunAsUser *int64 has no such natural default. ptr.Deref(..., 0) reports got 0 — and 0 is exactly the UID that means root, which is the bug this PR exists to fix. So "nobody set a UID" would get reported as though the spec explicitly pinned root.

I reverted the fix locally to confirm, and it printed Pod SecurityContext should set RunAsUser. With ptr.Deref it would have said Expected RunAsUser 1000, got 0, sending the reader off to grep for an explicit 0.

t.Errorf("Expected RunAsUser 1000, got %d", *podSecurityContext.RunAsUser)
}

if container.SecurityContext == nil {
t.Fatal("Container SecurityContext should be set")
}
if !ptr.Deref(container.SecurityContext.RunAsNonRoot, false) {
t.Error("RunAsNonRoot should be true")
}
if ptr.Deref(container.SecurityContext.AllowPrivilegeEscalation, true) {
t.Error("AllowPrivilegeEscalation should be false")
}
if !ptr.Deref(container.SecurityContext.ReadOnlyRootFilesystem, false) {
t.Error("ReadOnlyRootFilesystem should be true")
}
if container.SecurityContext.Capabilities == nil ||
!reflect.DeepEqual(container.SecurityContext.Capabilities.Drop, []corev1.Capability{"ALL"}) {
t.Errorf("Expected all capabilities dropped, got %v", container.SecurityContext.Capabilities)
}
}

Expand Down Expand Up @@ -3206,6 +3238,7 @@ func Test_reconciler_reconcileKASConnectionCheckerDeployment(t *testing.T) {
verifyKASCheckerResources(t, container)
verifyKASCheckerTolerations(t, dep)
verifyKASCheckerAnnotations(t, dep)
verifyKASCheckerSecurityContext(t, dep, container)

cm := &corev1.ConfigMap{}
if err := c.Get(context.Background(), client.ObjectKey{Name: manifests.KASConnectionCheckerConfigMapName, Namespace: manifests.KASConnectionCheckerNamespace}, cm); err != nil {
Expand Down Expand Up @@ -3256,6 +3289,10 @@ func Test_reconciler_reconcileKASConnectionCheckerDeployment(t *testing.T) {
Labels: map[string]string{
"app": "old-label",
},
// Written by an older HCCO; must be cleared on upgrade.
Annotations: map[string]string{
"openshift.io/required-scc": "restricted-v2",
},
},
Spec: corev1.PodSpec{
Containers: []corev1.Container{
Expand Down Expand Up @@ -3283,6 +3320,7 @@ func Test_reconciler_reconcileKASConnectionCheckerDeployment(t *testing.T) {
t.Errorf("Expected ServiceAccountName %s, got %s", manifests.KASConnectionCheckerName, dep.Spec.Template.Spec.ServiceAccountName)
}
verifyKASCheckerAnnotations(t, dep)
verifyKASCheckerSecurityContext(t, dep, container)
},
},
}
Expand Down