Skip to content

NET-7179: Update MeshGateway to use new proto with workload selector#3465

Merged
NiniOak merged 5 commits intomainfrom
NET-7179_add_workload_selector_support
Jan 18, 2024
Merged

NET-7179: Update MeshGateway to use new proto with workload selector#3465
NiniOak merged 5 commits intomainfrom
NET-7179_add_workload_selector_support

Conversation

@NiniOak
Copy link
Copy Markdown
Collaborator

@NiniOak NiniOak commented Jan 11, 2024

Changes proposed in this PR

  • import the workload selector via go.mod to be accessible in consul-k8s. See proto PR here in Consul
  • Ensure workloadSelector field is added to gateways CRD

How I've tested this PR

local install and uninstall of Consul

How I expect reviewers to test this PR

Checklist

@NiniOak NiniOak requested review from a team, andrewstucki and t-eckert and removed request for a team January 11, 2024 00:14
@NiniOak NiniOak added pr/no-changelog PR does not need a corresponding .changelog entry theme/mesh-gw pr/no-backport signals that a PR will not contain a backport label labels Jan 11, 2024
@NiniOak NiniOak requested a review from nathancoleman January 11, 2024 00:14
Copy link
Copy Markdown
Member

@nathancoleman nathancoleman left a comment

Choose a reason for hiding this comment

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

Nice! You got through the hard part.

To update the CRD definitions, you can just run make ctrl-manifests from here and see the changes to the 2 ...meshgateways.yaml files. You should see the workload bits pop in after running that command.

The interesting thing is that we actually don't want the workload bits to appear in the CRD, so part of this work is figuring out how to make that happen. I suspect that we have 2 options:

  • some kubebuilder annotation that tells the CRD generator to skip the field, or
  • making our MeshGateway Go type a little more custom to omit the workload selector

@NiniOak NiniOak force-pushed the NET-7179_add_workload_selector_support branch from 1490933 to 429d817 Compare January 11, 2024 23:39
@NiniOak NiniOak changed the title [WIP] NET-7179: Update MeshGateway to use new proto with workload selector NET-7179: Update MeshGateway to use new proto with workload selector Jan 12, 2024
@NiniOak NiniOak marked this pull request as ready for review January 12, 2024 00:53
@NiniOak NiniOak force-pushed the NET-7179_add_workload_selector_support branch from 2e045c8 to 3bf7e7d Compare January 12, 2024 18:59
@NiniOak NiniOak requested a review from missylbytes January 12, 2024 19:00
metav1.ObjectMeta `json:"metadata,omitempty"`

Spec pbmesh.MeshGateway `json:"spec,omitempty"`
Spec pbmesh.MeshGatewaySpec `json:"spec,omitempty"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So we need to also include the WorkloadSelector on this type, so that we can send it in when we register to Consul. The tricky part is making sure this is not added to the CRD, even though it will be present here.

@NiniOak NiniOak requested a review from nathancoleman January 16, 2024 19:02
Copy link
Copy Markdown
Member

@nathancoleman nathancoleman left a comment

Choose a reason for hiding this comment

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

Nice work! Just a couple of small things I noticed

@NiniOak NiniOak force-pushed the NET-7179_add_workload_selector_support branch from 0b40357 to a1efde0 Compare January 17, 2024 19:53
Copy link
Copy Markdown
Member

@nathancoleman nathancoleman left a comment

Choose a reason for hiding this comment

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

couple of nits on review, about to test it out.

for anyone not in the conversations outside of this PR, we've decided to include the workload selector in the CRD for now and come back to remove it later. This lets us spend our time in the most valuable place possible right now, which is getting TCP traffic working through mesh-gateway.

NiniOak and others added 3 commits January 17, 2024 15:21
@NiniOak NiniOak requested a review from nathancoleman January 18, 2024 17:36
Copy link
Copy Markdown
Member

@nathancoleman nathancoleman left a comment

Choose a reason for hiding this comment

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

  • Confirmed that MeshGateway CRD created in consul-k8s includes the workload selector
  • Confirmed that MeshGateway resource created in consul includes the workload selector
  • Confirmed that the ServiceEndpoints resource in consul for the mesh gateway still selects the correct Workloads
  • Confirmed that the protocol field on the listeners is written all the way down

@NiniOak NiniOak merged commit 602ffde into main Jan 18, 2024
@NiniOak NiniOak deleted the NET-7179_add_workload_selector_support branch January 18, 2024 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr/no-backport signals that a PR will not contain a backport label pr/no-changelog PR does not need a corresponding .changelog entry theme/mesh-gw

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants