Skip to content

Fix external hostname for root shard#70

Closed
mjudeikis wants to merge 2 commits intokcp-dev:mainfrom
mjudeikis:mjudeikis/fix.hostname
Closed

Fix external hostname for root shard#70
mjudeikis wants to merge 2 commits intokcp-dev:mainfrom
mjudeikis:mjudeikis/fix.hostname

Conversation

@mjudeikis
Copy link
Contributor

@mjudeikis mjudeikis commented Jul 6, 2025

Summary

After the change:

virtualWorkspaces:
    - url: https://alpha-shard-kcp.kcp-faros.svc.cluster.local:6443/services/apiexport/36nt823y7lbbeust/tenancy.faros.sh
    - url: https://root-shard-kcp.kcp-faros.svc.cluster.local:6443/services/apiexport/36nt823y7lbbeust/tenancy.faros.sh

What Type of PR Is This?

Related Issue(s)

Fixes ##69

Release Notes

Fix root shard URL setting

@kcp-ci-bot kcp-ci-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the DCO. labels Jul 6, 2025
@kcp-ci-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from mjudeikis. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kcp-ci-bot kcp-ci-bot added do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jul 6, 2025
@mjudeikis mjudeikis marked this pull request as draft July 6, 2025 17:03
@kcp-ci-bot kcp-ci-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 6, 2025
@mjudeikis mjudeikis marked this pull request as ready for review July 7, 2025 05:36
@kcp-ci-bot kcp-ci-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 7, 2025
@kcp-ci-bot
Copy link
Contributor

@mjudeikis: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kcp-operator-lint 03412d0 link true /test pull-kcp-operator-lint
pull-kcp-operator-test-e2e-canary 03412d0 link false /test pull-kcp-operator-test-e2e-canary

Full PR test history

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

clusterDomain = "cluster.local"
}

return fmt.Sprintf("%s-kcp.%s.svc.%s", r.Name, r.Namespace, clusterDomain)
Copy link
Member

Choose a reason for hiding this comment

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

This feels wrong. So far, e.g. in the Helm chart, we've set --external-hostname to the ... well, external hostname, i.e. the public DNS entry that points to the front-proxy. Now this is a purely cluster-internal name, and these URLs won't be useful to users (and in fact leak information like the namespace).

Copy link
Member

Choose a reason for hiding this comment

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

But I can also see from the PR description what you are trying to do. I'm confused now what this flag should be ... I wonder if we have a more general architecture problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once OIDC lands I will come back to this. Having a lot of testing in my env for these things.

@mjudeikis
Copy link
Contributor Author

I think this is wrong too now.

@mjudeikis mjudeikis closed this Jul 10, 2025
@mjudeikis mjudeikis deleted the mjudeikis/fix.hostname branch July 10, 2025 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dco-signoff: yes Indicates the PR's author has signed the DCO. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants