Copilot commented on code in PR #744:
URL: https://github.com/apache/solr-operator/pull/744#discussion_r4514448585
##########
controllers/util/solr_security_util.go:
##########
@@ -240,11 +245,16 @@ func cmdToPutSecurityJsonInZk() string {
cmd := " solr zk cp zk:/security.json /tmp/current_security.json
>/dev/null 2>&1; " +
" GET_CURRENT_SECURITY_JSON_EXIT_CODE=$?; " +
"if [ ${GET_CURRENT_SECURITY_JSON_EXIT_CODE} -eq 0 ]; then " +
// JSON already exists
- "if [ ! -s /tmp/current_security.json ] || grep -q '^{}$'
/tmp/current_security.json ]; then " + // File doesn't exist, is empty, or is
just '{}'
+ "if [ ! -s /tmp/current_security.json ] || grep -q '^{}$'
/tmp/current_security.json; then " + // File doesn't exist, is empty, or is
just '{}'
" echo $SECURITY_JSON > /tmp/security.json;" +
" solr zk cp /tmp/security.json zk:/security.json >/dev/null
2>&1; " +
" echo 'Blank security.json found. Put new security.json in
ZK'; " +
- "fi; " + // TODO: Consider checking a diff and still applying
over the top
+ "elif [ \"${SECURITY_JSON_OVERWRITE}\" = true ] && [ \"$(cat
/tmp/current_security.json)\" != \"$(echo $SECURITY_JSON)\" ]; then " + // We
want to overwrite the security config if there's a diff
+ " echo $SECURITY_JSON > /tmp/security.json;" +
+ " solr zk cp /tmp/security.json zk:/security.json >/dev/null
2>&1; " +
+ " echo 'Diff found. Overwriting security.json in ZK'; " +
+ " else " +
+ " echo 'Not overwriting security.json'; fi; " +
Review Comment:
The overwrite/diff logic is unsafe for JSON content because `$SECURITY_JSON`
is unquoted and written via `echo`. This can corrupt JSON (word-splitting,
globbing, dropped newlines, `-n` handling) and makes the diff comparison
unreliable (`$(echo $SECURITY_JSON)` collapses whitespace/newlines). Use a safe
write (e.g., `printf '%s' \"$SECURITY_JSON\" > /tmp/security.json`) and compare
based on file contents (e.g., create `/tmp/security.json` first then compare
with `cmp -s`/`diff -q`) to avoid false positives/negatives and accidental
overwrites.
##########
api/v1beta1/solrcloud_types.go:
##########
@@ -1621,6 +1624,15 @@ const (
Basic AuthenticationType = "Basic"
)
+type BootstrapSecurityJson struct {
+ SecurityJsonSecret *corev1.SecretKeySelector
`json:"bootstrapSecurityJson,omitempty"`
+
+ // Flag to indicate if the operator should overwrite an existing
security.json if there are changes
+ // as compared to the underlying secret
+ // +optional
+ Overwrite bool `json:"overwrite,omitempty"`
+}
Review Comment:
The `SecurityJsonSecret` field has an incorrect JSON tag
(`bootstrapSecurityJson`). This causes the serialized API shape to be
nested/ambiguous (and is consistent with the CRD diff showing
`bootstrapSecurityJson.bootstrapSecurityJson.*`). Rename the tag to match the
field name (e.g. `securityJsonSecret`) and regenerate CRDs so the schema
becomes `bootstrapSecurityJson.securityJsonSecret.{name,key,optional}` plus
`bootstrapSecurityJson.overwrite`.
##########
api/v1beta1/solrcloud_types.go:
##########
@@ -691,7 +693,8 @@ type ManagedUpdateOptions struct {
// The maximum number of replicas for each shard that can be
unavailable during the update.
// Value can be an absolute number (ex: 5) or a percentage of replicas
in a shard (ex: 25%).
// Absolute number is calculated from percentage by rounding down.
- // If the provided number is 0 or negative, then all replicas will be
allowed to be updated in unison.
+ // If the provided number is 0 , then all replicas will be allowed to
be updated in unison.
Review Comment:
Extra space in doc comment ('0 ,'). This shows up in generated docs/CRDs
too; please correct to '0,'.
##########
tests/scripts/manage_e2e_tests.sh:
##########
@@ -130,7 +130,7 @@ function run_tests() {
GINKGO_PARAMS+=("${param}" "${!envName}")
fi
done
- GINKGO_PARAMS+=("${RAW_GINKGO[@]}")
+ GINKGO_PARAMS+=("${RAW_GINKGO[@]:-}")
Review Comment:
`${RAW_GINKGO[@]:-}` is not a robust way to default an array expansion when
the variable may be unset; depending on shell options (e.g., `set -u`) and Bash
semantics, this can behave unexpectedly. Prefer an explicit length check before
appending (e.g., only append when the array has elements) or initialize
`RAW_GINKGO` to an empty array earlier so `GINKGO_PARAMS+=(${RAW_GINKGO[@]})`
is always safe.
##########
tests/e2e/solrcloud_security_json_test.go:
##########
@@ -50,7 +55,115 @@ var _ = FDescribe("E2E - SolrCloud - Security JSON", func()
{
solrCloud = expectSolrCloudToBeReady(ctx, solrCloud)
By("creating a first Solr Collection")
- createAndQueryCollection(ctx, solrCloud, "basic", 1, 1)
+ createAndQueryCollection(ctx, solrCloud, "basic", 2, 1)
+
+ // Because overwrite=false, this won't apply new security json
+ // Pods will start back up, demonstrating that this was not
applied
+ By("updating security.json secret")
+ generateBreakingSolrSecuritySecret(ctx, solrCloud)
+
+ patchedSolrCloud := solrCloud.DeepCopy()
+ patchedSolrCloud.Spec.CustomSolrKubeOptions.PodOptions =
&solrv1beta1.PodOptions{
+ Annotations: map[string]string{
+ "test": "restart-1",
+ },
+ }
+ By("triggering a restart via pod annotations")
+ Expect(k8sClient.Patch(ctx, patchedSolrCloud,
client.MergeFrom(solrCloud))).To(Succeed(), "Could not add annotation to
SolrCloud pod to initiate restart")
+
+ // Trimmed down check where we just want the rolling restart to
begin then end, as we're not testing restarts here
+ By("waiting for the first restart to begin")
+ expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, cloud
*solrv1beta1.SolrCloud) {
+ g.Expect(cloud.Status.UpToDateNodes).To(BeZero(),
"Cloud did not get to a state with zero up-to-date replicas when rolling
restart began.")
+ for _, nodeStatus := range cloud.Status.SolrNodes {
+ g.Expect(nodeStatus.SpecUpToDate).To(BeFalse(),
"Node not starting as out-of-date when rolling restart begins: %s",
nodeStatus.Name)
+ }
+ })
+
+ // After all pods are ready, make sure that the SolrCloud
status is correct
+ By("waiting for the first restart to complete")
+ expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, cloud
*solrv1beta1.SolrCloud) {
+
g.Expect(cloud.Status.UpToDateNodes).To(Equal(int32(2)), "The SolrCloud did not
finish the rolling restart as not all nodes are up to date. Only reached %d",
solrCloud.Status.UpToDateNodes)
+
g.Expect(cloud.Status.ReadyReplicas).To(Equal(int32(2)), "The SolrCloud did not
finish the rolling restart as not all nodes are ready. Only reached %d",
solrCloud.Status.ReadyReplicas)
+ })
+
+ By("waiting for the balanceReplicas to finish")
+ expectStatefulSetWithChecksAndTimeout(ctx, solrCloud,
solrCloud.StatefulSetName(), time.Second*70, time.Second, func(g Gomega, found
*appsv1.StatefulSet) {
+ clusterOp, err := controllers.GetCurrentClusterOp(found)
+ g.Expect(err).ToNot(HaveOccurred(), "Error occurred
while finding clusterLock for SolrCloud")
+ g.Expect(clusterOp).To(BeNil(), "StatefulSet should not
have a balanceReplicas lock after balancing is complete.")
+ })
+
+ By("Waiting for the SolrCloud to come up healthy with old
security secret")
+ expectSolrCloudToBeReady(ctx, solrCloud)
+
+ By("querying existing Solr Collection")
+ queryCollection(ctx, solrCloud, "basic", 0, 0)
+
+ // Now with overwrite set to true, we'll update the applied
security to allow "new-oper" user
+ // Operator will continue to use "test-oper" user until we
update the basic auth secret
+ By("updating security.json secret")
+ generateUpdatedSolrSecuritySecret(ctx, solrCloud)
+
+ secondPatchedSolrCloud := solrCloud.DeepCopy()
+
secondPatchedSolrCloud.Spec.SolrSecurity.BootstrapSecurityJson.Overwrite = true
+ secondPatchedSolrCloud.Spec.CustomSolrKubeOptions.PodOptions =
&solrv1beta1.PodOptions{
+ Annotations: map[string]string{
+ "test": "restart-2",
+ },
+ }
+
+ By("triggering a restart to overwrite the security.json")
+ Expect(k8sClient.Patch(ctx, secondPatchedSolrCloud,
client.MergeFrom(solrCloud))).To(Succeed(), "Could not update security spec for
SolrCloud pod to initiate restart")
+
+ // Trimmed down check where we just want the rolling restart to
begin then end, as we're not testing restarts here
+ By("waiting for the second restart to begin")
+ newFoundCloud := expectSolrCloudWithChecks(ctx, solrCloud,
func(g Gomega, cloud *solrv1beta1.SolrCloud) {
+ g.Expect(cloud.Status.UpToDateNodes).To(BeZero(),
"Cloud did not get to a state with zero up-to-date replicas when rolling
restart began.")
+ })
+
+ // After all pods are ready, make sure that the SolrCloud
status is correct
+ By("waiting for the second restart to complete")
+ expectSolrCloudWithChecks(ctx, newFoundCloud, func(g Gomega,
cloud *solrv1beta1.SolrCloud) {
+
g.Expect(cloud.Status.UpToDateNodes).To(Equal(int32(2)), "The SolrCloud did not
finish the rolling restart as not all nodes are up to date. Only reached %d",
solrCloud.Status.UpToDateNodes)
+
g.Expect(cloud.Status.ReadyReplicas).To(Equal(int32(2)), "The SolrCloud did not
finish the rolling restart as not all nodes are ready. Only reached %d",
solrCloud.Status.ReadyReplicas)
+ })
+
+ By("waiting for the balanceReplicas to finish")
+ expectStatefulSetWithChecksAndTimeout(ctx, newFoundCloud,
newFoundCloud.StatefulSetName(), time.Second*70, time.Second, func(g Gomega,
found *appsv1.StatefulSet) {
+ clusterOp, err := controllers.GetCurrentClusterOp(found)
+ g.Expect(err).ToNot(HaveOccurred(), "Error occurred
while finding clusterLock for SolrCloud")
+ g.Expect(clusterOp).To(BeNil(), "StatefulSet should not
have a balanceReplicas lock after balancing is complete.")
+ })
+
+ // This takes effect immediately (e.g. operater starts using it)
+ // We will perform one more rolling update to demonstrate that
the operater can still auth to Solr
Review Comment:
Typos in test comments/step text: 'operater' should be 'operator', and
'uesrname' should be 'username'.
##########
tests/e2e/test_utils_test.go:
##########
@@ -584,17 +585,88 @@ func generateBaseSolrCloud(replicas int)
*solrv1beta1.SolrCloud {
// Uses default password from docs : SolrRocks
// The hash is generated as: base64(sha256(sha256(salt+password)))
base64(salt))
// See
https://solr.apache.org/guide/solr/latest/deployment-guide/basic-authentication-plugin.html
-func generateSolrSecuritySecret(ctx context.Context, solrCloud
*solrv1beta1.SolrCloud) {
- securityJsonSecret := &corev1.Secret{
- ObjectMeta: metav1.ObjectMeta{
- Name: solrCloud.Name + "-security-secret",
- Namespace: solrCloud.Namespace,
- },
- StringData: map[string]string{
- "security.json": `{
+func generateBasicSolrSecuritySecret(ctx context.Context, solrCloud
*solrv1beta1.SolrCloud) {
+ generateSolrSecuritySecret(ctx, solrCloud, `{
+ "authentication": {
+ "blockUnknown": false,
+ "class": "solr.BasicAuthPlugin",
+ "credentials": {
+ "test-oper":
"IV0EHq1OnNrj6gvRCwvFwTrZ1+z1oBbnQdiVC3otuq0=
Ndd7LKvVBAaZIF0QAVi1ekCfAJXr1GGfLtRUXhgrF8c="
+ },
+ "realm": "Solr Basic Auth",
+ "forwardCredentials": false
+ },
+ "authorization": {
+ "class": "solr.RuleBasedAuthorizationPlugin",
+ "user-role": {
+ "test-oper": "test-oper"
+ },
+ "permissions": [
+ {
+ "name": "cluster",
+ "role": null
+ },
+ {
+ "name": "collections",
+ "role": null,
+ "collection": "*"
+ }
+ ]
+ }
+ }`)
+
+}
+
+// Uses default password from docs : SolrRocks
+// The hash is generated as: base64(sha256(sha256(salt+password)))
base64(salt))
+// See
https://solr.apache.org/guide/solr/latest/deployment-guide/basic-authentication-plugin.html
+// Reusing the password as the salt is not entirely clear
+//
+// Same as the basic security json, except we allow a new user that the
operater will use
Review Comment:
Typo in comment: 'operater' should be 'operator'.
##########
tests/e2e/solrcloud_security_json_test.go:
##########
@@ -50,7 +55,115 @@ var _ = FDescribe("E2E - SolrCloud - Security JSON", func()
{
solrCloud = expectSolrCloudToBeReady(ctx, solrCloud)
By("creating a first Solr Collection")
- createAndQueryCollection(ctx, solrCloud, "basic", 1, 1)
+ createAndQueryCollection(ctx, solrCloud, "basic", 2, 1)
+
+ // Because overwrite=false, this won't apply new security json
+ // Pods will start back up, demonstrating that this was not
applied
+ By("updating security.json secret")
+ generateBreakingSolrSecuritySecret(ctx, solrCloud)
+
+ patchedSolrCloud := solrCloud.DeepCopy()
+ patchedSolrCloud.Spec.CustomSolrKubeOptions.PodOptions =
&solrv1beta1.PodOptions{
+ Annotations: map[string]string{
+ "test": "restart-1",
+ },
+ }
+ By("triggering a restart via pod annotations")
+ Expect(k8sClient.Patch(ctx, patchedSolrCloud,
client.MergeFrom(solrCloud))).To(Succeed(), "Could not add annotation to
SolrCloud pod to initiate restart")
+
+ // Trimmed down check where we just want the rolling restart to
begin then end, as we're not testing restarts here
+ By("waiting for the first restart to begin")
+ expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, cloud
*solrv1beta1.SolrCloud) {
+ g.Expect(cloud.Status.UpToDateNodes).To(BeZero(),
"Cloud did not get to a state with zero up-to-date replicas when rolling
restart began.")
+ for _, nodeStatus := range cloud.Status.SolrNodes {
+ g.Expect(nodeStatus.SpecUpToDate).To(BeFalse(),
"Node not starting as out-of-date when rolling restart begins: %s",
nodeStatus.Name)
+ }
+ })
+
+ // After all pods are ready, make sure that the SolrCloud
status is correct
+ By("waiting for the first restart to complete")
+ expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, cloud
*solrv1beta1.SolrCloud) {
+
g.Expect(cloud.Status.UpToDateNodes).To(Equal(int32(2)), "The SolrCloud did not
finish the rolling restart as not all nodes are up to date. Only reached %d",
solrCloud.Status.UpToDateNodes)
+
g.Expect(cloud.Status.ReadyReplicas).To(Equal(int32(2)), "The SolrCloud did not
finish the rolling restart as not all nodes are ready. Only reached %d",
solrCloud.Status.ReadyReplicas)
+ })
+
+ By("waiting for the balanceReplicas to finish")
+ expectStatefulSetWithChecksAndTimeout(ctx, solrCloud,
solrCloud.StatefulSetName(), time.Second*70, time.Second, func(g Gomega, found
*appsv1.StatefulSet) {
+ clusterOp, err := controllers.GetCurrentClusterOp(found)
+ g.Expect(err).ToNot(HaveOccurred(), "Error occurred
while finding clusterLock for SolrCloud")
+ g.Expect(clusterOp).To(BeNil(), "StatefulSet should not
have a balanceReplicas lock after balancing is complete.")
+ })
+
+ By("Waiting for the SolrCloud to come up healthy with old
security secret")
+ expectSolrCloudToBeReady(ctx, solrCloud)
+
+ By("querying existing Solr Collection")
+ queryCollection(ctx, solrCloud, "basic", 0, 0)
+
+ // Now with overwrite set to true, we'll update the applied
security to allow "new-oper" user
+ // Operator will continue to use "test-oper" user until we
update the basic auth secret
+ By("updating security.json secret")
+ generateUpdatedSolrSecuritySecret(ctx, solrCloud)
+
+ secondPatchedSolrCloud := solrCloud.DeepCopy()
+
secondPatchedSolrCloud.Spec.SolrSecurity.BootstrapSecurityJson.Overwrite = true
+ secondPatchedSolrCloud.Spec.CustomSolrKubeOptions.PodOptions =
&solrv1beta1.PodOptions{
+ Annotations: map[string]string{
+ "test": "restart-2",
+ },
+ }
+
+ By("triggering a restart to overwrite the security.json")
+ Expect(k8sClient.Patch(ctx, secondPatchedSolrCloud,
client.MergeFrom(solrCloud))).To(Succeed(), "Could not update security spec for
SolrCloud pod to initiate restart")
+
+ // Trimmed down check where we just want the rolling restart to
begin then end, as we're not testing restarts here
+ By("waiting for the second restart to begin")
+ newFoundCloud := expectSolrCloudWithChecks(ctx, solrCloud,
func(g Gomega, cloud *solrv1beta1.SolrCloud) {
+ g.Expect(cloud.Status.UpToDateNodes).To(BeZero(),
"Cloud did not get to a state with zero up-to-date replicas when rolling
restart began.")
+ })
+
+ // After all pods are ready, make sure that the SolrCloud
status is correct
+ By("waiting for the second restart to complete")
+ expectSolrCloudWithChecks(ctx, newFoundCloud, func(g Gomega,
cloud *solrv1beta1.SolrCloud) {
+
g.Expect(cloud.Status.UpToDateNodes).To(Equal(int32(2)), "The SolrCloud did not
finish the rolling restart as not all nodes are up to date. Only reached %d",
solrCloud.Status.UpToDateNodes)
+
g.Expect(cloud.Status.ReadyReplicas).To(Equal(int32(2)), "The SolrCloud did not
finish the rolling restart as not all nodes are ready. Only reached %d",
solrCloud.Status.ReadyReplicas)
+ })
+
+ By("waiting for the balanceReplicas to finish")
+ expectStatefulSetWithChecksAndTimeout(ctx, newFoundCloud,
newFoundCloud.StatefulSetName(), time.Second*70, time.Second, func(g Gomega,
found *appsv1.StatefulSet) {
+ clusterOp, err := controllers.GetCurrentClusterOp(found)
+ g.Expect(err).ToNot(HaveOccurred(), "Error occurred
while finding clusterLock for SolrCloud")
+ g.Expect(clusterOp).To(BeNil(), "StatefulSet should not
have a balanceReplicas lock after balancing is complete.")
+ })
+
+ // This takes effect immediately (e.g. operater starts using it)
+ // We will perform one more rolling update to demonstrate that
the operater can still auth to Solr
+ By("updating basic auth secret with new username")
+ generateSolrBasicAuthSecret(ctx, solrCloud, "new-oper")
+
+ finalPatchedSolrCloud := solrCloud.DeepCopy()
+
finalPatchedSolrCloud.Spec.SolrSecurity.BootstrapSecurityJson.Overwrite = true
+ finalPatchedSolrCloud.Spec.CustomSolrKubeOptions.PodOptions =
&solrv1beta1.PodOptions{
+ Annotations: map[string]string{
+ "test": "restart-3",
+ },
+ }
+
+ By("triggering final restart to show operater authorizing with
new uesrname")
Review Comment:
Typos in test comments/step text: 'operater' should be 'operator', and
'uesrname' should be 'username'.
--
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]
