CORS-3755: AWS: Add Ingress LB IPs to Infra CR when in-cluster DNS is enabled #1167

sadasu · 2024-11-19T23:25:11Z

Add Ingress LB IPs to Infra CR and set DNS unmanaged when BYO DNS is enabled on the AWS platform.

Replicating the changes added for GCP as part of #1016 for the AWS platform.

openshift-ci-robot · 2024-11-19T23:31:42Z

@sadasu: This pull request references CORS-3755 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.18.0" version, but no target version was set.

In response to this:

Replicating the changes added as part of #1016 for the AWS platform.

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.

openshift-ci-robot · 2024-11-19T23:32:15Z

@sadasu: This pull request references CORS-3755 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.18.0" version, but no target version was set.

In response to this:

Add Ingress LB IPs to Infra CR and set DNS unmanaged when BYO DNS is enabled on the AWS platform.

Replicating the changes added as part of #1016 for the AWS platform.

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 · 2024-11-19T23:34:16Z

@gcs278, @candita could you PTAL? You had reviewed similar changes for the GCP platform.

This feature is in tech preview in 4.18.

sadasu · 2024-11-20T12:45:51Z

/retest-required

gcs278

Started review, but I have a concern that should be addressed before moving.

pkg/operator/controller/ingress/controller.go

sadasu · 2024-11-20T21:47:44Z

/test e2e-hypershift

pkg/operator/controller/ingress/controller.go

gcs278 · 2024-11-21T03:00:25Z

/assign

sadasu · 2024-11-21T14:12:20Z

/retest-required

sadasu · 2024-11-21T16:24:25Z

/label acknowledge-critical-fixes-only

pkg/operator/controller/ingress/controller_test.go

pkg/operator/controller/ingress/controller.go

gcs278 · 2024-11-21T17:31:54Z

@sadasu Do you mind putting a link to https://issues.redhat.com//browse/CORS-3755 in your second commit (the actual implementation)? Just a convenience for those trying to find the history.

pkg/operator/controller/ingress/controller_test.go

openshift-ci-robot · 2024-11-21T17:42:49Z

@sadasu: This pull request references CORS-3755 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.18.0" version, but no target version was set.

In response to this:

Add Ingress LB IPs to Infra CR and set DNS unmanaged when BYO DNS is enabled on the AWS platform.

Replicating the changes added for GCP as part of #1016 for the AWS platform.

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.

gcs278 · 2024-11-21T20:25:32Z

