Move service templates to be ptr to struct

stuggi commented 5 months ago

Moves template parameter in the service sections to be a ptr to have them omitted in case its not provided. Also only triggers the defaulting for the service in the webhook if the service is enabled or a template provided.

If the service was disabled and gets enabled, at least an empty template: {} needs to be passed for correct defaulting to happen.

As background, current operator behavior

This examples only show keystone and placement

When deploy with a service disabled, the template section gets added with no proper defaulting:

    keystone:
      apiOverride: {}
      enabled: false
      template:
        adminProject: ""
        adminUser: ""
        databaseAccount: ""
        databaseInstance: ""
        enableSecureRBAC: false
        memcachedInstance: ""
        override: {}
        passwordSelectors:
          admin: ""
        preserveJobs: false
        rabbitMqClusterName: ""
        region: ""
        replicas: 1
        resources: {}
        secret: ""
        tls:
          api:
            internal: {}
            public: {}
        trustFlushArgs: ""
        trustFlushSchedule: ""
        trustFlushSuspend: false
    placement:
      apiOverride: {}
      enabled: false
      template:
        customServiceConfig: ""
        databaseAccount: ""
        databaseInstance: ""
        override: {}
        passwordSelectors:
          service: ""
        preserveJobs: false
        replicas: 1
        resources: {}
        secret: ""
        serviceUser: ""
        tls:
          api:
            internal: {}
            public: {}

when enable e.g. placement and provide template: {}, the section obviously stays as is, as it was already filled before:

    placement:
      apiOverride: {}
      enabled: true
      template:
        customServiceConfig: ""
        databaseAccount: ""
        databaseInstance: ""
        override: {}
        passwordSelectors:
          service: ""
        preserveJobs: false
        replicas: 1
        resources: {}
        secret: ""
        serviceUser: ""
        tls:
          api:
            internal: {}
            public: {}

the created placementapi cr gets partially defaulted, no passwordSelector, no databaseAccount:

spec:
  containerImage: quay.io/podified-antelope-centos9/openstack-placement-api@sha256:c36e9c0adf778926963592df947c025b87e05099324c8b9c0a0c5b300727ebf2
  customServiceConfig: ""
  databaseAccount: ""
  databaseInstance: openstack
  override:
    service:
      internal:
        metadata:
          labels:
            osctlplane: ""
            osctlplane-service: placement
      public:
        metadata:
          labels:
            osctlplane: ""
            osctlplane-service: placement
  passwordSelectors:
    service: ""
  preserveJobs: false
  replicas: 1
  resources: {}
  secret: osp-secret
  serviceUser: ""
  tls:
    api:
      internal: {}
      public: {}
    caBundleSecretName: combined-ca-bundle

with keystone/placement enabled from start with template: {} the default works properly

keystone:
  apiOverride: {}
  enabled: true
  template:
    adminProject: admin
    adminUser: admin
    databaseAccount: keystone
    databaseInstance: ""
    enableSecureRBAC: true
    memcachedInstance: memcached
    override: {}
    passwordSelectors:
      admin: AdminPassword
    preserveJobs: false
    rabbitMqClusterName: rabbitmq
    region: regionOne
    replicas: 1
    resources: {}
    secret: ""
    tls:
      api:
        internal: {}
        public: {}
    trustFlushArgs: ""
    trustFlushSchedule: 1 * * * *
    trustFlushSuspend: false
placement:
  apiOverride: {}
  enabled: true
  template:
    customServiceConfig: ""
    databaseAccount: placement
    databaseInstance: ""
    override: {}
    passwordSelectors:
      service: PlacementPassword
    preserveJobs: false
    replicas: 1
    resources: {}
    secret: ""
    serviceUser: placement
    tls:
      api:
        internal: {}
        public: {}

operator behavior with the change

When osctlplane gets created with service disabled and no template provided, no template is defaulted and shown
```
keystone:
  apiOverride: {}
  enabled: false
placement:
  apiOverride: {}
  enabled: false
```

when enable the service and provide template: {} , the defaulting happens properly

keystone:
  apiOverride: {}
  enabled: true
  template:
    adminProject: admin
    adminUser: admin
    databaseAccount: keystone
    databaseInstance: ""
    enableSecureRBAC: true
    memcachedInstance: memcached
    override: {}
    passwordSelectors:
      admin: AdminPassword
    preserveJobs: false
    rabbitMqClusterName: rabbitmq
    region: regionOne
    replicas: 1
    resources: {}
    secret: ""
    tls:
      api:
        internal: {}
        public: {}
    trustFlushArgs: ""
    trustFlushSchedule: 1 * * * *
    trustFlushSuspend: false

placement:
  apiOverride: {}
  enabled: true
  template:
    customServiceConfig: ""
    databaseAccount: placement
    databaseInstance: ""
    override: {}
    passwordSelectors:
      service: PlacementPassword
    preserveJobs: false
    replicas: 1
    resources: {}
    secret: ""
    serviceUser: placement
    tls:
      api:
        internal: {}
        public: {}

Also the resulting service gets defaulted as expected resulting service:

spec:
  adminProject: admin
  adminUser: admin
  containerImage: quay.io/podified-antelope-centos9/openstack-keystone@sha256:8616532664fb47a2fc0b6c9176a765d50e5e1d1e983155c6620fb63d16a8510a
  databaseAccount: keystone
  databaseInstance: openstack
  enableSecureRBAC: true
  memcachedInstance: memcached
  override:
    service:
      internal:
        metadata:
          labels:
            osctlplane: ""
            osctlplane-service: keystone
      public:
        endpointURL: https://keystone-public-openstack.apps-crc.testing
        metadata:
          labels:
            osctlplane: ""
            osctlplane-service: keystone
  passwordSelectors:
    admin: AdminPassword
  preserveJobs: false
  rabbitMqClusterName: rabbitmq
  region: regionOne
  replicas: 1
  resources: {}
  secret: osp-secret
  tls:
    api:
      internal:
        secretName: cert-keystone-internal-svc
      public:
        secretName: cert-keystone-public-svc
    caBundleSecretName: combined-ca-bundle
  trustFlushArgs: ""
  trustFlushSchedule: 1 * * * *
  trustFlushSuspend: false

create osctlplane with service enabled + template: {} provided, defaults correct:

keystone:
  apiOverride: {}
  enabled: true
  template:
    adminProject: admin
    adminUser: admin
    databaseAccount: keystone
    databaseInstance: ""
    enableSecureRBAC: true
    memcachedInstance: memcached
    override: {}
    passwordSelectors:
      admin: AdminPassword
    preserveJobs: false
    rabbitMqClusterName: rabbitmq
    region: regionOne
    replicas: 1
    resources: {}
    secret: ""
    tls:
      api:
        internal: {}
        public: {}
    trustFlushArgs: ""
    trustFlushSchedule: 1 * * * *
    trustFlushSuspend: false
placement:
  apiOverride: {}
  enabled: true
  template:
    customServiceConfig: ""
    databaseAccount: placement
    databaseInstance: ""
    override: {}
    passwordSelectors:
      service: PlacementPassword
    preserveJobs: false
    replicas: 1
    resources: {}
    secret: ""
    serviceUser: placement
    tls:
      api:
        internal: {}
        public: {}

Note: The only remaining scenario which does not result in proper defaulting is when enabling the service without specifying at least a template: {}. This will result in the known behavior where the parameters get initialized with their go vars defaults

    keystone:
      apiOverride: {}
      enabled: true
      template:
        adminProject: ""
        adminUser: ""
        databaseAccount: ""
        databaseInstance: ""
        enableSecureRBAC: false
        memcachedInstance: ""
        override: {}
        passwordSelectors:
          admin: ""
        preserveJobs: false
        rabbitMqClusterName: ""
        region: ""
        replicas: 1
        resources: {}
        secret: ""
        tls:
          api:
            internal: {}
            public: {}
        trustFlushArgs: ""
        trustFlushSchedule: ""
        trustFlushSuspend: false
    placement:
      apiOverride: {}
      enabled: true
      template:
        customServiceConfig: ""
        databaseAccount: ""
        databaseInstance: ""
        override: {}
        passwordSelectors:
          service: ""
        preserveJobs: false
        replicas: 1
        resources: {}
        secret: ""
        serviceUser: ""
        tls:
          api:
            internal: {}
            public: {}

And the resulting services misses details as e.g. empty strings are set:

spec:
  adminProject: ""
  adminUser: ""
  containerImage: quay.io/podified-antelope-centos9/openstack-keystone@sha256:8616532664fb47a2fc0b6c9176a765d50e5e1d1e983155c6620fb63d16a8510a
  databaseAccount: ""
  databaseInstance: openstack
  enableSecureRBAC: false
  memcachedInstance: ""
  override:
    service:
      internal:
        metadata:
          labels:
            osctlplane: ""
            osctlplane-service: keystone
      public:
        metadata:
          labels:
            osctlplane: ""
            osctlplane-service: keystone
  passwordSelectors:
    admin: ""
  preserveJobs: false
  rabbitMqClusterName: ""
  region: ""
  replicas: 1
  resources: {}
  secret: osp-secret
  tls:
    api:
      internal: {}
      public: {}
    caBundleSecretName: combined-ca-bundle
  trustFlushArgs: ""
  trustFlushSchedule: ""
  trustFlushSuspend: false

