forked fromnginxinc/nginx-loadbalancer-kubernetes
- Notifications
You must be signed in to change notification settings - Fork0
Merge in changes made by NGINXaaS for Azure project#1
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Open
arussellf5 wants to merge114 commits intomainChoose a base branch fromnginxaas-merge-devops
base:main
Could not load branches
Branch not found:{{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline, and old review comments may become outdated.
+3,929 −3,485
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
The primary intent behind this is to keep retrying updates which may be madebefore the controlplane has registered the existence of a named upstream inthe customer's NGINX configuration.
NOTE: This commit was accidentally missed out inthe first iteration of this fork and is present inupstream:https://github.com/nginxinc/nginx-loadbalancer-kubernetes/blob/main/internal/configuration/settings.go#L189The code needs to move forward to start thewatchers and health server (side note: we shouldalso think about the ordering of these at somepoint) and not running the infomer in a go routineprevents the program from further execution untilthe context is canceled.In the current iteration on main, the controlleris stuck on the informer, and then k8s kills theservice and restarts it since the health server isnot up.
The user specifies the ingress service whose events the application should watch through setting the "service-annotation-match" annotation on the application's config map. Only events with a matching service annotation will be passed by the watcher to the handlers. The informer now listens to events from all namespaces. This frees the end user from the restriction of only using the nginx ingress controller.
…eam nameThe port name should now be formatted like this: "http-tea", where the first partof the string is the context type (either "http" or "stream") and the second partof the string after the hyphen is the name of the upstream.
We need to be able to publish the operator todockerhub in order to be publicly available forcustomers.Following what we have in ARP as a releasestrategy where a release tag action would publish theimage to dockerhub.
go test does not have a good way to captureunit-test coverage as part of test runs. Thiscommit captures the error code of the unit testrun, runs the coverage generation and then exitsbased on the test status.
Helm will be used as part of the user story todeploy the operator but it is also a good tool todeploy the operator while developing it.This commit adds the ability to publish helmcharts:- to the dev registry for local iteration.- to the regular devops registry for CI iteration and testing.This will also help us test the helm chart itself.
0.0.1 is okay but it's not really a bug fix.
We should keep a single release script that willpublish docker images and helm charts for theofficial release. This commit just updates thecurrent release script to handle both artifacttypes. Helm logic will follow.
Previously, owing to a bug, if the name of the upstream included hyphens it would be rejected by the operator.
We are going to release with `0.x.y` and keepingthe internal version inline with what will getpublished out will reduce confusion.
This repository is going to produce artifacts thatwill be available publicly and the end users willcare about semantic versioning.We need to be able to map a public facing versionto internally produced artifacts easily and havingsemver internally eases that work.This commit does not enforce the versioning butadds a version file that has the semver,which will be used to version the product.We can follow a workflow where during releasetime, we cut a release, which creates a tag and weretag existing dev artifacts to be shipped as anofficial artifact.
While a cosmetic change, it does impact howdokerhub repo needs to be setup to publish thehelm chart. In addition to that, it impacts whatthe user see on k8s itself and it should not benginx-loadbalancer-kubernetes as that isconfusing.
While the chartname (which was renamed in theprior commit) gets used for naming stuff, it makessense to also change the name value in the chartitself to use the new name.
Now that official images exist on docker hub, weshould use those images in our charts.
Now that the chart has been renamed, gitignoreneeds to know about ignoring new charts.
This lets us test the operator from the privateregistry. Also, in case a customer does not wantto pull from docker hub and instead use their ownregistry, they can do so and specify a pull secretfor the image.
With the change to make the operator read a configfile (to reduce code complexity), we need to makesure that the file exists for the service tostart.
Once the operator reads from the configmap mountedas a volume, it does not need access to the k8sAPI to read the config map itself.
This is needed temporarily while nlk still needsto read the configmap and the code for swapping itout with a config file does not land. The order ofmerges that I am thinking:- Get this MR landed.- Land testenv changes to use helm.- Land code to read settings from configfile.- Remove configmap permissions.
This means that we do not have to grant the operator app any permissions to access kubernetes secrets. Reading from the config file changes the application's behavior in that settings are no longer changed whenever a config map is updated at run time, as they used to be. Settings are now concurrency-safe, because they pass new values instead of a shared pointer to their consumers in separate goroutines. Settings also no longer pass the application context to consumers as this is a well-document go anti-pattern. Context should always be passed as a function parameter. Any operator modules that need access to a kubernetes client are constructed with a reference to the client, instead of gaining access to the client through settings.
The latest versions of the kubernetes libraries recommend using a typed workqueue and this reduces a bit of boilerplate and error handling, because we no longer have to cast the workitems returned by the queue into the desired types.
Multiple parallel tests were all accessing the same pointer to a single variable for the DefaultTransport in the http package. This was leading to data races in unit tests.
There's a syntax issue in the "listen" directive. Should be "listen 9113;", not "listen 9113:". Using the current file, I got an error upon reloading the NGINX service ("nginx: [emerg] invalid port in "9113:" of the "listen" directive in /etc/nginx/conf.d/prometheus.conf:11")
There was a change in the API for the NGINX Plus Client that was missed when updating to the latest version. This corrects that.
These functions were being used to create IDs that were not reallynecessary for the business logic and which were generating securityalerts because of weak cryptography techniques.
The http client is processing requests created by the nginx plusclient library, and that library should always include a sensible numberof headers. But the lack of change on the number of headers was causingsecurity vulnerability flags to be raised over denial of serviceresource exhaustion attacks.
Go cache in the CI is seeded in the projectworking directory. We should skip the mod cachefrom lint/formatting as it's upstream code andthere are high chances of the linting failing asupstream lint rules != our lint rules.
…inx hostsIn order for the nginx-hosts yaml field to be parsed correctly by viper the template needs to:1. not put double quotes around the value (this causes viper to interpret it as a string)2. render it as a JSON array rather than a go representation of a slice.
The biggest change here is to remove most the TLS modes to enable mTLS and self-signed certificates. Product decided that this was too complex and there was not enough user demand for most of these options. We decided to pare down the code and remove tests that were no longer well maintained. The remaining configuration allows users to toggle a single switch: whether to make the http client verify the NGINX host's certificate chain and host name if https is being used. If the user wishes to enable https with self-signed certs they can use the "skip-verify-tls" setting to allow this. The default behavior is to perform this verification.We are maintaining the deprecated "no-tls" and "ca-tls" inputs for NGINXaaS backwards comptability reasons. The "no-tls" setting name was highly misleading, because all it did was disable TLS verification: it DID NOT disable TLS altogether in https mode. Similarly, the "ca-tls" setting did not enable TLS itself. TLS is enabled by default when the URL of the NGINX host includes the https protocol. The user setting merely enforced the verification of the certificate chain and host as described above.
Now that the plus go client allows users to check the http status code of the error, handle the upstream not found case by doing nothing.
This is a go anti-pattern
kuthiala approved these changesJul 18, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading.Please reload this page.
This PR adds changes made by the NGINXaaS for Azure project to this repo.