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.

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

FEATURE : Add full support for imagePullSecrets #368

Conversation

walsm232
Copy link

It seems that currently only some of the ReportPortal Deployments and StatefulSets support imagePullSecrets which may be required in some cases to pull images from private registries. From what I can see it needs to also be supported for the dependent Helm charts below:

minio
opensearch
postgresql
rabbitmq

These changes should ensure imagePullSecrets can be applied to all ReportPortal components and all components required in the dependent charts.

raikbitters
raikbitters previously approved these changes Mar 5, 2024
@raikbitters raikbitters self-requested a review March 5, 2024 13:41
@@ -5,6 +5,9 @@
nameOverride: ""
fullnameOverride: ""

global:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see anything that refers to those values.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rabbitmq, postgresql, and minio dependencies use these global values. If you build the dependencies and expand the archives you will see it in each of their chart values. It's generally good to include imagePullSecrets as a global value option so it can be shared with all dependent charts, avoiding redundant configuration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it works for RP dependencies when referred here (e.g. rabbitmq)

rabbitmq:
  global:
    imagePullSecrets: []

Template: https://github.com/bitnami/charts/blob/main/bitnami/rabbitmq/templates/_helpers.tpl#L25

Copy link
Contributor

@raikbitters raikbitters Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

global.imagePullSecrets doesn't resolve your issue. It's only RP value, and it doesn't have any relation with the chart dependencies. I don't see any profits from this change. We already have global value .Values.imagePullSecrets. Switch from .Values.imagePullSecrets to .Values.global.imagePullSecrets nothing changes.

Copy link
Contributor

@raikbitters raikbitters Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add YAML anchors for deps imagePullSecrets from our .Values.imagePullSecrets

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the dependent charts are including a block for

global:
  imagePullSecrets: []

then setting this value in the RP values will ensure it is passed down to all of the dependencies.

Copy link
Author

@walsm232 walsm232 Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you set this global imagePullSecrets value and template the chart you will see it is getting rendered in any relevant Deployments or StatefulSets for the dependent charts.
See example:

 ~ helm template reportportal --set "global.imagePullSecrets={regcred}" -n reportportal

Sample snippet of rendered dependency:

---
# Source: reportportal/charts/minio/templates/standalone/deployment.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: release-name-minio
  namespace: reportportal
  labels:
    app.kubernetes.io/name: minio
    helm.sh/chart: minio-6.7.7
    app.kubernetes.io/instance: release-name
    app.kubernetes.io/managed-by: Helm
spec:
  strategy:
    type: Recreate
  selector:
    matchLabels:
      app.kubernetes.io/name: minio
      app.kubernetes.io/instance: release-name
  template:
    metadata:
      labels:
        app.kubernetes.io/name: minio
        helm.sh/chart: minio-6.7.7
        app.kubernetes.io/instance: release-name
        app.kubernetes.io/managed-by: Helm
    spec:
      imagePullSecrets:
        - name: regcred <----- applied via the global value to the MinIO chart dependency
      serviceAccountName: release-name-minio

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@raikbitters raikbitters Mar 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the dependent charts are including a block for

global:
  imagePullSecrets: []

then setting this value in the RP values will ensure it is passed down to all of the dependencies.

I've got it.
However, why are there {{ .Values.global.imagePullSecrets }} and {{ .Values.imagePullSecrets }} in the Values file? What is the profit of using both? Do you have cases when you use different imagePullSecrets for deps charts and RP?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I provide {{ .Values.global.imagePullSecrets }} in my Value chart, I expect this value to be used in all my templates, not only sub-charts. Isn't that so?

@walsm232 walsm232 force-pushed the feature/support-imagepullsecrets branch from 4535e64 to 5780380 Compare March 5, 2024 13:54
@hlebkanonik hlebkanonik added this to the 24.1 milestone Mar 5, 2024
This was referenced Mar 5, 2024
@raikbitters raikbitters dismissed their stale review March 26, 2024 10:21

Need more clarification

@hlebkanonik hlebkanonik closed this Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants