OCPBUGS-44641: GCP placing *.apps record in wrong managed zone #9216

barbacbd · 2024-11-19T17:08:49Z

** The *.apps record is not being placed in the correct DNS Managed Zone. The existing managed zone should be used only during xpn installs. The ingress operator was supplied with the incorrect managed zone information.

openshift-ci-robot · 2024-11-19T17:08:57Z

@barbacbd: This pull request references Jira Issue OCPBUGS-44641, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.18.0) matches configured target version for branch (4.18.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @jianli-wei

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

** The *.apps record is not being placed in the correct DNS Managed Zone. The existing managed zone should be used only during xpn installs. The ingress operator was supplied with the incorrect managed zone information.

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.

barbacbd · 2024-11-19T17:10:04Z

/cc @patrickdillon
/cc @r4f4

barbacbd · 2024-11-19T17:10:13Z

/label platform/google

** The *.apps record is not being placed in the correct DNS Managed Zone. The existing managed zone should be used only during xpn installs. The ingress operator was supplied with the incorrect managed zone information.

r4f4

/lgtm

r4f4 · 2024-11-19T20:24:22Z

pkg/asset/manifests/dns.go

@@ -67,6 +67,8 @@ func (*DNS) Dependencies() []asset.Asset {
 }

 // Generate generates the DNS config and its CRD.
+//
+//nolint:gocyclo


Nit: it's nice to add a reason why it's ok to silence the linter.

The way to fix this linting issue is to move the code into its own function, especially if you make the changes to check the PrivateVisabilityConfig of the zone

patrickdillon

I agree with this change: we should only do this for xpn installs, but it doesn't fix the bug in question. To fix the bug, you need to look at the zone.PrivateVisibilityConfig.Networks on the returned zone and ensure that it contains the network(s?) from the install config.

patrickdillon · 2024-11-20T02:22:20Z

pkg/asset/manifests/dns.go

@@ -67,6 +67,8 @@ func (*DNS) Dependencies() []asset.Asset {
 }

 // Generate generates the DNS config and its CRD.
+//
+//nolint:gocyclo


The way to fix this linting issue is to move the code into its own function, especially if you make the changes to check the PrivateVisabilityConfig of the zone

…twork that is expected to be used for installations.

jianli-wei

/label qe-approved

openshift-ci-robot · 2024-11-21T09:02:44Z

@barbacbd: This pull request references Jira Issue OCPBUGS-44641, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.18.0) matches configured target version for branch (4.18.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @jianli-wei

In response to this:

** The *.apps record is not being placed in the correct DNS Managed Zone. The existing managed zone should be used only during xpn installs. The ingress operator was supplied with the incorrect managed zone information.

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 · 2024-11-21T09:02:54Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jianli-wei
Once this PR has been reviewed and has the lgtm label, please ask for approval from barbacbd. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

r4f4 · 2024-11-21T21:52:12Z

pkg/asset/manifests/dns.go

+		if err != nil {
+			return "", fmt.Errorf("failed to get private zone for %q: %w", installConfig.Config.BaseDomain, err)
+		}
+		if zone != nil {


So the GetDNSZone call can return nil even if there weren't any errors?

Yes, it returns nil when there are no zones found. It is not an error to find no zones. This is an indicator that a zone must be created.

r4f4 · 2024-11-22T14:42:00Z

pkg/asset/manifests/dns.go

+				expectedNetworkURL := fmt.Sprintf(
+					"https://www.googleapis.com/compute/v1/projects/%s/global/networks/%s",
+					installConfig.Config.GCP.NetworkProjectID,
+					installConfig.Config.GCP.Network,
+				)


This is repeated in the capi code so maybe it's a candidate for a small shared function?

r4f4

/lgtm

…with the network provided.

r4f4

/lgtm

barbacbd · 2024-11-25T12:04:43Z

/retest-required

openshift-ci · 2024-11-25T15:04:34Z

@barbacbd: 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-gcp-ovn	`c6d93c8`	link	true	`/test e2e-gcp-ovn`

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.

openshift-ci bot requested a review from jianli-wei November 19, 2024 17:09

openshift-ci bot requested review from patrickdillon and r4f4 November 19, 2024 17:10

openshift-ci bot added the platform/google label Nov 19, 2024

openshift-ci bot requested review from rna-afk and rwsu November 19, 2024 17:11

OCPBUGS-44641: GCP placing *.apps record in wrong managed zone

7cf7241

** The *.apps record is not being placed in the correct DNS Managed Zone. The existing managed zone should be used only during xpn installs. The ingress operator was supplied with the incorrect managed zone information.

barbacbd force-pushed the OCPBUGS-44641 branch from f7729c1 to 7cf7241 Compare November 19, 2024 18:19

r4f4 reviewed Nov 19, 2024

View reviewed changes

openshift-ci bot assigned r4f4 Nov 19, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 19, 2024

patrickdillon reviewed Nov 20, 2024

View reviewed changes

** Compare the private zone (if a precreated zone is found) to the ne…

9dee075

…twork that is expected to be used for installations.

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2024

jianli-wei approved these changes Nov 21, 2024

View reviewed changes

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Nov 21, 2024

openshift-ci bot requested a review from jianli-wei November 21, 2024 09:02

barbacbd requested review from patrickdillon and r4f4 November 21, 2024 15:58

r4f4 reviewed Nov 21, 2024

View reviewed changes

r4f4 reviewed Nov 22, 2024

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 22, 2024

** Update cluster api to determine if the private zone is associated …

c6d93c8

…with the network provided.

barbacbd force-pushed the OCPBUGS-44641 branch from c9598d2 to c6d93c8 Compare November 22, 2024 20:07

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 22, 2024

r4f4 reviewed Nov 22, 2024

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-44641: GCP placing *.apps record in wrong managed zone #9216

OCPBUGS-44641: GCP placing *.apps record in wrong managed zone #9216

barbacbd commented Nov 19, 2024

openshift-ci-robot commented Nov 19, 2024

barbacbd commented Nov 19, 2024

barbacbd commented Nov 19, 2024

r4f4 left a comment

r4f4 Nov 19, 2024

patrickdillon Nov 20, 2024

patrickdillon left a comment

patrickdillon Nov 20, 2024

jianli-wei left a comment

openshift-ci-robot commented Nov 21, 2024

openshift-ci bot commented Nov 21, 2024

r4f4 Nov 21, 2024

barbacbd Nov 22, 2024

r4f4 Nov 22, 2024

r4f4 left a comment

r4f4 left a comment

barbacbd commented Nov 25, 2024

openshift-ci bot commented Nov 25, 2024

OCPBUGS-44641: GCP placing *.apps record in wrong managed zone #9216

Are you sure you want to change the base?

OCPBUGS-44641: GCP placing *.apps record in wrong managed zone #9216

Conversation

barbacbd commented Nov 19, 2024

openshift-ci-robot commented Nov 19, 2024

barbacbd commented Nov 19, 2024

barbacbd commented Nov 19, 2024

r4f4 left a comment

Choose a reason for hiding this comment

r4f4 Nov 19, 2024

Choose a reason for hiding this comment

patrickdillon Nov 20, 2024

Choose a reason for hiding this comment

patrickdillon left a comment

Choose a reason for hiding this comment

patrickdillon Nov 20, 2024

Choose a reason for hiding this comment

jianli-wei left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Nov 21, 2024

openshift-ci bot commented Nov 21, 2024

r4f4 Nov 21, 2024

Choose a reason for hiding this comment

barbacbd Nov 22, 2024

Choose a reason for hiding this comment

r4f4 Nov 22, 2024

Choose a reason for hiding this comment

r4f4 left a comment

Choose a reason for hiding this comment

r4f4 left a comment

Choose a reason for hiding this comment

barbacbd commented Nov 25, 2024

openshift-ci bot commented Nov 25, 2024