-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Update the IC so that GlobalConfiguration is not mandatory when configured #1492
Conversation
0089619
to
67800c5
Compare
67800c5
to
52cb1ab
Compare
This change means we don't need a GlobalConfig resource to start the IC. We do need to specify the namespace/name though. What do the reviewers think about changing the default from empty string to a valid namespace/name ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @soneillf5
We do need to specify the namespace/name though. What do the reviewers think about changing the default from empty string to a valid namespace/name ?
Better to not have a default. The GlobalConfiguration is only required for TCP/UDP load balancing. Most of the people use only HTTP load balancing in the Ingress Controller, so there is no need to monitor for GlobalConfiguration resource.
Also, because multiple KICs could be deployed in the same namespace, having the same default will make those KICs depend on the same configuration, which isn't the usual case - because those are different KICs, their ConfigMaps and GlobalConfiguration will probably be different
52cb1ab
to
773686f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hi @soneillf5 A few docs issues: could you restore the docs for helm (both in deployments/helm-chart/README.md and https://github.com/nginxinc/kubernetes-ingress/pull/1492/files ) ? Also, in the installation with manifests, we have However, the file |
773686f
to
8b0aa34
Compare
@pleshakov I've removed the reference to creating the GlobalConfig from |
8b0aa34
to
727e881
Compare
727e881
to
8fc7617
Compare
Proposed changes
The globalConfig parameter for the IC was a dependency, it was parsed and validated on starting the IC.
This meant it was requirement for the helm chart and the operator. This change removes the parsing
but keeps the namespace/name definition so watchers are setup correctly, enabling GlobalConfig
resources to be added/updated/removed while the IC is running.
Checklist
Before creating a PR, run through this checklist and mark each as complete.