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.

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

Ingress endpoint #1597

Merged
merged 1 commit into from
Aug 8, 2022
Merged

Ingress endpoint #1597

merged 1 commit into from
Aug 8, 2022

Conversation

pcrentsil
Copy link
Contributor

@pcrentsil pcrentsil commented Jul 21, 2022 •
edited
Loading

Configure ingress controller with endpoint publishing strategy as Private. Expose this configuration setting as an annotation on the HostedCluster/HostedControlPlane CRs. And will be generic to be applicable to any providers. Support ingress endpoint publishing strategy scope like we do today. Support for this was recently added to the IBM ROKS Toolkit and our ROKS 4 ansible code. (Focus on scoping this up to Hypershift).

Issue: #1599

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes #

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2022
@netlify
Copy link

netlify bot commented Jul 21, 2022 •
edited
Loading

✅ Deploy Preview for hypershift-docs ready!

Name Link
🔨 Latest commit 46a353f
🔍 Latest deploy log https://app.netlify.com/sites/hypershift-docs/deploys/62df0716d2d5d3000872a8ce
😎 Deploy Preview https://deploy-preview-1597--hypershift-docs.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.

@openshift-ci openshift-ci bot requested review from csrwng and sjenning July 21, 2022 12:24
@pcrentsil pcrentsil mentioned this pull request Jul 21, 2022
Closed
4 tasks
@enxebre
Copy link
Member

enxebre commented Jul 22, 2022

Thanks @pcrentsil

/hold
Please include a commit/PR desc (and link to issue if any) explaining "why" this change and a commit message stating "what" https://github.com/openshift/hypershift/blob/main/docs/content/contribute/index.md

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2022
@hasueki
Copy link
Contributor

hasueki commented Jul 25, 2022

@pcrentsil We will also need to adjust ingress.ReconcileDefaultIngressController() to take in an additional function input, isPrivate bool, which you can just pass in p.IsPrivate.

@pcrentsil pcrentsil force-pushed the IngressEndpoint branch 2 times, most recently from a1d466b to 46a353f Compare July 25, 2022 21:11
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2022
@pcrentsil
Copy link
Contributor Author

Thanks @pcrentsil

/hold Please include a commit/PR desc (and link to issue if any) explaining "why" this change and a commit message stating "what" https://github.com/openshift/hypershift/blob/main/docs/content/contribute/index.md

This has been updated.

