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

[bitnami/vault] fix injector #26611

Merged
merged 10 commits into from
Jun 10, 2024
Merged

[bitnami/vault] fix injector #26611

merged 10 commits into from
Jun 10, 2024

Conversation

SpeedGriffon
Copy link
Contributor

Description of the change

Fixes the Vault Injector Deployment and NetworkPolicy.

Benefits

Possible drawbacks

Applicable issues

Additional information

Checklist

  • Chart version bumped in Chart.yaml according to semver. This is not necessary when the changes only affect README.md files.
  • Variables are documented in the values.yaml and added to the README.md using readme-generator-for-helm
  • Title of the pull request follows this pattern [bitnami/<name_of_the_chart>] Descriptive title
  • All commits signed off and in agreement of Developer Certificate of Origin (DCO)

@github-actions github-actions bot added vault triage Triage is needed labels Jun 3, 2024
@github-actions github-actions bot requested a review from javsalgar June 3, 2024 17:57
Signed-off-by: Bitnami Containers <[email protected]>
@carrodher carrodher added verify Execute verification workflow for these changes in-progress labels Jun 3, 2024
@github-actions github-actions bot removed the triage Triage is needed label Jun 3, 2024
@github-actions github-actions bot removed the request for review from javsalgar June 3, 2024 22:27
@github-actions github-actions bot requested a review from fmulero June 3, 2024 22:27
carrodher and others added 4 commits June 4, 2024 12:06
Signed-off-by: Carlos Rodríguez Hernández <[email protected]>
Signed-off-by: Bitnami Containers <[email protected]>
Signed-off-by: Bitnami Containers <[email protected]>
Copy link
Collaborator

@fmulero fmulero left a comment

Choose a reason for hiding this comment

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

Hi @SpeedGriffon Thanks a lot for your contribution and for taking care of this issue.

I left a little comment. Could you take it glance?

@@ -105,7 +105,7 @@ spec:
- name: AGENT_INJECT_LISTEN
value: :{{ .Values.injector.containerPorts.https }}
- name: AGENT_INJECT_VAULT_ADDR
value: http://{{ include "vault.server.fullname" . }}:{{ .Values.server.service.general.ports.http }}
value: http://{{ include "vault.server.fullname" . }}.{{ include "common.names.namespace" . }}.svc:{{ .Values.server.service.general.ports.http }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why don't use the FQDN?

Suggested change
value: http://{{ include "vault.server.fullname" . }}.{{ include "common.names.namespace" . }}.svc:{{ .Values.server.service.general.ports.http }}
value: http://{{ include "vault.server.fullname" . }}.{{ include "common.names.namespace" . }}.svc.{{ .Values.clusterDomain }}:{{ .Values.server.service.general.ports.http }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with what they use in the official chart.
Also, this way it's consistent with AGENT_INJECT_TLS_AUTO_HOSTS and it works with the default values.yaml even if the cluster domain is not cluster.local.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks a lot @SpeedGriffon. LGTM

SpeedGriffon and others added 2 commits June 6, 2024 22:24
Signed-off-by: Jiří Matěják <[email protected]>
Signed-off-by: Bitnami Containers <[email protected]>
@@ -105,7 +105,7 @@ spec:
- name: AGENT_INJECT_LISTEN
value: :{{ .Values.injector.containerPorts.https }}
- name: AGENT_INJECT_VAULT_ADDR
value: http://{{ include "vault.server.fullname" . }}:{{ .Values.server.service.general.ports.http }}
value: http://{{ include "vault.server.fullname" . }}.{{ include "common.names.namespace" . }}.svc:{{ .Values.server.service.general.ports.http }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks a lot @SpeedGriffon. LGTM

@fmulero fmulero merged commit 28b0770 into bitnami:main Jun 10, 2024
8 checks passed
@SpeedGriffon SpeedGriffon deleted the fix-26570 branch June 10, 2024 16:31
longnbao pushed a commit to longnbao/bitnami-helm-charts that referenced this pull request Jun 12, 2024
* [bitnami/vault] fix injector networkpolicy ingress port

Signed-off-by: Jiří Matěják <[email protected]>

* [bitnami/vault] fix AGENT_INJECT_VAULT_ADDR

Signed-off-by: Jiří Matěják <[email protected]>

* [bitnami/vault] bump chart version

Signed-off-by: Jiří Matěják <[email protected]>

* Update CHANGELOG.md

Signed-off-by: Bitnami Containers <[email protected]>

* Update CHANGELOG.md

Signed-off-by: Bitnami Containers <[email protected]>

* Update CHANGELOG.md

Signed-off-by: Bitnami Containers <[email protected]>

* Update CHANGELOG.md

Signed-off-by: Bitnami Containers <[email protected]>

---------

Signed-off-by: Jiří Matěják <[email protected]>
Signed-off-by: Bitnami Containers <[email protected]>
Signed-off-by: Carlos Rodríguez Hernández <[email protected]>
Signed-off-by: Fran Mulero <[email protected]>
Co-authored-by: Bitnami Containers <[email protected]>
Co-authored-by: Carlos Rodríguez Hernández <[email protected]>
Co-authored-by: Fran Mulero <[email protected]>
Signed-off-by: Long Bao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solved vault verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bitnami/vault] injector does not work
5 participants