add PublicViewer support

[filariow]

linked to codeready-toolchain/api#410

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Needs approval from an approver in each of these files:

• OWNERS

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

[mfrancisc]

Thanks for creating the PRs for the public-viewer effort 🙏

I have few comments.

[mfrancisc]

I'm wondering if we should pass this here or read it at "Reconcile" time?

In this way any change to this config would need a restart of the operator. I don't think there would be many changes but just something to consider.

[MatousJobanek]

definitely at the reconcile time - we shouldn't need to restart the container when we change the configuration.
There is also a logic that wraps the ToolchainConfig, does some caching and provides the default values if nothing is set.

[filariow]

@MatousJobanek, just to be sure, like what's done here?

out of curiosity - why not doing this with k8s volume mounting and just fetch the data from filesystem?

[mfrancisc]

like what's done here?

yes that's one example of how to fetch the config/default values

[mfrancisc]

minor - but should we create a test for

Config: toolchainv1alpha1.PublicViewerConfig{
				Enabled:  true,
				Username: "",
			},

when the Username is empty ? WDYT ?

[filariow]

good catch. What would be here the expected behavior? panic?
To prevent this from happening I'd propose to have some default and validation, like follows

// PublicViewer used to enable public-viewer support
// +k8s:openapi-gen=true
type PublicViewerConfig struct {
	// +kubebuilder:default:=false
	Enabled bool `json:"enabled"`

	// +optional
+	// +kubebuilder:validation:MinLength=4
	// +kubebuilder:validation:MaxLength=63
	// +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`
+	// +kubebuilder:default:=public-viewer
	Username string `json:"username,omitempty"`
}

[filariow]

WDYT?

[mfrancisc]

TBH maybe we could remove the Enabled field and just use the username, if provided then the feature is enabled. This will reduce the amount of permutations to check/test. Would also simplify a bit the configuration.

[mfrancisc]

The default validation using kubebuilder markers works fine, but also just checking that the len(username) > 0 , in case it's empty we can assume that the feature is disabled.

[sadlerap]

I'd like to have validation where possible, but if we want to deduce enablement from the Username field only, then we're going to have to change these validation rules.

Maybe change the validation pattern to this?

// +kubebuilder:validation:Pattern=`^([a-z0-9]([-a-z0-9]*[a-z0-9])?)?$`

This will make the empty string valid, but still validate that usernames must be shaped like a username if they're set.

[mfrancisc]

I haven't checked the other PRs yet ( member, reg-serv) but I'm not sure I'm following here, how is "public-viewer" getting access to the Spaces, isn't it through the usage of SpaceBindingRequests?

I see you are blocking SBRs for "public-viewer" here 🤔 , but I'm sure I'm missing something.

[filariow]

the actual implementation relies on the direct creation of a SpaceBinding on the Host cluster.
The check introduced here, is used to avoid users granted the permission to share the workspace to make the workspace publicly visible by creating SBRs for the public-viewer user

[sonarcloud]

Quality Gate failed

Failed conditions
21.7% Duplication on New Code (required ≤ 20%)

See analysis details on SonarCloud

[openshift-merge-robot]

PR needs rebase.

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.

[codecov]

Codecov Report

Merging #998 (84e431d) into master (de95481) will increase coverage by 0.02%.
Report is 4 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #998      +/-   ##
==========================================
+ Coverage   84.87%   84.90%   +0.02%     
==========================================
  Files          55       55              
  Lines        5066     5080      +14     
==========================================
+ Hits         4300     4313      +13     
- Misses        583      584       +1     
  Partials      183      183              

Files	Coverage Δ
...ebindingcleanup/spacebinding_cleanup_controller.go	75.53% <100.00%> (+0.80%)	⬆️
...cebindingrequest/spacebindingrequest_controller.go	88.16% <100.00%> (+0.32%)	⬆️
controllers/toolchainconfig/configuration.go	92.30% <100.00%> (+0.20%)	⬆️

... and 1 file with indirect coverage changes