postgres-operator icon indicating copy to clipboard operation
postgres-operator copied to clipboard

adding appProtocol to all services

Open szelenka opened this issue 3 years ago • 8 comments

Checklist:

  • [x] Have you added an explanation of what your changes do and why you'd like them to be included?
  • [x] Have you updated or added documentation for the change, as applicable?
  • [x] Have you tested your changes on all related environments with successful results, as applicable?
  • [x] Have you added automated tests?

Type of Changes:

  • [ ] New feature
  • [x] Bug fix
  • [ ] Documentation
  • [ ] Testing enhancement
  • [ ] Other

What is the current behavior (link to any open issues here)?

When the operator generates a new service, it will add the appProtocol field on that service, to allow it to be used by services which look for appProtocol

What is the new behavior (if this is a feature change)?

  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

Other Information:

Issue: #3435

szelenka avatar Oct 25 '22 20:10 szelenka

@cbandy when it's set explicitly to tcp nothing much happens TBH. It'll still default to plain TCP.

In the past this was the default we used to enable some of the features in Istio, and some tooling explicitly looks for appProtocol; such as Kiali.

Now that I know there's a IANA for postgres, it makes sense to use that instead. I'll work on updating them

szelenka avatar Oct 26 '22 14:10 szelenka

I also updated the Endpoints, such that they'd copy over the appProtocol from the Service as well

szelenka avatar Oct 26 '22 15:10 szelenka

@cbandy this is my first time contributing to this repo. Not sure what the next steps are after resolving the conversations, can you help guide me?

szelenka avatar Oct 27 '22 17:10 szelenka

I've kicked off some PR checks. I expect an unrelated failure from kubernetes-k3d (latest) but the rest need to pass before we merge.

cbandy avatar Oct 27 '22 21:10 cbandy

I ran into trouble testing this on OCP 4.8. API calls succeed but the appProtocol field isn't saved in the API. The update event(s) trigger another reconcile which writes again which causes more update event(s) causing more reconciles ad nauseam.

cbandy avatar Nov 16 '22 23:11 cbandy

I don't have access to an OCP cluster to debug on.

@cbandy is it just version 4.8, or does it happen on the later builds as well?

This suggests OCP may have just gotten around to "GA" appProtocol in version 4.8.52? https://mirror.openshift.com/pub/openshift-v4/clients/ocp/stable-4.8/changelog.html

szelenka avatar Nov 23 '22 03:11 szelenka

@cbandy is it just version 4.8, or does it happen on the later builds as well?

I only tried on that one version. Looking at the dates, I probably tested on 4.8.51. The cluster updated to 4.8.52 two days after my last comment.

This suggests OCP may have just gotten around to "GA" appProtocol in version 4.8.52? https://mirror.openshift.com/pub/openshift-v4/clients/ocp/stable-4.8/changelog.html

That lists all the changes since 4.7.0, so I think you're seeing the fact that OCP 4.8 is based on K8s 1.21. A more granular list is here: https://docs.openshift.com/container-platform/4.8/release_notes/ocp-4-8-release-notes.html#ocp-4-8-asynchronous-errata-updates

I don't have access to an OCP cluster to debug on.

This is the cluster version today. Maybe you can see something similar on another distro?

Server Version: 4.8.52
Kubernetes Version: v1.21.11+5cc9227

The API field was present/available when I tested. I was able to set it using kubectl edit. Maybe something with SSA?

Now that I think about it... the symptoms could also be from an old controller running. Maybe I had a dirty environment.

cbandy avatar Nov 23 '22 17:11 cbandy

I'm not having luck with getting an OpenShift cluster setup.

I've gone through these steps to create a cluster on my Mac with a M1 chip: https://console.redhat.com/openshift/create/local

But when attempting to run the PGO, it'll assert with:

exec /usr/local/bin/postgres-operator: exec format error

Rather than debugging OpenShift, would it be better to just use the isOpenshift method in the PGO to prevent adding the appProtocol field in OpenShift clusters?

szelenka avatar Dec 23 '22 18:12 szelenka