Skip to content

Conversation

@suleymanakbas91
Copy link
Contributor

@suleymanakbas91 suleymanakbas91 commented May 18, 2022

This change implements a check for ingresscontrollers with a spec.domain that does not match the spec.baseDomain of the cluster DNS config. As a result of this check, operator does not create DNS records and sets DNSManaged condition to False for the respective ingresscontroller.

  • pkg/operator/controller/ingress/controller.go: platformStatus was being calculated in many different functions before. With this change, it is only calculated once in controller.go and passed to the other functions as an argument. It also uses manageDNSForDomain function while admitting the ingresscontroller to emit a warning event.
  • pkg/operator/controller/ingress/controller_test.go: This updates the tests to use platformStatus instead of infraConfig.
  • pkg/operator/controller/ingress/deployment.go: platformStatus calculation moved to controller.go.
  • pkg/operator/controller/ingress/dns.go: This implements and uses manageDNSForDomain function to skip creation of DNS records for ingresscontrollers having a domain not matching the baseDomain on AWS.
  • pkg/operator/controller/ingress/dns_test.go: This introduces a unit test for manageDNSForDomain function.
  • pkg/operator/controller/ingress/load_balancer_service.go: platformStatus calculation moved to controller.go.
  • pkg/operator/controller/ingress/status.go: platformStatus calculation moved to controller.go. This also sets DNSManaged-False as a result of manageDNSForDomain function.
  • test/e2e/domain_not_matching_base_test.go: This implements an e2e test to check if the operator skips creation of DNS records and sets DNSManaged=False for ingresscontrollers having a domain not matching the baseDomain on AWS.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 18, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 18, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot requested review from alebedev87 and ironcladlou May 18, 2022 08:38
@suleymanakbas91 suleymanakbas91 force-pushed the diff-domain branch 5 times, most recently from 7a0de42 to 4de3979 Compare May 19, 2022 14:54
@suleymanakbas91 suleymanakbas91 changed the title Do not manage DNS with a different domain than baseDomain Bug 2041616: Do not create DNSRecords for IngressControllers with different domain than baseDomain May 19, 2022
@openshift-ci openshift-ci bot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels May 19, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 19, 2022

@suleymanakbas91: This pull request references Bugzilla bug 2041616, which is invalid:

  • expected the bug to target the "4.11.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 2041616: Do not create DNSRecords for IngressControllers with different domain than baseDomain

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.

@suleymanakbas91 suleymanakbas91 changed the title Bug 2041616: Do not create DNSRecords for IngressControllers with different domain than baseDomain Bug 2041616: Do not create DNS records for IngressControllers with different domain than baseDomain May 19, 2022
@suleymanakbas91
Copy link
Contributor Author

/bugzilla refresh

@openshift-ci openshift-ci bot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels May 19, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 19, 2022

@suleymanakbas91: This pull request references Bugzilla bug 2041616, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.11.0) matches configured target release for branch (4.11.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @quarterpin

Details

In response to this:

/bugzilla 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 kubernetes/test-infra repository.

@openshift-ci openshift-ci bot requested a review from quarterpin May 19, 2022 15:13
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 19, 2022

@suleymanakbas91: This pull request references Bugzilla bug 2041616, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.11.0) matches configured target release for branch (4.11.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @quarterpin

Details

In response to this:

Bug 2041616: Do not create DNS records for IngressControllers with different domain than baseDomain

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.

@suleymanakbas91 suleymanakbas91 force-pushed the diff-domain branch 5 times, most recently from 59b2685 to 5a9c6f4 Compare May 19, 2022 16:27
@suleymanakbas91 suleymanakbas91 changed the title Bug 2041616: Do not create DNS records for IngressControllers with different domain than baseDomain Bug 2041616: Do not create DNS records for IngressControllers with domain not matching baseDomain May 19, 2022
@Miciah
Copy link
Contributor

Miciah commented Jun 2, 2022

/test verify

@Miciah
Copy link
Contributor

Miciah commented Jun 2, 2022

@suleymanakbas91, the verify job is now failing since #756 merged. It looks like the new E2E test can run in parallel with other tests, so it should call t.Parallel() and be included in the list of parallel tests in test/e2e/all_test.go.

@brandisher
Copy link
Contributor

/label tide/merge-method-squash

@openshift-ci openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jun 9, 2022
@brandisher
Copy link
Contributor

/retest

@brandisher
Copy link
Contributor

/test e2e-aws-operator

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 22, 2022
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 22, 2022
…t match the baseDomain of the cluster DNS config
@suleymanakbas91
Copy link
Contributor Author

/retest

@Miciah
Copy link
Contributor

Miciah commented Jun 22, 2022

/lgtm

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

openshift-ci bot commented Jun 22, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah, suleymanakbas91

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

The pull request process is described here

Details 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 Jun 22, 2022
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 2 against base HEAD 7dc3f4d and 8 for PR HEAD 0f5f96e in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 1 against base HEAD 7dc3f4d and 7 for PR HEAD 0f5f96e in total

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 22, 2022

@suleymanakbas91: 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-single-node 0f5f96e link false /test e2e-aws-single-node

Full PR test history. Your PR dashboard.

Details

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-ci openshift-ci bot merged commit 88d3c14 into openshift:master Jun 22, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 22, 2022

@suleymanakbas91: All pull requests linked via external trackers have merged:

Bugzilla bug 2041616 has been moved to the MODIFIED state.

Details

In response to this:

Bug 2041616: Do not create DNS records for IngressControllers with domain not matching baseDomain

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.

@suleymanakbas91 suleymanakbas91 deleted the diff-domain branch June 23, 2022 04:44
arkadeepsen pushed a commit to arkadeepsen/cluster-ingress-operator that referenced this pull request Jul 25, 2022
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. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants