OCPBUGS-33237: Run haproxy to connect to kas from data plane if noproxy settings contain kas #3999

enxebre · 2024-05-08T11:42:08Z

Without this change, even if the kas is contained in no_proxy config we would deploy the adhoc kubernetes-default-proxy which would forward connections targetting kas pod through the proxy.

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes #

Checklist

Subject and description added to both, commit and PR.
Relevant issues have been referenced.
This change includes docs.
This change includes unit tests.

openshift-ci-robot · 2024-05-08T11:42:13Z

@enxebre: This pull request references Jira Issue OCPBUGS-33237, which is invalid:

expected the bug to target the "4.16.0" version, but no target version was set

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

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

In response to this:

Without this change, even if the kas is contained in no_proxy config we would deploy the adhoc kubernetes-default-proxy which would forward connections targetting kas pod through the proxy.

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes #

Checklist

Subject and description added to both, commit and PR.

Relevant issues have been referenced.

This change includes docs.

This change includes unit tests.

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.

enxebre · 2024-05-08T11:42:32Z

/hold
To understand better this scenario setup before shipping

enxebre · 2024-05-08T11:43:14Z

/jira refresh

openshift-ci-robot · 2024-05-08T11:43:21Z

@enxebre: This pull request references Jira Issue OCPBUGS-33237, 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.16.0) matches configured target version for branch (4.16.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (jiezhao@redhat.com), skipping review request.

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.

openshift-ci · 2024-05-08T11:43:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

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~~ [enxebre]

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

bverschueren · 2024-05-08T12:18:10Z

hypershift-operator/controllers/nodepool/haproxy.go

+		for _, substr := range substrings {
+			if strings.Contains(s, substr) {
+				return true
+			}


The apiServerAddr can be an IP and spec.configuration.proxy.noProxy (ProxySpec) could be a CIDR/IP so that may need to be checked as well.

netlify · 2024-05-08T14:14:48Z

✅ Deploy Preview for hypershift-docs ready!

Name	Link
🔨 Latest commit	`f1504a3`
🔍 Latest deploy log	https://app.netlify.com/sites/hypershift-docs/deploys/663bac35608b3100084e2203
😎 Deploy Preview	https://deploy-preview-3999--hypershift-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

stevekuznetsov · 2024-05-08T14:53:12Z

hypershift-operator/controllers/nodepool/haproxy.go

 	}

 	machineConfig := manifests.MachineConfigAPIServerHAProxy()
 	ignition.SetMachineConfigLabels(machineConfig)
-	serializedConfig, err := apiServerProxyConfig(haProxyImage, controlPlaneOperatorImage, apiServerExternalAddress, apiServerInternalAddress, apiServerExternalPort, apiServerInternalPort, apiserverProxy)
+	serializedConfig, err := apiServerProxyConfig(haProxyImage, controlPlaneOperatorImage, apiServerExternalAddress, apiServerInternalAddress, apiServerExternalPort, apiServerInternalPort, apiserverProxy, noProxy, hcluster.Spec.Networking.ServiceNetwork[0].CIDR.String(), hcluster.Spec.Networking.ClusterNetwork[0].CIDR.String())


Where are we checking that hcluster.Spec.Networking.ServiceNetwork is not zero-length?

This is defaulted in the API, I can still add a safety check

stevekuznetsov · 2024-05-08T14:54:38Z

hypershift-operator/controllers/nodepool/haproxy.go

-	if proxyAddr == "" {
+
+	// Check if no proxy contains any address that should result in skipping the system proxy
+	containsSubstring := func(s string, substrings ...string) bool {


Can we use slices.ContainsFunc()?

stevekuznetsov · 2024-05-08T14:56:08Z

...APIServerHAProxyConfig_When_noproxy_matches_internalAddress_it_should_create_an_haproxy.yaml

@@ -0,0 +1 @@
+zz_fixture_TestAPIServerHAProxyConfig.yaml


Something went wrong here, should be serializing the config as YAML

they are syslinks and I think that's how GH represent the change

Did they get generated from UPDATE=true go test ./... ? I never saw symlinked fixtures before

nvm symlink was causing troubles so I recreated then with test

Without this change, even if the kas is contained in no_proxy config we would deploy the adhoc kubernetes-default-proxy which would forward connections targetting kas pod through the proxy.

enxebre · 2024-05-08T17:35:40Z

/test e2e-aws

sjenning · 2024-05-08T20:44:51Z

/retest

stevekuznetsov · 2024-05-08T22:33:52Z

/lgtm

enxebre · 2024-05-09T07:15:47Z

/hold cancel

enxebre · 2024-05-09T07:41:00Z

/test e2e-azure

openshift-ci · 2024-05-09T10:24:26Z

@enxebre: 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-azure	`f1504a3`	link	false	`/test e2e-azure`

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-robot · 2024-05-09T10:28:52Z

@enxebre: Jira Issue OCPBUGS-33237: All pull requests linked via external trackers have merged:

openshift/hypershift#3999

Jira Issue OCPBUGS-33237 has been moved to the MODIFIED state.

In response to this:

Without this change, even if the kas is contained in no_proxy config we would deploy the adhoc kubernetes-default-proxy which would forward connections targetting kas pod through the proxy.

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes #

Checklist

Subject and description added to both, commit and PR.

Relevant issues have been referenced.

This change includes docs.

This change includes unit tests.

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.

celebdor · 2024-05-10T09:24:53Z

/cherry-pick release-4.15

openshift-cherrypick-robot · 2024-05-10T09:25:37Z

@celebdor: new pull request created: #4014

In response to this:

/cherry-pick release-4.15

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-merge-robot · 2024-05-14T16:57:40Z

Fix included in accepted release 4.16.0-0.nightly-2024-05-14-095225

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels May 8, 2024

openshift-ci bot added the do-not-merge/needs-area label May 8, 2024

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

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels May 8, 2024

openshift-ci bot requested review from csrwng and hasueki May 8, 2024 11:43

openshift-ci bot added area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release and removed do-not-merge/needs-area labels May 8, 2024

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

enxebre force-pushed the kas-skip-proxy branch from 8106c5e to 955fbc5 Compare May 8, 2024 12:00

bverschueren reviewed May 8, 2024

View reviewed changes

enxebre force-pushed the kas-skip-proxy branch from 955fbc5 to 2f7ad13 Compare May 8, 2024 14:13

enxebre force-pushed the kas-skip-proxy branch 2 times, most recently from d4dfca4 to 7b1327d Compare May 8, 2024 14:23

stevekuznetsov reviewed May 8, 2024

View reviewed changes

enxebre force-pushed the kas-skip-proxy branch 5 times, most recently from b7a6c03 to 909d70f Compare May 8, 2024 16:38

Add noproxy checks to run haproxy on dataplane and skip system proxy

f1504a3

Without this change, even if the kas is contained in no_proxy config we would deploy the adhoc kubernetes-default-proxy which would forward connections targetting kas pod through the proxy.

enxebre force-pushed the kas-skip-proxy branch from 909d70f to f1504a3 Compare May 8, 2024 16:45

openshift-ci bot assigned stevekuznetsov May 8, 2024

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

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 9, 2024

openshift-merge-bot bot merged commit 905708a into openshift:main May 9, 2024
12 of 13 checks passed

openshift-cherrypick-robot mentioned this pull request May 10, 2024

[release-4.15] OCPBUGS-33510: Run haproxy to connect to kas from data plane if noproxy settings contain kas #4014

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-33237: Run haproxy to connect to kas from data plane if noproxy settings contain kas #3999

OCPBUGS-33237: Run haproxy to connect to kas from data plane if noproxy settings contain kas #3999

enxebre commented May 8, 2024

openshift-ci-robot commented May 8, 2024

enxebre commented May 8, 2024

enxebre commented May 8, 2024

openshift-ci-robot commented May 8, 2024

openshift-ci bot commented May 8, 2024

bverschueren May 8, 2024

netlify bot commented May 8, 2024 •

edited

stevekuznetsov May 8, 2024

enxebre May 8, 2024

stevekuznetsov May 8, 2024

stevekuznetsov May 8, 2024

enxebre May 8, 2024

stevekuznetsov May 8, 2024 •

edited

enxebre May 8, 2024

enxebre commented May 8, 2024

sjenning commented May 8, 2024

stevekuznetsov commented May 8, 2024

enxebre commented May 9, 2024

enxebre commented May 9, 2024

openshift-ci bot commented May 9, 2024

openshift-ci-robot commented May 9, 2024

celebdor commented May 10, 2024

openshift-cherrypick-robot commented May 10, 2024

openshift-merge-robot commented May 14, 2024

		@@ -0,0 +1 @@
		zz_fixture_TestAPIServerHAProxyConfig.yaml

OCPBUGS-33237: Run haproxy to connect to kas from data plane if noproxy settings contain kas #3999

OCPBUGS-33237: Run haproxy to connect to kas from data plane if noproxy settings contain kas #3999

Conversation

enxebre commented May 8, 2024

openshift-ci-robot commented May 8, 2024

enxebre commented May 8, 2024

enxebre commented May 8, 2024

openshift-ci-robot commented May 8, 2024

openshift-ci bot commented May 8, 2024

bverschueren May 8, 2024

Choose a reason for hiding this comment

netlify bot commented May 8, 2024 • edited

✅ Deploy Preview for hypershift-docs ready!

stevekuznetsov May 8, 2024

Choose a reason for hiding this comment

enxebre May 8, 2024

Choose a reason for hiding this comment

stevekuznetsov May 8, 2024

Choose a reason for hiding this comment

stevekuznetsov May 8, 2024

Choose a reason for hiding this comment

enxebre May 8, 2024

Choose a reason for hiding this comment

stevekuznetsov May 8, 2024 • edited

Choose a reason for hiding this comment

enxebre May 8, 2024

Choose a reason for hiding this comment

enxebre commented May 8, 2024

sjenning commented May 8, 2024

stevekuznetsov commented May 8, 2024

enxebre commented May 9, 2024

enxebre commented May 9, 2024

openshift-ci bot commented May 9, 2024

openshift-ci-robot commented May 9, 2024

celebdor commented May 10, 2024

openshift-cherrypick-robot commented May 10, 2024

openshift-merge-robot commented May 14, 2024

netlify bot commented May 8, 2024 •

edited

stevekuznetsov May 8, 2024 •

edited