NLB-7285: Add dataplane_api_key as an optional #1012
Conversation
|
🎉 Thank you for your contribution! It appears you have not yet signed the F5 Contributor License Agreement (CLA), which is required for your changes to be incorporated into an F5 Open Source Software (OSS) project. Please kindly read the F5 CLA and reply on a new comment with the following text to agree: I have hereby read the F5 CLA and agree to its terms You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
github-actions
bot
added
the
documentation
|
I have hereby read the F5 CLA and agree to its terms |
|
recheck |
fblr
left a comment
fblr
left a comment
There was a problem hiding this comment.
Hello,
You are suggesting a change to a generic open source project. I understand this is the change required by NGINXaaS, but you are actually adding an API key in the Authorization header - there is nothing N4A-specific here. Why not remove the data plane mentioning (DataplaneAPIKey -> APIKey and so on) from the code and leave it as an example in the description?
The second related problem: you unconditionally add a Content-Type header - please make it configurable.
| func NewHTTPClient(cfg *commonConfig) *http.Client { | ||
| headers := NewHeaders(cfg) | ||
| baseTransport := &http.Transport{} | ||
|
|
||
| return &http.Client{ | ||
| Transport: &headerTransport{ | ||
| headers: headers, | ||
| transport: baseTransport, | ||
| }, | ||
| Timeout: connTimeoutInSecs * time.Second, | ||
| } | ||
| } |
There was a problem hiding this comment.
we don't want to add a performance penalty if a customer is not using dataplane_api_key.
As written, all requests are going to jump through headerTransport, whether it needs to or not. This creates a little more pressure on the garbage collector and uses more compute than needed. Let's add a check in here to only add headers if we need to.
Maybe something like (untested):
| func NewHTTPClient(cfg *commonConfig) *http.Client { | |
| headers := NewHeaders(cfg) | |
| baseTransport := &http.Transport{} | |
| return &http.Client{ | |
| Transport: &headerTransport{ | |
| headers: headers, | |
| transport: baseTransport, | |
| }, | |
| Timeout: connTimeoutInSecs * time.Second, | |
| } | |
| } | |
| func NewHTTPClient(cfg *commonConfig) *http.Client { | |
| var rt http.RoundTripper = &http.Transport{} | |
| if h := NewHeaders(cfg); len(h) > 0 { | |
| rt = &headerTransport{headers: h, transport: rt} | |
| } | |
| return &http.Client{ | |
| Transport: rt | |
| Timeout: connTimeoutInSecs * time.Second, | |
| } | |
| } |
| func (t *headerTransport) RoundTrip(req *http.Request) (*http.Response, error) { | ||
| clonedReq := req.Clone(req.Context()) | ||
|
|
||
| for key, values := range t.headers { | ||
| for _, value := range values { | ||
| clonedReq.Header.Add(key, value) | ||
| } | ||
| } | ||
|
|
||
| if len(clonedReq.Header) > maxHeaders { | ||
| return nil, fmt.Errorf("number of headers in request exceeds the maximum allowed (%d)", maxHeaders) | ||
| } |
There was a problem hiding this comment.
it's nice to have these sanity checks at the top of function to conserve CPU. If we have too many headers, we've wasted some time cloning and adding.
I think we can tweak the condition and prevent some doomed allocations.
Something like (untested):
| func (t *headerTransport) RoundTrip(req *http.Request) (*http.Response, error) { | |
| clonedReq := req.Clone(req.Context()) | |
| for key, values := range t.headers { | |
| for _, value := range values { | |
| clonedReq.Header.Add(key, value) | |
| } | |
| } | |
| if len(clonedReq.Header) > maxHeaders { | |
| return nil, fmt.Errorf("number of headers in request exceeds the maximum allowed (%d)", maxHeaders) | |
| } | |
| func (t *headerTransport) RoundTrip(req *http.Request) (*http.Response, error) { | |
| if len(req.Header) + len(t.headers) > maxHeaders { | |
| return nil, fmt.Errorf("number of headers in request exceeds the maximum allowed (%d)", maxHeaders) | |
| } | |
| clonedReq := req.Clone(req.Context()) | |
| for key, values := range t.headers { | |
| for _, value := range values { | |
| clonedReq.Header.Add(key, value) | |
| } | |
| } |
| ``` | ||
| - The `api_endpoint` key defines the NGINX Plus API endpoint. | ||
| - The `dataplane_api_key` key (optional) defines the API key for authenticating with the [Dataplane API](https://docs.nginx.com/nginxaas/azure/loadbalancer-kubernetes/#view-nginxaas-data-plane-api-endpoint-using-the-azure-portal) |
There was a problem hiding this comment.
let's get specific here
| - The `dataplane_api_key` key (optional) defines the API key for authenticating with the [Dataplane API](https://docs.nginx.com/nginxaas/azure/loadbalancer-kubernetes/#view-nginxaas-data-plane-api-endpoint-using-the-azure-portal) | |
| - The `dataplane_api_key` key (optional) defines the API key for authenticating with the [NGINXaaS For Azure Dataplane API](https://docs.nginx.com/nginxaas/azure/loadbalancer-kubernetes/#view-nginxaas-data-plane-api-endpoint-using-the-azure-portal). |
| - The `dataplane_api_key` key (optional) defines the API key for authenticating with the [Dataplane API](https://docs.nginx.com/nginxaas/azure/loadbalancer-kubernetes/#view-nginxaas-data-plane-api-endpoint-using-the-azure-portal) | ||
| of [NGINXaaS for Azure](https://docs.nginx.com/nginxaas/azure). |
There was a problem hiding this comment.
we can use the same phrasing in both places
| - The `dataplane_api_key` key (optional) defines the API key for authenticating with the [Dataplane API](https://docs.nginx.com/nginxaas/azure/loadbalancer-kubernetes/#view-nginxaas-data-plane-api-endpoint-using-the-azure-portal) | |
| of [NGINXaaS for Azure](https://docs.nginx.com/nginxaas/azure). | |
| - The `dataplane_api_key` key (optional) defines the API key for authenticating with the [NGINXaaS for Azure Dataplane API](https://docs.nginx.com/nginxaas/azure/loadbalancer-kubernetes/#view-nginxaas-data-plane-api-endpoint-using-the-azure-portal). |
| } | ||
|
|
||
| // Helper function to check if a string contains a substring. | ||
| func containsString(s, substr string) bool { |
There was a problem hiding this comment.
| } else { | ||
| log.Printf("[optional] DataplaneAPIKey not configured") | ||
| } |
There was a problem hiding this comment.
this log message doesn't seem that useful
| } else { | |
| log.Printf("[optional] DataplaneAPIKey not configured") | |
| } | |
| } |
| # optional dataplane_api_key | ||
| # dataplane_api_key: your-api-key |
There was a problem hiding this comment.
alternate design idea:
What if instead of dataplane_api_key we kept it very generic, like additional_headers?
So for NGINXaaS integration the user would add:
additional_headers:
Authorization: ApiKey $BASE_64_KEYThat'd keep the open source codebase very decoupled from our cloud offering, but put a little more work onto the user to get the headers right.
What do y'all think?
| { | ||
| name: "with empty API key", | ||
| config: &commonConfig{ | ||
| DataplaneAPIKey: "", |
There was a problem hiding this comment.
due to golang zero values, this test case is the same as the "without API key" case, let's drop it. Each test case should improve our test code coverage, and if it doesn't then we don't need to keep it around.
3 participants
Proposed changes
UseCase
We need
dataplane_api_keyin authorization header to make api call toDataplane API. We want to makeapi_endpointvalue as dataplane api which is proxy/copy of nginx plus api that we use as part of NGINXaaS for Azure.So, when the VMSS (azure scaling group) scales out/scales in,
nginx-asg-syncmake api calls to dataplane api which updates the upstream servers.Upstream server update logs
Load balancing incase of 2 instances
Manually scaled to 4 instances
Load balancing after upstream servers updated by nginx-asg-sync
Checklist
Before creating a PR, run through this checklist and mark each as complete.