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

Add envoyExtensions field to serviceDefaults and proxyDefaults CRDs #1823

Merged
merged 12 commits into from
Jan 12, 2023

Conversation

wilkermichael
Copy link
Contributor

@wilkermichael wilkermichael commented Jan 9, 2023

NOTE TO THE REVIEWER: I organize my commits so that they're meaningful, I suggest reviewing my code commit by commit. There are a lot of files changed, but some of these are generated/fixed terraform formatting.

Changes proposed in this PR:
This PR adds envoyExtensions to the serviceDefaults and proxyDefaults CRDs. The goal is to create parity with what is already in Consul:
serviceDefaults
proxyDefaults

In addition to adding envoyExtensions I also added the following:

  • added the missing BalanceInboundConnections field to serviceDefaults
  • added CI for checking terraform formatting and fixed some terraform formatting
  • fixed README for CRD contributions

How I've tested this PR:

  • Created unit tests
  • Created consul-k8s-control-plane and latest Consul main images and helm installed them to K8s
  • applied new proxyDefault and serviceDefault resources and verified the validation logic and that they synced correctly with Consul
  • verified Consul config-entry exists for both proxyDefault and serviceDefault with the new fields

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

@wilkermichael wilkermichael force-pushed the mw/update-crd-envoy-extensions branch 9 times, most recently from 27254aa to 0df18ab Compare January 10, 2023 23:00
@wilkermichael wilkermichael marked this pull request as ready for review January 10, 2023 23:01
@wilkermichael wilkermichael changed the title Mw/update crd envoy extensions Add envoyExtensions field to serviceDefaults and proxyDefaults CRDs Jan 10, 2023
"meshgateway.mode": {
&ProxyDefaults{
input: &ProxyDefaults{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can ignore most of these, I just autogenerated the field names rather than leaving them blank

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 is excellent!! thanks for also adding the terraform linting 🙏
Have a few comments inline but other than that, it looks really good!! great work on the PR!!

control-plane/api/v1alpha1/servicedefaults_types.go Outdated Show resolved Hide resolved
control-plane/api/v1alpha1/servicedefaults_types.go Outdated Show resolved Hide resolved
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.

:chefkiss:

@@ -239,7 +244,7 @@ func (in *ProxyDefaults) validateConfig(path *field.Path) *field.Error {
}
var outConfig map[string]interface{}
if err := json.Unmarshal(in.Spec.Config, &outConfig); err != nil {
return field.Invalid(path, in.Spec.Config, fmt.Sprintf(`must be valid map value: %s`, err))
return field.Invalid(path, string(in.Spec.Config), fmt.Sprintf(`must be valid map value: %s`, err))
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@wilkermichael wilkermichael force-pushed the mw/update-crd-envoy-extensions branch 3 times, most recently from 655f93d to 19f0f6d Compare January 12, 2023 18:25
- Added fields to support EnvoyExtensions
- Added missing BalanceInboundConnections field
- most of these changes are parallel to what was done in serviceDefaults
- proxyDefaults makes use of the envoyExtension(s) definition in serviceDefaults and also makes use of the validation/toConsul logic defined there
- Added tests for proxyDefault
- Added tests for serviceDefault
EnvoyExtension test cases should be basically the same for both CRDs
@wilkermichael wilkermichael force-pushed the mw/update-crd-envoy-extensions branch from 19f0f6d to 26fd456 Compare January 12, 2023 19:46
@wilkermichael wilkermichael merged commit a5cc951 into main Jan 12, 2023
@wilkermichael wilkermichael deleted the mw/update-crd-envoy-extensions branch January 12, 2023 20:59
david-yu pushed a commit that referenced this pull request Feb 14, 2023
…1823)

* updated consul api to the latest

* added new ServiceDefault fields to the serviceDefaults CRD
- Added fields to support EnvoyExtensions
- Added missing BalanceInboundConnections field

* added EnvoyExtensions to proxyDefaults CRD
- most of these changes are parallel to what was done in serviceDefaults
- proxyDefaults makes use of the envoyExtension(s) definition in serviceDefaults and also makes use of the validation/toConsul logic defined there

* added tests for new fields
- Added tests for proxyDefault
- Added tests for serviceDefault
- EnvoyExtension test cases should be basically the same for both CRDs

* generate the manifests and generate deepcopy

* updated the changelog

* added CI test to catch bad terraform formatting

* formatted terraform files

* update contributing doc
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.

2 participants