Update default INGRESS_NGINX_CHART_VERSION

[jarpat]

Updated the how the default for INGRESS_NGINX_CHART_VERSION value gets set. If the user does not specify a value for INGRESS_NGINX_CHART_VERSION in their vars file the version will depend on the minor K8s cluster version they are deploying against. Version 3.20.1 will be used for K8s clusters whose version is <= 1.21.X and version 4.0.13 will be used for K8s clusters whose version is >= 1.22.X. If a user explitly sets a value for INGRESS_NGINX_CHART_VERSION that helm chart version will always be used.

[manoatsas]

Without being able to tell what Nginx Ingress version does the chart deploys, my only question would be, is this change compatible with K8s version < 1.22? See this blog for info - https://kubernetes.io/blog/2021/07/26/update-with-ingress-nginx/

[jarpat]

@manoatsas

The helm chart version 4.0.13 will deploy the app version of ingress-nginx at v1.1.0 https://kubernetes.github.io/ingress-nginx/index.yaml

Currently v1.1.0 provides supports for K8s 1.22, 1.21, 1.20, & 1.19 https://github.com/kubernetes/ingress-nginx#support-versions-table

[AWSmith0216]

Refer to IAC-342 for discussion. While ingress-nginx v1.1.0 is compatible with 1.19-1.22, it is not necessarily true that cadences prior to stable/2021.2.4 will be compatible with ingress-nginx v1.1.0. If possible, it would seem ideal to base the default on the K8S version. If 1.19-1.21, deploy v0.4x; if 1.22 or later, deploy v1.x.x.

[jarpat]

@AWSmith0216 Just looked at the most recent comments, I believe that should be doable. I'll talk with Thomas about this.

[jarpat]

Updated behavior: If the user does not explicitly set INGRESS_NGINX_CHART_VERSION in their ansible-vars file, version 3.20.1 will be used for K8s clusters whose version is <= 1.21.X and version 4.0.13 will be used for K8s clusters whose version is >= 1.22.X

[AWSmith0216]

@thpang , as previously mentioned, while ingress-nginx v1.1.0 supports all current K8S versions, cadences prior to stable/2021.2.4 do not support ingress-nginx v1.1.0. Cadence stable/2021.2.3 just went out this week (1/19/2022), and supports K8S 1.19-1.21, ingress-nginx v0.4x. If viya4-deployment defaults to ingress-nginx v1.1.0, you'll be deploying something not supported for the current production cadence.

[thpang]

Ok, that's the rub. Then you simply include the correct ingress version here and create a major update to the deployment code base here. So folks using the previous version get the old ingress and those using this fix get the new version. As I stated, the code here moves forward.

[AWSmith0216]

At any given time, SAS supports a couple LTS releases and several stable releases. Determining ingress-nginx version based on K8S minor version would allow for viya4-deployment to:

• Support all currently available SAS releases
• Avoid a major version bump
• Avoid any doc changes necessary for users to be able to map SAS releases to the proper version of viya4-deployment

I don't see the downside to that approach, besides introducing some additional logic. How is that approach not moving forward?

[jarpat]

Ok, that's the rub. Then you simply include the correct ingress version here and create a major update to the deployment code base here. So folks using the previous version get the old ingress and those using this fix get the new version. As I stated, the code here moves forward.

Reverted back to the previous commit, the INGRESS_NGINX_CHART_VERSION will have the default value of 4.0.13. Those with older Viya orders will either specify their own INGRESS_NGINX_CHART_VERSION value in the vars file or use the previous release of viya4-deployment which defaults to the older ingress.

[thpang]

@AWSmith0216 if you can show clean logic that does not change the current templating, I am fine with that. Just hard-coding values and changing 1 item of several just to solve a problem is not the answer. The answer must be more thought out than brute force.

This issue does raise a flag in that with api depreciation in k8s, we have to be ready to alter our baseline based on some criteria. I just believe the solution could be better. Maybe have a dictionary of items used to show or map between k8s versions and api versions needed.

Something like:

