fix: weight can not set to 0

[MregXN]

What type of PR is this?

/kind bug
What this PR does / why we need it:

Fixed the issue where canary weight cannot be set to 0.
During testing or debugging, it is common practice to temporarily set the weight of a specific backend to 0 but it is not supported currently in this project

Which issue(s) this PR fixes:

Fixes #136

Does this PR introduce a user-facing change?:

Add translation when canary-weight is set to 0

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: MregXN (e255011, 6d45ccd)

[k8s-ci-robot]

Welcome @MregXN!

It looks like this is your first PR to kubernetes-sigs/ingress2gateway 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/ingress2gateway has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @MregXN. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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

[LiorLieberman]

Thanks @MregXN !
/ok-to-test

[LiorLieberman]

looks good to me, @levikobi can you take a look?
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LiorLieberman, MregXN

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 [LiorLieberman]

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

[LiorLieberman]

@MregXN mind putting something meaningful in the release note?

[MregXN]

@MregXN mind putting something meaningful in the release note?

do you mean my commit message?

[levikobi]

@LiorLieberman I'll play around with this change probably tomorrow
@MregXN in your main PR comment, there's a section Does this PR introduce a user-facing change? in which you wrote None - this is the release note

[levikobi]

btw thank you for this PR @MregXN!

[levikobi]

This looks good to me but I couldn't actually verify it. I tried to run it locally but it seems like we introduced a bug in ExtractObjectsFromReader (not related to this PR), since I can't use the tool with local files when running the nginx provider @LiorLieberman

[MregXN]

@LiorLieberman I'll play around with this change probably tomorrow @MregXN in your main PR comment, there's a section Does this PR introduce a user-facing change? in which you wrote None - this is the release note

Thanks. I have updated the release note.

[LiorLieberman]

This looks good to me but I couldn't actually verify it. I tried to run it locally but it seems like we introduced a bug in ExtractObjectsFromReader (not related to this PR), since I can't use the tool with local files when running the nginx provider @LiorLieberman

@levikobi whats the bug? can you elaborate?

[levikobi]

@LiorLieberman my mistake, it was ok, I configured the cli flags wrong.
@MregXN, can you please give me a concrete example (yaml files) of which this addition can help with?

@MregXN If I have a canary deployment and want to configure one of my backends with 0 weight, I can just set the other one to weigh 100. From reading the documentation, this is how you should drain one of the backends.
Please correct me if I'm wrong.

[MregXN]

@LiorLieberman my mistake, it was ok, I configured the cli flags wrong. @MregXN, can you please give me a concrete example (yaml files) of which this addition can help with?

@MregXN If I have a canary deployment and want to configure one of my backends with 0 weight, I can just set the other one to weigh 100. From reading the documentation, this is how you should drain one of the backends. Please correct me if I'm wrong.

The scenatio you mentioned is exactly the example what I am using. The yaml is like:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: ingress-main
  namespace: ingress-conformance-infra
  annotations:
spec:
  ingressClassName: test
  rules:
  - http:
      paths:
      - pathType: Prefix
        path: "/"
        backend:
          service:
            name: infra-backend-v1
            port:
              number: 8080
---
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: ingress-canary
  namespace: ingress-conformance-infra
  annotations:
    nginx.ingress.kubernetes.io/canary: "true"
    nginx.ingress.kubernetes.io/canary-weight: "0"
spec:
  ingressClassName: test
  rules:
  - http:
      paths:
      - pathType: Prefix
        path: "/"
        backend:
          service:
            name: infra-backend-v2
            port:
              number: 8080

If annotation nginx.ingress.kubernetes.io/canary-weight is set as 0 the translation will not be triggered. There will be 2 backends with the same hostname, path and nil pointer of weight，which will make the subsequent unpredictable.

And I dont think adding annotaion to another ingress is a friendly way. When setting weight to 0 (for just turnning off canary temporarily) I should modify 2 ingresses, however in other case I can modify the annotation of the current ingress easily.

[levikobi]

OK, I understand now. Can you please add an additional test case that checks this exact situation?
We have the main deployment without canary annotations and a canary deployment with a weight of 0 - I expect to have a weight of 100 on my main deployment (correct me if I'm wrong).

Once this additional test case is added and passed, I will happily accept this

[LiorLieberman]

@MregXN let us know if there is anything else we could help with to get this done.

[MregXN]

OK, I understand now. Can you please add an additional test case that checks this exact situation? We have the main deployment without canary annotations and a canary deployment with a weight of 0 - I expect to have a weight of 100 on my main deployment (correct me if I'm wrong).

Once this additional test case is added and passed, I will happily accept this

Thanks. Have added the test cases.

[MregXN]

@MregXN let us know if there is anything else we could help with to get this done.

Can you help to run the test?

[levikobi]

Thanks @MregXN
/lgtm
/cc @LiorLieberman