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.

Sign up for GitHub

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

Jump to bottom

cache.external_cache_secret does not get updated in Pulp CR #1343

Open
vkukk opened this issue Sep 9, 2024 · 6 comments · May be fixed by #1355
Open

cache.external_cache_secret does not get updated in Pulp CR #1343

vkukk opened this issue Sep 9, 2024 · 6 comments · May be fixed by #1355
Labels
enhancement New feature or request

Comments

@vkukk
Copy link

vkukk commented Sep 9, 2024

Version
Please provide the versions of the pulp-operator and pulp images in use.
helm -n pulp list
NAME NAMESPACE REVISION UPDATED STATUS CHART APP VERSION
pulp pulp 1 2024-09-03 16:54:04.919341315 +0300 EEST deployed pulp-operator-0.1.0 1.0.1-beta.4
default images

Describe the bug
When changing field cache.external_cache_secret value and then applying it using 'kubectl apply -f pulp.yaml'

apiVersion: repo-manager.pulpproject.org/v1beta2
kind: Pulp
metadata:
  name: pulp
  namespace: pulp
spec:
  object_storage_s3_secret: s3-secret

  database:
    # when changing pulp-postgresql env values, update secret suffix here
    external_db_secret: pulp-postgresql-secret-kfd8672bgh

  cache:
    enabled: true
    external_cache_secret: pulp-redis-secret-8h6c85m7d4

  api:
    replicas: 2
    resource_requirements:
      requests:
        cpu: 250m
        memory: 256Mi
      limits:
        cpu: 1
        memory: 512Mi

  content:
    replicas: 2
    resource_requirements:
      requests:
        cpu: 250m
        memory: 256Mi
      limits:
        cpu: 500m
        memory: 512Mi

  worker:
    replicas: 4
    resource_requirements:
      requests:
        cpu: 500m
        memory: 500Mi
      limits:
        cpu: 2
        memory: 1Gi

Now check what are the actual running Pulp CR properties:

$ kubectl -n pulp get pulp -oyaml
apiVersion: v1
items:
- apiVersion: repo-manager.pulpproject.org/v1beta2
  kind: Pulp
  metadata:
    annotations:
      kubectl.kubernetes.io/last-applied-configuration: |
        {"apiVersion":"repo-manager.pulpproject.org/v1beta2","kind":"Pulp","metadata":{"annotations":{},"name":"pulp","namespace":"pulp"},"spec":{"api":{"replicas":2,"resource_requirements":{"limits":{"cpu":1,"memory":"512Mi"},"requests":{"cpu":"250m","memory":"256Mi"}}},"cache":{"enabled":true,"external_cache_secret":"pulp-redis-secret-8h6c85m7d4"},"content":{"replicas":2,"resource_requirements":{"limits":{"cpu":"500m","memory":"512Mi"},"requests":{"cpu":"250m","memory":"256Mi"}}},"database":{"external_db_secret":"pulp-postgresql-secret-kfd8672bgh"},"object_storage_s3_secret":"s3-secret","worker":{"replicas":4,"resource_requirements":{"limits":{"cpu":2,"memory":"1Gi"},"requests":{"cpu":"500m","memory":"500Mi"}}}}}
    creationTimestamp: "2024-09-05T08:15:23Z"
    generation: 35
    name: pulp
    namespace: pulp
    resourceVersion: "44167475287"
    uid: c4b2e685-c6d1-4ca6-ab07-ae34a165f567
  spec:
    admin_password_secret: pulp-admin-password
    api:
      gunicorn_timeout: 90
      gunicorn_workers: 2
      replicas: 2
      resource_requirements:
        limits:
          cpu: 1
          memory: 512Mi
        requests:
          cpu: 250m
          memory: 256Mi
    cache:
      enabled: true
      external_cache_secret: pulp-redis-secret-88tk2thgc5
    container_auth_private_key_name: container_auth_private_key.pem
    container_auth_public_key_name: container_auth_public_key.pem
    container_token_secret: pulp-container-auth
    content:
      gunicorn_timeout: 90
      gunicorn_workers: 2
      replicas: 2
      resource_requirements:
        limits:
          cpu: 500m
          memory: 512Mi
        requests:
          cpu: 250m
          memory: 256Mi
    database:
      external_db_secret: pulp-postgresql-secret-kfd8672bgh
    db_fields_encryption_secret: pulp-db-fields-encryption
    deployment_type: pulp
    image: quay.io/pulp/pulp-minimal
    image_pull_policy: IfNotPresent
    image_version: stable
    image_web: quay.io/pulp/pulp-web
    image_web_version: stable
    mount_trusted_ca: false
    object_storage_s3_secret: s3-secret
    pulp_secret_key: pulp-secret-key
    worker:
      replicas: 4
      resource_requirements:
        limits:
          cpu: 2
          memory: 1Gi
        requests:
          cpu: 500m
          memory: 500Mi
  status:
    admin_password_secret: pulp-admin-password
    conditions:
    - lastTransitionTime: "2024-09-09T10:01:09Z"
      message: pulp operator tasks running
      reason: OperatorRunning
      status: "False"
      type: Pulp-Operator-Finished-Execution
    - lastTransitionTime: "2024-09-09T10:01:09Z"
      message: Reconciling pulp-server Secret
      reason: UpdatingSecret
      status: "False"
      type: Pulp-API-Ready
    - lastTransitionTime: "2024-09-09T10:01:10Z"
      message: Reconciling pulp-content Deployment
      reason: UpdatingDeployment
      status: "False"
      type: Pulp-Content-Ready
    - lastTransitionTime: "2024-09-09T10:04:39Z"
      message: Worker deployment not ready yet
      reason: UpdatingWorkerDeployment
      status: "False"
      type: Pulp-Worker-Ready
    - lastTransitionTime: "2024-09-05T08:15:25Z"
      message: All Web tasks ran successfully
      reason: WebTasksFinished
      status: "True"
      type: Pulp-Web-Ready
    container_token_secret: pulp-container-auth
    db_fields_encryption_secret: pulp-db-fields-encryption
    deployment_type: pulp
    external_cache_secret: pulp-redis-secret-88tk2thgc5
    image: quay.io/pulp/pulp-minimal:stable
    last_deployment_update: "2024-09-09T10:01:09Z"
    object_storage_s3_secret: s3-secret
    pulp_secret_key: pulp-secret-key
kind: List
metadata:
  resourceVersion: ""

external_cache_secret is pulp-redis-secret-88tk2thgc5 but should be pulp-redis-secret-8h6c85m7d4
To Reproduce
Change cache.external_cache_secret value and appy changes.
Change is not reflected in Pulp CR in Kube cluster.

Expected behavior
Secret name is updated.

Deplyment is not ready due to broken secret that can't be updated to new secret.
@git-hyagi
Copy link
Collaborator

Hi, @vkukk!

This is because of a design decision we took a long time ago, as a safe measure, to avoid users losing data. Here is another similar issue with more details: #1096

From #1096 and this issue, it seems like this "field config rollback" is causing more confusion than being helpful, do you think we should allow modifying the storage fields? Right now, whenever a user needs to change the storage, we recommend doing a new installation.

@StopMotionCuber any inputs?

note: since k8s 1.29 the transition rules seems to be GA, so maybe we can revisit this instead of using the validation webhooks or the current "rollback" implementation.

@StopMotionCuber
Copy link
Contributor

Ah, the Useless Machine behavior again.

As I'm actively asked for input here, frankly I do not really get why those fields should not be changed.

This is because of a design decision we took a long time ago, as a safe measure, to avoid users losing data

I do not get how that is supposed to be the case. Thinking of user stories where this would prevent data loss but none come to my mind that is not an obvious misconfiguration from a k8s admin. A (sane) kubernetes admin is aware that changing your S3 bucket to a new one which is missing the previous data, would lead to errors. But that misconfiguration is already achievable by updating the secret itself. And if I want to have another way to shoot myself in the foot I could find 20+ ways to do so.

On the other hand, I see use cases for migrating to a new bucket (or redis cluster), doing internal cluster changes regarding secret management (e.g. migrating from directly placing secrets on the cluster to using vault). All of these might profit from updating your secret, depending on your migration workflow.

In the end though I'm not the one having to maintain the operator, so it's not on me to decide. Recreating the pulp resource is still possible and provides a workaround for my use cases as a short downtime is acceptable for my use cases.

@vkukk
Copy link
Author

vkukk commented Sep 11, 2024

When I create Pulp CR, I expect the pulp-operator to read and use what I've created. Not to ignore the provided configuration.

Making mistakes is entirely different problem and should not be handled by ignoring users/admin configuration changes.

@gerrod3 gerrod3 added enhancement New feature or request and removed Issue Triage-Needed labels Sep 17, 2024
@gerrod3
Copy link

gerrod3 commented Sep 17, 2024

We probably won't be able to get around to changing the behavior anytime soon. We would appreciate any contributions.

@StopMotionCuber
Copy link
Contributor

We probably won't be able to get around to changing the behavior anytime soon. We would appreciate any contributions.

Could you quickly confirm that you are open to a change in behavior here (that is, not reverting the change back)? I think code-wise it's rather trivial to implement, I took a look at it the last time I stumbled upon it. I think the bigger issue here is agreeing on a way forward, and obviously as an outside contributor these kind of decision processes are hard to gain insights to.

Should the answer here be a "yes, we're open to the change" I could prepare a PR

@gerrod3
Copy link

gerrod3 commented Sep 17, 2024

Yes, we would be open to the change. 😄

StopMotionCuber added a commit to StopMotionCuber/pulp-operator that referenced this issue Sep 18, 2024
@StopMotionCuber
Make secret fields mutable
350b3e7 
To allow for migrations, leveraging different secrets for the same pulp instance, fields have been made mutable

Closes pulp#1343
@StopMotionCuber StopMotionCuber linked a pull request Sep 18, 2024 that will close this issue
Open
StopMotionCuber added a commit to StopMotionCuber/pulp-operator that referenced this issue Sep 18, 2024
@StopMotionCuber
Make secret fields mutable
aa893a3 
To allow for migrations, leveraging different secrets for the same pulp instance, fields have been made mutable

Closes pulp#1343
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make secret fields not immutable anymore StopMotionCuber/pulp-operator
4 participants
@StopMotionCuber @gerrod3 @vkukk @git-hyagi