ingress_versions:
  version_celing: 1.21
    api:
      ingress_nginx: 0.4.3
  version_floor: 1.21
    api:
      ingress_nginx: 1.1.0

And then you look at the k8s version being used, pull that ingress_nginx value and replace that in the helm chart template. But the default would always be forward facing so in this case 1.1.0

Not saying this is the answer, but just showing how this solution would create a mechanism to perform a mapping based on other criteria and then use ansible's set_fact function to determine what the value should be.

[AWSmith0216]

Could you clarify the helm templating issue? I can see why having a one-off solution just for ingress-nginx wouldn't be desired, but it seems like the end result at 'helm install' time should be the same.

I'm not necessarily advocating for this particular fix, so much as advocating against simply changing the default to v1.1.0. From a customer's perspective, no one likes doc. If the problem can be handled through code rather than relying on people reading doc to know the correct tag to pull for their SAS release, that seems preferable to me. For internal users (me among them), not having to swap between project versions based on the target cluster would be nice; I'd prefer the code handle that for me.

[thpang]

As I stated, I believe we need a coding solution to this type of problem to help our customers. I am simply saying the brute force push toward VAR_V043 and VAR_V1110 are not the solution as was submitted earlier. Again, a design of how to handle updated API changes at kubernetes boundaries would be an overall better solution and needs to be decided on now and coded. No disagreement on moving forward with a coded way to ensure the customer has the correct version the software supports.

Regardless of how much we code the solution, the customer should still be able to choose their own version of the helm chart to install that falls within the purpose of this release of the software. And with this release our focus is on supporting ingress_nginx v1.1.0 and higher. So the default should for this release would be the baseline version 4.0.13 for the Helm chart.

As for always supported current and previous releases this repo has two approaches there. For this software repo it is always moving forward and is only backwards compatible up to the last major release. So if you want previous functionality you use the previous major release. However, if you're looking for SAS supported versions the code that generates the kustomization.yaml and site.yaml files is backwards compatible to the first official release of SAS.

However, with this change and the need to now add the dependency of the kubernetes version with baseline installed objects because the SAS software is not flexible enough to support both api versions of the ingress-nginx api, this is where a new dependency is being added. Which does break the code quite literally with older releases. This is outside of our contro. This is where the new design needs to come into play on how to move forward with as little impact as possible. The current patches here do not answer that question.

As for your doc comment, I disagree. The complexity of setting up and deploying the SAS software is not trivial and we have several documents that outline what and how to do this. We tell everyone, read the documents. If they all choose not too, that does not mean the documents are bad, it simply means those folks are lazy and should take the time to understand what the tool is and how it works based on the docs provided. I do agree we should make the software as easy as possible to use within the confines of the problem set and the docs provided. But just saying we don't expect folks to read the docs is not, in my mind, the right approach.

[AWSmith0216]

Glad to hear you're open to a code based solution. And agreed that users should be able to choose their own chart version if they wish to do so. I'm not that familiar with this project's code base so I can't make an informed suggestion.

It's perhaps worth pointing out that I'm not aware of a reason something like stable/2021.2.3 won't work on ingress-nginx v1.1.0, but rather it's not considered supported. So while it may work fine, the doc (at this time) explicitly says it's not supported.

As for doc, I wasn't implying that we should assume users won't read the doc. SAS does have lots of doc, to the point that it's overwhelming, and this project is one way that burden is alleviated for users. I was just saying that if there's a solution that doesn't rely on doc to guide users to a specific project tag based on SAS release, that would seem preferable.

[thpang]

LGTM.

[jarpat]

Minor scope increase, after comments in the originating ticket, we are also bumping the default of CERT_MANAGER_CHART_VERSION to 1.6.1

[jarpat]

Minor scope increase, after comments in the originating ticket, we are also bumping the default of CERT_MANAGER_CHART_VERSION to 1.6.1

Tested against a GKE 1.21.5 Cluster & and 1.22.3. Dockerfile change to bump up the kubectl version was also tested

[sayeun]

+2 👍