Helm Chart and deployment manifests

[lucacome]

I'd like to get early feedback about a "refactor" of the Helm chart directory and the generation of deployment manifests based on use cases.

The first part is about moving the helm chart to the root of the project and inside the nginx-ingress folder to follow Helm's best practices. This means that the new structure would be charts/nginx-ingress instead of deployments/helm-chart/.
Will this cause any problems? Maybe with existing automation? Would release notes be enough to cover this?
I would expect most people to pull the chart locally if they want to modify the values.yaml file, with something like helm pull oci://ghcr.io/nginxinc/charts/nginx-ingress --untar --version 0.18.0, but feedback is appreciated.

The second part is to create deployments for different use cases, generating them from the Helm Chart to have a single source of truth, something like:

deploy/
├── README.md
├── app-protect-dos
│   └── deploy.yaml
├── app-protect-waf
│   └── deploy.yaml
├── aws-nlb
│   └── deploy.yaml
...
└── read-only-fs
    └── deploy.yaml

These manifest files will have everything that's needed for the deployment in a single file, Service, Service Account, Namespace, Deployment, etc.
Would this be helpful? What use cases should we consider adding? Should we also include CRDs in there?

Any feedback would be greatly appreciated 🙏

[sigv]

The first part is about moving the helm chart to the root of the project and inside the nginx-ingress folder to follow Helm's best practices. This means that the new structure would be charts/nginx-ingress instead of deployments/helm-chart/.
Will this cause any problems? Maybe with existing automation? Would release notes be enough to cover this?

It is a breaking change, and should be mentioned as such in release notes for upgrade guidance. People will surely encounter issues by running an outdated command. But keep in mind that it will be very tricky to decide for removing any mitigation that gets put in place to support the old directory structure. So I would say to just allow the failure, and let people go read release notes (or check the updated installation page).

I would expect most people to pull the chart locally if they want to modify the values.yaml file, with something like helm pull oci://ghcr.io/nginxinc/charts/nginx-ingress --untar --version 0.18.0, but feedback is appreciated.

I don't think there is a major concern here, as long as the structure changes are covered in release notes.

When people run helm install and helm upgrade, I would expect a custom values YAML file locally, such as:

controller:
  config:
    entries:
      set-real-ip-from: '10.1.1.64/27'
  readOnlyRootFilesystem: true
  service:
    annotations:
      service.beta.kubernetes.io/azure-load-balancer-internal: 'true'
      service.beta.kubernetes.io/azure-load-balancer-ipv4: 10.1.2.4

The deployment would be done as helm install nginx-ingress oci://ghcr.io/nginxinc/charts/nginx-ingress --version 0.18.0 --values nginx-ingress.yml.

These manifest files will have everything that's needed for the deployment in a single file, Service, Service Account, Namespace, Deployment, etc.
Would this be helpful? What use cases should we consider adding? Should we also include CRDs in there?

This depends on what your goal for the files is...
I would assume it's to facilitate "quick-start" situations. If so, all bells and whistles make sense to include in this YAML file.

Including Namespace resource is beneficial so that people can run a simple kubectl apply -f https://github.com/nginxinc/kubernetes-ingress/raw/v3.3.0/deploy/use-case/deploy.yaml without worrying about pre-requisites (create namespace/specify namespace for kubectl invocation). This could fit into a simple pipeline that prepares the NIC without a dependency on Helm. If people have advanced scenarios, where they want a different namespace name, they would most likely know what to do with the YAML.

The Deployment should utilize a custom Service Account, and it needs RBAC, so these make sense to me as well.

CRDs are being pushed as the primary configuration mechanism. To make the project structure easier to maintain, maybe it makes sense to leave them out of the use-case-specific YAMLs, and have a separate deploy/crds.yaml that people apply first. Depends.

Service makes sense to include for some use-cases (such as cloud-specific configurations, where there isn't much flexibility expected). You probably want to include some basic Service definition for other general-purpose scenarios (e.g. the mentioned read-only rootfs). This way, all the YAML files deploy a similar set of resources (with different configurations).

[lucacome]

It is a breaking change, and should be mentioned as such in release notes for upgrade guidance. People will surely encounter issues by running an outdated command

I didn't see it as a breaking change since it's only changing the location of the folder that contains the sources. It shouldn't impact any of the commands. Not that bumping the release number would be a problem, but I didn't think it would be necessary. 🤔

I would assume it's to facilitate "quick-start" situations. If so, all bells and whistles make sense to include in this YAML file.

Yep, that's the goal, make it easier to get it up and running with fewer commands.

CRDs are being pushed as the primary configuration mechanism. To make the project structure easier to maintain, maybe it makes sense to leave them out of the use-case-specific YAMLs, and have a separate deploy/crds.yaml that people apply first. Depends.

I'm going back and forth about this. I think we still want to have all the CRDs in a single file either way like deploy/crds.yaml to make it easier to deploy for everybody.
Maybe we can add an additional use case where the -enable-custom-resources is set and the deploy.yaml file contains the CRDs as well.

I think I'm going to open a draft PR soon with the initial refactor and we can always keep adding use cases 🙂

Thanks @sigv !

[sigv]

I didn't see it as a breaking change since it's only changing the location of the folder that contains the sources. It shouldn't impact any of the commands.

@lucacome, if people are installing the packaged Helm chart, nothing changes for them. However Installation with Manifests currently instructs people to cd kubernetes-ingress/deployments which would no longer work. That's why I would classify it as "breaking change".

[coolbry95]

I think instead of just having the deploy yaml files have different values.yaml files that show the different forms of configuration with a command on how to generate the deploy.yaml. The values.yaml files could live next to the deploy.yaml. I imagine there needs to be a chart.yaml with a dependency on nginx-ingress as well.

[sigv]

Keep in mind, with values.yaml files you can't just kubctl apply -- you instead would run helm install with specific values YAML file (helm install nginx-ingress oci://ghcr.io/nginxinc/charts/nginx-ingress --version 0.18.0 --values custom-values.yml).

This is a potential alternative to generated/hard-coded manifest files.

[coolbry95]

You can just have in the readme how to generate the deploy.yaml. This is assuming that you have a chart.yaml also in the same directory.

helm template . > deploy.yaml then kubectl apply -f deploy.yaml

[sigv]

If you are going to invoke Helm, then these deploy items could just be values files, and they are used in helm install via --values.

If the goal is to have a tutorial-oriented/quick-start method, then you want to avoid additional steps where possible (such as invocation of Helm). For this, many projects provide ready-to-use YAML files for people starting out.

While I am against asking the user to do two steps (one to compile a YAML and second to apply), it could be used to prepare YAML files as part of internal build pipeline.
I at the same time see the point in offering values YAML files that users can deploy Helm charts with, as mentioned above.

Really depends on the target audience of these files, as it can be done in multiple ways.

[lucacome]

If the goal is to have a tutorial-oriented/quick-start method, then you want to avoid additional steps where possible (such as invocation of Helm). For this, many projects provide ready-to-use YAML files for people starting out.

While I am against asking the user to do two steps (one to compile a YAML and second to apply), it could be used to prepare YAML files as part of internal build pipeline.
I at the same time see the point in offering values YAML files that users can deploy Helm charts with, as mentioned above.

You're spot on @sigv 🙂 My plan is to add a folder in the examples with values.yaml files that can be used with helm and at the same time use them to generate deploy.yaml that can just be used with kubectl and no additional tools.

[coolbry95]

Makes sense ot wanting people to run extra commands for tutorials and docs. The standard way we do things is use charts as dependencies and use ArgoCD so running helm template is what I am used to.

[brianehlert]

I know that internally we have also discussed producing a large, combined manifest, instead a number of small manifest files to make this easier for upgrades and folks who exclusively automate Helm.
Can this proposal address that use case as well?

[lucacome]

It addresses this as well 🙂

[lucacome]

I opened #4278