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(nginx): add an option to set static ip for clusterIP service #1623

Merged
merged 6 commits into from
Dec 5, 2023

Conversation

Adibov
Copy link
Contributor

@Adibov Adibov commented Oct 23, 2023

I needed to create a clusterIP service for nginx which has static IP and there was no option to do so, therefore I created this MR to solve this issue.

@Adibov Adibov force-pushed the nginx-service-static-ip branch from 18ef086 to be06e2d Compare October 23, 2023 09:54
@zyyw
Copy link
Collaborator

zyyw commented Nov 8, 2023

Thanks for contributing to harbor-helm! Please undo the change in this file:

  • Chart.yaml

BTW, also I see there is a large diff on README.md. It seems that you format it a little bit with only one new entry for expose.clusterIP.clusterIP, right?

@zyyw
Copy link
Collaborator

zyyw commented Nov 8, 2023

Why your env is static IP specifically?

@Adibov
Copy link
Contributor Author

Adibov commented Nov 8, 2023

Thanks for contributing to harbor-helm! Please undo the change in this file:

  • Chart.yaml

BTW, also I see there is a large diff on README.md. It seems that you format it a little bit with only one new entry for expose.clusterIP.clusterIP, right?

I roll-backed changes on Chart.yaml file. Also, you're right about the changes on README.md file. I only add documentation about expose.clusterIP.clusterIP field.

@Adibov
Copy link
Contributor Author

Adibov commented Nov 8, 2023

Why your env is static IP specifically?

Actually, we run self-hosted GitLab runners in a Kubernetes cluster in which we mounted the docker socket of all jobs' executors to their corresponding nodes. Therefore, we needed to access the harbor nginx service from nodes, and to achieve this functionality, we had to create a static IP service for harbor and a host alias in all nodes. (didn't want to expose the harbor by ingress or node port)

@Adibov Adibov force-pushed the nginx-service-static-ip branch 2 times, most recently from 6841405 to 4fbd450 Compare November 8, 2023 07:49
values.yaml Outdated Show resolved Hide resolved
@@ -13,6 +13,9 @@ metadata:
{{- end }}
spec:
type: ClusterIP
{{- if .Values.expose.clusterIP.staticClusterIP }}
clusterIP: {{ .Values.expose.clusterIP.clusterIP }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

line 17. it should also be .Values.expose.clusterIP.staticClusterIP I suppose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're totally right, Fixed.

@zyyw
Copy link
Collaborator

zyyw commented Nov 21, 2023

@Adibov , please rebase main branch code and resolve the conflicts in README.md file.
Thank you!

amirhesam.adibinia and others added 3 commits November 21, 2023 13:10
@Adibov Adibov force-pushed the nginx-service-static-ip branch from f353526 to 20ac847 Compare November 21, 2023 10:25
@Adibov Adibov force-pushed the nginx-service-static-ip branch from 20ac847 to e7172dd Compare November 21, 2023 10:25
@Adibov
Copy link
Contributor Author

Adibov commented Nov 21, 2023

@Adibov , please rebase main branch code and resolve the conflicts in README.md file. Thank you!

Rebased to the main branch and resolved conflicts. Any more comment would be welcomed :)

Copy link
Contributor

@wy65701436 wy65701436 left a comment

Choose a reason for hiding this comment

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

lgtm

@zyyw zyyw merged commit ae4d73a into goharbor:main Dec 5, 2023
6 checks passed
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