feat: add portRangeUpperBound to be exposed in the helm chart #1469

cescribanohs · 2024-10-10T14:39:04Z

We have a large amount of pvc deploys in our cluster and often we reach the error Could not mount pvc due to available port

Even the port_range_upper_bound property has been increased recently

aws-efs-csi-driver/pkg/driver/efs_watch_dog.go

Line 74 in b0af6bc

# Define the port range that the TLS tunnel will choose from

still we need this number to be configurable as FIPS it is at the moment.

For this reason, adding a new helm value that can instantiated from a kustomize.config.k8s.io/v1beta1 component it is our goal.

What testing is done?

We have deployed in our dev environment through argoCD

linux-foundation-easycla · 2024-10-10T14:39:09Z

The committers listed above are authorized under a signed CLA.

✅ login: cescribanohs / name: cescribanohs (63247a9)

k8s-ci-robot · 2024-10-10T14:39:14Z

Welcome @cescribanohs!

It looks like this is your first PR to kubernetes-sigs/aws-efs-csi-driver 🎉. 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/aws-efs-csi-driver 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 · 2024-10-10T14:39:15Z

Hi @cescribanohs. 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.

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.

avanish23

Add check to ensure PORT_RANGE_UPPER_BOUND is greater than 20049.

Mostly the PR looks good.
Thanks!

avanish23 · 2024-10-12T03:28:43Z

pkg/driver/efs_watch_dog.go

