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 Helm Chart for running API Gateways in Agentless config and with external servers #1694

Merged
merged 13 commits into from
Nov 15, 2022

Conversation

t-eckert
Copy link
Contributor

@t-eckert t-eckert commented Nov 10, 2022

Changes proposed in this PR:

  • Add a key to the Helm chart externalServers.httpPort.
  • Set CONSUL_HTTP_ADDR to the first external server host when externalServers is enabled.
  • Set CONSUL_HTTP_ADDR using Kube DNS when running with local servers.
  • Change addresses to not have http(s):// and instead set this using the CONSUL_HTTP_SSL environment variable.
  • Add environment variable to API Gateway controller deployment for CONSUL_TLS_SERVER_NAME to support SNI with external servers.
  • Set the consul.address for the GatewayClassConfig whether using external or local servers.
  • Set ports for the three protocols when running with external servers.
  • Add the BATS tests for these changes.

How I've tested this PR:

  • BATS!
  • Deployed API Gateways 0.4.0 with Consul-k8s 1.0.0-beta5 (Agentful) and local servers
  • Deployed API Gateways 0.5.0 with Consul-k8s 1.0.0-beta5 (Agentful) and local servers
  • Deployed API Gateways 0.4.0 with Consul-k8s 1.0.0-beta5 (Agentless) and local servers
  • Deployed API Gateways 0.5.0 with Consul-k8s 1.0.0-beta5 (Agentless) and local servers
  • Deployed API Gateways 0.4.0 with Consul-k8s 1.0.0-beta5 (Agentful) and external servers
  • Deployed API Gateways 0.5.0 with Consul-k8s 1.0.0-beta5 (Agentful) and external servers
  • Deployed API Gateways 0.5.0 with Consul-k8s 1.0.0-beta5 (Agentless) and external servers

Also verified that "Deployed API Gateways 0.4.0 with Consul-k8s 1.0.0-beta5 (Agentless) and external servers" does not work as is expected.

How I expect reviewers to test this PR:

  • BATS!

Checklist:

  • Tests added
  • CHANGELOG entry added

    HashiCorp engineers only, community PRs should not add a changelog entry.
    Entries should use present tense (e.g. Add support for...)

@andrewstucki
Copy link
Contributor

andrewstucki commented Nov 10, 2022

Hey @t-eckert just heads up, but just for some clarity I think we probably want the following for full compatibility. Checked are the ones that I think are the highest priority.

Agent-based

  • Deployed hashicorp/consul-api-gateway:0.4 with Consul-k8s 1.0.0-beta5 (Agentful) and local servers
  • Deployed hashicorppreview/consul-api-gateway:0.5-dev with Consul-k8s 0.49.0 (Agentful) and local servers
  • Deployed hashicorppreview/consul-api-gateway:0.5-dev with Consul-k8s 1.0.0-beta5 (Agentful) and local servers
  • Deployed hashicorppreview/consul-api-gateway:0.5-dev with Consul-k8s 1.0.0-beta5 (Agentful) and external servers

Agentless

  • Deployed hashicorppreview/consul-api-gateway:0.5-dev with Consul-k8s 1.0.0-beta5 (Agentless) and local servers
  • Deployed hashicorppreview/consul-api-gateway:0.5-dev with Consul-k8s 1.0.0-beta5 (Agentless) and external servers

The agentless/external setup will not work with our 0.4 release.

Copy link
Contributor

@andrewstucki andrewstucki left a comment

Choose a reason for hiding this comment

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

Generally LGTM other than adding a test for CONSUL_HTTP_SSL. If you need any help verifying the configurations, let me know.

@t-eckert t-eckert force-pushed the te/gateway-agentless branch 2 times, most recently from f8e1898 to b5730b3 Compare November 10, 2022 18:27
@t-eckert t-eckert marked this pull request as ready for review November 11, 2022 20:03
@t-eckert t-eckert requested review from andrewstucki, a team, ndhanushkodi and ishustava and removed request for a team November 11, 2022 20:03
Copy link
Contributor

@andrewstucki andrewstucki left a comment

Choose a reason for hiding this comment

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

One last change to add a test and then I'm good on feedback at this point, though you should probably wait for someone from the k8s team to sign off too.

Copy link
Contributor

@mikemorris mikemorris left a comment

Choose a reason for hiding this comment

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

Minor suggested tweaks to the comments for clarity, overall LGTM and feels like we'll have a good backwards compatibility/upgrade path story with this implemented.

charts/consul/values.yaml Show resolved Hide resolved
@t-eckert t-eckert merged commit bb97310 into main Nov 15, 2022
@t-eckert t-eckert deleted the te/gateway-agentless branch November 15, 2022 16:12
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.

4 participants