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

feat: add kurl proxy resources to AdminConsole helm chart #32

Merged
merged 7 commits into from
Jan 12, 2024

Conversation

banjoh
Copy link
Member

@banjoh banjoh commented Jan 11, 2024

The kurl proxy allows generating self-signed certs and fronting Admin Console via an encrypted connection

Related to https://app.shortcut.com/replicated/story/94724

@banjoh banjoh marked this pull request as draft January 11, 2024 13:38
@banjoh banjoh marked this pull request as ready for review January 11, 2024 18:51
@@ -1,3 +1,4 @@
{{ if not .Values.kurlProxy.enabled }}
Copy link
Contributor

@cbodonnell cbodonnell Jan 11, 2024

Choose a reason for hiding this comment

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

could this break existing integrations that might rely on this admin-console service being in the cluster? as I understand it, kurl-proxy sits in front of the kotsadm service and proxies requests to it in-cluster, so we might need it either way

Copy link
Member

Choose a reason for hiding this comment

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

Nothing besides embedded-cluster uses this helm chart, and we don't rely on that service existing

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add a service.enabled field which to allow embedded-cluster to explicitly disable the service, and also remove this dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, this makes sense. i think i was mixing this service up with the internal kotsadm service.

- name: NODE_PORT
value: "8800"
- name: UPSTREAM_ORIGIN
value: http://kotsadm:3000
Copy link
Contributor

Choose a reason for hiding this comment

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

will kotsadm resolve to anything? the upstream service would be admin-console, which differs from the normal kotsadm service (not sure why)

Copy link
Member

@laverya laverya Jan 12, 2024

Choose a reason for hiding this comment

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

We fixed that thankfully! There is a kotsadm service here by default now

https://github.com/replicatedhq/kots-helm/blob/main/templates/kotsadm-internal-service.yaml

- name: UPSTREAM_ORIGIN
value: http://kotsadm:3000
- name: DEX_UPSTREAM_ORIGIN
value: http://kotsadm-dex:5556
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar question to the above. Also, dex is not even something that is shipped with this Helm chart, so i'm not sure this would ever come into play?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing...

imagePullPolicy: IfNotPresent
env:
- name: NODE_PORT
value: "8800"
Copy link
Contributor

Choose a reason for hiding this comment

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

does this need to be {{ .Values.kurlProxy.nodePort }}?

Copy link
Member

@laverya laverya Jan 12, 2024

Choose a reason for hiding this comment

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

Yes it does (if we don't want to make 8800 the nodeport, of course)

EDIT: actually it needs to be set to 8800! (or unset)
https://github.com/replicatedhq/kots/blob/6d660205e8f8cc8fc634f328ebf0c1b309b60c50/kurl_proxy/cmd/main.go#L123

Copy link
Member Author

@banjoh banjoh Jan 12, 2024

Choose a reason for hiding this comment

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

The variable name is misleading. It should be KURL_PROXY_TARGET_PORT or something similar. Its the port that kurl-proxy container listens to. I can add a values variable instead to better link it to its purpose, and in case, there is some rare occasion one would like to change it, they have a way to.

@banjoh banjoh merged commit 419f1f2 into main Jan 12, 2024
1 check passed
@banjoh banjoh deleted the em/sc-94724/kurl-proxy branch January 12, 2024 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants