Skip to content
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

Added info for routes to accept ingress objects #8109

Merged
merged 1 commit into from
Mar 16, 2018

Conversation

bfallonf
Copy link

For #8099

Thanks, @knobunc in the end, I thought it was better to create a new PR rather than merging and editing. I have some questions, I'll ask them inline.

NOTE: this is not to be cherrypicked into another branch until this is found to be a supported procedure. Ben, do you know when this will be?

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 13, 2018
[[admin-guide-managing-networking-ingress]]
== Setting up the HAProxy router to accept ingress objects

By default, the HAProxy router only accepts route objects. You can set up the
Copy link
Author

Choose a reason for hiding this comment

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

In order to describe the comparison, I mentioned the router only accepts route objects. Is this accurate?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is correct.

link:https://kubernetes.io/docs/concepts/services-networking/ingress/[Ingress
Controller] so that it accepts both route and ingress objects. If configured,
the same rules apply for claiming a domain for a namespace based on the creation
time of the object.
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure what the above means. Can this be elaborated on? Is this listed somewhere else in the docs (it mentions "same rules")?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm... the best I can find is a tangential reference in install_config/router/default_haproxy_router.adoc at arond line 1218.

* Ingress objects store the keys and certificates in secrets, so the router needs permission to read all secrets in the system.
* Ingress objects only support edge termination for `https` routes.

To configure an existing router to have Ingress support (assuming the default name of `router` for the deployment configuration and the ):
Copy link
Author

Choose a reason for hiding this comment

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

This sentence trailed off. Do you remember what was supposed to be here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops. "and the service-account"

[[admin-guide-managing-networking-ingress]]
== Setting up the HAProxy router to accept ingress objects

By default, the HAProxy router only accepts route objects. You can set up the
Copy link
Contributor

Choose a reason for hiding this comment

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

That is correct.

link:https://kubernetes.io/docs/concepts/services-networking/ingress/[Ingress
Controller] so that it accepts both route and ingress objects. If configured,
the same rules apply for claiming a domain for a namespace based on the creation
time of the object.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm... the best I can find is a tangential reference in install_config/router/default_haproxy_router.adoc at arond line 1218.

* Ingress objects store the keys and certificates in secrets, so the router needs permission to read all secrets in the system.
* Ingress objects only support edge termination for `https` routes.

To configure an existing router to have Ingress support (assuming the default name of `router` for the deployment configuration and the ):
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops. "and the service-account"

@knobunc
Copy link
Contributor

knobunc commented Mar 14, 2018

The supported procedure may be openshift/origin#18658 which if we can get consensus should be in 3.10.

@bfallonf
Copy link
Author

Thanks @knobunc . We can look at this again for 3.10.

In the meantime, let me know if there's anything more. @openshift/team-documentation PTAL

time of the object. For example, ownership of routes is based on the claims made
for a host name with the namespace with the oldest route winning against any
other claimants.

Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence seems awkward (or a run on?) because of the two with the phrases. But, I see that the same sentence is in the Install Guide, so maybe I am mistaken.
https://docs.openshift.com/container-platform/3.7/install_config/router/default_haproxy_router.html#using-wildcard-routes.


* Ingress objects store the keys and certificates in secrets, so the router needs permission to read all secrets in the system.
* Ingress objects only support edge termination for `https` routes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should the https be bolded with asterisks?

@mburke5678
Copy link
Contributor

@bfallonf A couple of nits. Otherwise LGTM

@bfallonf
Copy link
Author

Thanks @mburke5678 I've made changes as you suggested. I'll merge this.

Note that this is only for Origin as of now, but I'll continuously poke @knobunc until this is ready to be cherry-picked into the OCP branches.

@vikram-redhat
Copy link
Contributor

@bfallonf - please make sure to get a QE review before merging.

@bfallonf bfallonf merged commit 3c1eaee into openshift:master Mar 16, 2018
@bfallonf
Copy link
Author

@vikram-redhat I merged before I saw your comment...

@bmeng Can I please get a QA of this PR? If there's issues, please let me know. I'll create another PR if needed.

. Add the cluster-admin role to the router:
+
----
$ oc adm policy add-role-to-user cluster-admin router`

Choose a reason for hiding this comment

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

Below two role should be added to sa router, oc adm policy add-role-to-user cluster-admin router is invalid.

oc adm policy add-cluster-role-to-user cluster-reader system:serviceaccount:default:router
oc adm policy add-cluster-role-to-user system:openshift:controller:service-serving-cert-controller system:serviceaccount:default:router

Copy link

Choose a reason for hiding this comment

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

maybe oc adm policy add-cluster-role-to-user cluster-reader -z router also work here.

@lihongan
Copy link

And another issue is how haproxy handle the situation if ingress and route claim the same hostname ?
From my testing, it shows only the first created resource will be served by haproxy.
@knobunc could you please help confirm ? if that true, then we should also write down them here I think.

@knobunc
Copy link
Contributor

knobunc commented Mar 20, 2018

Yes, internally ingress objects and route objects get turned into the same data structure in the router, so there is no real difference except that an ingress object can turn into multiple objects. Once that has been done, the same rules apply. Then the object that with the earliest creation date claims a hostname for a namespace, it doesn't matter if it is a route or ingress. BUT... since an ingress object could have two hostnames, one hostname can be claimed by the ingress and be active, but the other might not be able to claim, and be ignored.

There are docs in default_haproxy_router.adoc about the claim rules. But they seem to be under the wildcard information. It would be good to make them a little more prominent.

@knobunc
Copy link
Contributor

knobunc commented Mar 20, 2018

@mburke5678 PLEASE NOTE that this is likely only going to be Origin docs. We have a better solution coming that will supersede all of this for 3.10 so we don't want to support it in OSE for 3.9 just to replace it.

@mburke5678
Copy link
Contributor

@bfallonf See Ben's notes ^^

@bfallonf bfallonf deleted the ingress_edit branch March 22, 2018 01:09
@bfallonf bfallonf restored the ingress_edit branch March 22, 2018 01:56
@bfallonf
Copy link
Author

Thanks @lihongan @bmeng @knobunc .

I've created #8337 as a followup. Please let me know if you have any further thoughts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants