gerlowskija commented on code in PR #744:
URL: https://github.com/apache/solr-operator/pull/744#discussion_r1937407517
##########
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
Review Comment:
[Q] Am I reading this correctly, that the first two blocks both upload the
JSON and only differ in the message they echo out? There might be a better way
to structure that to avoid the duplication...
##########
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:"probesRequireAuth,omitempty"`
Review Comment:
[-1] The 'probesRequireAuth' bit looks like a copy/paste error, unless
there's a reason I'm missing to use that name for the yaml property?
##########
config/crd/bases/solr.apache.org_solrclouds.yaml:
##########
@@ -10205,29 +10205,37 @@ spec:
description: |-
Configure a user-provided security.json from a secret to
allow for advanced security config.
If not specified, the operator bootstraps a
security.json with basic auth enabled.
- This is a bootstrapping config only; once Solr is
initialized, the security config should be managed by the security API.
properties:
- key:
- description: The key of the secret to select from.
Must be
- a valid secret key.
- type: string
- name:
- default: ""
+ bootstrapSecurityJson:
+ description: SecretKeySelector selects a key of a
Secret.
+ properties:
+ key:
+ description: The key of the secret to select from.
Must
+ be a valid secret key.
+ type: string
+ name:
+ default: ""
+ description: |-
+ Name of the referent.
+ This field is effectively required, but due to
backwards compatibility is
+ allowed to be empty. Instances of this type with
an empty value here are
+ almost certainly wrong.
+ More info:
https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
+ type: string
+ optional:
+ description: Specify whether the Secret or its key
must
+ be defined
+ type: boolean
+ required:
+ - key
+ type: object
+ x-kubernetes-map-type: atomic
+ probesRequireAuth:
Review Comment:
[0] See above comment in solrcloud_types.go about this name likely being a
copy/paste error.
##########
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:
[0] I've run into the "RAW_GINKGO unset" error before but was never sure
whether it was a bad/unnecessary assumption made by the script, or whether it
indicated an actual environmental issue on my part.
I don't understand the GINKGO stuff in general well enough to chime in.
Curious if you had any thoughts @HoustonPutman ?
##########
api/v1beta1/solrcloud_types.go:
##########
@@ -1621,6 +1624,15 @@ const (
Basic AuthenticationType = "Basic"
)
+type BootstrapSecurityJson struct {
Review Comment:
[-0] If I'm reading this right, the YAML path for this property would be
`.solrSecurity.bootstrapSecurityJson.bootstrapSecurityJson`?
Would have to think a bit about how to fix it, but the redundancy there
feels like it'd be confusing to users IMO.
##########
api/v1beta1/solrcloud_types.go:
##########
@@ -1649,7 +1661,5 @@ type SolrSecurityOptions struct {
// Configure a user-provided security.json from a secret to allow for
advanced security config.
// If not specified, the operator bootstraps a security.json with basic
auth enabled.
- // This is a bootstrapping config only; once Solr is initialized, the
security config should be managed by the security API.
- // +optional
- BootstrapSecurityJson *corev1.SecretKeySelector
`json:"bootstrapSecurityJson,omitempty"`
+ BootstrapSecurityJson *BootstrapSecurityJson
`json:"bootstrapSecurityJson,omitempty"`
Review Comment:
[0] I'm a bit torn on the CRD/interface for this functionality. Continuing
to have "bootstrap" in the name feels a little misleading if this becomes
something that isn't just about initial creation.
But is it worth the hassle of deprecating this section and creating a nearly
identical one with a clearer name, idk.
Ignore my musing here, just thinking aloud.
##########
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 '{}'
Review Comment:
[Q] I guess the unmatched `]` is a bash syntax error that pre-exists your
PR? I wonder how we didn't notice it sooner: presumably it would've caused
errors for the initContainer or elsewhere?
Anyways, great catch!
--
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]
