[specific ci=Group23-Vic-Machine-Service] syslog support added to VCH create and inspect API #6582

AngieCris · 2017-10-18T19:08:11Z

syslog support is now added to vic-machine create API and inspect API.

Details

vic-machine create API
POST request body includes a syslog_addr field of type string that specifies the syslog server address.
syslog_addr must have format transport://host[:port], where transport is udp or tcp (ipv6 is not officially supported yet)
(For example: tcp://127.0.0.1:4444)

An example of vic-machine create API request body:

{
	"name": "vch-test-1",
	"storage":{
		"image_stores":["ds://datastore1"]
	},
	"network":{
		"bridge":{
			"ip_range":"172.16.0.0/12",
			"port_group":{"name":"bridge"}
		},
		"public":{
			"port_group":{"name":"vm-network"}
		},
		"management":{
			"port_group":{"name":"management"}
		}
	},
	"auth":{
		"no_tls":true
	},
	"syslog_addr": "tcp://127.0.0.1:4444"
}

vic-machine inspect API
The response body of POST request now includes a field syslog_addr field of type string that includes the syslog server address that the user is specified when the VCH is first created.

Example vic-machine inspect GET response body:

{
  "auth": {
    "client": {
      "certificate_authorities": []
    }
  },
  "compute": {
    "cpu": {
      "shares": {
        "level": "normal"
      }
    },
    "memory": {
      "shares": {
        "level": "normal"
      }
    },
    "resource": {
      "id": "resgroup-v35"
    }
  },
  "endpoint": {
    "cpu": {
      "sockets": 1
    },
    "memory": {
      "units": "MiB",
      "value": 2048
    },
    "operations_credentials": {}
  },
  "name": "vch-test-1",
  "network": {
    "bridge": {
      "ip_range": "172.16.0.0/12",
      "port_group": {
        "id": "dvportgroup-23"
      }
    },
    "client": {
      "nameservers": [],
      "port_group": {
        "id": "dvportgroup-22"
      }
    },
    "container": [],
    "management": {
      "nameservers": [],
      "port_group": {
        "id": "dvportgroup-21"
      }
    },
    "public": {
      "nameservers": [],
      "port_group": {
        "id": "dvportgroup-22"
      }
    }
  },
  "registry": {
    "blacklist": null,
    "certificate_authorities": [],
    "insecure": [],
    "whitelist": []
  },
  "runtime": {
    "admin_portal": "https://10.160.213.153:2378",
    "docker_host": "10.160.213.153:2375",
    "power_state": "poweredOn",
    "upgrade_status": "Up to date"
  },
  "storage": {
    "base_image_size": {
      "units": "KiB",
      "value": 8000000
    },
    "image_stores": [
      "ds://datastore1/vch-test-1"
    ],
    "volume_stores": []
  },
  "syslog_addr": "tcp://127.0.0.1:4444",
  "version": "v1.2.0-rc1-0-a3b3132"
}

Remaining work
Pending integration tests that checks if syslog server configuration on create and inspect, and unit tests.
Will be integrated after create API test and inspect API test are ready.
(tracking story: #6018 )

andrewtchin · 2017-10-18T20:15:10Z

Are there any unit or integration tests required for this?

AngieCris · 2017-10-18T20:56:16Z

@andrewtchin yes but not implemented yet. Since it's part of VCH create and inspect APIs (part of create API handler input and inspect API output), I would expect the tests be included in the create API and inspect API tests (I'm not sure how to test this separately

mdharamadas1

lgtm, pls make sure to track somewhere to add integration test for this later as in desc.

zjs · 2017-10-19T16:22:30Z

lib/apiservers/service/swagger.json

+        "syslog_addr": {
+          "type": "string",
+          "format": "uri",
+          "pattern": "^(tc|ud)p.*"


The documentation on SysLogConfig (in container_vm.go) says "Network can be udp, tcp, udp6, or tcp6". I think we should add the optional "6" to this regex.

actually from the output of vic-machine-linux create -x | grep syslog:

--syslog-address value Address of the syslog server to send Virtual Container Host logs to. Must be in the format transport://host[:port], where transport is udp or tcp.

It seems like only "udp" or "tcp" transport is supported.
I modified the swagger tried with address tcp6://127.0.0.1:4444, it won't go into vchConfig, so even if build succeeds, inspect won't give it as output.

https://github.com/vmware/vic/blob/master/lib/install/validate/validator.go#L860
Here in validator, only udp and tcp transport goes into config. It doesn't accept anything else
Not sure if it's a bug...
@zjs

confirmed with @hmahmood that ipv6 is not officially supported by syslogs yet. So we'll go with ^(tc|ud)p for now

zjs · 2017-10-19T16:23:08Z

lib/apiservers/service/restapi/handlers/vch_create.go

@@ -79,7 +80,7 @@ func (h *VCHCreate) Handle(params operations.PostTargetTargetVchParams, principa

 	validator, err := validateTarget(params.HTTPRequest.Context(), d)
 	if err != nil {
-		return operations.NewPostTargetTargetVchDefault(400).WithPayload(&models.Error{Message: err.Error()})
+		return operations.NewPostTargetTargetVchDefault(http.StatusBadRequest).WithPayload(&models.Error{Message: err.Error()})


I think this is a really good change. Can you file an issue so that we remember to clean this up in the rest of the handlers?

i can do it in this pr

zjs · 2017-10-19T16:24:48Z

lib/apiservers/service/restapi/handlers/vch_create.go

 				}
 			}
 		}
+
+		if len(vch.SyslogAddr) > 0 {


Other places in this handler, we seem to be checking != "". I don't know which pattern is better, but I think it would be good to be consistent.

!= "" makes more sense.
I was still in the java mindset that strings cannot be directly compared with equals

mhagen-vmware · 2017-10-20T06:22:40Z

lib/apiservers/service/restapi/handlers/vch_create.go

+			if err := c.ProcessSyslog(); err != nil {
+				return nil, util.NewError(http.StatusBadRequest, fmt.Sprintf("Error processing syslog server address: %s", err))
+			}
+		}


Why can't you write a simple unit test to verify these new code paths?

This is part of the create API handler and we haven't figured out a pattern to do unit tests on swagger handlers yet.
The new code path validates a field in http GET request JSON, and it's very hard to test it separately from all the other fields related to vch create params.
We're planning to integrate this in unit tests for create API as a whole (the tracking story is: #6018)

* syslog support added to VCH create API * syslog addr added to inspect output * syslog_addr format changed in swagger * use status code in http package * empty syslog_addr string checking * swagger format changed for syslog_addr * roll back in swagger: only tcp and udp are accepted

AngieCris added 2 commits October 17, 2017 14:25

syslog support added to VCH create API

a3b3132

syslog addr added to inspect output

e518893

AngieCris requested review from jzt, zjs and andrewtchin October 18, 2017 19:08

vmwclabot added the cla-not-required label Oct 18, 2017

AngieCris self-assigned this Oct 18, 2017

AngieCris requested review from jak-atx and cristianfalcone October 18, 2017 20:21

andrewtchin approved these changes Oct 18, 2017

View reviewed changes

jzt approved these changes Oct 18, 2017

View reviewed changes

matthewavery approved these changes Oct 19, 2017

View reviewed changes

syslog_addr format changed in swagger

dd03e5b

mdharamadas1 approved these changes Oct 19, 2017

View reviewed changes

zjs reviewed Oct 19, 2017

View reviewed changes

AngieCris added 3 commits October 19, 2017 11:35

use status code in http package

17e3514

empty syslog_addr string checking

77eee58

swagger format changed for syslog_addr

16c412c

mhagen-vmware reviewed Oct 20, 2017

View reviewed changes

roll back in swagger: only tcp and udp are accepted

56fb57a

zjs approved these changes Oct 20, 2017

View reviewed changes

AngieCris merged commit 7008981 into vmware:feature/vic-machine-service Oct 24, 2017

AngieCris mentioned this pull request Oct 24, 2017

Add syslog support to VCH creation API #6476

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[specific ci=Group23-Vic-Machine-Service] syslog support added to VCH create and inspect API #6582

[specific ci=Group23-Vic-Machine-Service] syslog support added to VCH create and inspect API #6582

AngieCris commented Oct 18, 2017 •

edited

Loading

andrewtchin commented Oct 18, 2017

AngieCris commented Oct 18, 2017 •

edited

Loading

mdharamadas1 left a comment

zjs Oct 19, 2017

AngieCris Oct 19, 2017 •

edited

Loading

AngieCris Oct 19, 2017 •

edited

Loading

AngieCris Oct 20, 2017 •

edited

Loading

zjs Oct 19, 2017

AngieCris Oct 19, 2017

AngieCris Oct 20, 2017

zjs Oct 19, 2017

AngieCris Oct 19, 2017

mhagen-vmware Oct 20, 2017

AngieCris Oct 20, 2017

[specific ci=Group23-Vic-Machine-Service] syslog support added to VCH create and inspect API #6582

[specific ci=Group23-Vic-Machine-Service] syslog support added to VCH create and inspect API #6582

Conversation

AngieCris commented Oct 18, 2017 • edited Loading

andrewtchin commented Oct 18, 2017

AngieCris commented Oct 18, 2017 • edited Loading

mdharamadas1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AngieCris Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

AngieCris Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

AngieCris Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AngieCris commented Oct 18, 2017 •

edited

Loading

AngieCris commented Oct 18, 2017 •

edited

Loading

AngieCris Oct 19, 2017 •

edited

Loading

AngieCris Oct 19, 2017 •

edited

Loading

AngieCris Oct 20, 2017 •

edited

Loading