🐛 give the front proxy a distinct config for direct(internal) shard communication. #2382

p0lyn0mial · 2022-11-18T13:01:12Z

Summary

The front proxy needs a direct and privileged way of authNZ with any shard. At the same time it needs to be able to validate an incoming certificate from a shard.

This PR allows the above by introducing a kubeconfig that holds a client certificate and a CA certificate for validating incoming certs.
The new kubeconfig can be passed via shards-kubeconfig flag.

Previously an incorrect configuration was used for communication with shards. As a result the proxy didn't work in a multi shard environment.

Related issue(s)

required by #342

fixes #2274

p0lyn0mial · 2022-11-18T13:02:44Z

cmd/sharded-test-server/frontproxy.go

+ }
+ kubeConfig.Clusters = map[string]*clientcmdapi.Cluster{
+ "base": {
+ CertificateAuthority: filepath.Join(workDirPath, ".kcp/serving-ca.crt"),


normally you would provide a server name here, we overwrite it for convenience

p0lyn0mial · 2022-11-18T13:04:08Z

pkg/proxy/config.go

+ c.ShardsConfig, err = clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
+ &clientcmd.ClientConfigLoadingRules{ExplicitPath: c.Options.ShardsKubeconfig},
+ // We override the Server here so that the user doesn't have to specify unused server value
+ // The Server must have HTTPS scheme otherwise CA won't be loaded (see IsConfigTransportTLS method)


alternatively we could provide certificates via separate flags and construct the config manually

That would probably be my preference, but I'm ok merging as-is and adjusting later if we decide to go that route, unless @sttts @stevekuznetsov prefer making the change now.

It would require at least 2 flags, one for the client cert pair and the other one for the public CA's certificate.

On the other hand kubeconfig has an advantage of specifying other means of authNZ, although I'm not sure we would ever use i.e. tokens.

+1 for merging as is.

p0lyn0mial · 2022-11-18T13:07:52Z

pkg/proxy/options/options.go

@@ -88,6 +90,9 @@ func (o *Options) Validate() []error {
 if o.MappingFile == "" {
 errs = append(errs, fmt.Errorf("--mapping-file is required"))
 }
+ if len(o.ShardsKubeconfig) == 0 {
+ errs = append(errs, fmt.Errorf("--shards-kubeconfig is required"))


the flag is now required

hardys · 2022-11-18T14:55:11Z

pkg/proxy/options/options.go

@@ -56,6 +57,7 @@ func (o *Options) AddFlags(fs *pflag.FlagSet) {
 fs.StringVar(&o.MappingFile, "mapping-file", o.MappingFile, "Config file mapping paths to backends")
 fs.StringVar(&o.RootDirectory, "root-directory", o.RootDirectory, "Root directory.")
 fs.StringVar(&o.RootKubeconfig, "root-kubeconfig", o.RootKubeconfig, "The path to the kubeconfig of the root shard.")
+ fs.StringVar(&o.ShardsKubeconfig, "shards-kubeconfig", o.ShardsKubeconfig, "The path to the kubeconfig used for communication with other shards.")


nit - AFAICS these credentials are used to create ClusterWorkspace informers for all shards (including root), so I'd s/other/all here

It's still a little confusing given we also require root-kubeconfig, but perhaps that can be deprecated/removed in a future iteration

yeah, i'm not sure. In general we need a privileged credentials for making wildcard requests against all shards. In addition to that we also need access to the root cluster for identities.

Not blocking on it, but as previously mentioned I'd adjust the description slightly e.g:

The path to the kubeconfig used for communication with all shards.

sorry, fixed :)

hardys · 2022-11-18T15:07:56Z

pkg/proxy/config.go

+ &clientcmd.ClientConfigLoadingRules{ExplicitPath: c.Options.ShardsKubeconfig},
+ // We override the Server here so that the user doesn't have to specify unused server value
+ // The Server must have HTTPS scheme otherwise CA won't be loaded (see IsConfigTransportTLS method)
+ &clientcmd.ConfigOverrides{ClusterInfo: clientcmdapi.Cluster{Server: "https://fakeserver.io"}}).ClientConfig()


The fakeserver override here looks strange, is that needed, or can we just use whatever is specified in the shard kubeconfig?

Oh I see now that the server is empty in the file, if it can't remain empty here would it perhaps be better to default it to the root-shard URL?

yes, please see #2382 (comment) for alternatives.

But even if we stick with the kubeconfig approach instead of the separate flags (which personally I think is fine, and probably easier if folks already have a cert based root-kubeconfig that allows access to all shards), we could switch the fakeserver.io to the root shard URL, right?

sure, we could do that. I think we could do that just to read the new config and then erase the host value.

I'd like to hear one more opinion on it. I'm not convinced we should change it to use the root shard URL.

I hope that's okay with you :)

sttts · 2022-12-01T08:28:49Z

pkg/proxy/options/options.go

@@ -56,6 +57,7 @@ func (o *Options) AddFlags(fs *pflag.FlagSet) {
 fs.StringVar(&o.MappingFile, "mapping-file", o.MappingFile, "Config file mapping paths to backends")
 fs.StringVar(&o.RootDirectory, "root-directory", o.RootDirectory, "Root directory.")
 fs.StringVar(&o.RootKubeconfig, "root-kubeconfig", o.RootKubeconfig, "The path to the kubeconfig of the root shard.")
+ fs.StringVar(&o.ShardsKubeconfig, "shards-kubeconfig", o.ShardsKubeconfig, "The path to the kubeconfig used for communication with all shards")


add that the server name is replaced later on with the shard hostname.

it holds a path to the kubeconfig which is used to communicate with shards

…proxy

sttts · 2022-12-01T08:38:34Z

/lgtm
/approve

openshift-ci · 2022-12-01T08:39:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/OWNERS~~ [sttts]
~~pkg/proxy/OWNERS~~ [sttts]

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

p0lyn0mial · 2022-12-01T08:56:30Z

/retest

openshift-ci bot requested review from csams and s-urbaniak November 18, 2022 13:01

p0lyn0mial requested review from sttts and hardys November 18, 2022 13:01

p0lyn0mial commented Nov 18, 2022

View reviewed changes

p0lyn0mial force-pushed the front-proxy-shards-kubeconfig branch from a8fc11d to 5adbfa6 Compare November 18, 2022 13:07

p0lyn0mial commented Nov 18, 2022

View reviewed changes

hardys reviewed Nov 18, 2022

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2022

hardys mentioned this pull request Nov 24, 2022

bug: e2e-sharded failed to list *v1alpha1.ClusterWorkspace: Unauthorized #2274

Closed

p0lyn0mial force-pushed the front-proxy-shards-kubeconfig branch from 5adbfa6 to a3a003b Compare November 29, 2022 08:32

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 29, 2022

p0lyn0mial force-pushed the front-proxy-shards-kubeconfig branch from a3a003b to 07eac6e Compare November 29, 2022 10:16

sttts reviewed Dec 1, 2022

View reviewed changes

p0lyn0mial added 3 commits December 1, 2022 09:34

proxy: add shards-kubeconfig flag

e7e0898

it holds a path to the kubeconfig which is used to communicate with shards

proxy: create rest.Config used to communicate with shards

0e345b2

sharded-test-server: prepare and pass shards-kubeconfig to the front …

9d1434c

…proxy

p0lyn0mial force-pushed the front-proxy-shards-kubeconfig branch from 07eac6e to 9d1434c Compare December 1, 2022 08:35

openshift-ci bot assigned sttts Dec 1, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 1, 2022

openshift-merge-robot merged commit b8e2f95 into kcp-dev:main Dec 1, 2022

hardys mentioned this pull request Dec 9, 2022

🌱 Update kcp version redhat-cps/front-proxy#6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 give the front proxy a distinct config for direct(internal) shard communication. #2382

🐛 give the front proxy a distinct config for direct(internal) shard communication. #2382

p0lyn0mial commented Nov 18, 2022 •

edited

Loading

p0lyn0mial Nov 18, 2022

p0lyn0mial Nov 18, 2022

ncdc Nov 18, 2022

p0lyn0mial Nov 21, 2022

sttts Dec 1, 2022

p0lyn0mial Nov 18, 2022

hardys Nov 18, 2022

p0lyn0mial Nov 21, 2022

hardys Nov 29, 2022

p0lyn0mial Nov 29, 2022

hardys Nov 18, 2022 •

edited

Loading

hardys Nov 18, 2022

p0lyn0mial Nov 21, 2022

hardys Nov 22, 2022

p0lyn0mial Nov 23, 2022

p0lyn0mial Nov 29, 2022

p0lyn0mial Nov 29, 2022

sttts Dec 1, 2022

sttts commented Dec 1, 2022

openshift-ci bot commented Dec 1, 2022

p0lyn0mial commented Dec 1, 2022

🐛 give the front proxy a distinct config for direct(internal) shard communication. #2382

🐛 give the front proxy a distinct config for direct(internal) shard communication. #2382

Conversation

p0lyn0mial commented Nov 18, 2022 • edited Loading

Summary

Related issue(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hardys Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Dec 1, 2022

openshift-ci bot commented Dec 1, 2022

p0lyn0mial commented Dec 1, 2022

p0lyn0mial commented Nov 18, 2022 •

edited

Loading

hardys Nov 18, 2022 •

edited

Loading