contour: Fix the hostPort regression

[surajssd]

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

[surajssd]

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.

[surajssd]

@invidian PTAL all the comments have been addressed.

[invidian]

Looking better :)

[invidian]

I would avoid disabling gocritic.

[surajssd]

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

[invidian]

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

[iaguis]

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

[invidian]

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.

[surajssd]

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

[surajssd]

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)

[invidian]

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]

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.

[surajssd]

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

[invidian]

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.

[iaguis]

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

[surajssd]

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.

[invidian]

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

[surajssd]

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.

[invidian]

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.

[surajssd]

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

[invidian]

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

[invidian]

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

[invidian]

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.

[iaguis]

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

[iaguis]

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

[knrt10]

NITs

[iaguis]

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

Dismissing my review.

[iaguis]

AWS CI fail seems legit

This has been incorporated.