Depends-on: https://github.com/openstack-k8s-operators/data-plane-adoption/pull/486

softwarefactory-project-zuul[bot] commented 5 months ago

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. Warning: Error merging github.com/openstack-k8s-operators/openstack-operator for 838,f3931df8580a711863a7a1d56c3c90d7c26adcb6

stuggi commented 5 months ago

/hold

stuggi commented 5 months ago

Also run the defaulting when a service is disabled, but a template provided. e.g. in the sample where manila is disabled and a template provided. if we do not calling the defaulting for that we'll hit:

$ oc apply -f core_v1beta1_openstackcontrolplane_network_isolation.yaml                                                                                                           
The OpenStackControlPlane "openstack-network-isolation" is invalid: spec.manila.template.dbPurge.age: Invalid value: 0: spec.manila.template.dbPurge.age in body should be greater than or equal to 1

softwarefactory-project-zuul[bot] commented 5 months ago

Build failed (check pipeline). Post recheck (without leading slash) to rerun all jobs. Make sure the failure cause has been resolved before you rerun jobs.

https://review.rdoproject.org/zuul/buildset/78aaabf6f52a4807a5057b67b13981c0

:heavy_check_mark: openstack-k8s-operators-content-provider SUCCESS in 2h 26m 00s :heavy_check_mark: podified-multinode-edpm-deployment-crc SUCCESS in 1h 12m 22s :heavy_check_mark: cifmw-crc-podified-edpm-baremetal SUCCESS in 1h 16m 37s :x: cifmw-data-plane-adoption-osp-17-to-extracted-crc FAILURE in 2h 11m 02s :heavy_check_mark: openstack-operator-tempest-multinode SUCCESS in 1h 35m 50s

stuggi commented 5 months ago

https://github.com/openstack-k8s-operators/data-plane-adoption/pull/486 is needed for the failed adoption job

stuggi commented 5 months ago

rebased

softwarefactory-project-zuul[bot] commented 5 months ago

This change depends on a change that failed to merge.

Change https://github.com/openstack-k8s-operators/data-plane-adoption/pull/486 is needed.

stuggi commented 5 months ago

I understand the logic better now. Thanks. I have couple of nits inline still.

Also one more question. Is it intentional that the template is only validated if the service is enabled? I mean there is a case when the user provides a template but disables the service during create. The validateCreate will not run on the template provided by the user. It is probably OK as nobody will see that template as the controller will not pass it to the service operator. And when the service is enabled later, the validateUpdate will check the template.

right, this is what we have and had for all the previews. need to check if there are issues with the current implemented validations if we change it.

stuggi commented 5 months ago

I understand the logic better now. Thanks. I have couple of nits inline still. Also one more question. Is it intentional that the template is only validated if the service is enabled? I mean there is a case when the user provides a template but disables the service during create. The validateCreate will not run on the template provided by the user. It is probably OK as nobody will see that template as the controller will not pass it to the service operator. And when the service is enabled later, the validateUpdate will check the template.

right, this is what we have and had for all the previews. need to check if there are issues with the current implemented validations if we change it.

@gibizer this require changes, especially in the functional tests. I suggest that we'll do that in a follow up

stuggi commented 5 months ago

rebased

gibizer commented 5 months ago

I understand the logic better now. Thanks. I have couple of nits inline still. Also one more question. Is it intentional that the template is only validated if the service is enabled? I mean there is a case when the user provides a template but disables the service during create. The validateCreate will not run on the template provided by the user. It is probably OK as nobody will see that template as the controller will not pass it to the service operator. And when the service is enabled later, the validateUpdate will check the template.

right, this is what we have and had for all the previews. need to check if there are issues with the current implemented validations if we change it.

@gibizer this require changes, especially in the functional tests. I suggest that we'll do that in a follow up

follow up is OK to me.

fmount commented 5 months ago

+1 from me on this change, I understand that you're going to have a follow up patch. Thanks @gibizer for the multiple reviews on this one, I'm in favor of moving to ptr and adoption would benefit of this change!

openshift-ci[bot] commented 5 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abays, stuggi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/openstack-k8s-operators/openstack-operator/blob/main/OWNERS)~~ [abays,stuggi] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

openstack-k8s-operators / openstack-operator

Move service templates to be ptr to struct #838

As background, current operator behavior

operator behavior with the change