feat(update-security-json): Update security.json when secret is updated #744
Conversation
mcarroll1
force-pushed
the
488-update-security
branch
from
83702c2 to
1a7bb57
Compare
There was a problem hiding this comment.
Hey @mcarroll1 - sorry this sat for awhile before I was able to give it a look 😬
The code here looks pretty good IMO 👍. Aside from the few comments I left, the biggest thing that jumps out to me are "docs" - we've got a number of places that discuss our security.json support, and those will definitely need updated. Let me kick the tires a bit and think through the functionality before you tackle any of that though; I still need to think a bit deeper about the overall approach for my review to be worth much haha.
Hoping to chime back in here sometime next week with more thorough thoughts.
| @@ -1621,6 +1624,15 @@ const ( | |||
| Basic AuthenticationType = "Basic" | |||
| ) | |||
|
|
|||
| type BootstrapSecurityJson struct { | |||
There was a problem hiding this 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.
There was a problem hiding this comment.
Agreed, this is a bit ugly. I'll wait for resolution on the naming change before changing this
| // 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"` |
There was a problem hiding this 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.
| @@ -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 '{}' | |||
There was a problem hiding this 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!
| " 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 |
There was a problem hiding this 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...
There was a problem hiding this 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?
| @@ -130,7 +130,7 @@ function run_tests() { | |||
| GINKGO_PARAMS+=("${param}" "${!envName}") | |||
| fi | |||
| done | |||
| GINKGO_PARAMS+=("${RAW_GINKGO[@]}") | |||
| GINKGO_PARAMS+=("${RAW_GINKGO[@]:-}") | |||
There was a problem hiding this 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 ?
This feature is targeting this issue
Ovewriteflag (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 diffmaxPodsUnavailable, as negative numbers don't appear to be workGINGKO_PARAMSwhen running integration tests, so added a defaultThe integration test is a bit blunt, but I wasn't sure how to demonstrate auth being applied. I tried creating a curl pod to curl solr, but was facing issues connecting to the two. I ended up changing the auth that the operator connects to solr with as a means of demonstrating.
The test takes a couple minutes to run. Please give any feedback on what I could do differently there. You can see the curl attempt in 1a7bb57. Also I'm unsure if I updated the CRD correctly, as it's generating some unexpected diffs.
This feature is important to the CI for my team, hence me adding two features around security.json in short order.