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

Add translation for GKE Ssl Policy #195

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

sawsa307
Copy link
Contributor

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adding a new feature for GKE Ingress and Gateway conversion.

Which issue(s) this PR fixes:
Adding more feature for #87.

Does this PR introduce a user-facing change?:

Ingress2gateway now supports translating Ssl Policy on GKE Ingress to GCPGatewayPolicy on GKE Gateway.

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 17, 2024
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 17, 2024
@sawsa307
Copy link
Contributor Author

/assign @spencerhance
/assign @LiorLieberman

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 17, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sawsa307
Once this PR has been reviewed and has the lgtm label, please ask for approval from liorlieberman. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 19, 2024
@LiorLieberman
Copy link
Member

Hi, just checking in, anything I can do to help with this?

Ca this be reviewed by someone from GKE?
/cc @spencerhance @robscott

Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks @sawsa307! Mostly LGTM, just one nit.

@@ -883,6 +886,119 @@ func Test_convertToIR(t *testing.T) {
},
expectedErrors: field.ErrorList{},
},
{
name: "ingress with a Frontend Config specifying Ssl Policy",
ingresses: map[types.NamespacedName]*networkingv1.Ingress{
Copy link
Member

Choose a reason for hiding this comment

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

These kinds of tests can become really difficult to follow if you have to spell out the entirety of each manifest for each test case. I'd recommend moving towards a pattern where there's a base resource that each test case makes small modifications to. Here's an example of that pattern in Kubernetes: https://github.com/kubernetes/kubernetes/blob/80941e3e87d3909aa70e4f38f37b447f4376661f/pkg/apis/storage/validation/validation_test.go#L1741.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! Just updated based on the example.
For now I only make sure in each test case, we modify the input ingress based on a template ingress, and I'm still keeping the expected output in details to make sure we compare with to detailed outputs.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 28, 2024
* Each test case will based on the same Ingress and Service template and
  make modification on the resources.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants