Copilot commented on code in PR #875:
URL: https://github.com/apache/solr-operator/pull/875#discussion_r4409910227
##########
controllers/solrcloud_controller_test.go:
##########
@@ -769,13 +786,21 @@ var _ = FDescribe("SolrCloud controller - General",
func() {
g.Expect(customConfigVol.VolumeSource.ConfigMap, "solr-xml Volume should have a
ConfigMap source")
g.Expect(customConfigVol.VolumeSource.ConfigMap.Name).To(Equal(testCustomConfigMap),
"solr-xml has the wrong configMap as its source")
+ // The "solr-xml" volume is mounted twice in
this case, once for the solr.xml (via a subPath)
+ // and once for the directory containing the
log4j2.xml.
var logXmlVolMount *corev1.VolumeMount = nil
+ var solrXmlVolMount *corev1.VolumeMount = nil
for _, mount := range
found.Spec.Template.Spec.Containers[0].VolumeMounts {
if mount.Name == "solr-xml" {
- logXmlVolMount = &mount
- break
+ if mount.SubPath ==
util.SolrXmlFile {
+ solrXmlVolMount = &mount
+ } else {
+ logXmlVolMount = &mount
+ }
}
}
Review Comment:
This stores pointers to the range loop variable (`mount`), which is reused
each iteration in Go; both `solrXmlVolMount` and `logXmlVolMount` can end up
pointing at the same final value. Iterate by index (or take pointers to slice
elements) so each pointer refers to the actual element you matched.
##########
controllers/util/security_context_util.go:
##########
@@ -0,0 +1,211 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package util
+
+import (
+ solr "github.com/apache/solr-operator/api/v1beta1"
+ corev1 "k8s.io/api/core/v1"
+ "k8s.io/utils/ptr"
+)
+
+// Names of the containers and init containers that the Solr Operator
generates.
+const (
+ VolumePrepInitContainer = "volume-prep"
+ SetupZkInitContainer = "setup-zk"
+ Pkcs12InitContainer = "gen-pkcs12-keystore"
+ TLSWrapperScriptInitContainer = "create-tls-wrapper-script"
+ // InitdbInitContainer is defined in solr_tls_util.go
+)
+
+// operatorContainerOptions describes a container that the operator generates.
+type operatorContainerOptions struct {
+ // imageRunsAsRoot is true if the container's image has no non-root
user of its own.
+ // The "restricted" profile requires runAsNonRoot, which the kubelet
refuses to honor for such
+ // an image unless a runAsUser is also given, so these containers get
one.
+ imageRunsAsRoot bool
+
+ // readOnlyRootFilesystem is true if the container is known not to
write to its root filesystem.
+ // This is not part of the "restricted" profile and is not applied to
containers that did not
+ // already have it, see
https://github.com/apache/solr-operator/issues/801.
+ readOnlyRootFilesystem bool
+}
+
+// operatorContainers lists every container and init container that the
operator generates, and is
+// therefore responsible for hardening, along with the options that container
needs. Containers the
+// user supplies through podOptions are not listed: their security context is
the user's to set.
+//
+// A container missing from this map gets no security context, which makes the
pod fail "restricted"
+// evaluation. The unit tests evaluate the generated pods against the profile,
so a new operator
+// container that is not listed here fails the build.
+//
+// A name in this map is not on its own proof that the operator generated the
container, since a
+// user can give a container of their own the same name.
setSecurityContextOnOperatorContainers
+// therefore excludes the user's containers by name before consulting this map.
+var operatorContainers = map[string]operatorContainerOptions{
+ SolrNodeContainer: {},
+ SolrPrometheusExporterContainer: {},
+ SetupZkInitContainer: {readOnlyRootFilesystem: true},
+ Pkcs12InitContainer: {},
+ VolumePrepInitContainer: {imageRunsAsRoot: true},
+ InitdbInitContainer: {imageRunsAsRoot: true},
+ TLSWrapperScriptInitContainer: {imageRunsAsRoot: true},
+}
+
+// defaultPodSecurityContext builds the pod-level security context, starting
from the one the user
+// provided, if any, and filling in every field they left unset. User-provided
fields always win.
+//
+// The "restricted" profile asks for a seccomp profile, which is set once here
rather than on every
+// container. fsGroup is not part of the profile, it is a long-standing
operator default.
+func defaultPodSecurityContext(userProvided *corev1.PodSecurityContext,
defaultFSGroup int64) *corev1.PodSecurityContext {
+ securityContext := &corev1.PodSecurityContext{}
+ if userProvided != nil {
+ securityContext = userProvided.DeepCopy()
+ }
+
+ if securityContext.SeccompProfile == nil {
+ securityContext.SeccompProfile = &corev1.SeccompProfile{Type:
corev1.SeccompProfileTypeRuntimeDefault}
+ }
+ if securityContext.FSGroup == nil {
+ securityContext.FSGroup = ptr.To(defaultFSGroup)
+ }
+
+ return securityContext
+}
+
+// defaultContainerSecurityContext builds the security context for one
operator-generated container,
+// starting from the one the user provided, if any, and filling in every field
they left unset.
+// User-provided fields always win, so a single field is enough to opt out of
a single default.
+//
+// podRunAsUser is the pod-level runAsUser, which containers inherit when they
do not set their own.
+func defaultContainerSecurityContext(userProvided *corev1.SecurityContext,
options operatorContainerOptions, podRunAsUser *int64) *corev1.SecurityContext {
+ securityContext := &corev1.SecurityContext{}
+ if userProvided != nil {
+ securityContext = userProvided.DeepCopy()
+ }
+
+ // The user can ask for a container to run as root, which some setups
need (see the backup
+ // repository preparation in generateSolrSetupInitContainers). Such a
container cannot also run
+ // as non-root, and dropping all capabilities would leave it unable to
do the work root was
+ // asked for, so neither default is applied to it.
+ effectiveRunAsUser := securityContext.RunAsUser
+ if effectiveRunAsUser == nil {
+ effectiveRunAsUser = podRunAsUser
+ }
+ runsAsRoot := effectiveRunAsUser != nil && *effectiveRunAsUser == 0
+
+ // Kubernetes rejects allowPrivilegeEscalation: false alongside
privileged: true or an added
+ // CAP_SYS_ADMIN, so the default is not applied when the user asked for
either of those. An
+ // explicitly provided value is always kept, even an invalid one, so
the user sees the error.
+ if securityContext.AllowPrivilegeEscalation == nil &&
!securityContextRequiresPrivilegeEscalation(securityContext) {
+ securityContext.AllowPrivilegeEscalation = ptr.To(false)
+ }
+ if !runsAsRoot {
+ if securityContext.RunAsNonRoot == nil {
+ securityContext.RunAsNonRoot = ptr.To(true)
+ }
+ if securityContext.Capabilities == nil {
+ securityContext.Capabilities =
&corev1.Capabilities{Drop: []corev1.Capability{"ALL"}}
+ }
+ if options.imageRunsAsRoot && effectiveRunAsUser == nil {
+ securityContext.RunAsUser =
ptr.To(int64(DefaultSolrUser))
+ }
+ }
Review Comment:
If a user supplies `capabilities: {}` (non-nil struct but no `add`/`drop`),
this code won’t apply the default `drop: [\"ALL\"]`, and the pod will fail the
restricted profile unexpectedly. Consider treating an empty `Capabilities`
object as unset (or default `Drop` when it’s nil/empty and `Add` is empty) so
the ‘merged over defaults’ behavior is consistent.
##########
helm/solr-operator/Chart.yaml:
##########
@@ -145,6 +145,23 @@ annotations:
url: https://solr.apache.org/docs/9_11_0/changes/Changes.html
- name: Solr Security News
url: https://solr.apache.org/security-news.html
+ - kind: changed
+ description: The `solr.xml` is now mounted into the Solr home directory
from the ConfigMap, via a subPath volumeMount, instead of being copied there by
the `cp-solr-xml` initContainer. That initContainer is only created for
`volume` backupRepositories now, renamed to `volume-prep`
+ links:
+ - name: Github Issue
Review Comment:
Use the correct casing for the brand name in link titles: change `Github
Issue` to `GitHub Issue`.
##########
helm/solr-operator/Chart.yaml:
##########
@@ -145,6 +145,23 @@ annotations:
url: https://solr.apache.org/docs/9_11_0/changes/Changes.html
- name: Solr Security News
url: https://solr.apache.org/security-news.html
+ - kind: changed
+ description: The `solr.xml` is now mounted into the Solr home directory
from the ConfigMap, via a subPath volumeMount, instead of being copied there by
the `cp-solr-xml` initContainer. That initContainer is only created for
`volume` backupRepositories now, renamed to `volume-prep`
+ links:
+ - name: Github Issue
+ url: https://github.com/apache/solr-operator/issues/161
+ - kind: security
+ description: Solr and Prometheus exporter pods now satisfy the
Kubernetes Pod Security Standards "restricted" profile by default. The operator
sets a seccompProfile on the pod and runAsNonRoot, allowPrivilegeEscalation
false and capabilities.drop ALL on every container it generates.
podSecurityContext, containerSecurityContext and
defaultInitContainerSecurityContext are now merged over these defaults field by
field
+ links:
+ - name: Github Issue
+ url: https://github.com/apache/solr-operator/issues/873
+ - name: Pod Security Standards
+ url:
https://kubernetes.io/docs/concepts/security/pod-security-standards/#restricted
+ - kind: security
+ description: The Solr Operator's own container now drops all
capabilities and sets a seccompProfile, so the operator pod satisfies the
"restricted" profile too
+ links:
+ - name: Github Issue
Review Comment:
Use the correct casing for the brand name in link titles: change `Github
Issue` to `GitHub Issue`.
##########
helm/solr/Chart.yaml:
##########
@@ -70,6 +70,13 @@ annotations:
links:
- name: Solr Security News
url: https://solr.apache.org/security-news.html
+ - kind: security
+ description: Solr and Prometheus exporter pods now satisfy the
Kubernetes Pod Security Standards "restricted" profile by default. Anything set
in podOptions.podSecurityContext, podOptions.containerSecurityContext or
podOptions.defaultInitContainerSecurityContext is merged over those defaults
field by field. A volume backupRepository directory must now be writable by the
Solr user, or set podOptions.defaultInitContainerSecurityContext.runAsUser to 0
to have the operator take ownership of it
+ links:
+ - name: Github Issue
Review Comment:
Use the correct casing for the brand name: change `Github Issue` to `GitHub
Issue` (also matches the URL’s host casing elsewhere in the repo).
##########
helm/solr-operator/Chart.yaml:
##########
@@ -145,6 +145,23 @@ annotations:
url: https://solr.apache.org/docs/9_11_0/changes/Changes.html
- name: Solr Security News
url: https://solr.apache.org/security-news.html
+ - kind: changed
+ description: The `solr.xml` is now mounted into the Solr home directory
from the ConfigMap, via a subPath volumeMount, instead of being copied there by
the `cp-solr-xml` initContainer. That initContainer is only created for
`volume` backupRepositories now, renamed to `volume-prep`
+ links:
+ - name: Github Issue
+ url: https://github.com/apache/solr-operator/issues/161
+ - kind: security
+ description: Solr and Prometheus exporter pods now satisfy the
Kubernetes Pod Security Standards "restricted" profile by default. The operator
sets a seccompProfile on the pod and runAsNonRoot, allowPrivilegeEscalation
false and capabilities.drop ALL on every container it generates.
podSecurityContext, containerSecurityContext and
defaultInitContainerSecurityContext are now merged over these defaults field by
field
+ links:
+ - name: Github Issue
Review Comment:
Use the correct casing for the brand name in link titles: change `Github
Issue` to `GitHub Issue`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
