-
Notifications
You must be signed in to change notification settings - Fork 321
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
Net 6289 improve consul api gateway annotations #3437
Net 6289 improve consul api gateway annotations #3437
Conversation
References NET-6289-Improve-Consul-API-Gateway-annotations](#3171 |
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.
Thanks for pushing this across the finish line!
a767518
to
15d073b
Compare
|
||
// Fallback to "Sidecar" as a default | ||
if proxyType == "" { | ||
default: |
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 a nice clean up for this switch statement
cd77a71
to
4fc14d1
Compare
// TODO this block can be deleted if and when we decide we are ok with no longer listing pods of people using previous API Gateway | ||
// versions. |
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.
It might be worth signing this with your GitHub id so people know who wrote it (without using Git blame).
namespacedName := func(pod v1.Pod) string { | ||
return pod.Namespace + pod.Name | ||
} |
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.
Probably fine in this context, but this has the same name as a struct in Kubernetes that is structured somewhat differently. If you wanted to leverage this struct, it might be more "idiomatic". Because it has a string
method itself, the behavior will be identical when used with formatting or with string()
.
https://github.com/kubernetes/apimachinery/blob/master/pkg/types/namespacedname.go
Again, not a blocker, just a little note.
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 such a great quality of life improvement!
func isGateway(pod corev1.Pod) bool { | ||
anno, ok := pod.Annotations[constants.AnnotationGatewayKind] | ||
return ok && anno != "" | ||
return ok && anno != "" && anno != apiGateway |
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.
Nice catch!
…eway This makes api-gateway behave the same as ingress, mesh and terminating gateways; however, it won't pick up pods created by the legacy consul-api-gateway controller.
Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
4fc14d1
to
67bef5a
Compare
Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
* Add api-gateway to allow list for gateway-kind annotation * Update godoc for AnnotationGatewayKind to list correct allowable values * Exclude api-gateway kind when checking if endpoints controller should act on gateway * Add gateway-kind and gateway-consul-service-name annotations to API gateway pods * Add appropriate component label to api-gateway pods * Update consul-k8s CLI to rely on standard component label for api-gateway This makes api-gateway behave the same as ingress, mesh and terminating gateways; however, it won't pick up pods created by the legacy consul-api-gateway controller. * add backwards compatabiilty, additional tests * add changelog * Update .changelog/3437.txt Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com> * Update cli/cmd/proxy/list/command.go Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com> * Update cli/cmd/proxy/list/command_test.go Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com> * Update cli/cmd/proxy/list/command_test.go Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com> * Update cli/cmd/proxy/list/command.go Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com> * cleanup * Update cli/cmd/proxy/list/command.go Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com> --------- Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
…ase/1.2.x (#3627) Net 6289 improve consul api gateway annotations (#3437) * Add api-gateway to allow list for gateway-kind annotation * Update godoc for AnnotationGatewayKind to list correct allowable values * Exclude api-gateway kind when checking if endpoints controller should act on gateway * Add gateway-kind and gateway-consul-service-name annotations to API gateway pods * Add appropriate component label to api-gateway pods * Update consul-k8s CLI to rely on standard component label for api-gateway This makes api-gateway behave the same as ingress, mesh and terminating gateways; however, it won't pick up pods created by the legacy consul-api-gateway controller. * add backwards compatabiilty, additional tests * add changelog * Update .changelog/3437.txt * Update cli/cmd/proxy/list/command.go * Update cli/cmd/proxy/list/command_test.go * Update cli/cmd/proxy/list/command_test.go * Update cli/cmd/proxy/list/command.go * cleanup * Update cli/cmd/proxy/list/command.go --------- Co-authored-by: sarahalsmiller <100602640+sarahalsmiller@users.noreply.github.com> Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
Changes proposed in this PR
How I've tested this PR
How I expect reviewers to test this PR
Checklist