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

[kong] consolidate Ingress and Service templates #251

Merged
merged 4 commits into from
Jan 18, 2021

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Jan 11, 2021

What this PR does / why we need it:

Create helper functions for Ingress and Service generation and refactor existing service templates to use them. Per-service templates now just gate whether to render these for a given service and set up some boilerplate variables.

This avoids copy-paste work when adding new features to these resources, which has historically been a source of inconsistent feature support across them (e.g. previously only the proxy supported explicitly setting a predefined ClusterIP because we forgot to add it to anything else).

This deprecates support for multi-host proxy Ingress resources. Although this has use in some environments (namely when Kong is behind some other ingress controller), it's unnecessary for the stock Ingress we generate, and prevents us from unifying the template. Users will need to write their own Ingress manually (or patch the proxy Ingress after creation--Helm 3 will preserve the change) to use this style of configuration.

Special notes for your reviewer:

  • We have to populate a bunch of variables for the helper context, since templates that receive a non-root context don't allow accessing things like .Chart.Name otherwise, and other helpers the new helpers use rely on them.
  • This places per-service Service and Ingress templates in the same file, mostly because they need similar sets of boilerplate variables.
  • Admin and proxy templates are kind of a mess since they just dump the old legacy template alongside the new template. After 2.0 (which will remove those deprecated configuration options), they'll look like the Manager template.
  • This technically allows you to create stream Service configuration on non-proxy services, but nobody should actually do so. It's ultimately harmless (the Service will have some ports that don't actually work) if someone does for some reason.

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • PR is based off the current tip of the next branch and targets next, not main
  • New or modified sections of values.yaml are documented in the README.md
  • Title of the PR and commit headers start with chart name (e.g. [kong])

@rainest rainest force-pushed the feat/consolidate-ingress-service branch from 44f6d4d to 1251231 Compare January 13, 2021 15:24
@rainest rainest requested review from hbagdi and mflendrich and removed request for hbagdi January 13, 2021 15:28
@rainest rainest marked this pull request as ready for review January 13, 2021 15:28
@rainest rainest requested a review from hbagdi as a code owner January 13, 2021 15:28
@rainest
Copy link
Contributor Author

rainest commented Jan 13, 2021

I'm unsure if multi-host proxy Ingresses are commonly in use. They're something of a historical artifact from the early days of the chart, before our ingress controller existed. Placing Kong behind some other controller's proxy and using that controller to direct traffic to Kong (to match route configuration that had no corresponding K8S configuration) made more sense then. I'd argue it still makes more sense to define Ingresses per application (even if they point to Kong's Service rather than to the application Service) instead of having an omnibus Ingress for all applications, but can't easily say what's in the wild.

If we end up getting user complaints after releasing the deprecation warning, restoring support for it shouldn't be difficult, as the Ingress template is simple (for now, at least, Ingress has far fewer options than Service). We would need to write additional helpers for the rules section of the spec, one supporting the single host+TLS Secret config and one supporting the multi config, and choose which to render using the current legacy logic (whether hostname or hosts is present).

@rainest
Copy link
Contributor Author

rainest commented Jan 13, 2021

I performed additional manual testing by comparing template output from current next against the consolidated version with ALL THE OPTIONS. full-k4k8s-with-kong-enterprise.yaml.txt and minimal-kong-standalone.yaml.txt cover most, though a few I turned on/off separately. Remaining discrepancies are:

  • The proxy metrics label moved from the end to the beginning of the label list. Doesn't actually matter.
  • externalIPs is now only rendered if it has content. Previously we rendered an empty block, which serves no purpose.
  • All admin templates now include namespace. We missed a bug in [kong] Add kong 'namespace' value #231 that only rendered it when using legacy admin configuration.

Add helper functions to handle the bulk of Ingress and Service
generation, and rework Ingress and Service templates to use them.
Individual service templates now mostly handle enabling/disabling
resources and legacy behavior (for admin and proxy). This avoids issues
where features (e.g. externalTrafficPolicy and LoadBalancer NodePort
overrides) were available on some templates but not others, and
eliminates the need to copy-paste template functionality across
templates when adding new features in the future. Additionally:

- Deprecate support for multi-host proxy Ingresses. As the proxy Ingress
  now uses the same template as all other services, it only supports a
  single hostname and TLS Secret.
- Remove externalIPs from the default values configurations and do not
  render it if it is not present. externalIPs should be uncommon (it is
  designed for bare metal clusters that cannot update Service status
  automatically) and is easy to confuse with other functionality.
- Add a loadBalancerIP comment to the proxy values. This is the setting
  actually used to request a specific IP from a cloud provider.

Note: the reworked service templates run a number of boilerplate
templates (e.g. kong.fullname) and store their result alongside the
configuration context before invoking kong.ingress and kong.service.
Although Helm documentation indicates that the "$" variable always
points to the chart root, this isn't actually true for named template
invocations: helm/helm#7915

Because of this, templates that require things like .Release.Name cannot
access it, and we must render them in advance.
@rainest rainest force-pushed the feat/consolidate-ingress-service branch from 1251231 to 54c73c0 Compare January 13, 2021 16:37
Copy link
Member

@mflendrich mflendrich left a comment

Choose a reason for hiding this comment

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

I'm happy to see a move in this direction. The fact that we need duplicate ingress/service templates to account for legacy stuff is unfortunate (and I'd love to see the consolidated template accommodating the legacy parameters) but I understand that this is outside of scope for this change.

There's some indentation inconsistency. Flagged several issues in line comments, but please join me in reviewing whitespace line by line because I may have missed something. Otherwise LGTM.

charts/kong/templates/_helpers.tpl Outdated Show resolved Hide resolved
{{- end -}}
{{- if .Values.proxy.ingress.tls }}
tls:
{{ toYaml .Values.proxy.ingress.tls | indent 4 }}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{{ toYaml .Values.proxy.ingress.tls | indent 4 }}
{{- toYaml .Values.proxy.ingress.tls | indent 4 }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indentation in the template does bad things to multi-line strings (it only affects the first line), and we do want to preserve the preceding newline here. Leaving as-is.

@rainest
Copy link
Contributor Author

rainest commented Jan 14, 2021

Missed a bug with legacy proxy TLS sections, hence c7bd903. We don't check those currently in CI because the attempts to start instances will fail if we reference a non-existent Secret, and we don't have any Secrets (TLS or otherwise) available in the test environment.

Edit: fixing this surfaced some very old stuff in CI checks that shouldn't actually be testing legacy config 😐

@mflendrich mflendrich merged commit cdb0176 into next Jan 18, 2021
@mflendrich mflendrich deleted the feat/consolidate-ingress-service branch January 18, 2021 15:23
rainest added a commit that referenced this pull request Mar 1, 2021
Removes support for proxy ingress configuration with multiple hostnames.
#251 added a shared Ingress and
Service template for all Kong services, which only supports a single
hostname.

Fix #289
rainest added a commit that referenced this pull request Mar 1, 2021
Removes support for proxy ingress configuration with multiple hostnames.
#251 added a shared Ingress and
Service template for all Kong services, which only supports a single
hostname.

Fix #289
rainest added a commit that referenced this pull request Mar 1, 2021
Removes support for proxy ingress configuration with multiple hostnames.
#251 added a shared Ingress and
Service template for all Kong services, which only supports a single
hostname.

Fix #289
rainest added a commit that referenced this pull request Mar 3, 2021
Removes support for proxy ingress configuration with multiple hostnames.
#251 added a shared Ingress and
Service template for all Kong services, which only supports a single
hostname.

Fix #289
rainest added a commit that referenced this pull request Mar 4, 2021
Removes support for proxy ingress configuration with multiple hostnames.
#251 added a shared Ingress and
Service template for all Kong services, which only supports a single
hostname.

Fix #289
ubergesundheit pushed a commit to giantswarm/kong-app that referenced this pull request Jun 14, 2021
Removes support for proxy ingress configuration with multiple hostnames.
Kong/charts#251 added a shared Ingress and
Service template for all Kong services, which only supports a single
hostname.

Fix #289
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants