Skip to content

OSDOCS2143: Added topic about configuring GCP Internal Ingress Load Balancer Global Access Option #32226

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.

Sign up for GitHub

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

Jump to bottom
Merged
bergerhoffer merged 1 commit into from
Jun 9, 2021
Merged

OSDOCS2143: Added topic about configuring GCP Internal Ingress Load Balancer Global Access Option #32226

bergerhoffer merged 1 commit into from
Jun 9, 2021

Conversation

@skrthomas
Copy link
Contributor

@skrthomas skrthomas commented May 5, 2021 •
edited
Loading

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 5, 2021
@netlify
Copy link

netlify bot commented May 5, 2021 •
edited
Loading

✔️ Deploy Preview for osdocs ready!

🔨 Explore the source changes: ac109e0

🔍 Inspect the deploy log: https://app.netlify.com/sites/osdocs/deploys/60c0fca035adf50008d67127

😎 Browse the preview: https://deploy-preview-32226--osdocs.netlify.app/openshift-enterprise/latest/networking/ingress-operator

@skrthomas skrthomas force-pushed the OSDOCS2143 branch 2 times, most recently from 2a69728 to abf2947 Compare May 5, 2021 17:46
@bmcelvee bmcelvee added this to the Future Release milestone May 5, 2021
bmcelvee
bmcelvee reviewed May 5, 2021
View reviewed changes
bmcelvee
bmcelvee reviewed May 5, 2021
View reviewed changes
modules/nw-ingress-controller-configuration-gcp-global-access.adoc Outdated
Copy link
Contributor

@bmcelvee bmcelvee May 5, 2021

Choose a reason for hiding this comment

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

s/balancing/balancing.

bmcelvee
bmcelvee reviewed May 5, 2021
View reviewed changes
@skrthomas
Copy link
Contributor Author

skrthomas commented May 6, 2021

@lihongan this PR is ready for QA.

@skrthomas
Copy link
Contributor Author

skrthomas commented May 6, 2021

@sgreene570 ready for technical review.

sgreene570
sgreene570 reviewed May 6, 2021
View reviewed changes
sgreene570
sgreene570 reviewed May 6, 2021
View reviewed changes
modules/nw-ingress-controller-configuration-gcp-global-access.adoc Outdated
Copy link

@sgreene570 sgreene570 May 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
As a cluster administrator with an internal load balancer on GCP, you can specify the global access option. Clients in any region within the same VPC network as the load balancer can reach the workloads running on your cluster.
As a cluster administrator with an ingress controller exposed via an internal load balancer on GCP, you can specify the global access option. Clients in any region within the same VPC network as the load balancer can reach the workloads running on your cluster.

sgreene570
sgreene570 reviewed May 6, 2021
View reviewed changes
sgreene570
sgreene570 reviewed May 6, 2021
View reviewed changes
modules/nw-ingress-controller-configuration-gcp-global-access.adoc Outdated
Copy link

@sgreene570 sgreene570 May 6, 2021

Choose a reason for hiding this comment

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

An existing ingress controller can be edited such that the GCP Global Access option is enabled.
However, a new ingress controller with the option specified could be created as well. I think it may be worth mentioning this in the docs.

Copy link
Contributor Author

@skrthomas skrthomas May 7, 2021

Choose a reason for hiding this comment

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

@sgreene570 would it be beneficial to provide the procedure for editing an existing Ingress Controller and creating a new one?

Copy link

@sgreene570 sgreene570 May 10, 2021

Choose a reason for hiding this comment

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

@sgreene570 would it be beneficial to provide the procedure for editing an existing Ingress Controller and creating a new one?

I would think so, yea.

Copy link

@sgreene570 sgreene570 May 10, 2021

Choose a reason for hiding this comment

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

Note that as is, the edit instructions are based off of the wrong resource. ingress.config and Ingress Controllers are 2 different resources.

Copy link
Contributor Author

@skrthomas skrthomas May 10, 2021

Choose a reason for hiding this comment

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

@sgreene570 sorry about that. I'm still a little new and trying to figure things out! I think I may be on the right track now. Can you take another peep at the code?

Copy link
Contributor Author

@skrthomas skrthomas May 10, 2021

Choose a reason for hiding this comment

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

Also, in response to your commend about specifying the option when you create a new ingress controller, I added a note about that in the prerequisites

Copy link

@sgreene570 sgreene570 May 10, 2021

Choose a reason for hiding this comment

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

No worries, I would agree it looks like you are on the right track! Thanks!

@skrthomas skrthomas force-pushed the OSDOCS2143 branch 3 times, most recently from 4560b19 to 15d68c2 Compare May 10, 2021 15:25
@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 10, 2021
sgreene570
sgreene570 reviewed May 10, 2021
View reviewed changes
Copy link

@sgreene570 sgreene570 left a comment

Choose a reason for hiding this comment

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

Overall looks good! One small comment.

@skrthomas skrthomas force-pushed the OSDOCS2143 branch 3 times, most recently from d0c58cc to 5d1504a Compare May 10, 2021 19:50
@openshift-ci openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 10, 2021
@skrthomas skrthomas force-pushed the OSDOCS2143 branch 2 times, most recently from 545ec06 to 3468048 Compare May 10, 2021 20:10
bmcelvee
bmcelvee reviewed May 11, 2021
View reviewed changes
@skrthomas skrthomas force-pushed the OSDOCS2143 branch 3 times, most recently from 6c21f28 to 12aa578 Compare May 11, 2021 21:01
@lihongan
Copy link

lihongan commented May 12, 2021

LGTM.
Thank you @skrthomas

@skrthomas
Copy link
Contributor Author

skrthomas commented May 12, 2021

Awesome thanks for your reviews all!

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2021
@openshift-ci
Copy link

openshift-ci bot commented May 19, 2021

@skrthomas: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@bergerhoffer bergerhoffer removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 7, 2021
@bergerhoffer
Copy link
Contributor

bergerhoffer commented Jun 7, 2021

Removing the "needs-rebase" label since there are no conflicts anymore. Looks like the bot didn't clear it properly.

bergerhoffer
bergerhoffer reviewed Jun 7, 2021
View reviewed changes
Copy link
Contributor

@bergerhoffer bergerhoffer left a comment

Choose a reason for hiding this comment

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

A few things! Some are just FYI or things to consider in the future.

modules/nw-ingress-controller-configuration-gcp-global-access.adoc Outdated
Copy link
Contributor

@bergerhoffer bergerhoffer Jun 7, 2021

Choose a reason for hiding this comment

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

For consistency with how we write it elsewhere, can you say "the OpenShift CLI (oc)"?

modules/nw-ingress-controller-configuration-gcp-global-access.adoc Outdated
Copy link
Contributor

@bergerhoffer bergerhoffer Jun 7, 2021

Choose a reason for hiding this comment

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

Not critical (since this is a technical comment) , but usually it would be better to use a command that outputs values when you're verifying something, as opposed to editing again just to view.

@bergerhoffer bergerhoffer added the peer-review-done Signifies that the peer review team has reviewed this PR label Jun 7, 2021
@skrthomas skrthomas force-pushed the OSDOCS2143 branch 4 times, most recently from 8a6ac19 to 97d96f3 Compare June 8, 2021 18:45
bergerhoffer
bergerhoffer reviewed Jun 9, 2021
View reviewed changes
@skrthomas skrthomas force-pushed the OSDOCS2143 branch 2 times, most recently from 94c609b to 97edcf9 Compare June 9, 2021 17:33
bergerhoffer
bergerhoffer reviewed Jun 9, 2021
View reviewed changes
Copy link
Contributor

@bergerhoffer bergerhoffer left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM to me now, mreging

@bergerhoffer bergerhoffer merged commit 4d926b5 into openshift:master Jun 9, 2021
@bergerhoffer
Copy link
Contributor

bergerhoffer commented Jun 9, 2021

/cherrypick enterprise-4.8

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Jun 9, 2021
Merged
@openshift-cherrypick-robot
Copy link

openshift-cherrypick-robot commented Jun 9, 2021

@bergerhoffer: new pull request created: #33286

Details

In response to this:

/cherrypick enterprise-4.8

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bergerhoffer bergerhoffer bergerhoffer left review comments

@bmcelvee bmcelvee bmcelvee left review comments

+2 more reviewers

@sgreene570 sgreene570 sgreene570 left review comments

@lihongan lihongan lihongan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

branch/enterprise-4.8 peer-review-done Signifies that the peer review team has reviewed this PR size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

OCP 4.8 GA

Development

Successfully merging this pull request may close these issues.

8 participants

@skrthomas @lihongan @bergerhoffer @openshift-cherrypick-robot @sgreene570 @bmcelvee @vikram-redhat @openshift-ci-robot