Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HOSTEDCP-1064: Add egress policy for private-router #2792

Merged
merged 1 commit into from Aug 8, 2023

Conversation

muraee
Copy link
Contributor

@muraee muraee commented Jul 11, 2023

What this PR does / why we need it:
The private router should only be able to use the pod network to contact considered request serving components.

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 #HOSTEDCP-1064

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 openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 11, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jul 11, 2023

@muraee: This pull request references HOSTEDCP-1064 which is a valid jira issue.

In response to this:

What this PR does / why we need it:
The private router should only be able to use the pod network to contact considered request serving components.

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 #HOSTEDCP-1064

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

@openshift-ci openshift-ci bot requested review from csrwng and sjenning July 11, 2023 12:19
@openshift-ci 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 Jul 11, 2023
@openshift-merge-robot openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 12, 2023
@muraee
Copy link
Contributor Author

muraee commented Jul 17, 2023

/retest-required

2 similar comments
@muraee
Copy link
Contributor Author

muraee commented Jul 18, 2023

/retest-required

@muraee
Copy link
Contributor Author

muraee commented Jul 19, 2023

/retest-required

@netlify
Copy link

netlify bot commented Jul 20, 2023

Deploy Preview for hypershift-docs ready!

Name Link
🔨 Latest commit 85815b7
🔍 Latest deploy log https://app.netlify.com/sites/hypershift-docs/deploys/64d0f4444823c20008c38e22
😎 Deploy Preview https://deploy-preview-2792--hypershift-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@muraee muraee force-pushed the private-router-egress branch 2 times, most recently from 1f98c4e to ec3053b Compare July 21, 2023 08:46
@muraee
Copy link
Contributor Author

muraee commented Jul 24, 2023

/retest-required

5 similar comments
@muraee
Copy link
Contributor Author

muraee commented Jul 26, 2023

/retest-required

@muraee
Copy link
Contributor Author

muraee commented Jul 26, 2023

/retest-required

@muraee
Copy link
Contributor Author

muraee commented Jul 28, 2023

/retest-required

@muraee
Copy link
Contributor Author

muraee commented Jul 31, 2023

/retest-required

@muraee
Copy link
Contributor Author

muraee commented Aug 1, 2023

/retest-required

@muraee
Copy link
Contributor Author

muraee commented Aug 2, 2023

/test e2e-kubevirt-aws-ovn

@enxebre
Copy link
Member

enxebre commented Aug 3, 2023

can we please add a t.run("EnsureEgressTrafficForPrivateRouter") within EnsureNetworkPolicies
And have positive/negative validation as in

stdOut, err := RunCommandInPod(ctx, c, "cluster-version-operator", hcpNamespace, command, "cluster-version-operator")
g.Expect(err).To(HaveOccurred())
// Expect curl to timeout https://curl.se/docs/manpage.html (exit code 28).
if err != nil && !strings.Contains(err.Error(), "command terminated with exit code 28") {
t.Errorf("cluster version pod was unexpectedly allowed to reach the management KAS. stdOut: %s. stdErr: %s", stdOut, err.Error())
}
stdOut, err = RunCommandInPod(ctx, c, "cluster-api", hcpNamespace, command, "manager")

lgtm otherwise

@openshift-ci openshift-ci bot added the area/testing Indicates the PR includes changes for e2e testing label Aug 4, 2023
@enxebre
Copy link
Member

enxebre commented Aug 4, 2023

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 4, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre, muraee

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:

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 Aug 4, 2023
@muraee
Copy link
Contributor Author

muraee commented Aug 7, 2023

/retest-required

}

// Allow to any destination not on the management cluster service network
policy.Spec.Egress = []networkingv1.NetworkPolicyEgressRule{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we cover IPv6 also in this PR? Could be done in a follow up one? same as we do in

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather we merged this as it is now, because it is needed for the ROSA security hardening epic.
We can have a follow-up for disconnected/ipv6 if it is needed

@jparrill
Copy link
Contributor

jparrill commented Aug 8, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 8, 2023
@muraee
Copy link
Contributor Author

muraee commented Aug 8, 2023

/retest-required

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 8, 2023

@muraee: 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/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit e425afe into openshift:main Aug 8, 2023
12 checks passed
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. area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release area/testing Indicates the PR includes changes for e2e testing jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants