contour: Fix the hostPort regression by surajssd · Pull Request #1342 · kinvolk/lokomotive

surajssd · 2021-01-22T12:09:38Z

This change was introduced in 8d44c85 - "don't expose Envoy using hostPorts". But a later commit overrode it: 6a6a3c5 - "contour: Update to v1.7.0". This commit fixes this regression.

Fixes: #1336

assets/charts/components/contour/templates/03-envoy.yaml

surajssd · 2021-01-29T15:40:02Z

Right now the way these test helper functions are written there is no easy way to write test that verify an error condition. Right now the test fails, right away because we pass test object t.

pkg/components/velero/component_test.go

pkg/components/internal/testutil/jsonpath.go

pkg/components/contour/component_test.go

surajssd · 2021-02-04T12:16:59Z

@invidian PTAL all the comments have been addressed.

pkg/components/util/helm.go

invidian

Looking better :)

assets/charts/components/contour/templates/03-envoy.yaml

pkg/k8sutil/create.go

pkg/components/internal/testutil/jsonpath.go

invidian · 2021-02-12T12:53:51Z

pkg/components/internal/testutil/jsonpath.go

+// what error to expect.
+func JSONPathExists(t *testing.T, yamlConfig string, jsonPath string, errExp string) {
+	_, err := jsonPathValue(yamlConfig, jsonPath)
+	if err != nil && errExp == "" { //nolint:gocritic


I would avoid disabling gocritic.

It is suggesting usage of a switch case instead of the if else. Given the kinda checks we have switch case is not ideal.

It also uses elses a lot, which are discouraged in Go..

I think we can just remove the else's here.

We could also use a const error here, then complexity of the whole thing is reduced to just errors.Is() call, which I think is elegant. I can share the code for that.

Removed else because they are each func is just a way to exit out of the function.

How is errors.Is() or any const we create going to help with the error coming from upstream lib? Especially when they have not declared an error var for us to consume but just an error created using fmt.Errorf.

lokomotive/vendor/k8s.io/client-go/util/jsonpath/jsonpath.go

Line 428 in 8e39195

return results, fmt.Errorf("%s is not found", node.Value)

How is errors.Is() or any const we create going to help with the error coming from upstream lib? Especially when they have not declared an error var for us to consume but just an error created using fmt.Errorf.

If we set allowMissingKeys for jsonpath, then we get no value, so we can return constant ValueNotFound error.

Though I'm not a big fan of this const error pattern, as it adds bunch of boilerplate. It would look something like:

type NotFoundError string func (e NotFoundError) Error() string { return string(e) } const ErrNotFound NotFoundError = "not found" type KeyNotFoundError struct { Key string } func (e KeyNotFoundError) Error() string { return fmt.Sprintf("%s %s", e.Key, ErrNotFound) } func (e KeyNotFoundError) Unwrap() error { return ErrNotFound } func jsonPathValue(data, key string) (string, error) { if !strings.Contains(data, key) { return "", KeyNotFoundError{Key: key} } return "", nil } func JSONPathExistsSimplified(t *testing.T, yamlConfig string, jsonPath string, errExp error) { _, err := jsonPathValue(yamlConfig, jsonPath) if !errors.Is(err, errExp) { t.Fatalf("Unexpected error: %v, expected: %v", err, errExp) } }

So I think returning nil value would probably also work fine.

invidian · 2021-02-12T12:55:29Z

pkg/components/internal/testutil/jsonpath.go


 	if len(v) == 0 || len(v[0]) == 0 {
-		t.Fatalf("No result found")
+		return reflect.Value{}, fmt.Errorf("no result found")


Rather than returning a error here, how about we change the return type to *reflect.Value and we return nil, nil.

This way, we know everything went well, including query parsing, but the value was simply not found. This will make checking more idiomatic.

It is reworked now. The functionality is not changed just returning an error to bubble up the caller stack.

But we still must use strings.Contains, which IMO should be used as a last resort tool, as it test specific implementation.

With approach I suggested, we avoid ambiguity and we work against the specification of function.

I agree *reflect.Value makes sense to distinguish errors from value not found.

The newly added test helper JSONPathExists is currently relying on the error from this function call:

v, err := jPath.FindResults(obj.Object)

So I don't understand how is the change to the pointer going to help in anything? We will still have to check for the error and match the string.

The error is not coming from this block:

if len(v) == 0 || len(v[0]) == 0 { return reflect.Value{}, fmt.Errorf("no result found") }

Also jPath.FindResults returns [][]reflect.Value which definitely not a array of pointers so we don't need to worry about deep copying it if we convert it to pointer.

@surajssd I think calling jPath.AllowMissingKeys() should help then?

What's wrong with getting an error on something that does not exist?

It is possible that user (in this case a developer) might be giving wrong jsonpath and expecting something to exist. Such cases are ignored with AllowMissingKeys or gives wrong results.

developer would expect certain value, so they must check for nil values. So either we have const error or return nil, nil, both have similar result with slightly different interface. IMO const error is cleaner from API point of view, but it brings more boilerplate for maintenance. I'm not sure which one is better, other than returning nil is simpler.

AllowMissingKeys gives us strong gaurantee on which we rely on, so not making at false. But yes returning nil instead of error, then it is up to the user to figure out if there is an empty result.

knrt10

Question

pkg/k8sutil/create.go

pkg/components/internal/testutil/jsonpath.go

invidian

I still think k8sutil: Rename YAML imports should be split and rebased.

pkg/k8sutil/create.go

pkg/components/internal/testutil/common.go

pkg/k8sutil/create.go

pkg/components/internal/testutil/jsonpath.go

invidian · 2021-02-15T16:35:37Z

pkg/components/internal/testutil/jsonpath.go

+// what error to expect.
+func JSONPathExists(t *testing.T, yamlConfig string, jsonPath string, errExp string) {
+	_, err := jsonPathValue(yamlConfig, jsonPath)
+	if err != nil && errExp == "" { //nolint:gocritic


It also uses elses a lot, which are discouraged in Go..

invidian · 2021-02-15T16:37:04Z

pkg/components/internal/testutil/jsonpath.go


 	if len(v) == 0 || len(v[0]) == 0 {
-		t.Fatalf("No result found")
+		return reflect.Value{}, fmt.Errorf("no result found")


But we still must use strings.Contains, which IMO should be used as a last resort tool, as it test specific implementation.

With approach I suggested, we avoid ambiguity and we work against the specification of function.

pkg/k8sutil/create_test.go

pkg/k8sutil/create.go

iaguis · 2021-02-17T11:25:05Z

pkg/components/internal/testutil/jsonpath.go


 	if len(v) == 0 || len(v[0]) == 0 {
-		t.Fatalf("No result found")
+		return reflect.Value{}, fmt.Errorf("no result found")


I agree *reflect.Value makes sense to distinguish errors from value not found.

pkg/components/internal/testutil/jsonpath.go

pkg/k8sutil/create.go

iaguis · 2021-02-17T12:04:24Z

pkg/components/internal/testutil/jsonpath.go

+// what error to expect.
+func JSONPathExists(t *testing.T, yamlConfig string, jsonPath string, errExp string) {
+	_, err := jsonPathValue(yamlConfig, jsonPath)
+	if err != nil && errExp == "" { //nolint:gocritic


I think we can just remove the else's here.

knrt10

NITs

pkg/k8sutil/create.go

pkg/components/internal/testutil/jsonpath.go

iaguis

A couple of nits. Looks good to me in general.

pkg/k8sutil/create.go

pkg/components/internal/testutil/common.go

Dismissing my review.

This (`k8s.io/apimachinery/pkg/util/yaml`) is renamed to `yamlutil` so that in the next commit when we import `sigs.k8s.io/yaml` it can simply be used as `yaml.`. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

This commit adds an object ObjectMetadata to identify any Kubernetes object uniquely. Adds three exported functions. YAMLToUnstructured converts any YAML object to `unstructured.Unstructured` type. SplitYAMLDocuments splits a YAML file with multiple YAML documents separated by `---` into separate YAML configs. YAMLToObjectMetadata takes in a YAML config as string and returns ObjectMetadata. Add unit tests for each of the exported functions. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

This commit removes the local function `unstructredObj` and uses the function from the `k8sutil` package. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

This commit modifies the behaviour of passing the test object of type `testing.T` to the helper functions, instead return the errors. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

Earlier the conversion tests were using unreliable file name generated by Helm to find a Kubernetes object's YAML. Now this commit changes that and the users should now provide Version, Kind and Name to find the specific object. This specially solves the problem when a single file has multiple YAML documents separated by `---`, the new functions provide reliable way of finding the objects. Change the signature of ConfigFromMap to accept `k8sutil.ObjectMetadata` instead of string indicating file name. Modify the conversion tests to provide `k8sutil.ObjectMetadata` to find specific object. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

This change was introduced in `8d44c8513f67512527218c8a538e79a61cb7cd34` - "don't expose Envoy using hostPorts". But a later commit overrode it: `6a6a3c5d849b981559c2cc2cf3797bfd6b6c9159` - "contour: Update to v1.7.0". This commit fixes this regression. Add test to verify if hostPort does not exist, check if the converted config does not contain the hostPort. This commit also adds test to check if a certain JSONPath exists, it is used in the above unit test. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

iaguis · 2021-03-03T15:18:38Z

AWS CI fail seems legit

This has been incorporated.

invidian suggested changes Jan 22, 2021

View reviewed changes

assets/charts/components/contour/templates/03-envoy.yaml Show resolved Hide resolved

surajssd force-pushed the surajssd/fix-contour-envoy-regression branch from 14b33db to 968745a Compare January 29, 2021 15:38

surajssd requested a review from invidian January 29, 2021 15:38

surajssd force-pushed the surajssd/fix-contour-envoy-regression branch from 968745a to 35fd587 Compare January 29, 2021 15:48

invidian suggested changes Jan 29, 2021

View reviewed changes

surajssd force-pushed the surajssd/fix-contour-envoy-regression branch from 35fd587 to 1e5d6e3 Compare February 1, 2021 08:31

surajssd requested a review from invidian February 2, 2021 07:35

surajssd force-pushed the surajssd/fix-contour-envoy-regression branch from 9f06d73 to d7e4f66 Compare February 4, 2021 07:06

invidian reviewed Feb 8, 2021

View reviewed changes

pkg/components/util/helm.go Outdated Show resolved Hide resolved

surajssd force-pushed the surajssd/fix-contour-envoy-regression branch from d7e4f66 to 56f55b1 Compare February 12, 2021 12:33

surajssd requested review from ipochi and knrt10 February 12, 2021 12:33

surajssd mentioned this pull request Feb 12, 2021

rook-ceph: Add reclaim_policy #1369

Merged

invidian suggested changes Feb 12, 2021

View reviewed changes

knrt10 reviewed Feb 12, 2021

View reviewed changes

pkg/k8sutil/create.go Outdated Show resolved Hide resolved

ipochi reviewed Feb 12, 2021

View reviewed changes

pkg/components/internal/testutil/jsonpath.go Show resolved Hide resolved

surajssd force-pushed the surajssd/fix-contour-envoy-regression branch from 56f55b1 to e7e25d3 Compare February 15, 2021 09:15

surajssd requested a review from invidian February 15, 2021 13:25

invidian previously requested changes Feb 15, 2021

View reviewed changes

iaguis suggested changes Feb 17, 2021

View reviewed changes

surajssd force-pushed the surajssd/fix-contour-envoy-regression branch 3 times, most recently from 77e998e to 01ea884 Compare February 24, 2021 12:17

surajssd added this to the v0.7.0 milestone Feb 24, 2021

surajssd force-pushed the surajssd/fix-contour-envoy-regression branch from 01ea884 to 0317077 Compare February 25, 2021 08:06

knrt10 previously requested changes Feb 25, 2021

View reviewed changes

pkg/k8sutil/create.go Outdated Show resolved Hide resolved

pkg/components/internal/testutil/jsonpath.go Show resolved Hide resolved

surajssd force-pushed the surajssd/fix-contour-envoy-regression branch 2 times, most recently from 968085c to 9c206e1 Compare February 26, 2021 10:31

iaguis previously approved these changes Mar 3, 2021

View reviewed changes

pkg/k8sutil/create.go Outdated Show resolved Hide resolved

pkg/k8sutil/create.go Outdated Show resolved Hide resolved

pkg/components/internal/testutil/common.go Show resolved Hide resolved

surajssd added 6 commits March 3, 2021 20:07

k8sutil: Rename apimachinery yaml to yamlutil

b7d81dd

This (`k8s.io/apimachinery/pkg/util/yaml`) is renamed to `yamlutil` so that in the next commit when we import `sigs.k8s.io/yaml` it can simply be used as `yaml.`. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

jsonpath-testutil: Use k8sutil.YAMLToUnstructured

2344214

This commit removes the local function `unstructredObj` and uses the function from the `k8sutil` package. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

testutil: Modify to return errors

af7aa09

This commit modifies the behaviour of passing the test object of type `testing.T` to the helper functions, instead return the errors. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

surajssd dismissed iaguis’s stale review via ce837ac March 3, 2021 14:38

surajssd force-pushed the surajssd/fix-contour-envoy-regression branch from 9c206e1 to ce837ac Compare March 3, 2021 14:38

iaguis approved these changes Mar 3, 2021

View reviewed changes

surajssd merged commit 2482601 into master Mar 3, 2021

surajssd deleted the surajssd/fix-contour-envoy-regression branch March 3, 2021 16:04

Conversation

surajssd commented Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

surajssd commented Jan 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

surajssd commented Feb 4, 2021

Uh oh!

Uh oh!

invidian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knrt10 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

invidian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

surajssd commented Jan 22, 2021 •

edited

Loading