enxebre
enxebre reviewed Jul 27, 2022
View reviewed changes
api/v1alpha1/hostedcluster_types.go Outdated
@@ -53,6 +53,9 @@ const (
// (performs container image verification).
PortierisImageAnnotation = "hypershift.openshift.io/portieris-image"

//Configure ingress controller with endpoint publishing strategy as Private.
Copy link
Member

Choose a reason for hiding this comment

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

Missing space before starting the sentence.

Can we elaborate details and context e.g:

// Configure ingress controller with endpoint publishing strategy as Private.
This overrides any opinionated strategy set by platform in ReconcileDefaultIngressController.
It's used by IBM cloud for...
NOTE: We'll expose this in the API if the use case gets generalised.

enxebre
enxebre reviewed Jul 27, 2022
View reviewed changes
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
@@ -592,7 +592,7 @@ func (r *reconciler) reconcileIngressController(ctx context.Context, hcp *hyperv
p := ingress.NewIngressParams(hcp)
ingressController := manifests.IngressDefaultIngressController()
if _, err := r.CreateOrUpdate(ctx, r.client, ingressController, func() error {
return ingress.ReconcileDefaultIngressController(ingressController, p.IngressSubdomain, p.PlatformType, p.Replicas, (hcp.Spec.Platform.IBMCloud != nil && hcp.Spec.Platform.IBMCloud.ProviderType == configv1.IBMCloudProviderTypeUPI))
Copy link
Member

Choose a reason for hiding this comment

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

since we are here, for readability: can we move (hcp.Spec.Platform.IBMCloud != nil && hcp.Spec.Platform.IBMCloud.ProviderType == configv1.IBMCloudProviderTypeUPI) up into a variable that is passed through the signature?

@enxebre
Copy link
Member

enxebre commented Jul 27, 2022 •
edited
Loading

Thanks! I know we discuss this on sync but could you remind me what's the particular case for IBM? Is there a need to choose isPrivate vs not depending on the cluster or is it always the same?
If this is always the same for the IBM platform I'd prefer to drop the annotation and keep being opinionated in ReconcileDefaultIngressController. Then if we ever want to enable this for more platforms/use cases we do through the API.
Every time we add a magic annotation like this the project grows in complexity perpetuating a hidden parallel API and makes debuggability and reproducibility more difficult.

nit: mind using imperative mood in commit, e.g "Private Type for Ingress Endpoints" -> "Add annotation to enable PrivateType for default guest cluster Ingress controller". Also fwiw you can include the PR desc in the commit as well git commit -m"what" -m"why/pr desc".

@hasueki
Copy link
Contributor

hasueki commented Jul 27, 2022 •
edited
Loading

Hey @enxebre , thank you for the review! I can provide a bit of context. For our OpenShift on IBM Cloud offering (ROKS), there is a capability where clients are able to toggle to update if they want the ingress controller exposed via LB or not. This enhancement is meant to expose such configuration for HyperShift managed clusters, so that our automation process can wrap around it.

@enxebre
Copy link
Member

enxebre commented Jul 28, 2022 •
edited
Loading

Thanks @hasueki, makes sense. I think this should be eventually in the API, possibly just not yet. I created https://issues.redhat.com/browse/HOSTEDCP-527 to track and follow up as we see fit when evolving the API.

lgtm, pending fixing commits.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 29, 2022 •
edited
Loading

@pcrentsil: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws f901129 link false /test e2e-aws

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@pcrentsil
Copy link
Contributor Author

Hey, @enxebre let me know your comments on any of the changes implemented!

enxebre
enxebre reviewed Aug 3, 2022
View reviewed changes
control-plane-operator/hostedclusterconfigoperator/controllers/resources/ingress/params.go Outdated
return &IngressParams{
IngressSubdomain: globalconfig.IngressDomain(hcp),
Replicas: replicas,
PlatformType: hcp.Spec.Platform.Type,
IsPrivate: isPrivate,
CloudUPI: CloudUPI,
Copy link
Member

Choose a reason for hiding this comment

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

nit: this is IBM specific, should be IBMCloudIBM.

enxebre
enxebre reviewed Aug 3, 2022
View reviewed changes
control-plane-operator/hostedclusterconfigoperator/controllers/resources/ingress/params.go Outdated
isPrivate := false
CloudUPI := false
if hcp.Spec.Platform.IBMCloud != nil && hcp.Spec.Platform.IBMCloud.ProviderType == configv1.IBMCloudProviderTypeUPI {
CloudUPI = true
Copy link
Member

Choose a reason for hiding this comment

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

nit: why is this variable exported? CloudUPI -> cloudUPI or make it ibm specific ibmCloudUPI

@enxebre
Copy link
Member

enxebre commented Aug 3, 2022

Thanks @pcrentsil
/approve
PTAL @alvaroaleman @csrwng

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 3, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre, pcrentsil

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2022
@csrwng
Copy link
Contributor

csrwng commented Aug 3, 2022

apart from the nit that @enxebre pointed out (local vars should never be capitalized), this lgtm

@pcrentsil
Configure ingress controller with endpoint publishing strategy as Pri…
3227508 
…vate.

Expose this configuration setting as an annotation on the HostedCluster/HostedControlPlane CRs. And will be generic to be applicable to any providers. Support ingress endpoint publishing strategy scope like we do today. Support for this was recently added to the IBM ROKS Toolkit and our ROKS 4 ansible code
@pcrentsil
Copy link
Contributor Author

@enxebre @csrwng Modified variable to be ibm specific ibmCloudUPI

Thanks Guys,

@alvaroaleman
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 8, 2022
@alvaroaleman
Copy link
Contributor

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 8, 2022
@openshift-merge-robot openshift-merge-robot merged commit b5b6b91 into openshift:main Aug 8, 2022
@pcrentsil
Copy link
Contributor Author

/cherry-pick release-4.11

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Aug 9, 2022
Merged
@openshift-cherrypick-robot
Copy link

@pcrentsil: new pull request created: #1667

In response to this:

/cherry-pick release-4.11

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

@hasueki hasueki hasueki left review comments

@enxebre enxebre enxebre left review comments

@csrwng csrwng Awaiting requested review from csrwng

@sjenning sjenning Awaiting requested review from sjenning

Assignees

@alvaroaleman alvaroaleman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@pcrentsil @enxebre @hasueki @csrwng @alvaroaleman @openshift-cherrypick-robot @openshift-merge-robot