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.

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

DeepCopy unstructured HTTPProxy causes panic #2123

Closed
awprice opened this issue Jan 16, 2020 · 2 comments · Fixed by #2133
Closed

DeepCopy unstructured HTTPProxy causes panic #2123

awprice opened this issue Jan 16, 2020 · 2 comments · Fixed by #2133

Comments

@awprice
Copy link
Contributor

awprice commented Jan 16, 2020

What steps did you take and what happened:
When converting an HTTPProxy object to an unstructured object and then performing a DeepCopy on that unstructured object, a panic is experienced.

The following code can be used to replicate:

package main

import (
	v1 "github.com/projectcontour/contour/apis/projectcontour/v1"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
	"k8s.io/apimachinery/pkg/runtime"
)

func main() {
	proxy := &v1.HTTPProxy{
		TypeMeta: metav1.TypeMeta{
			Kind:       "HTTPProxy",
			APIVersion: v1.SchemeGroupVersion.String(),
		},
		Spec: v1.HTTPProxySpec{
			Routes: []v1.Route{
				{
					Services: []v1.Service{
						{
							Weight: uint32(100),
						},
					},
				},
			},
		},
	}

	u, err := runtime.DefaultUnstructuredConverter.ToUnstructured(proxy.DeepCopyObject())
	if err != nil {
		panic(err)
	}

	unstr := &unstructured.Unstructured{Object:u}
	unstr.DeepCopy()
}

When run, results in:

panic: cannot deep copy uint64

What did you expect to happen:

For the above code to run without issue.

Anything else you would like to add:

I believe this is occurring due to the use of uint32 for the weight (and other fields) in the HTTPProxy struct. I've manually changed the weight field in the Service struct from uint32 to a signed integer and the code above runs fine.

Works:

	// Weight defines percentage of traffic to balance traffic
	// +optional
	Weight int32 `json:"weight,omitempty"`

Looking at the Kubernetes external API codebase, no unsigned integers are used in the public structs - https://github.com/kubernetes/api, most likely for this reason.

Environment:

  • Contour version: v1.1.0
  • Kubernetes version: (use kubectl version): v1.15.7
  • Kubernetes installer & version: N/A
  • Cloud provider or hardware configuration: N/A
  • OS (e.g. from /etc/os-release): N/A
@davecheney
Copy link
Contributor

Thanks for raising this issue. I think we can change some of these uints to ints pretty painlessly but there might be some that have a direct correlation with the value that envoy consumes so some care might need to be taken there.

The thing about using unsigned ints in general is the json schema makes harder for people to give negative values for weights.

@awprice
Copy link
Contributor Author

awprice commented Jan 16, 2020

Perfectly understand the reasoning behind the use of the uints. Is it possible to use validation rules in the schema to enforce that negative values aren't used? i.e. minimum: 0

awprice added a commit to awprice/contour that referenced this issue Jan 21, 2020
…dd validation

This commit replaces the unsigned integers in the HTTPProxy and IngressRoute CRDs
with signed integers. Unsigned integers are discouraged due to API compatibility
issues. Validation rules have been added to the fields to ensure that the value
provided is always positive.

Fixes projectcontour#2123

Signed-off-by: Alex Price <aprice@atlassian.com>
awprice added a commit to awprice/contour that referenced this issue Feb 2, 2020
…dd validation

This commit replaces the unsigned integers in the HTTPProxy and IngressRoute CRDs
with signed integers. Unsigned integers are discouraged due to API compatibility
issues. Validation rules have been added to the fields to ensure that the value
provided is always positive.

Fixes projectcontour#2123

Signed-off-by: Alex Price <aprice@atlassian.com>
youngnick pushed a commit that referenced this issue Feb 2, 2020
…dd validation

This commit replaces the unsigned integers in the HTTPProxy and IngressRoute CRDs
with signed integers. Unsigned integers are discouraged due to API compatibility
issues. Validation rules have been added to the fields to ensure that the value
provided is always positive.

Fixes #2123

Signed-off-by: Alex Price <aprice@atlassian.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants