-
Notifications
You must be signed in to change notification settings - Fork 326
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
initial support for API Gateway #925
Conversation
d41317c
to
a85c585
Compare
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.
Looking really good, Nick!! I'm excited about this work to be merged 🎉
I'm leaving some comments I have so far mostly about our Helm chart conventions and also some questions/suggestions. I didn't look at the bats tests though.
charts/consul/templates/api-gateway-controller-clusterrole.yaml
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,147 @@ | |||
{{- if (or (and (ne (.Values.apiGateway.enabled | toString) "-") .Values.apiGateway.enabled) (and (eq (.Values.apiGateway.enabled | toString) "-") .Values.global.enabled)) }} | |||
{{- if not .Values.client.grpc }}{{ fail "client.grpc must be true for api gateway" }}{{ end }} | |||
{{- if and .Values.global.adminPartitions.enabled (not .Values.global.enableConsulNamespaces) }}{{ fail "global.enableConsulNamespaces must be true if global.adminPartitions.enabled=true" }}{{ end }} |
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.
If the api gw has other prerequisites, it'd be good add those validations here.
Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>
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 good, just a couple of minor comments.
+1 To Luke's comment about the unit tests for the ACL token.
I know we talked about adding an acceptance test, but I'm curious if you decided not to do it or do it in a separate PR?
@test "apiGateway/Deployment: enable with global.enabled false, apiGateway.enabled true" { | ||
cd `chart_dir` | ||
local actual=$(helm template \ | ||
-s templates/api-gateway-controller-deployment.yaml \ | ||
--set 'global.enabled=false' \ | ||
--set 'apiGateway.enabled=true' \ | ||
--set 'apiGateway.image=foo' \ | ||
. | tee /dev/stderr | | ||
yq 'length > 0' | tee /dev/stderr) | ||
[ "${actual}" = "true" ] | ||
} |
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.
I think we don't need this test and disable with global.enabled
since the api gw is no longer inheriting from the global.enabled
charts/consul/test/unit/api-gateway-controller-serviceaccount.bats
Outdated
Show resolved
Hide resolved
charts/consul/test/unit/api-gateway-controller-serviceaccount.bats
Outdated
Show resolved
Hide resolved
# @type: string | ||
logLevel: info | ||
|
||
managedGatewayClass: |
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 one also needs docs
Thanks @ishustava. As far as acceptance test I had planned to follow up once we have release so there is a stable binary to reference from the test. |
Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>
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 good 🎉 I'm approving assuming the unit tests Luke mentioned are added. All of my recent comments are non-blocking
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.
Just a bunch of drive-by suggestions to sync this PR with #993
charts/consul/templates/api-gateway-controller-clusterrolebinding.yaml
Outdated
Show resolved
Hide resolved
charts/consul/templates/api-gateway-controller-clusterrolebinding.yaml
Outdated
Show resolved
Hide resolved
charts/consul/templates/api-gateway-controller-serviceaccount.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Ashwin Venkatesh <ashwin@hashicorp.com>
Changes proposed in this PR:
How I've tested this PR:
How I expect reviewers to test this PR:
Checklist: