-
Notifications
You must be signed in to change notification settings - Fork 316
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 support for field TransparentProxy to SvcDefaults and ProxyDefaults #485
Conversation
654a3b2
to
92f8fe0
Compare
…th checks (#472) remove health checks controller and use endpoints controller for health checks.
cd9bf12
to
71099b6
Compare
8bddde2
to
1bdf2ad
Compare
- Update the spec of ServiceDefaults and ProxyDefaults to support transparent proxy changes that are introduced as a part of Consul 1.10
1bdf2ad
to
52f4fb9
Compare
TransparentProxy: &capi.TransparentProxyConfig{ | ||
OutboundListenerPort: 0, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the zero field issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup!
// Valid values are "http" and "http2", defaults to "http". | ||
Protocol string `json:"protocol,omitempty"` | ||
// TransparentProxy controls configuration specific to proxies in transparent mode. | ||
TransparentProxy *TransparentProxyConfig `json:"transparentProxy,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: looks like our other config entries don't suffix Config
for the helper structs so maybe we can just use TransparentProxy
and swap MeshGatewayConfig
to MeshGateway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated ExposeConfig to Expose as well!!
api/v1alpha1/types.go
Outdated
@@ -30,6 +30,37 @@ const ( | |||
MeshGatewayModeRemote MeshGatewayMode = "remote" | |||
) | |||
|
|||
// ExposeConfig describes HTTP paths to expose through Envoy outside of Connect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: looks like the MeshGateway stuff above here isn't used at all and can be deleted
api/v1alpha1/types.go
Outdated
@@ -39,8 +70,8 @@ type MeshGatewayConfig struct { | |||
} | |||
|
|||
// toConsul returns the MeshGatewayConfig for the entry | |||
func (m MeshGatewayConfig) toConsul() capi.MeshGatewayConfig { | |||
mode := capi.MeshGatewayMode(m.Mode) | |||
func (in MeshGatewayConfig) toConsul() capi.MeshGatewayConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍺
api/v1alpha1/types.go
Outdated
} | ||
return nil | ||
} | ||
|
||
// toConsul returns the ExposeConfig for the entry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can kill these doc blocks, none of the other entries have them for toConsul
- Remove unused contstants for MeshGateway modes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great Ashwin!! Just had some non-blocking questions!
description: TransparentProxy controls configuration specific to proxies in transparent mode. | ||
properties: | ||
outboundListenerPort: | ||
description: The port of the listener where outbound application traffic is being redirected to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should add something like "this would not typically be set via the CRD, but this field exists to match the config-entry", or is it not worth it with the validation error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a user does try to explicitly set this field, a validation error is returned from the webhook 😄
The error message returned by the webhook is: Invalid value: v1alpha1.TransparentProxy{OutboundListenerPort:}: use the annotation consul.hashicorp.com/transparent-proxy-outbound-listener-port
to configure the Outbound Listener Port
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I had noticed that, and that should be enough :)
description: TransparentProxy controls configuration specific to proxies in transparent mode. | ||
properties: | ||
outboundListenerPort: | ||
description: The port of the listener where outbound application traffic is being redirected to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same q as above :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same answer as above ;)
@@ -560,6 +560,7 @@ func TestReconcileCreateEndpoint(t *testing.T) { | |||
DestinationServiceID: "pod1-service-created", | |||
LocalServiceAddress: "", | |||
LocalServicePort: 0, | |||
TransparentProxy: &api.TransparentProxyConfig{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these necessary if empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, not in this case as it expects a pointer. An empty value would equate to a nil
and not an empty field. this should be fixed in the future and we should be able to revert this change when Consul returns a nil value when provided a nil value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sweet, I wasn't sure if this fell under that case, thanks for the explanation!!
Changes proposed in this PR:
Add support for the new Transparent Proxy field on ServiceDefaults and ProxyDefaults. Something to note here is that currently, the state/config_entry.go always sets a zero value for the field if it is set to a
nil
value when it gets returned from Consul. Hence a lot of text fixtures needed to be updated to ensure that they explicitly expect the TransparentProxy to be set. Additionally, if the incoming CRD does not have the value set, we explicitly set the value to its zero value so thatMatchesConsul
returns true.This will be fixed in Consul before the next beta release potentially and parts of the fixture can be reverted.
Update the webhook versions from v1beta1 to v1 and v1beta1 support is deprecated in Kubernetes 1.16+
How I've tested this PR: Added some test. Successful acceptance test when the CRDs were updated in helm.
How I expect reviewers to test this PR: Code Review.
Checklist: