Skip to content

Conversation

bfallonf
Copy link

Updates from #8109

cc @lihongan @bmeng @knobunc

Will comment in the other PR. Thanks!

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 22, 2018

Choose a reason for hiding this comment

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

oc adm policy add-cluster-role-to-user system:openshift:controller:service-serving-cert-controller -z router is also needed for enabling ingress, otherwise the router pod logs will show many errors below:

E0323 06:22:50.408710       1 reflector.go:205] github.com/openshift/origin/pkg/router/controller/factory/factory.go:117: Failed to list *core.Secret: secrets is forbidden: User "system:serviceaccount:default:router" cannot list secrets at the cluster scope: User "system:serviceaccount:default:router" cannot list all secrets in the cluster
E0323 06:22:51.410501       1 reflector.go:205] github.com/openshift/origin/pkg/router/controller/factory/factory.go:117: Failed to list *core.Secret: secrets is forbidden: User "system:serviceaccount:default:router" cannot list secrets at the cluster scope: User "system:serviceaccount:default:router" cannot list all secrets in the cluster
E0323 06:22:52.417695       1 reflector.go:205] github.com/openshift/origin/pkg/router/controller/factory/factory.go:117: Failed to list *core.Secret: secrets is forbidden: User "system:serviceaccount:default:router" cannot list secrets at the cluster scope: User "system:serviceaccount:default:router" cannot list all secrets in the cluster

others looks good.

@bfallonf bfallonf added the peer-review-needed Signifies that the peer review team needs to review this PR label Mar 26, 2018
@bfallonf
Copy link
Author

Thanks @lihongan . I've changed to suggestions. Please let me know if I've done anything wrong.

@openshift/team-documentation PTAL

@lihongan
Copy link

LGTM. Thank you, @bfallonf

Copy link
Contributor

Choose a reason for hiding this comment

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

@bfallonf This is not clear to me (but, maybe that's OK). "one hostname can be claimed by the
ingress object and be active, but the other might not be able to claim, and be ignored."
What does "be ignored" mean here? If one host name is ignored, does that mean anything for the other? (Sorry if this is an ignorant question.)

Copy link
Contributor

Choose a reason for hiding this comment

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

s/claim/claimed ??

Copy link
Author

Choose a reason for hiding this comment

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

@mburke5678 True.

@knobunc Would it be more accurate to say "The claim rules above apply for each of these objects. However, because an ingress object can have two hostnames, one hostname can be claimed by the ingress object and be active, while the second cannot claim, and remains inactive and ignored."

Copy link
Contributor

Choose a reason for hiding this comment

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

Remains inactive is probably better.

@mburke5678
Copy link
Contributor

@bfallonf Couple of questions.

Copy link
Contributor

Choose a reason for hiding this comment

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

multiple route objects

Copy link
Contributor

Choose a reason for hiding this comment

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

Remains inactive is probably better.

@bfallonf
Copy link
Author

Thanks @knobunc . Changes made to suggestions. I'll merge.

@bfallonf bfallonf removed the peer-review-needed Signifies that the peer review team needs to review this PR label Mar 29, 2018
@bfallonf bfallonf merged commit 340a878 into openshift:master Mar 29, 2018
@bfallonf bfallonf deleted the ingress_edit_2 branch April 3, 2018 04:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants