add PublicViewer support #998
Conversation
Signed-off-by: Francesco Ilario <[email protected]>
Signed-off-by: Francesco Ilario <[email protected]>
Signed-off-by: Francesco Ilario <[email protected]>
Signed-off-by: Francesco Ilario <[email protected]>
Signed-off-by: Francesco Ilario <[email protected]>
Signed-off-by: Francesco Ilario <[email protected]>
Signed-off-by: Francesco Ilario <[email protected]>
Signed-off-by: Francesco Ilario <[email protected]>
filariow
requested review from
MatousJobanek,
xcoulon,
alexeykazakov,
rajivnathan,
ranakan19,
sbryzak and
mfrancisc
as code owners
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: filariow The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| Scheme: mgr.GetScheme(), | ||
| Namespace: namespace, | ||
| MemberClusters: clusterScopedMemberClusters, | ||
| PublicViewerConfig: crtConfig.PublicViewer(), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
like what's done here?
yes that's one example of how to fetch the config/default values
|
|
||
| redhatSpace := spacetest.NewSpace(test.HostOperatorNs, "redhat") | ||
| ibmSpace := spacetest.NewSpace(test.HostOperatorNs, "ibm") | ||
|
|
||
| laraMur := masteruserrecord.NewMasterUserRecord(t, "lara") | ||
| joeMur := masteruserrecord.NewMasterUserRecord(t, "joe") | ||
|
|
||
| t.Run("publicViewer config is enabled", func(t *testing.T) { |
There was a problem hiding this comment.
minor - but should we create a test for
Config: toolchainv1alpha1.PublicViewerConfig{
Enabled: true,
Username: "",
},
when the Username is empty ? WDYT ?
There was a problem hiding this comment.
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"`
}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| if r.PublicViewerConfig.IsPublicViewer(spaceBindingRequest.Spec.MasterUserRecord) { | ||
| // set ready condition on spaceBindingRequest | ||
| err = r.updateStatus(ctx, spaceBindingRequest, memberClusterWithSpaceBindingRequest, toolchainv1alpha1.Condition{ | ||
| Type: toolchainv1alpha1.ConditionReady, | ||
| Status: corev1.ConditionFalse, | ||
| Reason: toolchainv1alpha1.SpaceBindingRequestInvalidReason, | ||
| Message: fmt.Sprintf("%s is reserved and can not be used in SpaceBinding's MasterUserRecord", r.PublicViewerConfig.Username()), | ||
| }) | ||
| } else { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Signed-off-by: Francesco Ilario <[email protected]>
Signed-off-by: Francesco Ilario <[email protected]>
|
|
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 Report
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
|
linked to codeready-toolchain/api#410