Re: [PR] feat(update-security-json): Update security.json when secret is updated [solr-operator]
mcarroll1 commented on code in PR #744:
URL: https://github.com/apache/solr-operator/pull/744#discussion_r1939622256
##
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:
Agreed that this bit of bash could probably be optimized. I don't
immediately see a way of re-arranging the conditions. Maybe I can just create
function that takes "message" as an input?
--
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]
Re: [PR] feat(update-security-json): Update security.json when secret is updated [solr-operator]
mcarroll1 commented on code in PR #744:
URL: https://github.com/apache/solr-operator/pull/744#discussion_r1939611644
##
api/v1beta1/solrcloud_types.go:
##
@@ -1621,6 +1624,15 @@ const (
Basic AuthenticationType = "Basic"
)
+type BootstrapSecurityJson struct {
Review Comment:
Agreed, this is a bit ugly. I'll wait for resolution on the naming change
before changing this
--
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]
Re: [PR] feat(update-security-json): Update security.json when secret is updated [solr-operator]
mcarroll1 commented on code in PR #744:
URL: https://github.com/apache/solr-operator/pull/744#discussion_r1939586626
##
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:
Resolved!
##
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:
Resolved
--
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]
Re: [PR] feat(update-security-json): Update security.json when secret is updated [solr-operator]
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 whe
[PR] feat(update-security-json): Update security.json when secret is updated [solr-operator]
mcarroll1 opened a new pull request, #744: URL: https://github.com/apache/solr-operator/pull/744 - Added `Ovewrite` flag (defaults to false). If flag is true, setup-zk initContainer will check applied security.json against version in the secret and apply if there is a diff - Updated descriptions about `maxPodsUnavailable`, as negative numbers don't appear to be work - Was running into issues with `GINGKO_PARAMS` when running integration tests, so added a default I've updated the integration test for applying/updating security.json. I have this to the point that the feature is working via the test. However, I don't see an easy way to actually test the updated auth. My test attempts to adding a permission statement to the update handler and then force an HTTP 401. I've created a separate curl pod to handle this. However, I'm having trouble getting that curl to work. It took a lot of effort to get to this point in the test. Any advice here would be greatly appreciated. This feature is important to the CI for my team, hence me adding two features around security.json in short order. -- 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]
