Allow the usage of a string or int as a value in portMappings #455

cdrage · 2017-11-15T16:55:03Z

This allows the use of specifying either (for example) "80" or 80 within
portMappings of the Kedge file.

kadel · 2017-11-15T17:00:48Z

We should also have unit tests covering this.

cdrage · 2017-11-15T17:18:58Z

@kadel Yup. I'll add them the next few days. Just pushing this out first as proof of conversion (and for integration tests to successfully pass)

kadel · 2017-11-15T19:22:31Z

Code looks good.
This might be good inspiration for tests https://github.com/kubernetes/kubernetes/blob/dd819b5013c99f930a6d528e508a900fa7679c4f/staging/src/k8s.io/apimachinery/pkg/util/intstr/intstr_test.go#L45

cdrage · 2017-11-16T17:58:37Z

@kadel Thanks for the pointer! Tests have been added 👍

cdrage · 2017-11-17T16:29:04Z

@surajssd @kadel please review tests 💯 and then this will unblock #435 so we can push that in.

surajssd

I am trying to mix the portMapping of type string and int, so the diff looks like this:

$ git diff
diff --git a/tests/cmd/portmappings/port.yaml b/tests/cmd/portmappings/port.yaml
index 093de03..9eb00dd 100644
--- a/tests/cmd/portmappings/port.yaml
+++ b/tests/cmd/portmappings/port.yaml
@@ -7,3 +7,4 @@ services:
   - name: database
     portMappings:
       - 3306
+      - 3456:7890

Doing conversion gives error:

$ kedge generate -f port.yaml 
unable to perform controller operations: unable to unmarshal data: DeploymentSpecMod could not unmarshal
into internal struct: error unmarshaling JSON: Failed to unmarshal SliceArrayorIntArray

surajssd · 2017-11-20T03:06:20Z

pkg/spec/deployment.go

@@ -33,7 +33,7 @@ import (
 func (deployment *DeploymentSpecMod) Unmarshal(data []byte) error {
 	err := yaml.Unmarshal(data, &deployment)
 	if err != nil {
-		return errors.Wrap(err, "could not unmarshal into internal struct")
+		return errors.Wrap(err, "DeploymentSpecMod could not unmarshal into internal struct")


For the end user it is just Deployment!

surajssd · 2017-11-20T03:06:41Z

pkg/spec/types.go

@@ -61,7 +62,8 @@ type ServiceSpecMod struct {
 	// The list of portMappings, where each portMapping allows specifying port,
 	// targetPort and protocol in the format '<port>:<targetPort>/<protocol>'
 	// +optional
-	PortMappings []string `json:"portMappings,omitempty"`
+	// PortMappings []string `json:"portMappings,omitempty"`


remove the comment!

surajssd · 2017-11-20T03:11:57Z

pkg/yaml/types_test.go

+		input  string
+		result SliceArrayorIntArray
+	}{
+		{"{\"portMappings\":[3306]}", SliceArrayorIntArray([]string{"3306"})},


JIC: if you don't want to escape the double quotes, then you can use the back ticks to enclose the entire string like following:

kedge/pkg/spec/controller_test.go

Lines 155 to 162 in b7154f7

Data: []byte(`

name: test

containers:

- image: nginx

services:

- ports:

- port: 8080

`),

surajssd · 2017-11-20T03:23:35Z

pkg/yaml/types_test.go

+	}{
+		{"{\"portMappings\":[3306]}", SliceArrayorIntArray([]string{"3306"})},
+		{"{\"portMappings\":[\"3306\"]}", SliceArrayorIntArray([]string{"3306"})},
+		{"{\"portMappings\":[\"80:80\", \"443:443\"]}", SliceArrayorIntArray([]string{"80:80", "443:443"})},


Please add a test that has mix of string and int. something like this

portMappings: [80, "3306:3306"]

surajssd · 2017-11-20T03:40:36Z

Do we need to create a type of array type? Like it is done in this PR at https://github.com/cdrage/kedge/blob/859d75eb80269f7455d925d3877b6ff76bfc22f2/pkg/yaml/types.go#L25-L26

Can't we reuse the type defined in "k8s.io/apimachinery/pkg/util/intstr" which we already import in pkg/spec/resources.go.

kedge/vendor/k8s.io/kubernetes/staging/src/k8s.io/apimachinery/pkg/util/intstr/intstr.go

Lines 43 to 47 in b7154f7

    
           type IntOrString struct { 
        
           	Type   Type   `protobuf:"varint,1,opt,name=type,casttype=Type"` 
        
           	IntVal int32  `protobuf:"varint,2,opt,name=intVal"` 
        
           	StrVal string `protobuf:"bytes,3,opt,name=strVal"` 
        
           }

cdrage · 2017-11-20T16:46:27Z

@surajssd Unfortunately it has to be an array as @containscafeine 's implementation of portMappings iterates over an array. Simplest and easiest solution.

I'll go ahead and make updates to this PR.

cdrage · 2017-11-20T16:50:04Z

To be honest, I found this solution easier than iterating through Kubernetes code. I'll add more tests too 👍 I was able to re-create your error with using "mixed" of string and int with portMappings.

cdrage · 2017-11-20T17:21:33Z

@surajssd

Gone ahead and updated the code. I now extract to an interface{} and then iterate through the data to make sure it's either an int or string. Check out yaml/types.go

Also added another test (for a mix of string + int).

Thanks again for going through and doing a thorough test!

surajssd

Awesome, this LGTM!

concaf · 2017-11-21T09:46:21Z

pkg/yaml/types.go

@@ -0,0 +1,58 @@
+/*


A bit confused, why do we need to have a separate directory for this? Why can we not make this a part of pkg/spec/util.go? Or, if we think this is more global that pkg/spec, then maybe create a root level util directory and put this there?

because as we encounter more yaml types we can keep adding them here, I think what is done here makes sense!

Sure, then maybe a types directory with our custom types

Because it mainly uses yaml/json and it follows similar practices used in libcompose (although maybe we shouldn't use that as an example...)

concaf · 2017-11-21T09:52:09Z

pkg/yaml/types.go

+
+	// We create an array of strings by extracting to an interface and then iterating and figuring
+	// out each type. As per: https://golang.org/pkg/encoding/json/#Unmarshal
+	// we have a "case" for "string" and "float64"


why do we have a float64 here instead int?

Due to this, I think this is considered as valid input -

services: - portMappings: - 3306.42

and gets converted to -

apiVersion: v1 kind: Service metadata: creationTimestamp: null labels: app: database name: database spec: ports: - name: database-3306 port: 3306 protocol: TCP targetPort: 3306 selector: app: database type: NodePort status: loadBalancer: {}

which is not the correct behavior, we should be throwing an error in this case :(

agreed, and name says Int, float is not int ;-)

See the above comment 👍

Yes, I understand that it says float, that is the default behaviour for JSON unmarshal: https://golang.org/pkg/encoding/json/#Unmarshal

This will lead to a lot of errors. It has to be string or int, float can't be a valid value.

If filtering out the float, can't be handled here, then it has to be handled somewhere else.
But doing it in the unamshaller is the right place

I've just tried it in Kubernetes, and IntOrString from apimachinery is currently not allowing float : for: "asdf": json: cannot unmarshal number 3.3 into Go value of type int32

@kadel Okay. I will make the fix. I'll add an if statement to see if there's a decimal place added and error out.

concaf · 2017-11-21T09:54:32Z

pkg/yaml/types.go

+			case string:
+				// string, for JSON strings
+				stringArray = append(stringArray, v)
+			}


how about a default case where we return an error that some random type was passed which we don't support?

Sure, that works. Only other type would be bool or something like nil... 👍 But yeah.

kadel · 2017-11-21T10:43:52Z

pkg/yaml/types.go

+
+	// We create an array of strings by extracting to an interface and then iterating and figuring
+	// out each type. As per: https://golang.org/pkg/encoding/json/#Unmarshal
+	// we have a "case" for "string" and "float64"


agreed, and name says Int, float is not int ;-)

kadel · 2017-11-21T10:44:05Z

pkg/yaml/types.go

+)
+
+// SliceArrayorIntArray represents a slice or an integer
+type SliceArrayorIntArray []string


Could we have

type StringOrInt []string type SliceArrayorIntArray []StringOrInt func (s *StringOrInt) UnmarshalJSON(data []byte) error { ....

or something similar if possible,
so we can use it in #454 for appversion key that should have single value of IntOrString

Do you mind if we add this when we implement #454 instead since it's unrelated to this current commit? Reason being is that each "key" needs it's own Unmarshaler and thus we would have to add a custom UnmarshalJSON for each "key" used in types.go, for example:

func (s *StringOrInt) UnmarshalJSON(data []byte) error { .... func (s *SliceArrayorIntArray) UnmarshalJSON(data []byte) error { .... etc.

Or I could open a new PR for this after this PR #455 is implemented for the new type.

Another thing is that this it should be of type int instead of string. As the whole point of this is to parse string as an integer, so resulting value should be stored in integer

@kadel Actually.. the whole thing is to parse back into a string. Hence why the result is []string

Reason being is that our PortMappings implemention by @containscafeine checks for "80:80/tcp" for example within the string. Hence the reason why all int's are converting to string and not the other-way around.

The end result would be something like this: []string{"80", "80:80/tcp", "3306:3306"} etc.

Having the output as a []int64 or []int wouldn't be possible..

cdrage · 2017-11-21T15:14:33Z

@surajssd @kadel See: https://golang.org/pkg/encoding/json/#Unmarshal

When you use unmarshal, even though it's just an int, it will unmarshal to float64, hence the float and removing all decimal places.

I'll go ahead and implement the other things, but ignore the "float" section, it's the only way to get it from Unmarshal. 👍

cdrage · 2017-11-21T15:23:52Z

Ready for another round of reviews @kadel @surajssd

kadel · 2017-11-21T16:48:42Z

@cdrage I think we should use k8s.io/apimachinery/pkg/util/intstr it solves all our problems, and it is simple.
Check my this on how to use it: kadel@6575945

cdrage · 2017-11-21T17:30:39Z

Thanks for the link @kadel :) I'll try it out!

This allows the use of specifying either (for example) "80" or 80 within portMappings of the Kedge file.

cdrage · 2017-11-21T17:33:37Z

@kadel Thanks for the link. I've updated the code accordingly... it's way better than before.

Mind doing a quick-check and then I'll add some integration / unit tests?

kadel

👍 LGTM 😉
Let's get one more LGTM as @surajssd approval was before this PR was changed

concaf

+1 on the new implementation, LGTM!

cdrage force-pushed the fix-port-mappings branch from 1542b20 to 859d75e Compare November 16, 2017 17:58

surajssd added the status/work-in-progress label Nov 17, 2017

cdrage removed the status/work-in-progress label Nov 17, 2017

surajssd suggested changes Nov 20, 2017

View reviewed changes

cdrage force-pushed the fix-port-mappings branch from 859d75e to a7f8d67 Compare November 20, 2017 17:20

cdrage mentioned this pull request Nov 20, 2017

This will add description about the new field appversion #461

Merged

surajssd approved these changes Nov 21, 2017

View reviewed changes

concaf suggested changes Nov 21, 2017

View reviewed changes

kadel suggested changes Nov 21, 2017

View reviewed changes

cdrage mentioned this pull request Nov 21, 2017

Update our examples to leverage new kedge shortcuts #312

Closed

cdrage force-pushed the fix-port-mappings branch from a7f8d67 to 6875e89 Compare November 21, 2017 15:23

Allow the usage of a string or int as a value in portMappings

2a1958c

This allows the use of specifying either (for example) "80" or 80 within portMappings of the Kedge file.

cdrage force-pushed the fix-port-mappings branch from 6875e89 to 2a1958c Compare November 21, 2017 17:33

kadel approved these changes Nov 22, 2017

View reviewed changes

kadel mentioned this pull request Nov 22, 2017

This will add a new key appversion #454

Merged

concaf approved these changes Nov 22, 2017

View reviewed changes

concaf merged commit cb6fa14 into kedgeproject:master Nov 22, 2017

	Data: []byte(`
	name: test
	containers:
	- image: nginx
	services:
	- ports:
	- port: 8080
	`),

Allow the usage of a string or int as a value in portMappings #455

Allow the usage of a string or int as a value in portMappings #455

Conversation

cdrage commented Nov 15, 2017 • edited Loading

kadel commented Nov 15, 2017

cdrage commented Nov 15, 2017

kadel commented Nov 15, 2017

cdrage commented Nov 16, 2017

cdrage commented Nov 17, 2017

surajssd left a 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

surajssd commented Nov 20, 2017

cdrage commented Nov 20, 2017

cdrage commented Nov 20, 2017 • edited Loading

cdrage commented Nov 20, 2017

surajssd left a 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

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

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

cdrage commented Nov 21, 2017

cdrage commented Nov 21, 2017

kadel commented Nov 21, 2017 • edited Loading

cdrage commented Nov 21, 2017

cdrage commented Nov 21, 2017

kadel left a comment

Choose a reason for hiding this comment

concaf left a comment

Choose a reason for hiding this comment

cdrage commented Nov 15, 2017 •

edited

Loading

cdrage commented Nov 20, 2017 •

edited

Loading

kadel commented Nov 21, 2017 •

edited

Loading