pingidentity / helm-charts

Apache License 2.0
22 stars 31 forks source link

PODs are not getting the annotations #284

Closed zerowebcorp closed 6 months ago

zerowebcorp commented 6 months ago

My issue is similar to #281

global:
  annotations:
    backup.velero.io/backup-volumes: data

Velero (A backup plugin) requires the volume to be annotated with backup.velero.io/backup-volumes: name_of_the_volume.

It would seem like the annotation is being applied on the Deployment spec, however, this is not being applied to the Pod spec

apiVersion: apps/v1
kind: Deployment
metadata:
  annotations:
    backup.velero.io/backup-volumes: data
  labels:
    app: pingfederate
    app.kubernetes.io/instance: pf-adminconsole
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/name: pingfederate-admin
    helm.sh/chart: ping-devops-0.10.1
  name: pf-adminconsole-pingfederate-admin
spec:
  replicas: 1

In order to correctly apply the backup rule, this needs to be at this position.

apiVersion: apps/v1
kind: Deployment
metadata:
spec:
  replicas: 1
  template:
    metadata:
      annotations:
        checksum/config: 67ec8ce735020da0c0f40e5c02d995db42a63a37a59d13b1c0b5c04d2197ad2b
        backup.velero.io/backup-volumes: data

Can you review this? Either a global.annotations or an annotations under the pingfederate would be fine.

PingDavidR commented 6 months ago

Thanks @zerowebcorp ! We've reviewed your input, and given the relatively minor change that will be required, we should be able to squeeze this in for our next release cycle late next week or early the following.

I'll keep you posted in this issue.

PingDavidR commented 6 months ago

@zerowebcorp in researching, it seems our charts support the following that might not require us to make changes. I tested this and I think it meets your needs.

global:
  envs:
    PING_IDENTITY_ACCEPT_EULA: "YES"

#############################################################
# pingfederate-admin values
#############################################################
pingfederate-admin:
  enabled: true
  envs:
    SERVER_PROFILE_URL: https://github.com/pingidentity/pingidentity-server-profiles.git
    SERVER_PROFILE_PATH: getting-started/pingfederate

#############################################################
# pingfederate-engine values
#############################################################
pingfederate-engine:
  enabled: true
  envs:
    SERVER_PROFILE_URL: https://github.com/pingidentity/pingidentity-server-profiles.git
    SERVER_PROFILE_PATH: getting-started/pingfederate
  workload:
    annotations:
      backup.velero.io/backup-volumes: data

and what rendered was:

# Source: ping-devops/templates/pingfederate-engine/workload.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  labels:
    app.kubernetes.io/instance: test
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/name: pingfederate-engine
    helm.sh/chart: ping-devops-0.10.1
  name: test-pingfederate-engine
spec:
  replicas: 1
  selector:
    matchLabels:
      app.kubernetes.io/instance: test
      app.kubernetes.io/name: pingfederate-engine
  strategy:
<stuff here>
  template:
    metadata:
      annotations:
        backup.velero.io/backup-volumes: data
        checksum/config: e14182d42c4cbf0a29371f868ecdd12e8520dc2256de1b66a472b1ef77a808aa

Would you mind giving that a go and see if it works for you?

PingDavidR commented 6 months ago

it is the workload.annotations block that our charts know how to handle

PingDavidR commented 6 months ago

@zerowebcorp We are planning a chart release in the next day or so, and if the workload.annotations block doesn't meet your needs, we can get this in, but we need your feedback to know for sure.

Thanks!

zerowebcorp commented 6 months ago

@PingDavidR Just tested this by adding the annotation under the pingfederate-admin block and it gets added. This satisfies my requirement.

However, since this works for Deployment and not for Statefulset this could be classified as a bug. I would expect that anything under global would be applied for all the components ( pingfederate-admin/pingfederate-runtime) and don't have to specify them individually. Would be great to have this fixed for both the workload type.

PingDavidR commented 6 months ago

aha! I wasn't as clear as I thought, @zerowebcorp: Try this - this should do what I think you are looking for - specify workload at the global level:

global:
  envs:
    PING_IDENTITY_ACCEPT_EULA: "YES"
  workload:
    annotations:
      backup.velero.io/backup-volumes: data

This will attach the annotation to SS and Deployment both, at the Pod level

zerowebcorp commented 6 months ago

ohk, thank you for clarifying. Adding the annotations under global.workload instead of global works.

PingDavidR commented 6 months ago

Glad to hear it! And thanks again for using our charts!