Skip to content
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

Merged
merged 1 commit into from
Apr 7, 2021

Conversation

soneillf5
Copy link
Contributor

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.

  • 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 master
  • I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

@soneillf5 soneillf5 force-pushed the feature-remove-global-config-dependency branch from 0089619 to 67800c5 Compare March 31, 2021 11:05
@github-actions github-actions bot added the documentation Pull requests/issues for documentation label Mar 31, 2021
@soneillf5 soneillf5 force-pushed the feature-remove-global-config-dependency branch from 67800c5 to 52cb1ab Compare March 31, 2021 11:11
@soneillf5
Copy link
Contributor Author

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 ?

Copy link
Contributor

@pleshakov pleshakov left a 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

internal/configs/configurator.go Show resolved Hide resolved
deployments/helm-chart/values.yaml Outdated Show resolved Hide resolved
@soneillf5 soneillf5 force-pushed the feature-remove-global-config-dependency branch from 52cb1ab to 773686f Compare April 2, 2021 10:20
@soneillf5 soneillf5 requested a review from pleshakov April 2, 2021 10:21
Copy link
Member

@ciarams87 ciarams87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pleshakov
Copy link
Contributor

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 ) ?

image

Also, in the installation with manifests, we have
image

However, the file deployments/common/global-configuration.yaml was removed. It's better to leave it - it could be used as the base to add necessary listeners.

@soneillf5 soneillf5 force-pushed the feature-remove-global-config-dependency branch from 773686f to 8b0aa34 Compare April 6, 2021 09:22
@soneillf5
Copy link
Contributor Author

soneillf5 commented Apr 6, 2021

@pleshakov I've removed the reference to creating the GlobalConfig from installation-with-manifests.md to be consistent with the other custom resources.

@soneillf5 soneillf5 force-pushed the feature-remove-global-config-dependency branch from 8b0aa34 to 727e881 Compare April 6, 2021 12:12
@soneillf5 soneillf5 force-pushed the feature-remove-global-config-dependency branch from 727e881 to 8fc7617 Compare April 6, 2021 12:19
@soneillf5 soneillf5 merged commit 96c6e7d into master Apr 7, 2021
@soneillf5 soneillf5 deleted the feature-remove-global-config-dependency branch April 7, 2021 09:04
@pleshakov pleshakov changed the title Update the IC so GlobalConfig is not a dependency Update the IC so that GlobalConfiguration is not mandatory when configured Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests/issues for documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants