search

maistra / istio

Apache License 2.0
94 stars 92 forks source link

OSSM-5698 Rebase injection proxy UID/GID #1009

Closed mayleighnmyers closed 2 months ago

mayleighnmyers commented 3 months ago

[proxy-uid] OSSM-2292: Run sidecars with dynamically-determined user IDs (#689)

Squashed commit, incorporates changes from:

[maistra-2.4] OSSM-2324 Ensure ProxyUID and ProxyGID are never nil (#703)

If they are nil, Helm renders them as "nil" (with quotes), which the YAML parser can't parse as a float.

Please provide a description of this PR:

To help us figure out who should review this PR, please put an X in all the areas that this PR affects.

Please check any characteristics that apply to this pull request.

yxun commented 3 months ago

Would you like to close the previous #1004 PR ?

yxun commented 3 months ago

/test maistra-istio-lint-2-6

yxun commented 3 months ago

/test maistra-istio-unit-2-6

mayleighnmyers commented 3 months ago

/test maistra-istio-unit-2-6

jwendell commented 3 months ago

I wonder if this is necessary, since in Istio 1.20 we already have this feature on upstream...

yannuil commented 3 months ago

@jwendell are you talking about 056d019? It is only available since 1.21. Maistra-2.6 is based on 1.20.

jwendell commented 3 months ago

@yannuil https://github.com/istio/istio/pull/45394 and follow ups

jewertow commented 3 months ago

@jwendell I think we need this change, because your upstream implementation requires permissions for namespaces.

eoinfennessy commented 2 months ago

@jwendell I think we need this change, because your upstream implementation requires permissions for namespaces.

If I understand correctly, we should not be using the GetProxyIDs function defined in pkg/kube/inject/inject.go.

// GetProxyIDs returns the UID and GID to be used in the RunAsUser and RunAsGroup fields in the template
// Inspects the namespace metadata for hints and fallbacks to the usual value of 1337.
func GetProxyIDs(namespace *corev1.Namespace) (uid int64, gid int64) {
    uid = constants.DefaultProxyUIDInt
    gid = constants.DefaultProxyUIDInt
    if namespace == nil {
        return
    }
    // Check for OpenShift specifics and returns the max number in the range specified in the namespace annotation
    if _, uidMax, err := getPreallocatedUIDRange(namespace); err == nil {
        uid = *uidMax
    }
    if groups, err := getPreallocatedSupplementalGroups(namespace); err == nil && len(groups) > 0 {
        gid = groups[0].Max
    }
    return
}

If this is the case, the following lines (410-425) in the same file need to be changed to instead use the constants DefaultSidecarProxyUID and DefaultSidecarProxyGID:

    proxyUID, proxyGID := GetProxyIDs(params.namespace)

    data := SidecarTemplateData{
        TypeMeta:         params.typeMeta,
        DeploymentMeta:   params.deployMeta,
        ObjectMeta:       strippedPod.ObjectMeta,
        Spec:             strippedPod.Spec,
        ProxyConfig:      params.proxyConfig,
        MeshConfig:       meshConfig,
        Values:           params.valuesConfig.asMap,
        Revision:         params.revision,
        ProxyImage:       ProxyImage(params.valuesConfig.asStruct, params.proxyConfig.Image, strippedPod.Annotations),
        ProxyUID:         proxyUID,
        ProxyGID:         proxyGID,
        CompliancePolicy: common_features.CompliancePolicy,
    }
jwendell commented 2 months ago

@jewertow @eoinfennessy It's still not clear to me why we can't rely on upstream solution. Can you tell me more?

Is it because pilot reads namespace annotations and thus requires namespace reading permissions? Doesn't it already have this permission for all SMMR members? If not, can't we just add this permission?

eoinfennessy commented 2 months ago

@jewertow @eoinfennessy It's still not clear to me why we can't rely on upstream solution. Can you tell me more?

@jwendell, sorry, I'm also unsure why we can't use the changes that you contributed upstream, or what exactly the namespace permissions issue is.

jewertow commented 2 months ago

Doesn't it already have this permission for all SMMR members? If not, can't we just add this permission?

@jwendell no, we can't grant permissions for particular namespaces, because it's a cluster-scoped resource. You can grant access to all of them or none.

luksa commented 2 months ago

We actually can grant permissions to specific Namespace objects (using ClusterRole.rules.resourceNames), but I'm not sure if we should. We definitely can't grant write permissions, but even read permissions might be problematic in some cases. Users typically don't have access to the namespaces (not even read-only access). We should definitely consult with the security folks about this.

On a side note, if it turns out we can give read access to the namespaces, we can remove the locality controller (the one that copies Namespace labels to Pods).

Actually, if we can't give read access to the namespaces, we could extend the locality controller so that it also copies the userid range from the Namespace to the Pod so that the injection webhook could get the information from the Pod instead of the Namespace.

jewertow commented 2 months ago

@luksa thanks for correcting me.

I am still not sure that it's worth to do it, because we would have to implement additional logic in the operator to apply proper cluster role bindings on SMMR changes. Additionally, xns-informer knows only the namespace names, so we would have to change the namespace controller and all other components that rely on it.

luksa commented 2 months ago

@jewertow, ah, of course, the Namespace object is retrieved from the informer/lister, which means giving GET permissions only to specific Namespaces won't suffice, since the lister performs a LIST operation.

Anyway, in Sail, we don't have this problem, since istiod has the permission to list namespaces, so whatever solution we come up with, is only needed in 2.6 (and only in MultiTenant mode). And since we already have a solution for this in this PR, we might as well merge it.

dgn commented 2 months ago

Superseded by https://github.com/maistra/istio/pull/1019

jwendell commented 2 months ago

Safe to close this one, right?

@mayleighnmyers Thank you for your effort here!