CFE-888: Enable DNSNameResolver feature-gate by chiragkyal · Pull Request #2131 · openshift/cluster-network-operator

chiragkyal · 2023-11-27T14:49:59Z

Enable DNSNameResolver feature-gate for ovn-kubernetes

Related to EP : openshift/enhancements#1335

openshift-ci-robot · 2023-11-27T14:50:03Z

@chiragkyal: This pull request references CFE-888 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.15.0" version, but no target version was set.

Details

In response to this:

Related to EP : openshift/enhancements#1335

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 · 2023-11-27T14:51:02Z

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

chiragkyal · 2023-11-28T08:06:52Z

/retest

chiragkyal · 2023-11-29T07:09:12Z

/retest

chiragkyal · 2023-11-29T08:08:11Z

/retest

openshift-ci-robot · 2023-11-29T09:06:33Z

@chiragkyal: This pull request references CFE-888 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.15.0" version, but no target version was set.

Details

In response to this:

Enable DNSNameResolver feature-gate for ovn-kubernetes

Related to EP : openshift/enhancements#1335

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.

chiragkyal · 2023-12-04T11:00:51Z

/hold until CRD is available

arkadeepsen · 2023-12-09T16:32:37Z

/hold until ovn-kubernetes/ovn-kubernetes#4045 is meged downstream.

chiragkyal · 2023-12-21T11:58:45Z

/assign @npinaeva

npinaeva

generally looks good, we just need to wait for ovn-k PR to be merged to finalize it

npinaeva · 2024-01-26T08:26:07Z

bindata/network/ovn-kubernetes/common/002-rbac-node.yaml

+  verbs:
+  - create
+  - delete
+  - get
+  - list
+  - patch
+  - update
+  - watch


we'll need to split these rights between node and control pane once we finalize the ovn-k implementation. It is likely to be node can only read, and control-plane can create/update/delete

Sure, for now, I've dropped create, delete, patch and update permissions from node. Let me know if it needs further refinement.

bindata/network/ovn-kubernetes/common/003-rbac-controller.yaml

npinaeva · 2024-01-26T08:49:47Z

pkg/network/ovn_kubernetes_test.go

 		disableMultiNet          bool
 		enableMultiNetPolicies   bool
-		enableAdminNetPolicies   bool
+		featureGates             configv1.CustomFeatureGates


I am thinking, maybe to make less changes in the future, we could replace this parameter with enableFeatureGates []FeatureGateName and then build a real config by iterating over all known feature gates and adding the ones that are listed to Enabled and others to Disabled. In that case you don't need to change tests that don't enable any feature gates, and only make changes when a new feature gate should be enabled. What do you think?

Agreed, and that's a good suggestion. I've updated the structure, hope it looks cleaner now.

kyrtapz · 2024-04-23T09:45:01Z

bindata/network/ovn-kubernetes/common/004-rbac-control-plane.yaml

+  resources:
+  - dnsnameresolvers
+  verbs:
+  - create


Does the control-plane really require all of these verbs?

I think so, because cluster manager creates and deletes these objects (similar to cloudprivateipconfigs I suppose)

kyrtapz · 2024-04-23T09:48:34Z

bindata/network/ovn-kubernetes/common/008-script-lib.yaml

        admin_network_policy_enabled_flag="--enable-admin-network-policy"
      fi

+      dns_name_resolver_enabled_flag=


start-ovnkube-node is only used by ovnkube-node, this means that the ovnkube-cluster-manager won't rollout if DNS_NAME_RESOLVER_ENABLE changes.

I think it will, as it uses 004-config.yaml file for features setup

Lest consider a scenario in which the featuregate is enabled during cluster runtime(e.g cluster is moved to techpreview):

FeatureGate gets enabled

CNO restarts

CNO renders the new 008-script-lib.yaml and 004-config.yaml confimaps

ovnkube-node daemonset is rolled out because it is annotated with the has of the script:

cluster-network-operator/pkg/network/ovn_kubernetes.go

Lines 356 to 378 in 6aa2f0f

// Many ovnkube config options are stored in ConfigMaps; the ovnkube

// daemonsets need to know when those ConfigMaps change so they can

// restart with the new options. Render those ConfigMaps first and

// embed a hash of their data into the ovnkube-node daemonsets.

h := sha1.New()

for _, path := range cmPaths {

manifests, err := render.RenderTemplate(path, &data)

if err != nil {

return nil, progressing, errors.Wrapf(err, "failed to render ConfigMap template %q", path)

}

// Hash each rendered ConfigMap object's data

for _, m := range manifests {

bytes, err := json.Marshal(m)

if err != nil {

return nil, progressing, errors.Wrapf(err, "failed to marshal ConfigMap %q manifest", path)

}

if _, err := h.Write(bytes); err != nil {

return nil, progressing, errors.Wrapf(err, "failed to hash ConfigMap %q data", path)

}

}

}

data.Data["OVNKubeConfigHash"] = hex.EncodeToString(h.Sum(nil))

ovnkube-controller is not rolled out and it doesn't reload the configmap dynamically on its own.

does this make sense?

we agreed it is a bigger problem and created a card for it https://issues.redhat.com/browse/SDN-4832

kyrtapz · 2024-04-23T09:54:00Z

bindata/network/ovn-kubernetes/common/002-rbac-node.yaml

+{{- if .DNS_NAME_RESOLVER_ENABLE }}
+- apiGroups: ["network.openshift.io"]
+  resources:
+  - dnsnameresolvers


Where is the dnsnameresolvers CRD applied?

The CRD will be created when along with the cluster-dns-operator: https://github.com/openshift/cluster-dns-operator/pull/394/files#diff-d5348dedaad45595f0bd40f1b4730932489203aa511b1432951f2bfad8ab8a79

kyrtapz · 2024-04-23T09:55:09Z

pkg/network/ovn_kubernetes_test.go

 			disableMultiNet:          true,
 			enableMultiNetPolicies:   true,
-			enableAdminNetPolicies:   true,
+			enabledFeatureGates:      []configv1.FeatureGateName{configv1.FeatureGateAdminNetworkPolicy},


configv1.FeatureGateDNSNameResolver is not set in the featuregates here

I think this test is specific to ANP. The enableAdminNetPolicies field is changed to enabledFeatureGates for using the same field for different feature gates in different test cases. The test case for DNSNameResolver is added below.

pkg/network/ovn_kubernetes_test.go

chiragkyal · 2024-04-25T10:15:11Z

/retest-required

when DNSNameResolver feature-gate is enabled Signed-off-by: chiragkyal <ckyal@redhat.com>

lihongan · 2024-05-15T02:20:45Z

/retest-required

arkadeepsen · 2024-05-15T18:24:39Z

All other dev PRs are merged

/hold cancel

melvinjoseph86 · 2024-05-16T02:34:34Z

/retest-required

npinaeva · 2024-05-16T08:41:53Z

manual CI-bot report

passed: (37.3s) 2024-05-16T08:39:04 "[sig-network][OCPFeatureGate:DNSNameResolver][Feature:EgressFirewall] when using openshift ovn-kubernetes should ensure egressfirewall with wildcard dns rules is created [Suite:openshift/conformance/parallel]"

/lgtm

kyrtapz · 2024-05-16T08:47:05Z

/approve

openshift-ci · 2024-05-16T08:49:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chiragkyal, kyrtapz, npinaeva

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:

~~OWNERS~~ [kyrtapz]

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

npinaeva · 2024-05-16T08:50:26Z

/acknowledge-critical-fixes-only

npinaeva · 2024-05-16T08:56:46Z

/label acknowledge-critical-fixes-only

openshift-ci-robot · 2024-05-16T10:57:46Z

/retest-required

Remaining retests: 0 against base HEAD 0a03896 and 2 for PR HEAD 71ddff7 in total

npinaeva · 2024-05-16T14:07:10Z

/retest-required

openshift-ci · 2024-05-16T14:22:24Z

@chiragkyal: The following tests 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-sdn	`b2d2e55`	link	true	`/test e2e-gcp-sdn`
ci/prow/e2e-azure-ovn-dualstack	`b2d2e55`	link	false	`/test e2e-azure-ovn-dualstack`
ci/prow/e2e-vsphere-ovn-windows	`b2d2e55`	link	true	`/test e2e-vsphere-ovn-windows`
ci/prow/e2e-openstack-sdn	`b2d2e55`	link	false	`/test e2e-openstack-sdn`
ci/prow/e2e-network-mtu-migration-sdn-ipv4	`b2d2e55`	link	false	`/test e2e-network-mtu-migration-sdn-ipv4`
ci/prow/e2e-aws-ovn-shared-to-local-gateway-mode-migration-periodic	`b2d2e55`	link	false	`/test e2e-aws-ovn-shared-to-local-gateway-mode-migration-periodic`
ci/prow/e2e-vsphere-ovn-dualstack-primaryv6	`71ddff7`	link	false	`/test e2e-vsphere-ovn-dualstack-primaryv6`
ci/prow/security	`71ddff7`	link	false	`/test security`
ci/prow/e2e-aws-hypershift-ovn-kubevirt	`71ddff7`	link	false	`/test e2e-aws-hypershift-ovn-kubevirt`
ci/prow/e2e-ovn-ipsec-step-registry	`71ddff7`	link	false	`/test e2e-ovn-ipsec-step-registry`
ci/prow/e2e-azure-ovn	`71ddff7`	link	false	`/test e2e-azure-ovn`
ci/prow/e2e-vsphere-ovn-dualstack	`71ddff7`	link	false	`/test e2e-vsphere-ovn-dualstack`

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

npinaeva · 2024-05-16T14:26:52Z

build failed :(
/retest-required

openshift-bot · 2024-05-16T20:57:30Z

[ART PR BUILD NOTIFIER]

This PR has been included in build cluster-network-operator-container-v4.16.0-202405161711.p0.g019fa77.assembly.stream.el9 for distgit cluster-network-operator.
All builds following this will include this PR.

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

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2023

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 Nov 27, 2023

chiragkyal force-pushed the cfe-888 branch from a37ba3c to 5b63b89 Compare November 27, 2023 15:19

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2023

chiragkyal force-pushed the cfe-888 branch from 9b94727 to 980d4f1 Compare November 29, 2023 08:05

chiragkyal marked this pull request as ready for review November 29, 2023 09:04

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 29, 2023

chiragkyal changed the title ~~CFE-888: [WIP] Enable DNSNameResolver feature-gate~~ CFE-888: Enable DNSNameResolver feature-gate Nov 29, 2023

openshift-ci bot requested review from dcbw and tssurya November 29, 2023 09:05

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 29, 2023

chiragkyal force-pushed the cfe-888 branch from 980d4f1 to 6f2ad02 Compare November 30, 2023 08:32

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2023

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 4, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2023

chiragkyal force-pushed the cfe-888 branch from 6f2ad02 to acf66c2 Compare December 20, 2023 12:31

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2023

openshift-ci bot assigned npinaeva Dec 21, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2024

npinaeva reviewed Jan 26, 2024

View reviewed changes

kyrtapz reviewed Apr 23, 2024

View reviewed changes

chiragkyal force-pushed the cfe-888 branch from e09c3c6 to b05b507 Compare April 23, 2024 12:18

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 23, 2024

npinaeva reviewed Apr 23, 2024

View reviewed changes

pkg/network/ovn_kubernetes_test.go Outdated Show resolved Hide resolved

CFE-888: ovnkube:add enable-dns-name-resolver flag

71ddff7

when DNSNameResolver feature-gate is enabled Signed-off-by: chiragkyal <ckyal@redhat.com>

chiragkyal force-pushed the cfe-888 branch from a309f53 to 71ddff7 Compare May 7, 2024 09:03

npinaeva mentioned this pull request May 15, 2024

CFE-853: e2e test case for DNSNameResolver and EgressFirewall integration openshift/origin#28683

Merged

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 15, 2024

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

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 16, 2024

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

openshift-merge-bot bot merged commit 019fa77 into openshift:master May 16, 2024

chiragkyal deleted the cfe-888 branch May 16, 2024 18:15

	// Many ovnkube config options are stored in ConfigMaps; the ovnkube
	// daemonsets need to know when those ConfigMaps change so they can
	// restart with the new options. Render those ConfigMaps first and
	// embed a hash of their data into the ovnkube-node daemonsets.
	h := sha1.New()
	for _, path := range cmPaths {
	manifests, err := render.RenderTemplate(path, &data)
	if err != nil {
	return nil, progressing, errors.Wrapf(err, "failed to render ConfigMap template %q", path)
	}

	// Hash each rendered ConfigMap object's data
	for _, m := range manifests {
	bytes, err := json.Marshal(m)
	if err != nil {
	return nil, progressing, errors.Wrapf(err, "failed to marshal ConfigMap %q manifest", path)
	}
	if _, err := h.Write(bytes); err != nil {
	return nil, progressing, errors.Wrapf(err, "failed to hash ConfigMap %q data", path)
	}
	}
	}
	data.Data["OVNKubeConfigHash"] = hex.EncodeToString(h.Sum(nil))

Conversation

chiragkyal commented Nov 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Nov 27, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Nov 27, 2023

Uh oh!

chiragkyal commented Nov 28, 2023

Uh oh!

chiragkyal commented Nov 29, 2023

Uh oh!

chiragkyal commented Nov 29, 2023

Uh oh!

openshift-ci-robot commented Nov 29, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chiragkyal commented Dec 4, 2023

Uh oh!

arkadeepsen commented Dec 9, 2023

Uh oh!

chiragkyal commented Dec 21, 2023

Uh oh!

npinaeva left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chiragkyal commented Apr 25, 2024

Uh oh!

lihongan commented May 15, 2024

Uh oh!

arkadeepsen commented May 15, 2024

Uh oh!

melvinjoseph86 commented May 16, 2024

Uh oh!

npinaeva commented May 16, 2024

Uh oh!

kyrtapz commented May 16, 2024

Uh oh!

openshift-ci bot commented May 16, 2024

Uh oh!

npinaeva commented May 16, 2024

Uh oh!

npinaeva commented May 16, 2024

Uh oh!

openshift-ci-robot commented May 16, 2024

Uh oh!

npinaeva commented May 16, 2024

Uh oh!

openshift-ci bot commented May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

chiragkyal commented Nov 27, 2023 •

edited

Loading

openshift-ci-robot commented Nov 27, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Nov 29, 2023 •

edited by openshift-ci bot

Loading

openshift-ci bot commented May 16, 2024 •

edited

Loading