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.

Sign up for GitHub

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

Jump to bottom

Use new NGINX Plus API #7

Merged
merged 1 commit into from
Jun 26, 2018
Merged

Use new NGINX Plus API #7

merged 1 commit into from
Jun 26, 2018

Conversation

Dean-Coakley
Copy link
Contributor

Nginx Plus sdk needs to be updated before /upstream_conf and /status apis are deprecated in R16 in favour of a unified /api endpoint

@Dean-Coakley Dean-Coakley self-assigned this Jun 15, 2018
pleshakov
pleshakov requested changes Jun 22, 2018
View reviewed changes
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

Requested a couple of changes. Thanks

README.md Outdated
* The on-the-fly API is available at **127.0.0.1:8080/upstream_conf**
* The status API is available at **127.0.0.1:8080/status**
* We define a second virtual server listening on port 8080 and configure the NGINX Plus API on it, which is required by nginx-asg-sync:
* The API is available at **127.0.0.1:8080/api/2/**
Copy link
Contributor

Choose a reason for hiding this comment

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

127.0.0.1:8080/api/2/ -> 127.0.0.1:8080/api

README.md Outdated

location /upstream_conf {
upstream_conf;
location = /api {
Copy link
Contributor

Choose a reason for hiding this comment

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

location = /api -> location /api

also, add

    location = /dashboard.html {
      root   /usr/share/nginx/html;
    }

README.md Outdated
@@ -33,8 +33,7 @@ nginx-asg-sync uses the AWS API to get the list of IP addresses of the instances

1. [Create an IAM role](http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/iam-roles-for-amazon-ec2.html) and attach the predefined `AmazonEC2ReadOnlyAccess` policy to it. This policy allows read-only access to EC2 APIs.
1. When you launch the NGINX Plus instance, add this IAM role to the instance.

Alternatively, you can use the `AWS_ACCESS_KEY_ID` and `AWS_SECRET_ACCESS_KEY` environmental variables to provide credentials to nginx-asg-sync.
1. Set `AWS_ACCESS_KEY_ID` and `AWS_SECRET_ACCESS_KEY` environment variables to provide credentials to nginx-asg-sync.
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this line

cmd/sync/config_test.go Outdated
@@ -3,8 +3,7 @@ package main
import "testing"

var validYaml = []byte(`region: us-west-2
upstream_conf_endpoint: http://127.0.0.1:8080/upstream_conf
status_endpoint: http://127.0.0.1:8080/status
api_endpoint: http://127.0.0.1:8080/api/
Copy link
Contributor

Choose a reason for hiding this comment

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

remove trailing /

cmd/sync/config_test.go Outdated
@@ -33,8 +32,7 @@ func getValidConfig() *config {
}
cfg := config{
Region: "us-west-2",
UpstreamConfEndpont: "http://127.0.0.1:8080/upstream_conf",
StatusEndpoint: "http://127.0.0.1:8080/status",
APIEndpoint: "http://127.0.0.1:8080/api/",
Copy link
Contributor

Choose a reason for hiding this comment

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

remove trailing /

cmd/sync/main.go Outdated
if err != nil {
log.Printf("Couldn't update servers in NGINX: %v", err)
continue
}
if len(removed) > 0 || len(added) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

bring this back :)

cmd/sync/plus/nginx_api.go Outdated
)

// NginxAPIController stores a NginxClient.
type NginxAPIController struct {

This comment was marked as resolved.

Sign in to view

cmd/sync/plus/nginx_api.go Outdated
// HTTP Servers that are in the slice, but don't exist in NGINX, will be added to NGINX.
// HTTP Servers that are not in the slice, but exist in NIGNX, will be removed from NGINX.
func (nginx *NginxAPIController) UpdateServers(upstream string, servers []string, config ServerConfig) error {
var upsServers []UpstreamServer
Copy link
Contributor

Choose a reason for hiding this comment

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

move this logic - lines 34-41 into main
from main, call UpdateHTTPServers of NginxClient

Copy link
Contributor

Choose a reason for hiding this comment

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

also, set MaxFails to 1, as it is NGINX default

Copy link
Contributor

Choose a reason for hiding this comment

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

same applies to UpdateStreamServers

cmd/sync/plus/nginx_api.go Outdated
client *NginxClient
}

// ServerConfig stores a server configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need this type as well

Copy link
Contributor

Choose a reason for hiding this comment

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

we'll use NewNginxClient directly instead of NewNginxAPIController

cmd/sync/plus/nginx.go Outdated
@@ -0,0 +1,655 @@
package plus
Copy link
Contributor

Choose a reason for hiding this comment

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

move this file back to cmd/sync

@Dean-Coakley Dean-Coakley added the enhancement Pull requests for new features/feature enhancements label Jun 25, 2018
pleshakov
pleshakov requested changes Jun 25, 2018
View reviewed changes
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

A few comments

README.md Outdated

### nginx-asg-sync Configuration

nginx-asg-sync is configured in the file **aws.yaml** in the **/etc/nginx** folder. For our example, we define the following configuration:

```yaml
region: us-west-2
upstream_conf_endpoint: http://127.0.0.1:8080/upstream_conf
status_endpoint: http://127.0.0.1:8080/status
api_endpoint: http://127.0.0.1:8080/api/
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the trailing /

cmd/sync/main.go Outdated
added, removed, err = nginx.UpdateHTTPServers(ups.Name, backends)
if upstream.Kind == "http" {
var upsServers []UpstreamServer
for _, server := range backends {
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of iterating over backends, could we iterate over ips. same for line 120. this way we can remove lines 94-98.

@Dean-Coakley Dean-Coakley force-pushed the nginx-plus-sdk-update branch 3 times, most recently from bfcabe1 to 2958d28 Compare June 26, 2018 11:56
@pleshakov pleshakov changed the title Nginx Plus sdk update Use new NGINX Plus API Jun 26, 2018
pleshakov
pleshakov requested changes Jun 26, 2018
View reviewed changes
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

Added a few suggestions

cmd/sync/config_test.go
@@ -49,14 +47,6 @@ func getInvalidConfigInput() []*testInput {
invalidRegionCfg.Region = ""
input = append(input, &testInput{invalidRegionCfg, "invalid region"})

invalidUpstreamConfEndponCfg := getValidConfig()

This comment was marked as resolved.

Sign in to view

cmd/sync/nginx.go Outdated
)

// NginxClient lets you update HTTP/Stream servers in NGINX Plus via its upstream_conf API
// Client version - 380a939
Copy link
Contributor

Choose a reason for hiding this comment

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

Please double check that it is 380a939

@Dean-Coakley
Use new NGINX Plus API
300cc70 
* Update to use unified /api endpoint
* Update documentation with architecture
* Update NGINX Go SDK
* Update Go version
* Add api invalid endpoint test
@pleshakov pleshakov merged commit b750af3 into master Jun 26, 2018
@Dean-Coakley Dean-Coakley deleted the nginx-plus-sdk-update branch June 26, 2018 15:31
@Dean-Coakley
Copy link
Contributor Author

Fixes: #5

@Dean-Coakley Dean-Coakley mentioned this pull request Aug 2, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pleshakov pleshakov pleshakov requested changes

@isaachawley isaachawley isaachawley approved these changes

Assignees

@Dean-Coakley Dean-Coakley

Labels
enhancement Pull requests for new features/feature enhancements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Dean-Coakley @isaachawley @pleshakov