Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: avoid reset of security.json if get fails (#659) #660

Merged

Conversation

smoldenhauer-ish
Copy link
Contributor

No description provided.

@smoldenhauer-ish
Copy link
Contributor Author

adapted the expected setup-zk command and tested locally make prepare check
and the end tests make e2e-tests SOLR_IMAGE=solr:8.11

@HoustonPutman
Copy link
Contributor

@smoldenhauer-ish your PR is from an organizational fork, so I'm unable to push a changelog entry, but can you merge main into your branch, then add the following changelog entry to helm/solr-operator/chart.yaml?

    - kind: fixed
      description: Avoid reset of security.json if get request fails  
      links:
        - name: Github Issue
          url: https://github.com/apache/solr-operator/issues/659
        - name: Github PR
          url: https://github.com/apache/solr-operator/pull/660

I'll be good to merge this after you do that!

@smoldenhauer-ish
Copy link
Contributor Author

Added the changes entry - The 'edit by maintainers' is only available to private owned forks?

@HoustonPutman HoustonPutman merged commit 5fec11f into apache:main Apr 2, 2024
3 checks passed
@@ -238,7 +238,7 @@ func addHostHeaderToProbe(httpGet *corev1.HTTPGetAction, host string) {

func cmdToPutSecurityJsonInZk() string {
scriptsDir := "/opt/solr/server/scripts/cloud-scripts"
cmd := " ZK_SECURITY_JSON=$(%s/zkcli.sh -zkhost ${ZK_HOST} -cmd get /security.json); "
cmd := " ZK_SECURITY_JSON=$(%s/zkcli.sh -zkhost ${ZK_HOST} -cmd get /security.json || echo 'failed-to-get-security.json'); "

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HoustonPutman
This change appears to cause SolrCloud pods to be restarted upon upgrade of the operator. Should this be expected for a patch release; it's not mentioned in the release notes at https://apache.github.io/solr-operator/docs/upgrade-notes.html ?

2024-04-18T14:04:15Z	INFO	Update required because field changed	{"controller": "solrcloud", "controllerGroup": "solr.apache.org", "controllerKind": "SolrCloud", "SolrCloud": {"name":"solrcloud-test-2","namespace":"solrcloud-test-2"}, "namespace": "solrcloud-test-2", "name": "solrcloud-test-2", "reconcileID": "8e7b7ebc-4536-4c7e-828a-2c65142f77b9", "statefulSet": "solrcloud-test-2-solrcloud", "kind": "statefulSet", "field": "Spec.Template.Spec.InitContainers[1].Command", 
"from": ["sh", "-c", " ZK_SECURITY_JSON=$(/opt/solr/server/scripts/cloud-scripts/zkcli.sh -zkhost ${ZK_HOST} -cmd get /security.json); if [ ${#ZK_SECURITY_JSON} -lt 3 ]; then echo $SECURITY_JSON > /tmp/security.json; /opt/solr/server/scripts/cloud-scripts/zkcli.sh -zkhost ${ZK_HOST} -cmd putfile /security.json /tmp/security.json; echo \"put security.json in ZK\"; fi"], 
"to":   ["sh", "-c", " ZK_SECURITY_JSON=$(/opt/solr/server/scripts/cloud-scripts/zkcli.sh -zkhost ${ZK_HOST} -cmd get /security.json || echo 'failed-to-get-security.json'); if [ ${#ZK_SECURITY_JSON} -lt 3 ]; then echo $SECURITY_JSON > /tmp/security.json; /opt/solr/server/scripts/cloud-scripts/zkcli.sh -zkhost ${ZK_HOST} -cmd putfile /security.json /tmp/security.json; echo \"put security.json in ZK\"; fi"]}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So generally we try to avoid pod updates for patch versions, however for security fixes or bad bugs, then it becomes necessary. There was a security fix that required a pod template update, so this bug fix seemed like a good idea to include as well. Sorry for the confusion though. I do remember writing docs somewhere that security fixes might necessitate pod upgrades on a patch fix, but I can't seem to find it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update; much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

security.json reset with bootstrap by init container setup-zk on zookeeper errors
3 participants