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

Support WAN Address Annotations #3420

Merged
merged 7 commits into from
Jan 3, 2024

Conversation

t-eckert
Copy link
Contributor

@t-eckert t-eckert commented Dec 21, 2023

Changes proposed in this PR

  • Add wanAddress configuration to the configmap
  • Set the annotations on the mesh gateway CRD
  • Patch through the annotations from the Mesh Gateway

How I've tested this PR

I added BATs and I ran a deployment with the following configuration:

global:
  imageK8S: consul-k8s-control-plane:local
  experiments:
    - resource-apis

meshGateway:
  enabled: true

ui:
  enabled: false

Which produced a mesh gateway with the correct configuration.

Screenshot 2023-12-21 at 1 44 17 PM

How I expect reviewers to test this PR

Checklist

@t-eckert t-eckert added pr/no-changelog PR does not need a corresponding .changelog entry pr/no-backport signals that a PR will not contain a backport label labels Dec 21, 2023
@t-eckert
Copy link
Contributor Author

The original story for this wanted the values to be set on the GatewayClassConfig by extending its configuration. Talking it over with Nate and Melisa, we decided to add the annotations directly on the Mesh Gateway CRD and patch those through. That way, we don't get mesh gateway specific config on a "general gateway" configuration object.

Also, we aren't sure if we will need these annotations in V2 and if we don't, this model is a bit easier to remove.


# Validation
# For meshGateway.wanAddress, static must be set if source is "Static"
{{if (and (eq .Values.meshGateway.wanAddress.source "Static") (eq .Values.meshGateway.wanAddress.static ""))}}{{fail ".meshGateway.wanAddress.static must be set to a value if .meshGateway.wanAddress.source is Static"}}{{ end }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added validation here that will block the invalid config of a "Static" address with no address set.

@t-eckert t-eckert force-pushed the NET-6828/support-wan-address-annotations branch from d8d6789 to 5ef1cde Compare January 2, 2024 15:23
Copy link
Member

@jm96441n jm96441n left a comment

Choose a reason for hiding this comment

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

LGTM! nice work!

@t-eckert t-eckert requested a review from missylbytes January 3, 2024 20:23
@@ -2,18 +2,20 @@

load _helpers

target=templates/gateway-resources-configmap.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀

@t-eckert t-eckert merged commit 6310d7e into main Jan 3, 2024
48 checks passed
@t-eckert t-eckert deleted the NET-6828/support-wan-address-annotations branch January 3, 2024 20:50
jm96441n pushed a commit that referenced this pull request Jan 3, 2024
* Add wanAddress configuration to the configmap

* Set the annotations on the mesh gateway CRD

* Patch through the annotations from the Mesh Gateway

* Fix Job -> ConfigMap

* Use JSON to compare annotations

* Add annotations to deployment test

* Fix checking annotations in helm tests
sarahalsmiller pushed a commit that referenced this pull request Jan 5, 2024
* Add wanAddress configuration to the configmap

* Set the annotations on the mesh gateway CRD

* Patch through the annotations from the Mesh Gateway

* Fix Job -> ConfigMap

* Use JSON to compare annotations

* Add annotations to deployment test

* Fix checking annotations in helm tests
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants