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.

Sign up for GitHub

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

Jump to bottom

Add custom resource for egress nat policies #14998

Merged
merged 1 commit into from
Mar 24, 2021
Merged

Add custom resource for egress nat policies #14998

merged 1 commit into from
Mar 24, 2021

Conversation

blzhao-0
Copy link
Contributor

@blzhao-0 blzhao-0 commented Feb 16, 2021 •
edited by christarazi

Adds CiliumEgressNATPolicy crds for egress nat gateway configuration
apis. Specifically each policy consists of a list of endpoint selectors,
a list of destination cidrs and SNAT ip. This information
is used to update egress ebpf map in the datapath.

Signed-off-by: Bolun Zhao blzhao@google.com

Please ensure your pull request adheres to the following guidelines:

  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin

Related Issue: #13575

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 16, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Feb 16, 2021
@blzhao-0 blzhao-0 force-pushed the egress_control branch 3 times, most recently from a8f1d06 to 179580b Compare February 22, 2021 17:50
@blzhao-0 blzhao-0 marked this pull request as ready for review February 22, 2021 17:51
@blzhao-0 blzhao-0 requested review from a team as code owners February 22, 2021 17:51
@blzhao-0 blzhao-0 requested review from a team and nathanjsweet February 22, 2021 17:51
@christarazi christarazi self-requested a review February 24, 2021 00:08
christarazi
christarazi requested changes Feb 25, 2021
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

LGTM overall. A few comments to address below.

pkg/k8s/apis/cilium.io/v2/types.go Outdated Show resolved Hide resolved
pkg/k8s/apis/cilium.io/v2/types.go Outdated Show resolved Hide resolved
pkg/k8s/apis/cilium.io/v2/types.go Outdated Show resolved Hide resolved
pkg/k8s/apis/cilium.io/v2/types.go Outdated Show resolved Hide resolved
pkg/k8s/apis/cilium.io/v2/types.go Outdated Show resolved Hide resolved
@blzhao-0 blzhao-0 force-pushed the egress_control branch 3 times, most recently from 9adfcec to 9c3d259 Compare March 3, 2021 11:03
christarazi
christarazi reviewed Mar 3, 2021
View reviewed changes
pkg/k8s/apis/cilium.io/v2/types.go Outdated Show resolved Hide resolved
pkg/k8s/apis/cilium.io/v2/types.go Outdated Show resolved Hide resolved
@blzhao-0 blzhao-0 changed the title Add custom resource for egress gateway policies Add custom resource for egress nat policies Mar 3, 2021
@blzhao-0 blzhao-0 force-pushed the egress_control branch 2 times, most recently from 3b9eabc to 6c4eb8f Compare March 4, 2021 18:44
christarazi
christarazi reviewed Mar 4, 2021
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment •
edited

Choose a reason for hiding this comment

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

Looks good! Just one last minor item below. Also, please remove the release note section from the PR body since the release note will be taken from the title, as it is descriptive enough.

pkg/k8s/apis/cilium.io/v2/types.go Outdated Show resolved Hide resolved
@blzhao-0 blzhao-0 force-pushed the egress_control branch 2 times, most recently from 21dee39 to 526b06e Compare March 4, 2021 22:34
@anfernee anfernee mentioned this pull request Mar 8, 2021
Closed
10 tasks
@aanm
Copy link
Member

aanm commented Mar 10, 2021 •
edited

@MasterZ40 I've marked this PR as draft given that we discussed yesterday about the design doc. Feel free to mark it ready for review as you wish.

@aanm aanm marked this pull request as draft March 10, 2021 17:57
@pchaigno pchaigno added the release-note/misc This PR makes changes that have no direct user impact. label Mar 12, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 12, 2021
@pchaigno pchaigno added the sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. label Mar 12, 2021
@blzhao-0
Copy link
Contributor Author

@MasterZ40 I've marked this PR as draft given that we discussed yesterday about the design doc. Feel free to mark it ready for review as you wish.
@aanm thanks for letting me know. I think the discussion on Wednesday is mainly about the future CRD for mcc functionalities. I synced with Yuan who's the stakeholder of that feature and confirmed that this CRD change is not going to affect future design of other features. So as far as egress nat policy is concerned, we don't have any additional changes planned.
Let me know if you have other issues regarding this CRD in particular. I will make sure to update.

@blzhao-0 blzhao-0 marked this pull request as ready for review March 13, 2021 09:36
nathanjsweet
nathanjsweet requested changes Mar 18, 2021
View reviewed changes
Copy link
Member

@nathanjsweet nathanjsweet left a comment •
edited

Choose a reason for hiding this comment

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

Since this feature is not GA yet we need to change the version name.

examples/crds/ciliumegressnatpolicies.yaml Outdated Show resolved Hide resolved
@blzhao-0 blzhao-0 force-pushed the egress_control branch 2 times, most recently from 8a1dc43 to 07c9ff5 Compare March 19, 2021 09:12
christarazi
christarazi requested changes Mar 23, 2021
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

Looks good! Just one comment to try to address below and please squash the commits together. No need for them to be separate.

Makefile Outdated Show resolved Hide resolved
@anfernee
Copy link
Contributor

test-me-please

@blzhao-0
Add custom resource for egress nat gateway policy
42fad43 
Adds CiliumEgressNATPolicy crds for egress nat gateway configuration
apis. Specifically each policy consists of a list of endpoint selectors,
a list of destination cidrs and SNAT ip. This information
is used to update egress ebpf map in the datapath. The related api is
currently in v2alpha1.

Signed-off-by: Bolun Zhao <blzhao@google.com>
@christarazi
Copy link
Member

test-me-please

@blzhao-0
Copy link
Contributor Author

test-me-please

@pchaigno
Copy link
Member

I checked all team reviews are covered. Tests are passing. Marking as ready to merge.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 24, 2021
@brb brb merged commit 141cd63 into cilium:master Mar 24, 2021
1.10.0 automation moved this from In progress to Done Mar 24, 2021
@blzhao-0 blzhao-0 deleted the egress_control branch March 25, 2021 20:04
@joestringer joestringer moved this from Done to Merged Features in 1.10.0 Mar 29, 2021
@errordeveloper errordeveloper mentioned this pull request Mar 31, 2021
Merged
This was referenced Apr 28, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anfernee anfernee anfernee left review comments

@pchaigno pchaigno pchaigno left review comments

@aanm aanm aanm left review comments

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@christarazi christarazi christarazi approved these changes

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers.
Projects
No open projects
1.10.0
Merged Features
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@blzhao-0 @aanm @anfernee @christarazi @pchaigno @nathanjsweet @brb