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: Introduce Dedicated Istio Gateway Secret #2004

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

LeelaChacha
Copy link
Contributor

Closes #1890

@LeelaChacha LeelaChacha requested a review from a team as a code owner November 3, 2024 23:34
@kyma-bot kyma-bot added cla: yes Indicates the PR's author has signed the CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 3, 2024
@kyma-bot kyma-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 4, 2024
pkg/zerodw/gateway.go Outdated Show resolved Hide resolved
@@ -20,5 +20,5 @@ spec:
number: 443
protocol: HTTPS
tls:
credentialName: klm-watcher
credentialName: istio-gateway-secret
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you shortly explain why we have to change the credentailName here? also in this PR, you name it gateway-secret, and we previously have issue regarding alignment of resource name, #1721 (comment), please also have a reference to that decision for this renaming.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it make sense to have a short meeting to sync the decision, please don't hesitate bring it to team, you can sync this with @Tomasz-Smelcerz-SAP first.

Copy link
Contributor Author

@LeelaChacha LeelaChacha Nov 11, 2024

Choose a reason for hiding this comment

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

would klm-istio-gateway be more fitting then? Or klm-gatway?

I added the istio prefix since the last PR to make it more specific

Copy link
Contributor

@ruanxin ruanxin Nov 13, 2024

Choose a reason for hiding this comment

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

I would vote for klm-istio-gateway, it's an istio resource.

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'll use that for now, but let's discuss it briefly after a daily.

@@ -201,12 +204,21 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma
go cleanupStoredVersions(flagVar.DropCrdStoredVersionMap, mgr, setupLog)
go scheduleMetricsCleanup(kymaMetrics, flagVar.MetricsCleanupIntervalInMinutes, mgr, setupLog)

setupIstioGatewaySecretRotation(config, kcpClient, setupLog)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer put the goruntine declaration here so that aligns with above method and has a clear view that all of them are go rountine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was the first idea but a linter that check function length had a problem with that. should I move all go routines to a separate function?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates the PR's author has signed the CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: [Zero-Downtime] - safe migration
3 participants