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

Allow modification of the hostNetwork parameter to avoid the need of … #1046

Merged
merged 7 commits into from
Aug 30, 2024

Conversation

shuyanl-qualtrics
Copy link
Contributor

Allow modification of the hostNetwork parameter to avoid the need of a dedicated pod ip.
We would like our daemonsets to run in the hostNetwork so to avoid needing allocate IP for them.
Adding hostNetwork to this template allow us to do this.

@shuyanl-qualtrics shuyanl-qualtrics requested a review from a team as a code owner July 25, 2024 05:38
Copy link

hashicorp-cla-app bot commented Jul 25, 2024

CLA assistant check
All committers have signed the CLA.

@shuyanl-qualtrics
Copy link
Contributor Author

Is there any chance this PR can be reviewed by owner?

Copy link
Member

@tvoran tvoran left a comment

Choose a reason for hiding this comment

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

Thanks for bringing this up!

In general when adding a new parameter it's best to:

  • Set the default in values.yaml, and update values.schema.json
  • Add some helm template tests with the parameter set and unset (in test/unit/csi-daemonset.bats in this case)

templates/csi-daemonset.yaml Outdated Show resolved Hide resolved
@shuyanl-qualtrics
Copy link
Contributor Author

shuyanl-qualtrics commented Aug 14, 2024

@tvoran Thanks for the review all comment fixed. Please help take a look again.

Can you also review similar PR for CSI-Driver : kubernetes-sigs/secrets-store-csi-driver#1581 Thank you
(also comments in that repo for a question regarding value update)

Copy link
Member

@tvoran tvoran left a comment

Choose a reason for hiding this comment

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

This is looking good, just a minor fix in the tests.

Comment on lines 543 to 545
local actual=$(helm template \
--show-only templates/csi-daemonset.yaml \
. | tee /dev/stderr |
Copy link
Member

Choose a reason for hiding this comment

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

I think these will need csi.enabled=true set since by default the csi daemonset is not deployed in the chart.

Suggested change
local actual=$(helm template \
--show-only templates/csi-daemonset.yaml \
. | tee /dev/stderr |
local actual=$(helm template \
--show-only templates/csi-daemonset.yaml \
--set 'csi.enabled=true' \
. | tee /dev/stderr |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 552 to 555
local actual=$(helm template \
--show-only templates/csi-daemonset.yaml \
--set 'csi.hostNetwork=true' \
. | tee /dev/stderr |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
local actual=$(helm template \
--show-only templates/csi-daemonset.yaml \
--set 'csi.hostNetwork=true' \
. | tee /dev/stderr |
local actual=$(helm template \
--show-only templates/csi-daemonset.yaml \
--set 'csi.enabled=true' \
--set 'csi.hostNetwork=true' \
. | tee /dev/stderr |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

values.yaml Outdated
@@ -1119,6 +1119,9 @@ csi:
# generating secret versions.
hmacSecretName: ""

#Allow modification of the hostNetwork parameter to avoid the need of a dedicated pod ip
Copy link
Member

Choose a reason for hiding this comment

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

nit: formatting

Suggested change
#Allow modification of the hostNetwork parameter to avoid the need of a dedicated pod ip
# Allow modification of the hostNetwork parameter to avoid the need of a
# dedicated pod ip

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 200 to 202
"hostNetwork": {
"type": "boolean"
},
Copy link
Member

Choose a reason for hiding this comment

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

nit: These are in alphabetical order, so it would be good to move this to be after hmacSecretName.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@tvoran
Copy link
Member

tvoran commented Aug 14, 2024

Can you also review similar PR for CSI-Driver : kubernetes-sigs/secrets-store-csi-driver#1581 Thank you (also comments in that repo for a question regarding value update)

Sorry, that chart is beyond my purview.

@shuyanl-qualtrics
Copy link
Contributor Author

all comments fixed @tvoran Please help take a look. Thank you

Copy link
Member

@tvoran tvoran left a comment

Choose a reason for hiding this comment

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

Thanks!

test/unit/csi-daemonset.bats Outdated Show resolved Hide resolved
values.yaml Outdated Show resolved Hide resolved
@tvoran tvoran merged commit efa3ec5 into hashicorp:main Aug 30, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants