Make fullnameOverride override

[coolbry95]

Before 3.0.0 all resources were named nginx-ingress. We need to keep that functionality.

The fullnameOverride currently will use the fullNameOverride plus nginx-ingress or whatever .Values.controller.name is set to. The final result looks like nginx-ingress-something. The fullNameOverride should be the final name set and should not be modified after.

Since the deployment or daemonset matchLabels cannot be updated there needs to be a way to revert to the old labels. Adding selectorLabelsOverride allows a user to use the old labels and not force a recreation of the deployment/daemonset.

Proposed changes

Describe the use case and detail of the change. If this PR addresses an issue on GitHub, make sure to include a link to that issue here in this description (not in the title of the PR).

Checklist

Before creating a PR, run through this checklist and mark each as complete.

• I have read the CONTRIBUTING doc
• I have added tests that prove my fix is effective or that my feature works
• I have checked that all unit tests pass after adding my changes
• I have updated necessary documentation
• I have rebased my branch onto main
• I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

[brianehlert]

@coolbry95
Can you please describe the problem this is solving? Just to make it easy for the reviewers to really understand the problem this is fixing.

[brianehlert]

Is this an improvement on: #3802
or
#3933

[coolbry95]

Updated the PR message. It is in similar vein to #3802.

This is a blocker for us to upgrade to 3.x.x because of the resource name changes.

I also believe the current behavior is a bug when considering the name fullnameOverride because its not actually overriding when its being modified still.

[lucacome]

fullnameOverride is overriding nginx-ingress.fullname and the fullname is used in the controller name. It's not a bug.

Why is it preventing you from upgrading?

[coolbry95]

Why is it preventing you from upgrading?

Because it is changing the names of resources which is unacceptable for my team and application.

We are unable to preserve previous behavior. We would like all resources to be nginx-ingress.

There is also the option of introducing another override variable. Is that more acceptable than this? We could introduce an override variable that trumps all others.

[brianehlert]

fullnameOverride is overriding nginx-ingress.fullname and the fullname is used in the controller name.

@coolbry95 is it that you want to be able to have fullname be an empty string? And just have "nginx-ingress"

[coolbry95]

fullnameOverride is overriding nginx-ingress.fullname and the fullname is used in the controller name.

@coolbry95 is it that you want to be able to have fullname be an empty string? And just have "nginx-ingress"

Yes but that is still wrong. No matter what fullname or controller.name are set to you will always have '-' somewhere in the mix.

We would like to just be able to control the names ourselves.

[vepatel]

hi @coolbry95 we're looking at the issue and noticed doesn't covers daemon-set so we need to check if there's something else in the need of similar change.

[coolbry95]

hi @coolbry95 we're looking at the issue and noticed doesn't covers daemon-set so we need to check if there's something else in the need of similar change.

Both the deployment and daemonset use the same variable for name.

https://github.com/nginxinc/kubernetes-ingress/blob/v3.1.0/deployments/helm-chart/templates/controller-daemonset.yaml#L5
https://github.com/nginxinc/kubernetes-ingress/blob/v3.1.0/deployments/helm-chart/templates/controller-deployment.yaml#L5

[vepatel]

thanks for clarification @coolbry95 , we're also looking at #3976 as the issue seems related and just need to make sure changes don't conflict with each other. Thanks for your patience!

[coolbry95]

I don't see an issue.

Selector labels uses the nginx-ingress.name

{{- define "nginx-ingress.name" -}}
{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}
{{- end }}

Selector label
app.kubernetes.io/name: {{ include "nginx-ingress.name" . }}

This change affects the nginx-ingress.controller.fullname only. It doesn't look like this is used for any labels just names of manifests.

[coolbry95]

I think this could be solved by doing instead of the if statement. This would match what nginx-ingress.controller.service.name is doing.

{{- default (printf "%s-%s" (include "nginx-ingress.fullname" .) .Values.controller.name) .Values.fullnameOverride | trunc 63 | trimSuffix "-" -}}

[vepatel]

I tried changes from this PR and one below by editing controller-daemonset.yaml separately:
name: {{ default (include "nginx-ingress.controller.fullname" .) .Values.fullnameOverride }}
but both resulted in failed upgrade:

Error: UPGRADE FAILED: cannot patch "test-release-nginx-ingress" with kind DaemonSet: DaemonSet.apps "test-release-nginx-ingress" is invalid: spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{"app.kubernetes.io/instance":"test-release", "app.kubernetes.io/name":"nginx-ingress"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable

[coolbry95]

I tried changes from this PR and one below by editing controller-daemonset.yaml separately: name: {{ default (include "nginx-ingress.controller.fullname" .) .Values.fullnameOverride }} but both resulted in failed upgrade:

Error: UPGRADE FAILED: cannot patch "test-release-nginx-ingress" with kind DaemonSet: DaemonSet.apps "test-release-nginx-ingress" is invalid: spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{"app.kubernetes.io/instance":"test-release", "app.kubernetes.io/name":"nginx-ingress"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable

Your error is still unrelated to this PR. It looks like you are still hitting the issue in #3976.

Not sure what the exact steps you are taking. If you change the name of the daemonset it will create a new one. The workflow here for us is to keep all the names of all resources the same. In #3976 they are trying to keep the selector labels the same so the daemonset doesn't need to be recreated. The only way to change the daemonset labels is to change the .Release.Name or the nginx-ingress.name.

Before the labels change the daemonset would have the label app: {{ include "nginx-ingress.appName" . }} and after would be

app.kubernetes.io/name: {{ include "nginx-ingress.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}

This is a breaking change. There is no way to not recreate the daemonset without an override to the selector labels used.

[vepatel]

Pointed that out because the changes in PR weren't working because of the other issue and it seems appropriate to fix these together, now that PR is updated with selector changes we can try both both overrides.

[coolbry95]

I have tested this for my environment with the following values set. Basically we define the name for everything we can.

nginx-ingress:
  fullnameOverride: nginx-ingress
  serviceNameOverride: nginx-ingress
  selectorLabelsOverride:
    app: nginx-ingress
  controller:
    name: nginx-ingress
    serviceAccount:
      name: nginx-ingress
    service:
      name: nginx-ingress
    config:
      name: nginx-ingress

[vepatel]

Hi @coolbry95, I think we should be able to fix the issue without the changes in this PR, we've accepted #3977 to fix the issue with selectorLabels. Can you please try:

1. taking the changes from Add support for controller.selectorLabels #3977,
2. perform a helm upgrade with --set controller.serviceNameOverride="<old_service_name>" --set fullnameOverride="<old_deployment_name>" --set controller.name="" if your helm release name is nginx-ingress
3. perform a helm upgrade with --set controller.serviceNameOverride="<old_service_name>" --set controller.name="" if your helm release name is anything other than nginx-ingress

[coolbry95]

Hi @coolbry95, I think we should be able to fix the issue without the changes in this PR, we've accepted #3977 to fix the issue with selectorLabels. Can you please try:

1. taking the changes from Add support for controller.selectorLabels #3977,
2. perform a helm upgrade with --set controller.serviceNameOverride="<old_service_name>" --set fullnameOverride="<old_deployment_name>" --set controller.name="" if your helm release name is nginx-ingress
3. perform a helm upgrade with --set controller.serviceNameOverride="<old_service_name>" --set controller.name="" if your helm release name is anything other than nginx-ingress

This flow works. I would still prefer fullNameOverride to work how it does in this PR. I think it is confusing that overriding the name doesn't actually override it.