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.

Sign up for GitHub

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

Jump to bottom

feat: add peering config to mesh CRD #1478

Merged
merged 3 commits into from
Oct 7, 2022
Merged

feat: add peering config to mesh CRD #1478

merged 3 commits into from
Oct 7, 2022

Conversation

DanStough
Copy link
Contributor

@DanStough DanStough commented Sep 2, 2022 •
edited
Loading

Changes proposed in this PR:

  • Adding a new field to the mesh CRD that will be used to turn on routing of peering control-plane traffic through a mesh gateway(s). Consul PR link

How I've tested this PR:

  • I've just been running unit tests. Right now nothing in Consul is listening to this flag - more integration testing required at that point. Also assuming that the changelog will be deferred until that point (please correct me if this isn't convention).

How I expect reviewers to test this PR:

  • Hopefully CI tests are sufficient.

Checklist:

  • Tests added
  • CHANGELOG entry added (TBD)

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

@DanStough DanStough requested review from a team, curtbushko and thisisnotashwin and removed request for a team September 2, 2022 22:19
DanStough
DanStough commented Sep 6, 2022
View reviewed changes
control-plane/api/v1alpha1/mesh_types.go
Comment on lines +209 to +214
func (in *Mesh) Validate(consulMeta common.ConsulMeta) error {
var errs field.ErrorList
path := field.NewPath("spec")

errs = append(errs, in.Spec.TLS.validate(path.Child("tls"))...)
errs = append(errs, in.Spec.Peering.validate(path.Child("peering"), consulMeta.PartitionsEnabled, consulMeta.Partition)...)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an additional validation in Consul that mesh CRs are always in the default namespace, but that always seems to be implied here on creation. I don't see where namespace is transferred in ToConsul.

@curtbushko curtbushko requested review from a team and ndhanushkodi and removed request for curtbushko and a team September 8, 2022 15:28
thisisnotashwin
thisisnotashwin approved these changes Sep 12, 2022
View reviewed changes
Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

This looks great! Before we merge this though, is the peerThroughMeshGateways already supported in 1.13 or will this be a 1.14 feature? We might want to wait to merge it if it is for 1.14.

control-plane/api/v1alpha1/mesh_webhook.go Show resolved Hide resolved
@DanStough
Copy link
Contributor Author

@thisisnotashwin this will be for 1.14. I guess a few weeks before the release is a good time to merge?

@thisisnotashwin
Copy link
Contributor

@thisisnotashwin this will be for 1.14. I guess a few weeks before the release is a good time to merge?

Yup!! that sounds perfect!! Thanks Dan!

@david-yu
Copy link
Contributor

@DanStough Should this go out for 1.14 beta? If so, I think this needs a re-base off of main

@DanStough
Copy link
Contributor Author

@david-yu . Nope, not required for the beta. I will rebase shortly though.

@DanStough DanStough force-pushed the dans/NET-645/mesh-entry-updates branch from 515cc43 to 984f6fa Compare October 4, 2022 21:16
@DanStough
Copy link
Contributor Author

@ndhanushkodi would you be available to review? I finally rebased everything so there shouldn't be conflicts.

@ndhanushkodi ndhanushkodi force-pushed the dans/NET-645/mesh-entry-updates branch from 984f6fa to d988653 Compare October 7, 2022 17:43
ndhanushkodi
ndhanushkodi approved these changes Oct 7, 2022
View reviewed changes
Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

This is great and thank you for catching the missing validation!

@ndhanushkodi
Copy link
Contributor

Will wait for peering acceptance tests to pass then merge

@ndhanushkodi ndhanushkodi merged commit 9d18cd1 into main Oct 7, 2022
@ndhanushkodi ndhanushkodi deleted the dans/NET-645/mesh-entry-updates branch October 7, 2022 18:43
@natitomattis natitomattis mentioned this pull request Jun 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ndhanushkodi ndhanushkodi ndhanushkodi approved these changes

@thisisnotashwin thisisnotashwin thisisnotashwin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@DanStough @thisisnotashwin @david-yu @ndhanushkodi