Skip to content
This repository has been archived by the owner on Mar 19, 2024. It is now read-only.

Add partition identity to envoy deployment #537

Merged
merged 2 commits into from
Mar 21, 2023
Merged

Conversation

andrewstucki
Copy link
Contributor

@andrewstucki andrewstucki commented Mar 21, 2023

Changes proposed in this PR:

Gateways deployed in non-default partitions weren't properly identifying themselves to xDS servers. This adds the required metadata into the envoy bootstrapping file to make sure that the envoy instance identifies itself properly.

How I've tested this PR:

Unit tests + verified manually.

Checklist:

  • Tests added
  • CHANGELOG entry added

    Run make changelog-entry for guidance in authoring a changelog entry, and
    commit the resulting file, which should have a name matching your PR number.
    Entries should use imperative present tense (e.g. Add support for...)

@andrewstucki andrewstucki requested a review from a team March 21, 2023 15:45
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.

Could we add an e2e test for a non-default partition when running with Consul Enterprise? Aside from that fix LGTM.

@andrewstucki
Copy link
Contributor Author

@mikemorris that may take a bit of time since I don't believe any of the e2e testing code has any sort of partition awareness. Do you mind if I try and knock that out as a follow-up? This is unit tested well enough with the actual bootstrap template checking that I'm confident we're specifying the needed parameter that was missing previously, and we did vet this on a real k8s deployment configured for a secondary partition.

@andrewstucki andrewstucki merged commit 6e9154b into main Mar 21, 2023
@andrewstucki andrewstucki deleted the partition-identity-fix branch March 21, 2023 16:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants