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

do not generate new passwords if the secrets already exist #414

Merged
merged 10 commits into from
Feb 25, 2021

Conversation

jsanda
Copy link
Contributor

@jsanda jsanda commented Feb 23, 2021

What this PR does:

Which issue(s) this PR fixes:
Fixes #413

Checklist

  • Changes manually tested
  • Chart versions updated (if necessary)
  • Automated Tests added/updated
  • Documentation added/updated
  • CLA Signed: DataStax CLA

@jsanda jsanda added this to the 1.0.0 milestone Feb 23, 2021
@jsanda jsanda self-assigned this Feb 23, 2021
@jsanda jsanda added this to In progress in K8ssandra via automation Feb 23, 2021
@jsanda jsanda marked this pull request as ready for review February 23, 2021 20:11
@jsanda
Copy link
Contributor Author

jsanda commented Feb 23, 2021

When reviewing and testing changes, check the passwords of the secrets that get created. Make sure that they do not change after running helm upgrade.

@@ -14,6 +22,6 @@ data:
{{- if .Values.reaper.jmx.password }}
password: {{ .Values.reaper.jmx.password | b64enc | quote }}
{{- else }}
password: {{ include "k8ssandra-common.password" . }}
password: {{ $password }}
Copy link
Contributor

Choose a reason for hiding this comment

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

So this tackles the password, but is the username guaranteed to not be regenerated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch :) I missed that. I will update that now.

K8ssandra automation moved this from In progress to Reviewer approved Feb 24, 2021
Copy link
Contributor

@jdonenine jdonenine left a comment

Choose a reason for hiding this comment

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

I was able to test and confirm that the secrets remained unchanged between helm upgrade attempts.

I'm not sure if @burmanm was still reviewing or not, but I'll go ahead and approve so that you can move forward if you already talked to him @jsanda before he went offline for the night over there.

{{- include "k8ssandra.clusterName" . }}-superuser
{{- end }}

{{- define "k8ssandra.reaperUserSecrentName" }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo here in Secrent

@jsanda jsanda requested a review from burmanm February 24, 2021 04:53
{{- if and .Values.cassandra.auth.superuser.username (not .Values.cassandra.auth.superuser.secret) .Values.cassandra.auth.enabled }}

{{- if $secret }}
{{- $password = index $secret.data "password" }}
Copy link
Contributor

Choose a reason for hiding this comment

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

This question applies to this, medusa, reaper and stargate.. if I had a username in the Values-file, but changed it - won't my secrets be all messed up in the upgrade process? Currently, only the existing password is fetched in these, but not the existing username. So instead of existing username, we use the one from the values file, but old password. I'll be honest, I have no idea how messed up we could make the system if secret exists, but these are targetting a different username.

Copy link
Contributor Author

@jsanda jsanda Feb 24, 2021

Choose a reason for hiding this comment

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

if I had a username in the Values-file, but changed it - won't my secrets be all messed up in the upgrade process?

I tested this a bit, but it probably deserves more testing. I would not consider the secret messed up in this scenario because the user is intentionally changing them. What are the consequences? If the superuser username was foo and I change it to bar, then after upgrade I will have two superusers. For 1.0, we really cannot do anything other than document this behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For a first test, I installed with this configuration:

cassandra:
  auth:
    enabled: true
  datacenters:
  - name: dc1
    size: 1
reaper:
  enabled: true
stargate:
  enabled: true
  replicas: 1
reaper:
  enabled: true
  cassandraUser:

That results in the creation of a secret named test-reaper and a Cassandra role/user named reaper with password U59R30EzNxcNUjqi1koK. I then changed my configuration to:

cassandra:
  auth:
    enabled: true
  datacenters:
  - name: dc1
    size: 1
reaper:
  enabled: true
stargate:
  enabled: true
  replicas: 1
reaper:
  enabled: true
  cassandraUser:
    username: reaper-admin

After upgrading the username in the secret was changed to reaper-admin. The password in the secret did not. A new role was created, reaper-admin, same password as for reaper role.

I think a really good question is what if anything should happen to the reaper role. Should it be deleted in this case? Even if the answer is yes, it would be a post-1.0 change and probably require changes in cass-operator.

There is an additional change that I think should be made. When I change the username, I think a different password should be generated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I agree on all fronts there. I would generally expect that if the username is changed, that should effectively create a new user, not reuse the password from the older account. I also think the cleanup of the left-over role can wait until post-1.0.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/datastax/cass-operator/issues/382 is related and something with which we need to be involved.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with postponing these to post-1.0

Copy link
Contributor

@jdonenine jdonenine left a comment

Choose a reason for hiding this comment

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

@jsanda and I have been working together through each series of changes and I just wrapped up some end-to-end testing and things look good. Approving and we'll let @burmanm have another pass over things when he comes online and we should be able to merge!

@burmanm burmanm merged commit 158a80a into k8ssandra:main Feb 25, 2021
K8ssandra automation moved this from Reviewer approved to Done Feb 25, 2021
jeffreyscarpenter pushed a commit to jeffreyscarpenter/k8ssandra that referenced this pull request Mar 10, 2021
…#414)

* do not generate new passwords if the secrets already exist

* add helper templates

* do not regenerate username for reaper jmx secret

* fix typo

* Bring remaining typos in sync.

* add logic to handle usernames

If the secret exists, reuse the existing username.

If a username is provided, use the latter.

If secret does not exist and no username specified, use default.

* fix logic for reaper and stargate users

* update logic for medusa secret and fix logic for when to reuse secret (thanks Jeff D)

* update logic for default superuser and repaer jmx user

* update logic to determine if secrets should be created and add docs

Co-authored-by: Jeff DiNoto <jeff.dinoto@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
K8ssandra
  
Done
Development

Successfully merging this pull request may close these issues.

Running helm upgrade when auth is enabled can cause a cluster rolling restart
3 participants