pkg/operator/controller/ingress/controller_test.go

 					if len(ingress.IP) > 0 {
-						if !slices.Contains(infraConfig.Status.PlatformStatus.GCP.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs, configv1.IP(ingress.IP)) {
-							t.Errorf("expected Infra CR to contain %s", ingress.IP)
+						if !reflect.DeepEqual(infraConfig.Status.PlatformStatus.GCP.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs, tc.expectedLBIPs) {
+							t.Errorf("expected Infra CR to contain %s but found %s", tc.expectedLBIPs, infraConfig.Status.PlatformStatus.GCP.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs)
+						}
+					}
+					if len(ingress.Hostname) > 0 {
+						if !reflect.DeepEqual(infraConfig.Status.PlatformStatus.AWS.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs, tc.expectedLBIPs) {
+							t.Errorf("expected Infra CR to contain %s but found %s", tc.expectedLBIPs, infraConfig.Status.PlatformStatus.AWS.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs)
 						}


Instead of using the status.loadBalancer.ingress.[ip|hostname] to determine the platform, consider just using the platform provided in the test case. When we expand this to azure, this will have to be refactored because both GCP and Azure use ingress.IP.

Also, this code is somewhat confusing. We loop through each of the status.loadBalancer.ingress, but then in the reflect.DeepEqual we don't use the ingress variable. I don't think we need to check if len(ingress.IP) > 0 {, we can let the test compare if IngressLoadBalancerIPs and tc.expectedLBIPs are both nil too for better coverage (making sure it doesn't add an unwanted IP).

Suggested change

if len(ingress.IP) > 0 {

if !slices.Contains(infraConfig.Status.PlatformStatus.GCP.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs, configv1.IP(ingress.IP)) {

t.Errorf("expected Infra CR to contain %s", ingress.IP)

if !reflect.DeepEqual(infraConfig.Status.PlatformStatus.GCP.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs, tc.expectedLBIPs) {

t.Errorf("expected Infra CR to contain %s but found %s", tc.expectedLBIPs, infraConfig.Status.PlatformStatus.GCP.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs)

}

}

if len(ingress.Hostname) > 0 {

if !reflect.DeepEqual(infraConfig.Status.PlatformStatus.AWS.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs, tc.expectedLBIPs) {

t.Errorf("expected Infra CR to contain %s but found %s", tc.expectedLBIPs, infraConfig.Status.PlatformStatus.AWS.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs)

}

if updated {

switch tc.platform.Type {

case configv1.AWSPlatformType:

if !reflect.DeepEqual(infraConfig.Status.PlatformStatus.AWS.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs, tc.expectedLBIPs) {

t.Errorf("expected Infra CR to contain %s but found %s", tc.expectedLBIPs, infraConfig.Status.PlatformStatus.AWS.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs)

}

case configv1.GCPPlatformType:

if !reflect.DeepEqual(infraConfig.Status.PlatformStatus.GCP.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs, tc.expectedLBIPs) {

t.Errorf("expected Infra CR to contain %s but found %s", tc.expectedLBIPs, infraConfig.Status.PlatformStatus.GCP.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs)

}

}

}

Let me know if that makes sense.

Yes to both. Updated.

gcs278 · 2024-11-21T21:46:14Z

Thanks for the quick responses. I discussed with the team today, and we acknowledged the disadvantage of manually resolving the AWS load balancer hostname, but since this is restricted to tech preview, it's okay to move forward with this. Hopefully we can find a better design, but we will see how it works in tech preview.

/approve
/lgtm

openshift-ci · 2024-11-21T21:46:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gcs278

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:

~~OWNERS~~ [gcs278]

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

sadasu · 2024-11-21T21:48:42Z

/label acknowledge-critical-fixes-only

openshift-ci-robot · 2024-11-21T23:30:10Z

/retest-required

Remaining retests: 0 against base HEAD 0b561f8 and 2 for PR HEAD 0663c8c in total

Also set DNSManagementPolicy to Unmanaged when BYO DNS is enabled Implementation for https://issues.redhat.com//browse/CORS-3755

Add tests to verify value of EndpointPublishingStrategy on AWS platform when BYO DNS is enabled.

Add tests for the AWS Platform where the Ingress LB's Hostname is available but the Infra CR needs to be updated with its IP.

gcs278 · 2024-11-22T04:53:19Z

Not totally sure how we usually handle go vendor pruning updates, I noticed your first commit fails make test because it's removing vendored files still in use.

In this scenario, I think the vendoring update should actually be co-located with the commit that removed the import for "golang.org/x/exp/slices", so no commit is "broken". Feel free to fix it, but I don't think it's worth holding up the PR.

/lgtm

openshift-ci-robot · 2024-11-22T06:29:57Z

/retest-required

Remaining retests: 0 against base HEAD fb5c5d3 and 2 for PR HEAD a8096c0 in total

sadasu · 2024-11-22T08:58:54Z

/retest-required

sadasu · 2024-11-22T09:13:53Z

Not totally sure how we usually handle go vendor pruning updates, I noticed your first commit fails make test because it's removing vendored files still in use.

In this scenario, I think the vendoring update should actually be co-located with the commit that removed the import for "golang.org/x/exp/slices", so no commit is "broken". Feel free to fix it, but I don't think it's worth holding up the PR.

/lgtm

Yes, the 1st commit is changing the imports requiring the vendoring updates. Sorry, I was following the installer team convention of separating out the vendoring changes into its own commit.

sadasu · 2024-11-22T12:06:11Z

/test e2e-aws-ovn-upgrade

openshift-ci · 2024-11-22T14:24:43Z

@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-sigs/prow repository. I understand the commands that are listed here.

sadasu · 2024-11-22T14:49:39Z

/cherry-pick release-4.18

I think this landed shortly after automatic forwarding of release-4.18 to master stopped. So, initiating a backport to release-4.18.

openshift-cherrypick-robot · 2024-11-22T14:50:24Z

@sadasu: new pull request created: #1168

In response to this:

/cherry-pick release-4.18

I think this landed shortly after automatic forwarding of release-4.18 to master stopped. So, initiating a backport to release-4.18.

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-sigs/prow repository.

openshift-bot · 2024-11-22T17:43:33Z

[ART PR BUILD NOTIFIER]

Distgit: ose-cluster-ingress-operator
This PR has been included in build ose-cluster-ingress-operator-container-v4.19.0-202411221708.p0.g8be1749.assembly.stream.el9.
All builds following this will include this PR.

openshift-ci bot requested review from gcs278 and knobunc November 19, 2024 23:25

sadasu changed the title ~~AWS: Add Ingress LB IPs to Infra CR and set DNS unmanaged when BYO DNS is enabled~~ CORS-3755: AWS: Add Ingress LB IPs to Infra CR and set DNS unmanaged when BYO DNS is enabled Nov 19, 2024

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 19, 2024

sadasu changed the title ~~CORS-3755: AWS: Add Ingress LB IPs to Infra CR and set DNS unmanaged when BYO DNS is enabled~~ CORS-3755: AWS: Add Ingress LB IPs to Infra CR when in-cluster DNS is enabled Nov 19, 2024

sadasu force-pushed the aws-custom-dns branch from 8b4f0f1 to b0b548e Compare November 20, 2024 16:26

gcs278 reviewed Nov 20, 2024

View reviewed changes

pkg/operator/controller/ingress/controller.go Show resolved Hide resolved

pkg/operator/controller/ingress/controller.go Outdated Show resolved Hide resolved

sadasu force-pushed the aws-custom-dns branch from b0b548e to d7bc536 Compare November 20, 2024 19:02

gcs278 reviewed Nov 20, 2024

View reviewed changes

pkg/operator/controller/ingress/controller.go Show resolved Hide resolved

sadasu force-pushed the aws-custom-dns branch from d7bc536 to 0ae34b2 Compare November 21, 2024 00:01

openshift-ci bot assigned gcs278 Nov 21, 2024

openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Nov 21, 2024