Skip to content

SEC-5054-adding custom headers#142

Open
sjgupta19 wants to merge 2 commits into
masterfrom
SEC-5054-adding-custom-headers
Open

SEC-5054-adding custom headers#142
sjgupta19 wants to merge 2 commits into
masterfrom
SEC-5054-adding-custom-headers

Conversation

@sjgupta19

@sjgupta19 sjgupta19 commented Nov 2, 2023

Copy link
Copy Markdown

@sjgupta19 sjgupta19 requested a review from a team November 2, 2023 17:59
annotations:
{{- if .Values.cloudArmor.backendConfig.iap }}
cloud.google.com/backend-config: '{"ports": {"80":"{{ include "common.name" . }}", "443":"{{ include "common.name" . }}"},"default": "{{ include "common.name" . }}"}'
cloud.google.com/backend-config: '{"ports": {"80":"{{ include "common.name" . }}", "443":"{{ include "common.name" . }}"},"default": "{{ include "common.name" . }}", "customRequestHeaders": {{ .Values.cloudArmor.backendConfig.customHeaders | toJson }}}'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@sjgupta19 I'm not sure toJson is a valid type conversion, Did you mean toYaml instead?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@TavoDave TavoDave Nov 2, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@sjgupta19 ic, Can you share the helm template --dry-run where you tested this? Just to make sure does not error out

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

hmm.. erroring out -- Error: file '/Users/srajangupta/Desktop/dave-github/charts/charts/common/templates/service-cloudarmor.yaml' seems to be a YAML file, but expected a gzipped archive

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

thats the same error I am getting without these changes. So I guess its unrelated to my changes?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I am curious if there is any other way we can add these custom headers? I am following this guide - https://cloud.google.com/load-balancing/docs/https/custom-headers-global

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

just bumped the chart version

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

here is the documentation on the annotation you are modifying, doesn't look like it supports that directive https://cloud.google.com/kubernetes-engine/docs/how-to/ingress-configuration

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

So do i have to specifically mention that in backendConfig? isn't .Values.cloudArmor.backendConfig.customHeaders this essentially adding it to backendConfig?

@vafied vafied left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you need to validate for .Values.cloudArmor.backendConfig.customHeaders to have a value or this line will be malformed

@sjgupta19

Copy link
Copy Markdown
Author

@vafied Validate as in check if it exists or not?

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.

3 participants