-
Notifications
You must be signed in to change notification settings - Fork 631
adding appProtocol to all services #3436
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! I've been curious about this API field since pgBackRest registered with the IANA at the beginning of this year.
❓ The Istio docs for protocol selection seem to say that Istio will treat protocols it can't detect as TCP. What happens in Istio when all these Services are explicitly set to tcp?
I have a few more questions inline.
@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
I also updated the Endpoints, such that they'd copy over the appProtocol from the Service as well
@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?
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@szelenka Thank you for your patience! I have one nitpick on a doc comment.
GitHub didn't run any checks on your last push (not your fault). Do you mind squashing these commits, which will kick them off again?
85f78a6 to
07488c7
Compare
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.
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
@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.
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?
Checklist:
Type of Changes:
What is the current behavior (link to any open issues here)?
When the operator generates a new service, it will add the
appProtocolfield on that service, to allow it to be used by services which look forappProtocolWhat is the new behavior (if this is a feature change)?
Other Information:
Issue: #3435