@@ -284,7 +285,11 @@ func (w *execWatchdog) updateConfig(efsClientSource string) error {
 	// used on Fargate, IMDS queries suffice otherwise
 	region := os.Getenv("AWS_DEFAULT_REGION")
 	fipsEnabled := os.Getenv("FIPS_ENABLED")
-	efsCfg := efsUtilsConfig{EfsClientSource: efsClientSource, Region: region, FipsEnabled: fipsEnabled}
+	portRangeUpperBound, found := os.LookupEnv("PORT_RANGE_UPPER_BOUND")


You don't have to check the condition here, you can assign the portRangeUpperBound as you have already added it under env in the deployment helm charts.
portRangeUpperBound := os.Getenv("PORT_RANGE_UPPER_BOUND")

Also, one check should be added to ensure that portRangeUpperBound is always greater than portRangeLowerBound(20049).

Let me know if there is something else or just waiting you resolve the conversation and provide the approval.

Tagging @mskanth972 to help with approval.
Thanks!

Hi all, when do you think it can be approved? Many thanks

mskanth972 · 2024-10-21T20:07:48Z

/ok-to-test

cescribanohs · 2024-10-28T18:03:24Z

/ok-to-test

All test are passing right now. Ty

cescribanohs · 2024-11-01T10:00:26Z

Hi, any approvals or comments are welcomed. Thank you

mskanth972 · 2024-11-06T01:30:23Z

Don't we need to update node-daemonset yaml file?
I did not test this but is it necessary to update both Controller and Daemonset files to include the PORT_RANGE_UPPER_BOUND and also did you get chance to test this?

cescribanohs · 2024-11-07T09:57:06Z

Don't we need to update node-daemonset yaml file? I did not test this but is it necessary to update both Controller and Daemonset files to include the PORT_RANGE_UPPER_BOUND and also did you get chance to test this?

I have updated both daemonset and deployment but in this case they will pick the default value https://github.com/kubernetes-sigs/aws-efs-csi-driver/pull/1469/files#diff-5f5292175c0b29881ee742f0daf838980f771e03b22525cc574b4763b749a0ffR291 We have pushed this changes mapping a new image instead of repo: https://kubernetes-sigs.github.io/aws-efs-csi-driver/ in our dev cluster and working ok. Do you have in mind any other tests to be performed? Thank you in advance

cescribanohs · 2024-11-18T22:31:56Z

Any additional comments? Can we approve this please? Thank you in advance

mskanth972 · 2024-11-19T14:23:13Z

can you please squash the commits?

k8s-ci-robot · 2024-11-19T15:52:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cescribanohs
Once this PR has been reviewed and has the lgtm label, please assign justinsb for approval. 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:

OWNERS

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 · 2024-11-19T15:52:22Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cescribanohs
Once this PR has been reviewed and has the lgtm label, please assign justinsb for approval. 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:

OWNERS

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

feat: add portRangeUpperBound to be exposed in the helm chart feat: add portRangeUpperBound to be exposed in the helm chart

cescribanohs · 2024-11-19T18:27:25Z

can you please squash the commits?

Done. Thank you

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Oct 10, 2024

k8s-ci-robot requested review from justinsb and mskanth972 October 10, 2024 14:39

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 10, 2024

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 10, 2024

avanish23 reviewed Oct 12, 2024

View reviewed changes

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 14, 2024

cescribanohs force-pushed the add_portRangeUpperBound_helm_values branch from 61cd249 to b0fcad8 Compare November 19, 2024 15:33

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 19, 2024

cescribanohs closed this Nov 19, 2024

cescribanohs force-pushed the add_portRangeUpperBound_helm_values branch from b0fcad8 to a416c3d Compare November 19, 2024 15:35

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 19, 2024

cescribanohs reopened this Nov 19, 2024

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 19, 2024

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 19, 2024

cescribanohs force-pushed the add_portRangeUpperBound_helm_values branch from 0e62fa2 to 39d269f Compare November 19, 2024 16:54

cescribanohs force-pushed the add_portRangeUpperBound_helm_values branch from d6e2553 to 5069002 Compare November 19, 2024 17:26

chore: squash commits

63247a9

feat: add portRangeUpperBound to be exposed in the helm chart feat: add portRangeUpperBound to be exposed in the helm chart

cescribanohs force-pushed the add_portRangeUpperBound_helm_values branch from ab6dea3 to 63247a9 Compare November 19, 2024 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add portRangeUpperBound to be exposed in the helm chart #1469

feat: add portRangeUpperBound to be exposed in the helm chart #1469

cescribanohs commented Oct 10, 2024 •

edited

Loading

linux-foundation-easycla bot commented Oct 10, 2024 •

edited

Loading

k8s-ci-robot commented Oct 10, 2024

k8s-ci-robot commented Oct 10, 2024

avanish23 left a comment

avanish23 Oct 12, 2024 •

edited

Loading

cescribanohs Oct 15, 2024

avanish23 Oct 15, 2024

cescribanohs Oct 18, 2024

mskanth972 commented Oct 21, 2024

cescribanohs commented Oct 28, 2024

cescribanohs commented Nov 1, 2024

mskanth972 commented Nov 6, 2024

cescribanohs commented Nov 7, 2024

cescribanohs commented Nov 18, 2024

mskanth972 commented Nov 19, 2024

k8s-ci-robot commented Nov 19, 2024

k8s-ci-robot commented Nov 19, 2024

cescribanohs commented Nov 19, 2024

feat: add portRangeUpperBound to be exposed in the helm chart #1469

Are you sure you want to change the base?

feat: add portRangeUpperBound to be exposed in the helm chart #1469

Conversation

cescribanohs commented Oct 10, 2024 • edited Loading

linux-foundation-easycla bot commented Oct 10, 2024 • edited Loading

k8s-ci-robot commented Oct 10, 2024

k8s-ci-robot commented Oct 10, 2024

avanish23 left a comment

Choose a reason for hiding this comment

avanish23 Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

cescribanohs Oct 15, 2024

Choose a reason for hiding this comment

avanish23 Oct 15, 2024

Choose a reason for hiding this comment

cescribanohs Oct 18, 2024

Choose a reason for hiding this comment

mskanth972 commented Oct 21, 2024

cescribanohs commented Oct 28, 2024

cescribanohs commented Nov 1, 2024

mskanth972 commented Nov 6, 2024

cescribanohs commented Nov 7, 2024

cescribanohs commented Nov 18, 2024

mskanth972 commented Nov 19, 2024

k8s-ci-robot commented Nov 19, 2024

k8s-ci-robot commented Nov 19, 2024

cescribanohs commented Nov 19, 2024

cescribanohs commented Oct 10, 2024 •

edited

Loading

linux-foundation-easycla bot commented Oct 10, 2024 •

edited

Loading

avanish23 Oct 12, 2024 •

edited

Loading