Skip to content

Conversation

@Amrita42
Copy link
Contributor

@Amrita42 Amrita42 commented Mar 22, 2022

@sherine-k please review @quarterpin ptal

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 22, 2022
@netlify
Copy link

netlify bot commented Mar 22, 2022

Deploy Preview for osdocs ready!

Name Link
🔨 Latest commit a9854dd
🔍 Latest deploy log https://app.netlify.com/sites/osdocs/deploys/624ad48c2c1ce00008c588b1
😎 Deploy Preview https://deploy-preview-43576--osdocs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@Amrita42 Amrita42 changed the title [WIP] CFE-10: binding options CFE-10: binding options Mar 29, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 29, 2022
@Amrita42
Copy link
Contributor Author

@sherine-k ptal

@sherine-k
Copy link

Great work @Amrita42 . this is looking quite good now

@Amrita42 Amrita42 changed the title CFE-10: binding options [WIP]:CFE-10: binding options Mar 31, 2022
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 31, 2022
@Amrita42 Amrita42 changed the title [WIP]:CFE-10: binding options CFE-10: binding options Apr 4, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 4, 2022
@Amrita42 Amrita42 changed the title CFE-10: binding options CFE-374: binding options Apr 4, 2022
Copy link
Contributor

@nalhadef nalhadef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are different presentations of IngressController (ingress controller, Ingress Controller) all over these files. If possible, be consistent.

----
====
For most platforms, the `endpointPublishingStrategy` value can be updated. On GCP, you can configure the following `endpointPublishingStrategy` fields:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spell out first use of GCP.

https://kubernetes.io/docs/concepts/services-networking/ingress-controllers
IngressController describes a managed ingress controller for the cluster. The controller can service OpenShift Route and Kubernetes Ingress resources.
When an IngressController is created, a new ingress controller deployment is created to allow external traffic to reach the services that expose Ingress or Route resources. Updating this resource may lead to disruption for public facing network connections as a new ingress controller revision may be rolled out.
https://kubernetes.io/docs/concepts/services-networking/ingress-controllers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the preview, the line spacing is not happy.

It could use a bit of human text and maybe a lead-in (See XYZ for information about...) to give the link some context.

tls.crt: certificate file contents tls.key: key file contents
If unset, a wildcard certificate is automatically generated and used. The certificate is valid for the ingress controller domain (and subdomains) and the generated certificate's CA will be automatically integrated with the cluster's trust store.
If a wildcard certificate is used and shared by multiple HTTP/2 enabled routes (which implies ALPN) then clients (i.e., notably browsers) are at liberty to reuse open connections. This means a client can reuse a connection to another route and that is likely to fail. This behaviour is generally known as connection coalescing.
| defaultCertificate is a reference to a secret containing the default certificate served by the ingress controller. When Routes don't specify their own certificate, defaultCertificate is used.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should "ingress controller" have initial caps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both first letters should be capitalized as per the Glossary https://github.com/openshift/openshift-docs/blob/main/contributing_to_docs/term_glossary.adoc#ingress-controller. I had raised a PR to such effect #44002 (comment). Once I do a rebase on this PR , the changes should ideally reflect.

If a wildcard certificate is used and shared by multiple HTTP/2 enabled routes (which implies ALPN) then clients (i.e., notably browsers) are at liberty to reuse open connections. This means a client can reuse a connection to another route and that is likely to fail. This behaviour is generally known as connection coalescing.
| defaultCertificate is a reference to a secret containing the default certificate served by the ingress controller. When Routes don't specify their own certificate, defaultCertificate is used.
The secret must contain the following keys and data:
tls.crt: certificate file contents tls.key: key file contents
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make these two items a list.

| defaultCertificate is a reference to a secret containing the default certificate served by the ingress controller. When Routes don't specify their own certificate, defaultCertificate is used.
The secret must contain the following keys and data:
tls.crt: certificate file contents tls.key: key file contents
If unset, a wildcard certificate is automatically generated and used. The certificate is valid for the ingress controller domain (and subdomains) and the generated certificate's CA will be automatically integrated with the cluster's trust store.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about "ingress controller."

Comment on lines +1628 to +1630
Valid values are: * "Global": Specifying an internal load balancer with Global client access allows clients from any region within the VPC to communicate with the load balancer.
https://cloud.google.com/kubernetes-engine/docs/how-to/internal-load-balancing#global_access
* "Local": Specifying an internal load balancer with Local client access means only clients within the same region (and VPC) as the GCP load balancer can communicate with the load balancer. Note that this is the default behavior.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Use blank lines.
  • Should VPC be spelled out?
  • Tick the values, removing the quotation marks.

PROXY protocol can be used with load balancers that support it to communicate the source addresses of client connections when forwarding those connections to the IngressController. Using PROXY protocol enables the IngressController to report those source addresses instead of reporting the load balancer's address in HTTP headers and logs. Note that enabling PROXY protocol on the IngressController will cause connections to fail if you are not using a load balancer that uses PROXY protocol to forward connections to the IngressController. See http://www.haproxy.org/download/2.2/doc/proxy-protocol.txt for information about PROXY protocol.
The following values are valid for this field:
* The empty string. * "TCP". * "PROXY".
| protocol specifies whether the IngressController expects incoming connections to use plain TCP or whether the IngressController expects PROXY protocol.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Tick "protocol."
  • IngressController

Comment on lines +1654 to +1656
PROXY protocol can be used with load balancers that support it to communicate the source addresses of client connections when forwarding those connections to the IngressController. Using PROXY protocol enables the IngressController to report those source addresses instead of reporting the load balancer's address in HTTP headers and logs. Note that enabling PROXY protocol on the IngressController will cause connections to fail if you are not using a load balancer that uses PROXY protocol to forward connections to the IngressController. See http://www.haproxy.org/download/2.2/doc/proxy-protocol.txt for information about PROXY protocol.
The following values are valid for this field:
* The empty string. * "TCP". * "PROXY".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • IngressController
  • Use blank lines

| `ciphers`
| `array (string)`
| ciphers is used to specify the cipher algorithms that are negotiated during the TLS handshake. Operators may remove entries their operands do not support. For example, to use DES-CBC3-SHA (yaml):
| ciphers is used to specify the cipher algorithms that are negotiated during the TLS handshake. Operators may remove entries their operands do not support. For example, to use DES-CBC3-SHA (yaml):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Tick "ciphers"
  • Should "DES-CBC3-SHA" be ticked

| `string`
| minTLSVersion is used to specify the minimal version of the TLS protocol that is negotiated during the TLS handshake. For example, to use TLS versions 1.1, 1.2 and 1.3 (yaml):
minTLSVersion: TLSv1.1
| minTLSVersion is used to specify the minimal version of the TLS protocol that is negotiated during the TLS handshake. For example, to use TLS versions 1.1, 1.2 and 1.3 (yaml):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Tick "minTLSVersion"
  • Add comma after 1.2

Copy link
Contributor

@bobfuru bobfuru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really confused. We don't normally hardcode any of our auto-generated API docs. Before I review the rest_api/operator_apis/ingresscontroller-operator-openshift-io-v1.adoc file, I'd like @jboxman to weigh in on whether this PR should proceed in this way. Shouldn't the enhancement request be brought to the Engineering team?

@bobfuru bobfuru added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2022
@bobfuru
Copy link
Contributor

bobfuru commented Apr 8, 2022

Adding the hold label.

These changes have to happen in the upstream Operator repo here:
https://github.com/openshift/api/blob/master/operator/v1/types_ingress.go. From there, our auto-gen process is for that code to get ingested and injected into an OpenAPI spec file where it can then be auto-imported for our docs (the process of this import is unclear to me but @jboxman understands more).

@Amrita42
Copy link
Contributor Author

Amrita42 commented Apr 11, 2022

@sherine-k @quarterpin based on the recent updates in CFE-10 . I have created a new PR that does not touch the rest_api file.

@Amrita42
Copy link
Contributor Author

Closing this PR

@Amrita42 Amrita42 closed this Apr 11, 2022
@Amrita42 Amrita42 deleted the CFE-10 branch April 18, 2022 06:38
@kalexand-rh kalexand-rh removed this from the OCP 4.11 GA milestone Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants