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

CORS-2317: Add Ingress LB IPs to Infra CR and set DNS unmanaged when BYO DNS is enabled #1016

Merged
merged 5 commits into from
Feb 1, 2024

Conversation

sadasu
Copy link
Contributor

@sadasu sadasu commented Jan 11, 2024

  • Bring in the latest updates to Infrastructure CR from openshift/api
  • Update GCPPlatformStatus with the Ingress LB IPs when ClusterHosted DNS is enabled
  • Put DNS in Unmanaged state because an in-cluster DNS solution would be in use in place of the platform default DNS.

Based on enhancement: openshift/enhancements#1468

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jan 11, 2024
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 11, 2024

@sadasu: This pull request references CORS-2317 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

  • Bring in the latest updates to Infrastructure CR from openshift/api
  • Update GCPPlatformStatus with the Ingress LB IPs when ClusterHosted DNS is enabled
  • Put DNS in Unmanaged state because an in-cluster DNS solution would be in use in place of the platform default DNS.

Based on enhancement: openshift/enhancements#1468

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 openshift-eng/jira-lifecycle-plugin repository.

@sadasu
Copy link
Contributor Author

sadasu commented Jan 11, 2024

/jira refresh

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 11, 2024

@sadasu: This pull request references CORS-2317 which is a valid jira issue.

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

@sadasu
Copy link
Contributor Author

sadasu commented Jan 11, 2024

@Miciah could PTAL?

@openshift-ci openshift-ci bot requested review from alebedev87 and miheer January 11, 2024 16:21
@sadasu sadasu force-pushed the custom-dns branch 3 times, most recently from 28996d4 to d17eac6 Compare January 11, 2024 19:07
@sadasu
Copy link
Contributor Author

sadasu commented Jan 11, 2024

/retest-required

@sadasu sadasu changed the title CORS-2317: Add Ingress LB IPs to Infra CR and set leave DNS unmanaged when BYO DNS is enabled CORS-2317: Add Ingress LB IPs to Infra CR and set DNS unmanaged when BYO DNS is enabled Jan 11, 2024
@sadasu
Copy link
Contributor Author

sadasu commented Jan 16, 2024

/retest-required

@Miciah
Copy link
Contributor

Miciah commented Jan 17, 2024

/assign

@candita
Copy link
Contributor

candita commented Jan 17, 2024

/assign @gcs278

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.

Would you mind combining the first three commits?

  • "Updating openshift/api to bring in changes for BYO DNS for GCP"
  • "Update vendoring as result of openshift/api and k8s.io version bump"
  • "Update IngressController CRD with the openshift/api version bump"

The goal here is that each individual commit should pass make test verify. I do appreciate that you separated the vendor bump from the implementation commits!

pkg/operator/controller/ingress/controller.go Outdated Show resolved Hide resolved
pkg/operator/controller/ingress/controller.go Outdated Show resolved Hide resolved
pkg/operator/controller/ingress/controller.go Outdated Show resolved Hide resolved
pkg/operator/controller/ingress/controller.go Show resolved Hide resolved
Copy link
Contributor

@gcs278 gcs278 left a comment

Choose a reason for hiding this comment

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

No issues from me other than a nit pick and what @Miciah identified. I think a unit test for updateInfraIngressLoadBalancerIPs would be a nice-to-have, but I don't feel strongly about it and I won't hold up the PR for it.

pkg/operator/controller/ingress/controller.go Outdated Show resolved Hide resolved
@sadasu
Copy link
Contributor Author

sadasu commented Jan 23, 2024

@Miciah and @gcs278, thanks for the awesome comments! Afaik, I have addressed them all.

@sadasu
Copy link
Contributor Author

sadasu commented Jan 23, 2024

/retest-required

@gcs278
Copy link
Contributor

gcs278 commented Jan 26, 2024

/lgtm
@Miciah could you take a look and approve?

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 26, 2024
@sadasu
Copy link
Contributor Author

sadasu commented Jan 26, 2024

/test e2e-aws-ovn-single-node

pkg/operator/controller/ingress/controller.go Outdated Show resolved Hide resolved
pkg/operator/controller/ingress/controller.go Outdated Show resolved Hide resolved
pkg/operator/controller/ingress/controller.go Outdated Show resolved Hide resolved
pkg/operator/controller/ingress/controller.go Outdated Show resolved Hide resolved
pkg/operator/controller/ingress/controller.go Outdated Show resolved Hide resolved
pkg/operator/controller/ingress/controller.go Outdated Show resolved Hide resolved
pkg/operator/controller/ingress/controller_test.go Outdated Show resolved Hide resolved
pkg/operator/controller/ingress/controller_test.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2024
@sadasu
Copy link
Contributor Author

sadasu commented Jan 31, 2024

/retest-required

k8s.io/apiextensions-apiserver v0.28.3
k8s.io/apimachinery v0.28.3
k8s.io/apiserver v0.28.3
k8s.io/api v0.29.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we go ahead and take 0.29.1 for these K8S packages?

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR only bumps the k8s.io/* packages to v0.29.0 because the openshift/api bump requires doing so. I expect we'll bump to k8s.io/api v0.29.1 or later in a future PR (for example, for a controller-runtime bump or another openshift/api bump).

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine if we want do it later. It was more of a "why not use the latest if we are bumping now" sort of thing, but we can just plan on doing it later.

@Miciah
Copy link
Contributor

Miciah commented Jan 31, 2024

/approve
/lgtm
/hold
for @gcs278's comments and additional review if needed. @gcs278, feel free to release the hold if your comments have been satisfactorily addressed.

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jan 31, 2024
Copy link
Contributor

openshift-ci bot commented Jan 31, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah

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 Jan 31, 2024
@gcs278
Copy link
Contributor

gcs278 commented Jan 31, 2024

Thanks for the quick responses @sadasu
/unhold
/lgtm

@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 Jan 31, 2024
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD c2e2760 and 2 for PR HEAD 777f2ee in total

@sadasu
Copy link
Contributor Author

sadasu commented Feb 1, 2024

/retest-required

Copy link
Contributor

openshift-ci bot commented Feb 1, 2024

@sadasu: all tests passed!

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-merge-bot openshift-merge-bot bot merged commit d31546e into openshift:master Feb 1, 2024
14 checks passed
@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-cluster-ingress-operator-container-v4.16.0-202402010541.p0.gd31546e.assembly.stream for distgit ose-cluster-ingress-operator.
All builds following this will include this PR.

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. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants