Added support for choosing bind address #52

mvazquezc · 2020-07-07T15:58:21Z

As of today, the operator will always use "0.0.0.0" as the binding address for the different services. When running on IPv6 environments that cause the services to fail since there are not IPv4 addresses available in the pod. With this change, the user will be able to choose which address is used when binding.

By default, if no value is provided for the address property in the spec, we will configure "0.0.0.0", otherwise, we will configure the address configured by the user.

Let me know what you think about the implementation, if it looks good to you I'll update the docs accordingly.

openshift-ci-robot · 2020-07-07T15:58:36Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

pkg/apis/cincinnati/v1beta1/cincinnati_types.go

djzager

A couple of thoughts.

djzager · 2020-07-07T16:52:28Z

pkg/apis/cincinnati/v1beta1/cincinnati_types.go

@@ -25,6 +25,13 @@ type CincinnatiSpec struct {
 	// GraphDataImage is a container image that contains the Cincinnati graph
 	// data. The data is copied to /var/lib/cincinnati/graph-data.
 	GraphDataImage string `json:"graphDataImage"`
+
+	// IPv6 defines whether the services bind to "0.0.0.0" or to "::"
+	IPv6 bool `json:"ipv6,omitempty"`


https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#primitive-types

Think twice about bool fields. Many ideas start as boolean but eventually trend towards a small set of mutually exclusive options. Plan for future expansions by describing the policy options explicitly as a string type alias (e.g. TerminationMessagePolicy).

My gut feeling is that adding the Address to the spec is sufficient. That way we can default to '0.0.0.0' or force it to be defined.

The first implementation was done in that way, then I changed it based on Ryan's suggestions, anyway I'd say a boolean makes sense, a user won't likely know which address is going to get the pod when created, and between defining a property with the bind-address or just setting a boolean to decide if the services listen on IPv6 or IPv4 I'd rather go with the second option, but again, I'm open to suggestions.

I don't understand.

a user won't likely know which address is going to get the pod when created

Then why would we bother taking the Address on the spec?

I mean, if you add the Address to the spec so users can choose which address the services bind to, they will likely end using "0.0.0.0" or "::", the IP address that the SDN will assign to the pod is not something you can know beforehand easily. That's why I thought a boolean makes more sense.

Binding to "::" works on IPv4 and IPv6 clusters. I reverted the changes from the API types.

djzager · 2020-07-07T16:55:01Z

pkg/controller/cincinnati/new.go

@@ -225,18 +225,24 @@ func (k *kubeResources) newPolicyEngineRoute(instance *cv1beta1.Cincinnati) *rou
 }

 func (k *kubeResources) newEnvConfig(instance *cv1beta1.Cincinnati) *corev1.ConfigMap {
+	// If IPv6 boolean property is not configured in the Spec


I think the right thing to do, assuming we aren't going to write back these defualts to the api-server, is to put this logic in newKubeResources. However, I don't see handling of the scenario where someone defines the address to listen on.

I added Address to the spec because the spec is used for rendering the template for one of the ConfigMaps, I didn't know if there is a better way to do that, but in this case, we only use that field for rendering the CM, I don't see any value on persisting its value in the api-server. Maybe there is a better solution?

Binding to "::" works on IPv4 and IPv6 clusters. I reverted the changes from the API types.

mhrivnak · 2020-07-07T16:56:13Z

Is there ever a circumstance where we wouldn't want it to bind to both if available? Seems like that would be a sensible default. Is there a way to make cincinnati itself do that?

mvazquezc · 2020-07-07T17:06:05Z

Is there ever a circumstance where we wouldn't want it to bind to both if available? Seems like that would be a sensible default. Is there a way to make cincinnati itself do that?

When you say both, do you mean IPv4 and IPv6?

Dual stack is not supported yet, but in case you could have a pod with an IPv4 and an IPv6 address binding to one of the two will be enough for Cincinnati to work based on what I've seen.

In this case the tricky thing will be knowing if the cluster has dual-stack support.

mhrivnak · 2020-07-07T17:24:29Z

Yes, I mean both ipv4 and ipv6. As an example, both nginx and httpd allow you to either specify just a port or *:<port> to listen on, and then they will listen on all addresses whether ipv4 or ipv6. Can we just enable Cincinnati to do that?

It seems a shame to add options to the operator's API if there's no user story other than "As a user, I want cincinnati to listen on all addresses." If we need to do something short-term in the operator to make this work on ipv6 while waiting for cincinnati to support dual-stack, maybe we can come up with something that doesn't involve adding to the API.

rthallisey · 2020-07-07T20:16:30Z

Yes, I mean both ipv4 and ipv6. As an example, both nginx and httpd allow you to either specify just a port or *: to listen on, and then they will listen on all addresses whether ipv4 or ipv6. Can we just enable Cincinnati to do that?

I agree. Cincy is the only thing that's listening in this address space, so it would just make sense to support everything without exposing it.

I think we would need to change the default setting in both the operator and in Cincinnati. Setting the default to :: here should cover it on this end. I'll file a ticket on cincy as well.

…4 environments

mvazquezc · 2020-07-09T10:45:08Z

Hey, as Ryan suggested. Binding to "::" works for IPv6 and IPv4 environments. I just reverted the changes on the API Types and edited the configmaps created by the controller.

openshift-ci-robot · 2020-07-13T13:41:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mvazquezc, rthallisey

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rthallisey]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mhrivnak · 2020-07-13T13:53:49Z

I'm surprised that :: includes ipv4 addresses, since it is ipv6 notation. But if it works, it works. It would be a good idea to test on a single-stack ipv4 system to be sure, but generally I think this is a lot better than adding API options.

rthallisey · 2020-07-13T15:11:10Z

I was able to see it work on my local ipv4 env.

This was where I got the idea from originally: nodejs/node#9390. You can disable the duel-stack listening by configuring net.ipv6.bindv6only.

Without knowing more about what admins do with net.ipv6.bindv6only, it's hard for me to say if we'll run into an issue or not. I think for now, we can say this works as is, but I'll link this PR in the OTA duel stack jira card so it's on their radar when they do a more in depth analysis of this feature for 4.6.

stable-4.3: add 4.3.1

Added support for choosing bind address

7e49310

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 7, 2020

openshift-ci-robot requested review from djzager and rthallisey July 7, 2020 15:58

rthallisey suggested changes Jul 7, 2020

View reviewed changes

pkg/apis/cincinnati/v1beta1/cincinnati_types.go Outdated Show resolved Hide resolved

pkg/apis/cincinnati/v1beta1/cincinnati_types.go Outdated Show resolved Hide resolved

openshift-ci-robot assigned rthallisey Jul 7, 2020

updated to use a bool, instead of address

1b82500

djzager suggested changes Jul 7, 2020

View reviewed changes

openshift-ci-robot assigned djzager Jul 7, 2020

updated the operator to always bind to :: which works on IPv6 and IPv…

03c6f55

…4 environments

mvazquezc marked this pull request as ready for review July 9, 2020 10:46

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2020

rthallisey approved these changes Jul 13, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 13, 2020

openshift-merge-robot merged commit 0823dba into openshift:master Jul 13, 2020

PratikMahajan pushed a commit to PratikMahajan/cincinnati-operator that referenced this pull request Mar 17, 2021

Merge pull request openshift#52 from thiagoalessio/stable-4.3

fef06ad

stable-4.3: add 4.3.1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for choosing bind address #52

Added support for choosing bind address #52

mvazquezc commented Jul 7, 2020

openshift-ci-robot commented Jul 7, 2020

djzager left a comment

djzager Jul 7, 2020

mvazquezc Jul 7, 2020

djzager Jul 7, 2020

mvazquezc Jul 8, 2020

mvazquezc Jul 9, 2020

djzager Jul 7, 2020

mvazquezc Jul 7, 2020

mvazquezc Jul 9, 2020

mhrivnak commented Jul 7, 2020

mvazquezc commented Jul 7, 2020

mhrivnak commented Jul 7, 2020

rthallisey commented Jul 7, 2020

mvazquezc commented Jul 9, 2020

openshift-ci-robot commented Jul 13, 2020

mhrivnak commented Jul 13, 2020

rthallisey commented Jul 13, 2020

Added support for choosing bind address #52

Added support for choosing bind address #52

Conversation

mvazquezc commented Jul 7, 2020

openshift-ci-robot commented Jul 7, 2020

djzager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhrivnak commented Jul 7, 2020

mvazquezc commented Jul 7, 2020

mhrivnak commented Jul 7, 2020

rthallisey commented Jul 7, 2020

mvazquezc commented Jul 9, 2020

openshift-ci-robot commented Jul 13, 2020

mhrivnak commented Jul 13, 2020

rthallisey commented Jul 13, 2020