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

Re-use connectInject.consulNamespaces for API Gateway #1169

Merged
merged 6 commits into from
Apr 14, 2022

Conversation

nathancoleman
Copy link
Member

@nathancoleman nathancoleman commented Apr 13, 2022

Background:
If ACLs are enabled and a user doesn't have identical mappings for the helm chart under connectInject.consulNamespaces and apiGateway.consulNamespaces, everything breaks. This is because the API Gateway uses a combination of both internally which results in a gateway trying to register itself under a different Consul namespace than it is authorized to.

After talking with @lkysow, we determined that these two consulNamespaces options should really be one and the same. Letting them diverge breaks many uses cases. In addition, it's cumbersome for the practitioner to have to specify both with the exact same value.

Changes proposed in this PR:

  • Drop apiGateway.consulNamespaces and instead re-use connectInject.consulNamespaces for the API Gateway installation

How I've tested this PR:
Installed chart from this branch with various connectInject.consulNamespaces configurations, verifying that those values are piped through to the API Gateway controller Deployment spec just as they were previously from apiGateway.consulNamespaces.

Example
connectInject:
  enabled: true
  consulNamespaces:
    mirroringK8S: true
    mirroringK8SPrefix: "nate-"
  default: true
image

How I expect reviewers to test this PR:

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

CHANGELOG.md Outdated Show resolved Hide resolved
@nathancoleman nathancoleman marked this pull request as ready for review April 13, 2022 18:25
@nathancoleman nathancoleman requested review from sarahalsmiller, lkysow, andrewstucki, a team and ishustava and removed request for a team April 13, 2022 18:25
@andrewstucki
Copy link
Contributor

@nathancoleman looks like some stray empty waypoint server files accidentally got checked in.

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Andrew Stucki <andrew.stucki@hashicorp.com>
@nathancoleman nathancoleman merged commit 8c0c438 into main Apr 14, 2022
@nathancoleman nathancoleman deleted the nc-rm-apigw-consul-namespaces branch April 14, 2022 14:44
@@ -1,5 +1,9 @@
## UNRELEASED

BREAKING CHANGES:
* Helm
* API Gateway: Re-use connectInject.consulNamespaces instead of requiring that apiGateway.consulNamespaces have the same value when ACLs are enabled. [[GH-1169](https://github.com/hashicorp/consul-k8s/pull/1169)]
Copy link
Member

Choose a reason for hiding this comment

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

Not a big deal but for BC's it's nice to tell the user what to do. So in this case if their apiGateway.consulNamespaces is the same as their connectInject.consulNamespaces configuration, they don't have to do anything. If it's different, then they need to be aware that the connectInject.consulNamespaces will now be used.

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