Backward incompatibility in Reaper authentication when upgrading from 1.10 to 1.12

[c3-clement]

What happened?

We have multiple K8ssandra clusters running on k8ssandra-operator v1.10 with the following Reaper spec:

spec:
  reaper:
    uiUserSecretRef: {}

k8ssandra-operator will generate a secret named <cluster-name>-reaper-ui. I can use the username and password contained in this secret to log in to the Reaper UI.
In the Reaper Deployment, the env var REAPER_AUTH_ENABLED is set to "true".

After upgrading k8ssandra-operator to v1.12, it's possible to login into Reaper UI without authenticating.
Accessing /webui/ will not redirect the user to the login page.
In the Reaper Deployment, the env var REAPER_AUTH_ENABLED is now set to "false".

I believe that this backward incompatibility has been introduced in #1163 :
Prior to v1.12, setting uiUserSecretRef to {} will trigger Reaper UI secret reconciliation and REAPER_AUTH_ENABLED will be set true.

After v1.12, uiUserSecretRef has to be set to null to trigger the same behavior.

Did you expect to see something different?

I expect Reaper UI authentication to still be enabled when upgrading from v1.10 to v1.12 .

How to reproduce it (as minimally and precisely as possible):

1. Create a K8ssandra cluster with k8ssandra-operator v1.10 with the following Reaper spec:

spec:
  reaper:
    uiUserSecretRef: {}

2. Access to Reaper UI by using the reaper-ui secret
3. Upgrade k8ssandra-operator to v1.12 without changing the k8ssandra cluster spec
4. Try to access Reaper UI as in step 2.

Environment

• K8ssandra Operator version:
  v1.10 and v1.12

• Kubernetes version information:
  1.27

• Kubernetes cluster kind:
  AKS

• Manifests:

apiVersion: k8ssandra.io/v1alpha1
kind: K8ssandraCluster
spec:
  reaper:
    uiUserSecretRef: {}

[adejanovski]

Hi @c3-clement,

sorry for this, the problem we were trying to solve here is that it was impossible to distinguish the case where we wanted to disable auth in the Reaper UI and the case where we'd let the operator generate the credentials.
I guess we should've preserved backwards compatibility indeed by using "null" to disable authentication instead of an empty object, but we took the other route because it felt more natural to disable with the empty rather than null. Null being equivalent as unset in yaml land feels more like "let's let the system handle creating the credentials" while the empty object would be an explicit action to disable authentication.
I guess we should've thought about the behavior change though.
Sadly I think we need to stick with it now, which indeed requires a spec update 😕

[c3-clement]

Hi @c3-clement,

sorry for this, the problem we were trying to solve here is that it was impossible to distinguish the case where we wanted to disable auth in the Reaper UI and the case where we'd let the operator generate the credentials. I guess we should've preserved backwards compatibility indeed by using "null" to disable authentication instead of an empty object, but we took the other route because it felt more natural to disable with the empty rather than null. Null being equivalent as unset in yaml land feels more like "let's let the system handle creating the credentials" while the empty object would be an explicit action to disable authentication. I guess we should've thought about the behavior change though. Sadly I think we need to stick with it now, which indeed requires a spec update 😕

Thanks for your reply, I understand your design choice.

Sadly I think we need to stick with it now, which indeed requires a spec update

I understand that you have to stick with this behavior (otherwise it would introduce a new backward incompatibility ahah)

However, it would be great to have this behavior mentioned in CHANGELOG as a breaking change.

Also, I'm wondering if it could be a security issue? Some k8ssandra-operator users might expose the reaper UI over the Internet (not my case).

For potential future changes similar to #1163 , why not introduce a new boolean field disableAuth instead of making nullable an existing field?

[adejanovski]

I think we simply didn't realize that it would impact existing clusters.

Also, I'm wondering if it could be a security issue? Some k8ssandra-operator users might expose the reaper UI over the Internet (not my case).

I hope folks don't do this, but indeed it could be a security problem.
Thinking about it, we may have a shot at fixing this without changing the behavior again by just checking if a secret has been generated for Reaper already and enable auth if it exists 🤔
This would keep backwards compatibility while changing the behavior for new clusters (which could still come as a surprise then...).

@Miles-Garnsey, what's your take on this?

[Miles-Garnsey]

I agree with @c3-clement in many ways, I don't know if it is a "security issue" but I think it is fair to say that it is "surprising behaviour with a potential impact on security" which probably amounts to the same thing...

There are a few things going on here:

1. We intended that anyone defining uiUserSecretRef: "" would be explicitly stating that they didn't want auth, so it should be disabled for those users. In general, I'd probably suggest not defining fields in your manifests which you don't need to define, as this reduces the surface area for issues like this to take hold.
2. I did not realise that defining uiUserSecretRef: {} would actually map to a "" instead of a nil on the Go struct side, but this isn't necessarily fatal to the design of what we've done.
3. We were resistant to the idea of defining yet another field (e.g. authDisabled), since the convention in our APIs has historically been that defining an empty object (as opposed to a nil) explicitly disables functionality. We probably need to document some high level principles for our APIs so that this is clear to users.
4. I definitely agree that this change should have been in a release notes. We were not expecting there to be any user impact at all from this because we didn't think anyone would be defining either uiUserSecretRef: "" or indeed uiUserSecretRef: {}. Having read this report I realise now this was excessively optimistic.

I am sorry for the inconvenience @c3-clement, and I'm glad that this change didn't lead to any ill effects for you in this case. I think I will add a release note about this now for future users who are upgrading.

@adejanovski, I think the way to fix this is with a release notes entry. I'd be reluctant to build functionality in which inspects the state of the cluster for existing resources with a view to ensuring this sort of backwards compatibility. We've considered doing similar things a few times now (e.g. some of those malformed secrets that were created from pre-release versions of the closed source projects), and I think we'll end up with a lot of technical debt if we start trying to cater to every edge case.

[adejanovski]

@adejanovski, I think the way to fix this is with a release notes entry. I'd be reluctant to build functionality in which inspects the state of the cluster for existing resources with a view to ensuring this sort of backwards compatibility. We've considered doing similar things a few times now (e.g. some of those malformed secrets that were created from pre-release versions of the closed source projects), and I think we'll end up with a lot of technical debt if we start trying to cater to every edge case.

Fair enough 👍

[c3-clement]

Thanks for your reply and updating release notes @Miles-Garnsey

In general, I'd probably suggest not defining fields in your manifests which you don't need to define, as this reduces the surface area for issues like this to take hold.

Agree with you, but we are deploying K8ssandra clusters from our own operator by importing k8ssandra-operator as a library, instantiate the Go struct v1alpha1.K8ssandraCluster and then creating the resource by doing a call to the API server through the Kubernetes client provided by operator-sdk.
During this process, we instantiate Spec.Reaper to a non-nil struct and we set default values for some fields (not for Spec.Reaper.UiUserSecretRef)

So, Prior to v1.12, v1alpha1.K8ssandraCluster.Spec.Reaper.UiUserSecretRef was not a pointer. This means that in the K8ssandraCluster created in Kubernetes, UiUserSecretRef field would always be set to an empty non-null value (hence the uiUserSecretRef: {})

Then, when upgrading to v1.12, k8ssandra-operator will reconcile existing k8ssandra clusters and disable Reaper auth.
See what I mean?

The fix I found to avoid this issue is to set explicitly Reaper.UiUserSecretRef = nil in our operator after v1.12 upgrade. I think it's ok.

I know this behavior is not gonna change, but I think it will be pretty useful if you guys are aware of how we use the k8ssandra-operator project.

Appreciate this discussion!

[Miles-Garnsey]

Agree with you, but we are deploying K8ssandra clusters from our own operator by importing k8ssandra-operator as a library, instantiate the Go struct v1alpha1.K8ssandraCluster and then creating the resource by doing a call to the API server through the Kubernetes client provided by operator-sdk.

That's fair enough, however I'd still suggest that leaving this nil would have avoided your issue in this particular case.

The even more interesting thing is that you're operating on the Go structs directly, not the YAML API. We weren't aware that any users were doing this, and maybe we should be a little more respectful of that use case in future. Historically, we've been pretty free and easy about swapping pointer and value types, since this doesn't affect backwards compatibility for the YAML API.

Thanks for alerting us to the fact that some folks are doing this!

The fix I found to avoid this issue is to set explicitly Reaper.UiUserSecretRef = nil in our operator after v1.12 upgrade. I think it's ok.

This should indeed work. I believe that just the field absent entirely would also work, if that's a nicer solution.