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

Option to pass floating IP to ingress port #3855

Merged
merged 1 commit into from Jul 20, 2020

Conversation

iamemilio
Copy link

Customers have requested a more consistent experience for floating IPs in the installer, with a preference for how the loadbalancer FIP is implemented. To support this, we created an option for passing a floating ip for the ingress port that is identical in usage to the loadbalancer/api port implementation.

@iamemilio iamemilio changed the title Option to pass floating IP to ingress port [WIP] Option to pass floating IP to ingress port Jul 7, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 7, 2020
@iamemilio
Copy link
Author

/cc @adduarte

@iamemilio
Copy link
Author

will rebase on https://github.com/openshift/installer/pull/3864/files to fix validvaluesfetcher mock

@iamemilio
Copy link
Author

/hold until #3864

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 9, 2020
@iamemilio
Copy link
Author

Removed all validations, will add them and docs in a separate pull request due to numerous pending changes to the validations code base.

@iamemilio
Copy link
Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2020
@iamemilio iamemilio force-pushed the ingress_fip branch 2 times, most recently from 748a2f2 to c19be45 Compare July 10, 2020 20:24
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 13, 2020
@iamemilio iamemilio force-pushed the ingress_fip branch 2 times, most recently from fd1cd6e to 84a8855 Compare July 13, 2020 20:14
@iamemilio
Copy link
Author

/retest

@iamemilio iamemilio changed the title [WIP] Option to pass floating IP to ingress port Option to pass floating IP to ingress port Jul 14, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 14, 2020
@iamemilio
Copy link
Author

Will squash on merge. Validations and Docs in an upcoming pr to avoid merge conflicts

@@ -1313,6 +1318,7 @@ spec:
- cloud
- computeFlavor
- externalNetwork
- ingressFloatingIP
- lbFloatingIP
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be renamed as APIFloatingIP then, right?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure we can simply rename it, we should instead deprecate lbFloatingIP in favor of APIFloatingIP.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I think that should really be a separate PR

@mandre
Copy link
Member

mandre commented Jul 16, 2020

@iamemilio good to squash?

To make the user experience more cohesive, we are adding this feature to
allow users to pass a floating IP to be attached to the ingress port. This
mirrors the way users add the floating ip for the API port.
@mandre
Copy link
Member

mandre commented Jul 16, 2020

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2020
@iamemilio
Copy link
Author

/assign @sdodson pls review/approve when you can

@iamemilio
Copy link
Author

/assign @sdodson

@ghost
Copy link

ghost commented Jul 20, 2020

/approve
/lgtm

@ghost
Copy link

ghost commented Jul 20, 2020

Will this be backported to the lower version?

@mandre
Copy link
Member

mandre commented Jul 20, 2020

Will this be backported to the lower version?

This is planned for 4.6, with no intention to backport to previous releases. We try to avoid backporting features unless there is a strong reason for it (customer case for instance).

@sdodson
Copy link
Member

sdodson commented Jul 20, 2020

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mandre, sdodson, wjiangjay

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 20, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot
Copy link
Contributor

@iamemilio: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-libvirt ecda20e link /test e2e-libvirt
ci/prow/e2e-aws-fips ecda20e link /test e2e-aws-fips

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.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 23d73eb into openshift:master Jul 20, 2020
mandre added a commit to mandre/installer that referenced this pull request Aug 17, 2020
Since openshift#3855, the installer is
able to attach the `ingressFloatingIP` specified in the
`install-config.yaml` to the ingress-port. Update the documentation to
reflect the change.
mandre added a commit to mandre/installer that referenced this pull request Sep 11, 2020
Since openshift#3855, the installer is
able to attach the `ingressFloatingIP` specified in the
`install-config.yaml` to the ingress-port. Update the documentation to
reflect the change.
vrutkovs pushed a commit to vrutkovs/installer that referenced this pull request Sep 18, 2020
Since openshift#3855, the installer is
able to attach the `ingressFloatingIP` specified in the
`install-config.yaml` to the ingress-port. Update the documentation to
reflect the change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

7 participants