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

Expose new options for API Gateway's High Availability feature #1261

Merged
merged 10 commits into from
Jun 13, 2022

Conversation

nathancoleman
Copy link
Member

@nathancoleman nathancoleman commented Jun 7, 2022

Note: This will need to be included in a tagged release before consul-api-gateway v0.3.0 becomes available

Changes proposed in this PR:

  • Allow chart consumers to configure Consul API Gateway's new High Availability values on the GatewayClassConfig
    • Please note that this functionality will not be available until the release of Consul API Gateway 0.3.0 in the near future.

For example:

deployment:
  defaultInstances: 3
  maxInstances: 8
  minInstances: 1

How I've tested this PR:

helm install with various combinations of deployment config

  • Without updating gateway CRDs ($ kubectl apply --kustomize "github.com/hashicorp/consul-api-gateway/config/crd?ref=v0.2.1")
    • No deployment at all without updating gateway CRDs
      • This matches the situation users will be in after consuming the release with this code change but before consuming Consul API Gateway 0.3.0
  • After updating gateway CRDs ($ kubectl apply --kustomize "github.com/hashicorp/consul-api-gateway/config/crd?ref=main")
    • No deployment at all
    • deployment with one field at a time
    • deployment with all fields set

How I expect reviewers to test this PR:

See above

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

@nathancoleman nathancoleman marked this pull request as ready for review June 9, 2022 17:17
@nathancoleman nathancoleman requested review from mikemorris, sarahalsmiller, a team, ishustava and Oluwalogoye and removed request for a team June 9, 2022 17:17
@nathancoleman

This comment was marked as outdated.

@nathancoleman nathancoleman added area/chart-only Related to changes that simply require yaml Helm chart changes, e.g. exposing a new field theme/api-gateway Related to Consul API Gateway labels Jun 9, 2022
@nathancoleman nathancoleman changed the title Expose new options for API Gateway's GatewayClassConfig Expose new options for API Gateway's High Availability feature Jun 9, 2022
Copy link
Member

@sarahalsmiller sarahalsmiller left a comment

Choose a reason for hiding this comment

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

Syntax lines up with everything we added to the gateway yaml and I like that is all optional. Looks good to me.

CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines +2716 to +2719
# deployment:
# defaultInstances: 3
# maxInstances: 8
# minInstances: 1
Copy link
Contributor

Choose a reason for hiding this comment

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

How does a user know what are all possible values for deployment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Documentation is being added alongside our 0.3.0 release here.

The 3 listed above are all that's supported with our upcoming release.

Copy link
Member

Choose a reason for hiding this comment

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

Do you want to link to those docs here? Because folks will also see this through https://www.consul.io/docs/k8s/helm

Copy link
Member Author

Choose a reason for hiding this comment

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

I can link those, noting that the docs content in the PR linked above won't actually be there for about a week after the consul-k8s release containing this change.

Copy link
Member

Choose a reason for hiding this comment

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

oh understood. Yeah then we can add it later instead.

charts/consul/values.yaml Outdated Show resolved Hide resolved
@nathancoleman nathancoleman merged commit f685c9c into main Jun 13, 2022
@nathancoleman nathancoleman deleted the ha-apigw branch June 13, 2022 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/chart-only Related to changes that simply require yaml Helm chart changes, e.g. exposing a new field theme/api-gateway Related to Consul API Gateway
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants