docs: update README #155

ironcladlou · 2019-03-05T22:12:04Z

Add more detail to the README.

Depends on openshift/api#219.
Will depend on the followup to openshift/api#219 which integrates with the new API.

Miciah · 2019-03-05T22:17:55Z

README.md


-The operator tries to be useful out of the box by creating a working default deployment based on the cluster's configuration.
+#### Ingress Operator enables Routes and Kubernetes Ingress on OpenShift.


This heading seems superfluous and looks kind of strange (it renders in bold-face, so it looks like an emphatic statement rather than a heading).

Intention is to have a one-sentence tagline/"what is this thing"... recommendations?

"Ingress Operator enables external access to cluster services by configuring Ingress Controllers, which route traffic as specified by OpenShift Route and Kubernetes Ingress resources."

I like the tagline from @Miciah, with the addition of an OpenShift link. I also prefer removing the bold-face.

Updated... better?

ironcladlou · 2019-03-05T22:22:26Z

cc @rjhowe — would love your feedback on whether this is starting to look more useful. Note it references in-flight API changes/docs: openshift/api#219

ironcladlou · 2019-03-05T22:23:18Z

cc @ariordan-redhat

Miciah · 2019-03-05T22:23:41Z

README.md

+#### HostNetwork
+
+The [HostNetwork]() strategy uses host networking to publish the ingress
+controller on node ports where the ingress controller is deployed.


The reader could interpret "on node ports" to mean "using a nodeport service". How about "directly on the node host where the ingress controller is deployed"?

Agree... although we might be able to hold off on this strategy entirely for 4.0, need to discuss tomorrow.

Miciah · 2019-03-05T22:24:58Z

README.md

+
+![Image of Private](docs/images/endpoint-publishing-private.png)
+
+## Troubleshoot


"Troubleshooting"

Fixed throughout

Miciah · 2019-03-05T22:25:48Z

README.md

+$ oc describe --namespace=openshift-ingress-operator ingresscontroller/<name>
+```
+
+## Contribute


"Contributing"

Miciah · 2019-03-05T22:27:16Z

README.md

+
+```shell
+$ oc describe --namespace=openshift-ingress-operator ingresscontroller/<name>
+```


How about logs for the ingress controller (i.e., router)?

$ oc logs --namespace=openshift-ingress deployments/router-<name>

(Are we going to rename "router" to "ingress-controller"?)

Waffled on this one... is exposing a potentially volatile naming convention problematic? Is there some other way we could direct users to the logs via the ingresscontroller? Random ideas:

Implement oc logs for an ingresscontroller to abstract discovery of the deployment

Report the deployment name on status

Document a stable selector for the deployment to pass to oc logs --selector

Miciah · 2019-03-05T22:55:43Z

README.md


-## How to help
+To provide this functionality, Ingress Operator deploys and manages the
+[OpenShift router](https://github.com/openshift/router) — a


The name of the operator repo is "cluster-ingress-operator". The title of the doc is "OpenShift Ingress Operator". ClusterIngress has now transitioned to IngressController. OpenShift includes CR's for cluster/ingresses.config.openshift.io and supports the Kubernetes Ingress resource. I can see how a user would get confused. Should the name of the repo and readme title be more closely align to IngressController? For example, repo name "ingress-controller-operator" and title "OpenShift Ingress Controller Operator"?

Changing the name of the repository is a more onerous endeavor and goes beyond the scope of this PR IMO. Otherwise, the naming makes sense to me: the ingress operator manages the ingress controller, which works with the ingress (sort of) and (really) route resources.

Ingress Controller Operator vs. Ingress Operator sounds like a good list discussion.

Changing the labels, annotations, and resource names would have a few considerations, mostly self-contained to this project, although we have to be careful about resource names (which I believe are currently being used as an informal contract in some cases and which could potentially be replaced by stable label selectors).

Changing the repo name would have enough ripple effects to warrant its own design document (this isn't meant to be snarky — just a statement of fact for consideration).

danehans

The readme update is coming along nicely. I have a few questions/comments inline.

danehans · 2019-03-06T13:43:07Z

README.md


-The operator tries to be useful out of the box by creating a working default deployment based on the cluster's configuration.
+#### Ingress Operator enables Routes and Kubernetes Ingress on OpenShift.


I like the tagline from @Miciah, with the addition of an OpenShift link. I also prefer removing the bold-face.

danehans · 2019-03-06T13:54:56Z

README.md

+## Manage
+
+Ingress Operator provides the OpenShift [ingresscontroller API](). Create and
+edit `ingresscontroller.operator.openshift.io` resources to manage ingress


Ingress Operator provides the OpenShift ingresscontroller API

Can you help me better understand this? I thought the api type was moving to openshift/api and the operator is performing a list & watch on ingresscontroller (formerly clusteringress)?

Agreed, it would be clearer to say, "Ingress Operator implements the OpenShift ingresscontroller API."

Updated, PTAL

danehans · 2019-03-06T13:57:45Z

README.md

+controllers.
+
+Interact with ingress controllers using the `oc` command. Every ingress
+controller lives in the `openshift-ingress-operator` namespace.


Every ingress controller lives in the openshift-ingress-operator namespace.

should we say, by default?

For now, we do not handle a ingresscontroller it if it is not in openshift-ingress-operator, so I think this statement is fine for now... although there is some ambiguity between the ingresscontroller resource and the ingress controller (née router) itself.

danehans · 2019-03-06T14:02:17Z

README.md

+
+#### Private
+
+The [Private]() strategy does not publish the ingress controller.


What would be a use case for private? I've never heard of an ingress controller not being exposed outside a cluster.

On some clusters, the administrator wants to run an ingress controller that serves only to pods inside the cluster.

Or as a way to manage it yourself through some unmanaged solution (e.g. wrap it with a NodePort)

danehans · 2019-03-06T14:10:52Z

README.md

+
+The [Private]() strategy does not publish the ingress controller.
+
+![Image of Private](docs/images/endpoint-publishing-private.png)


I do not see any service in docs/images/endpoint-publishing-private.png. I thought a service of type ClusterIP is still created for internal traffic?

There is the "internal" service, which we create irrespective of the endpoint publishing strategy; might be worth adding to the diagrams.

Do admins care about the internal service? Is the internal service useful for anything but communications between the ingress controller and other OpenShift components (e.g. prometheus)? Would the internal service be a candidate for a separate internal architecture design?

The internal service is the only predictable endpoint by which pods can connect to a private ingresscontroller, without additional configuration on the part of the cluster administrator.

Any pods on the cluster, infrastructure or end-user, that need to connect to any routes that are served by the private ingresscontroller.

It may be that we want to recommend a different approach (other than using the internal service that the operator creates) for this use-case.

Is it fair to characterize the internal service as an implementation detail at this time?

Miciah · 2019-03-11T22:47:28Z

README.md

+To scale an ingress controller:
+
+```shell
+$ oc scale --replicas=1 --namespace=openshift-ingress-operator ingresscontroller/<name>


Link to kubernetes/kubernetes#75210?

Now we're showing two ways and making a note about oc scale

Miciah · 2019-03-11T22:49:01Z

README.md

+
+### Endpoint publishing
+
+The `.spec.endpointPublishingStrategy` field is used to publish the ingress


I'm not sure what "publish the ingress controller endpoints to other networks" means; is that referring to DNS?

Basically lifted from our new public API docs; do you feel the API docs are similarly confusing? What would you propose here as an alternative, and would you suggest we upstream a change as well?

Miciah · 2019-03-11T22:51:40Z

pkg/operator/controller/controller.go

+
+	// Get all ingresscontrollers
+	// if one already exists with same ci.spec.domain, set status, log event, etc.
+	// and skip reconciliation


🤦‍♂️

Miciah · 2019-03-11T22:52:14Z

What do you think of reversing the order in which the publishing strategies are described, in order to have a progression from simplest to most complex?

ironcladlou · 2019-03-11T23:30:45Z

What do you think of reversing the order in which the publishing strategies are described, in order to have a progression from simplest to most complex?

The current way is what I would characterize as "likely the default/most features first" to "things you only see if you customize last".

Do you see the strategies as a progressive series of layered concepts? It's not clear to me that they're layered so much as just "different"

Miciah · 2019-03-11T23:40:05Z

Do you see the strategies as a progressive series of layered concepts? It's not clear to me that they're layered so much as just "different"

I do at a conceptual level, although there are discrepancies in the implementation (host networking versus node ports) and presentation (showing multiple nodes versus leaving it implicit) that diminish this viewpoint. I don't feel strongly if you want to leave this as it is.

Miciah · 2019-03-11T23:44:37Z

/lgtm

openshift-ci-robot · 2019-03-11T23:44:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou, Miciah

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~~ [Miciah,ironcladlou]

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

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 Mar 5, 2019

openshift-ci-robot requested review from knobunc and pravisankar March 5, 2019 22:12

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 5, 2019

Miciah reviewed Mar 5, 2019

View reviewed changes

danehans reviewed Mar 6, 2019

View reviewed changes

ironcladlou force-pushed the docs branch from a5c6820 to 4a084c5 Compare March 11, 2019 22:31

Miciah reviewed Mar 11, 2019

View reviewed changes

docs/readme: Add more detail to the README

bf1bec7

ironcladlou force-pushed the docs branch from 59c0874 to bf1bec7 Compare March 11, 2019 23:20

ironcladlou changed the title ~~WIP: docs: update README~~ docs: update README Mar 11, 2019

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 Mar 11, 2019

openshift-ci-robot assigned Miciah Mar 11, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2019

openshift-merge-robot merged commit aaed79a into openshift:master Mar 12, 2019


		The operator tries to be useful out of the box by creating a working default deployment based on the cluster's configuration.
		#### Ingress Operator enables Routes and Kubernetes Ingress on OpenShift.


		![Image of Private](docs/images/endpoint-publishing-private.png)

		## Troubleshoot


		#### Private

		The [Private]() strategy does not publish the ingress controller.


		### Endpoint publishing

		The `.spec.endpointPublishingStrategy` field is used to publish the ingress

docs: update README #155

docs: update README #155

Conversation

ironcladlou commented Mar 5, 2019

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

ironcladlou commented Mar 5, 2019

ironcladlou commented Mar 5, 2019

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

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

danehans 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

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

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

Miciah commented Mar 11, 2019 • edited Loading

ironcladlou commented Mar 11, 2019

Miciah commented Mar 11, 2019

Miciah commented Mar 11, 2019

openshift-ci-robot commented Mar 11, 2019

Miciah commented Mar 11, 2019 •

edited

Loading