ObjectStorageS3Secret field is immutable

[StopMotionCuber]

Version
Pulp operator v1.0.0-beta.1, Image quay.io/pulp/pulp-operator@sha256:ab39229f535d6eaeaa7502e39581ba0c89319400512ac51f99ee8b905449b617

Pulp version is 3.32

Describe the bug
I was trying to update the ObjectStorageS3Secret field (or object_storage_s3_secret in json representation),
as we wanted to move to a new S3 bucket

To Reproduce
Steps to reproduce the behavior:

1. Create Pulp resource with valid object_storage_s3_secret defined. We did so via ArgoCD, but any other means should also work
2. Update the object_storage_s3_secret to a new value with another valid secret pointing to an S3 configuration and apply that change to the cluster.

Expected behavior
The secret is updated and the settings.py is re-rendered. Best case would be that the pods would get restarted to pick up the new settings.py

Actual Behavior
The following error occurs:

2023-09-25T14:50:34Z	ERROR	repo_manager/utils.go:177	Could not update ObjectStorageS3Secret field	{"error": "ObjectStorageS3Secret field is immutable"}

Afterwards the update is reverted and ArgoCD shows the cluster state as out of sync.

Additional context
This is caused by this function. I'm not sure how exactly the logic behind the immutability works, but I think this field should not be immutable

[git-hyagi]

Hi @StopMotionCuber,

Thank you for the detailed description!
The immutable field is the intended behavior, we thought that it would be better to block changes as a "safe measure".
To modify the S3 bucket (or any other s3 configuration) you can keep using the same S3 Secret resource and modify the Secret content itself.
For example:

$ kubectl get secrets pulp-object-storage -oyaml > /tmp/tmp_secret.yaml
$ echo -n "my-new-bucket-name" |base64
bXktbmV3LWJ1Y2tldC1uYW1l
$ sed -i 's/s3-bucket-name:\s.*/s3-bucket-name: bXktbmV3LWJ1Y2tldC1uYW1l/' /tmp/tmp_secret.yaml
$ kubectl apply -f /tmp/tmp_secret.yaml

After that, as you mentioned, the operator will automatically update the settings.py file and restart pulpcore pods to get the new configuration.

[StopMotionCuber]

Ah, I see. Yes, updating the secret was the approach that we followed in the end.
Personally I would disagree with the design decision to have immutability for this field in this specific case, but then again I'm not the maintainer and I don't bother me too much.

What I think is a bad design for an operator is to change the Spec field of a resource it doesn't create itself and throwing the UNO reverse card on the change that you made and that completed without errors.
I think having a validating webhook and flat out rejecting changes on immutable fields would be the cleaner and more K8s-like approach

[git-hyagi]

I totally agree with you on the usage of validating webhook.
When I was working on the immutable fields my first idea was to use the validating webhook but, since it would require a cert-manager operator or users would need to manually configure the certificates, after some tests we decided not to use it to avoid more burden for the operator adoption. If you think about it, the immutable field behavior is kind of a "home made" validating webhook (in both cases we are trying to deny a non-supported change).

The Pulp team is open to discussion and criticism, please, do criticize pulp-operator. The community feedback is very valuable.
If you still believe that we should remove the immutable field and put the logic into the validating webhook, what do you think about opening the discussion to a broader audience (creating a thread on discourse) and, if other community users are fine with the change, we work on it?