Skip to content

Conversation

@jboxman
Copy link
Contributor

@jboxman jboxman commented Oct 1, 2019

Describe how to configure the Ingress Controller to use an internal load balancer on cloud providers.

@jboxman jboxman added this to the Future Release milestone Oct 1, 2019
@jboxman jboxman self-assigned this Oct 1, 2019
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 1, 2019
@jboxman jboxman mentioned this pull request Oct 1, 2019
@openshift-docs-preview-bot

The preview will be available shortly at:

@jboxman
Copy link
Contributor Author

jboxman commented Oct 1, 2019

@Miciah, thanks. This was more of a point of a departure than a finished PR. I have a few other items to complete before I can revisit this and finalize it for a review.

@jboxman
Copy link
Contributor Author

jboxman commented Oct 7, 2019

@Miciah, I'm not yet familiar with the Ingress Operator or controllers. What does "the IngressController is published by a public cloud load balancer by default" mean? What does it mean to get published?

Thanks.

@Miciah
Copy link
Contributor

Miciah commented Oct 7, 2019

What does "the IngressController is published by a public cloud load balancer by default" mean? What does it mean to get published?

It means "made available" or "exposed"; "published" doesn't have a technical meaning beyond that one that we give it in the API definition (https://godoc.org/github.com/openshift/api/operator/v1#EndpointPublishingStrategy). To rephrase the statement that you quoted, the Ingress Operator will create a cloud load balancer through which clients can connect to the IngressController unless the IngressController specifies a different endpoint publishing strategy.

@jboxman jboxman force-pushed the create-internal-lb-for-cloud branch from 6645d92 to 3f5d209 Compare October 18, 2019 20:07
@jboxman jboxman added the peer-review-needed Signifies that the peer review team needs to review this PR label Oct 18, 2019
Copy link

@lamek lamek left a comment

Choose a reason for hiding this comment

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

One small question, otherwise LGTM.

@lamek lamek added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Oct 18, 2019
@jboxman jboxman force-pushed the create-internal-lb-for-cloud branch from 3f5d209 to b7e1682 Compare October 19, 2019 03:48
@jboxman
Copy link
Contributor Author

jboxman commented Oct 20, 2019

@Miciah, I've updated this; How does it look? Thanks!

Copy link
Contributor

@Miciah Miciah left a comment

Choose a reason for hiding this comment

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

I suggested one minor correction, but otherwise looks good.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jboxman, this restriction came up on the Group G arch call:

4.2.0 warning on switching ingress to Internal can cause all worker nodes to lose internet connectivity. The only way to prevent that would be to have at least one public k8s service even if it doesn’t do anything

I think that we might need a step to confirm that you have such a service.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised to learn there is such a restriction, and it seems like a very serious defect. Is there an associated Bugzilla report?

Copy link
Contributor

Choose a reason for hiding this comment

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

@abhinavdahiya, was there a bug for the need to have at least one public k8s service if you switch the ingress controller to private?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be noted that the restriction applies specifically to Azure, not to other cloud platforms. (It has already been noted in the Group G arch call, but I'm repeating it here for the benefit of anyone who learns of the issue from seeing this thread.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to talk with @Miciah and @abhinavdahiya and @wking about how we solve this transparently. I think we should treat the issue as a blocker bug that we'll solve before release and remove the note here entirely.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like the installer already sets up egress for private clusters in 4.3 by installing a public LB, so there's nothing to note here after all?

Choose a reason for hiding this comment

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

the installer only sets up egress by creating a dummy public standard load balancer, when users request Internal clusters.

so for public cluster users, that want to make their ingress private as Day-2, there should be warning that, all nodes of the cluster will loose egress to internet in case there is no public LB pointing to those nodes...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abhinavdahiya, is that for any public cloud provider user, or just for Azure? As I'm still trying to internalize this, my first pass at this is simply the following:

If your cloud provider is Azure, you must have at least one public load balancer that points to your nodes.
If you do not, all of your nodes will lose egress connectivity to the Internet.

But if I include this as part of the procedure, is there a way to confirm if there is at least one public load-balancer point to all nodes? Doesn't having a public load-balancer defeat the purpose of private ingress?

Thanks.

cc @ironcladlou @Miciah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ironcladlou @Miciah

Any thoughts on this ^^?

Thanks.

@jboxman
Copy link
Contributor Author

jboxman commented Oct 28, 2019

@kalexand-rh, does the work you're doing supersede this PR? If so, should I close it or finalize it?

Thanks!

@kalexand-rh
Copy link
Contributor

@jboxman, please finalize it. I'll incorporate the content in the change that I'm working on.

@jboxman jboxman force-pushed the create-internal-lb-for-cloud branch from b7e1682 to e31cfd4 Compare November 5, 2019 19:24
Copy link
Contributor

Choose a reason for hiding this comment

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

.spec.domain is required unless we're talking about the default resource (which can omit .spec.domain and claim the default domain).

Copy link
Contributor

Choose a reason for hiding this comment

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

@ironcladlou I'm helping out to get this PR merged and just want to verify - this is the only update this PR needs? Thanks!

(I'll also likely have follow-up questions related to "User defined IngressControllers at installation".)

@jboxman jboxman force-pushed the create-internal-lb-for-cloud branch from e31cfd4 to 976c6f6 Compare November 20, 2019 01:46
@bmcelvee
Copy link
Contributor

@jboxman - just in case you see this, I'm going to grab this PR first thing in the morning and get it merged. Thanks!

@ironcladlou
Copy link
Contributor

Configuring an Ingress Controller to use an internal load balancer

You can configure the default Ingress Controller for your cluster to be internal by deleting and recreating it.

Would it be clearer to represent these separately?

The procedure for Configuring an Ingress Controller to use an internal load balancer is similar to what's already here, except we can remove all the notes about the default ingresscontroller, as we're talking about creating additional ingress controllers that live alongside the default.

The procedure for configure the default Ingress Controller for your cluster to be internal by deleting and recreating it in its most basic form could be:

oc replace --force --wait --filename - <<EOF
apiVersion: operator.openshift.io/v1
kind: IngressController
metadata:
  namespace: openshift-ingress-operator
  name: default
spec:
  endpointPublishingStrategy:
    type: LoadBalancerService
    loadBalancer:
      scope: Internal
EOF

@jboxman
Copy link
Contributor Author

jboxman commented Dec 18, 2019

@bmcelvee, thanks! It's been floating in the ether for a while now.

I believe there's similar content that is trapped in our release notes for 4.2 that has been requested to come out.

@bmcelvee bmcelvee removed this from the Future Release milestone Dec 19, 2019
@bmcelvee
Copy link
Contributor

Merging this PR with the to_followup label and opening the follow-up PR. Will link when it's open.

@bmcelvee bmcelvee merged commit cf23df1 into openshift:master Dec 19, 2019
@bmcelvee
Copy link
Contributor

/cherrypick enterprise-4.2

@openshift-cherrypick-robot

@bmcelvee: new pull request created: #18760

Details

In response to this:

/cherrypick enterprise-4.2

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.

@bmcelvee
Copy link
Contributor

/cherrypick enterprise-4.3

@openshift-cherrypick-robot

@bmcelvee: new pull request created: #18761

Details

In response to this:

/cherrypick enterprise-4.3

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.

@bmcelvee bmcelvee mentioned this pull request Dec 19, 2019
@bmcelvee
Copy link
Contributor

Link to follow-up PR: #18764.

@jboxman jboxman deleted the create-internal-lb-for-cloud branch January 20, 2020 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.2 branch/enterprise-4.3 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. to_followup

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants