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

Drop nginx-ingress load balancer in favor of Istio #9038

Merged

Conversation

ScheererJ
Copy link
Contributor

@ScheererJ ScheererJ commented Jan 12, 2024

How to categorize this PR?

/area networking
/kind enhancement
/kind api-change

What this PR does / why we need it:
This change moves nginx-ingress behind istio. The consequence is that the load balancer of nginx-ingress becomes an ordinary cluster service without direct exposure.

nginx-ingress is exposed via istio resources (Gateway, VirtualService & DestinationRule) similar as kube-apiserver is exposed.

TLS is still terminated at nginx-ingress. The same holds true for authentication of the observability components.

As a side effect of this change, this PR also brings an improvements to the Gardener Operator: It adds a new ingress.domains field to replace ingress.domain. This allows to expose Ingress resources via multiple domains.

Which issue(s) this PR fixes:
Fixes #7232

Special notes for your reviewer:
Please note that the forwarding currently only works for domain names using the wildcard ingress domain of seed/garden. Additional names need to be added to the istio configuration.

Release note:

Istio is now used as the single entry point on seed clusters. The load balancer of nginx-ingress is removed and traffic goes through istio before being handled by nginx if necessary.
A new field `.spec.runtimeCluster.ingress.domains` was added to the `Garden` API. This field allows to use multiple ingress domains for components of the runtime cluster. All domains are assumed to be wildcard domains. Earlier, the API only accepted one domain name via `.spec.runtimeCluster.ingress.domain`.
⚠️ With this change `.spec.runtimeCluster.ingress.domain` is deprecated and will be removed in the next release. Please update your `Garden` resource to the new `.spec.runtimeCluster.ingress.domains` field by removing the existing domain configuration from `ingress.domain` and add it as the first entry of `ingress.domains`.

@gardener-prow gardener-prow bot added area/networking Networking related kind/enhancement Enhancement, improvement, extension needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 12, 2024
@gardener-prow gardener-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. labels Jan 12, 2024
@ScheererJ ScheererJ force-pushed the enhancement/move-nginx-behind-istio branch from eb5b482 to cb36a44 Compare January 12, 2024 15:41
@gardener-prow gardener-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2024
@ScheererJ ScheererJ force-pushed the enhancement/move-nginx-behind-istio branch from cb36a44 to e245a3f Compare January 15, 2024 08:56
@ScheererJ
Copy link
Contributor Author

Error in pull-gardener-e2e-kind-migration-ha-single-zone seems unrelated => retrying

  [FAILED] Expected success, but got an error:
      <*fmt.wrapError | 0xc0005831a0>: 
      failed updating binding for shoot "garden-local/e2e-migrate": Operation cannot be fulfilled on shoots.core.gardener.cloud "e2e-migrate": the object has been modified; please apply your changes to the latest version and try again
      {
          msg: "failed updating binding for shoot \"garden-local/e2e-migrate\": Operation cannot be fulfilled on shoots.core.gardener.cloud \"e2e-migrate\": the object has been modified; please apply your changes to the latest version and try again",
          err: <*errors.StatusError | 0xc0005b8140>{

/test pull-gardener-e2e-kind-migration-ha-single-zone

@ScheererJ
Copy link
Contributor Author

Issue in pull-gardener-e2e-kind-upgrade does not seem to be a general issue as local test succeeded => retrying

/test pull-gardener-e2e-kind-upgrade

@ScheererJ
Copy link
Contributor Author

Will add support for multiple ingress domains to Garden.

/hold

@gardener-prow gardener-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 17, 2024
@rfranzke
Copy link
Member

/assign

@gardener-prow gardener-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 26, 2024
@ScheererJ ScheererJ force-pushed the enhancement/move-nginx-behind-istio branch 3 times, most recently from d67afba to 0467441 Compare January 26, 2024 15:01
@gardener-prow gardener-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 26, 2024
@ScheererJ ScheererJ force-pushed the enhancement/move-nginx-behind-istio branch from a4c208d to 2b0e8db Compare January 29, 2024 08:32
@gardener-prow gardener-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2024
@ScheererJ
Copy link
Contributor Author

/cla

Copy link
Contributor

gardener-prow bot commented Jan 29, 2024

Successfully reached out to cla-assistant.io to initialize recheck of PR #9038

@ScheererJ ScheererJ force-pushed the enhancement/move-nginx-behind-istio branch from e623c77 to 7f42bcd Compare February 7, 2024 15:12
@ScheererJ
Copy link
Contributor Author

Rebased and adapted validation_test.go to fix unit test.

Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@gardener-prow gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2024
Copy link
Contributor

gardener-prow bot commented Feb 7, 2024

LGTM label has been added.

Git tree hash: 710fa1c0e3b3dd4d98843ccd24509d3f67b50c62

Copy link
Contributor

gardener-prow bot commented Feb 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rfranzke

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gardener-prow gardener-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2024
Copy link
Contributor

gardener-prow bot commented Feb 7, 2024

@ScheererJ: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-gardener-apidiff 7f42bcd link false /test pull-gardener-apidiff

Full PR test history. Your PR dashboard. Command help for this repository.
Please help us cut down on flakes by linking this test failure to an open flake report or filing a new flake report if you can't find an existing one. Also see our testing guideline for how to avoid and hunt flakes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@ScheererJ
Copy link
Contributor Author

/retest-required

@ScheererJ
Copy link
Contributor Author

/test pull-gardener-e2e-kind

@gardener-prow gardener-prow bot merged commit a5d1cd3 into gardener:master Feb 7, 2024
15 of 16 checks passed
ScheererJ added a commit to ScheererJ/gardener that referenced this pull request Feb 19, 2024
…stio.

With PR gardener#9038, nginx was moved behind the istio ingress gateway. The same
PR also enabled management of multiple ingress domains via the `Garden`
resource.
Unfortunately, istio is a bit picky about overlapping domains in certain
scenarios. This could lead to failing reconciliation of the nginx-ingress
managed resource.
Therefore, to prevent those situations this change splits the virtual
services into separate instances per host so that istio does not complain
and simply does the right thing.
ScheererJ added a commit to ScheererJ/gardener that referenced this pull request Feb 19, 2024
…stio.

With PR gardener#9038, nginx was moved behind the istio ingress gateway. The same
PR also enabled management of multiple ingress domains via the `Garden`
resource.
Unfortunately, istio is a bit picky about overlapping domains in certain
scenarios. This could lead to failing reconciliation of the nginx-ingress
managed resource.
Therefore, to prevent those situations this change splits the virtual
services into separate instances per host so that istio does not complain
and simply does the right thing.
gardener-prow bot pushed a commit that referenced this pull request Feb 20, 2024
…stio. (#9183)

With PR #9038, nginx was moved behind the istio ingress gateway. The same
PR also enabled management of multiple ingress domains via the `Garden`
resource.
Unfortunately, istio is a bit picky about overlapping domains in certain
scenarios. This could lead to failing reconciliation of the nginx-ingress
managed resource.
Therefore, to prevent those situations this change splits the virtual
services into separate instances per host so that istio does not complain
and simply does the right thing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/networking Networking related cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/enhancement Enhancement, improvement, extension lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop nginx-ingress load balancer in favor of Istio
3 participants