Make InitializerConfiguration per namespace level

[gyliu513]

Is this a BUG REPORT or FEATURE REQUEST?:

Uncomment only one, leave it on its own line:

/kind bug

/kind feature

What happened:
This is a issue from istio istio/istio.io#660

The current InitializerConfiguration is cluster level and all of the affected resources (deployment etc) will be adding the pending initializers, but if the initializers are down or the initializer does not clear the pending initializers, the resources will always hang.

It is better make the InitializerConfiguration namespace level.

/cc @caesarxuchao

/sig api-machinery

What you expected to happen:

How to reproduce it (as minimally and precisely as possible):

Anything else we need to know?:

Environment:

• Kubernetes version (use kubectl version):
• Cloud provider or hardware configuration**:
• OS (e.g. from /etc/os-release):
• Kernel (e.g. uname -a):
• Install tools:
• Others:

[dixudx]

@gyliu513 Well, I don't think making the InitializerConfiguration namespaced is a good idea. If so, we need to start multiple initializers per namespace, right? It is such a burden.

Also why not directly making your own initializer watching certain namespace, such as the namespace where the InitializerConfiguration is, and ignoring resources in other namespaces. This will be much simpler instead of changing Kubernetes.

Just as @ayj mentioned, istio/old_pilot_repo#1437 and excludeNamespace could possibly be enhanced to support your use case.

[gyliu513]

@dixudx I think it is the API server who added the pending initalizers to the kubernetes resources, so I was hoping that we can probably add a flag in InitializerConfiguration, such as

namespace: xxx

If namespace is configured, then InitializerConfiguration will be namespace level; if not configured, will still be global level. This can make the configuration more flexible.

[gyliu513]

@dixudx I think it is the API server who added the pending initalizers to the kubernetes resources, so I was hoping that we can probably add a new field in InitializerConfiguration, such as

namespace: xxx

If namespace is configured, then InitializerConfiguration will be namespace level; if not configured, will still be global level. This can make the configuration more flexible.

[gyliu513]

Of course I can update the initializer to handle such logic first, but I do hope that the initializer should be enhanced to support this as well.

[dixudx]

Since InitializerConfiguration does not contain any sensitive information and also we may not want anyone to alter it after applying, e.g. we want to enforce image pulling by adding related initializer controller. It will be redundant to make them namespaced.

If namespace is configured, then InitializerConfiguration will be namespace level; if not configured, will still be global level. This can make the configuration more flexible.

@gyliu513 Right. This will be better.

@ahmetb @caesarxuchao I am thinking whether we could add a new field like Namespaces: []string in Rule to indicate this.

kubernetes/staging/src/k8s.io/api/admissionregistration/v1alpha1/types.go

Lines 77 to 106 in 17a6115

	// Rule is a tuple of APIGroups, APIVersion, and Resources.It is recommended
	// to make sure that all the tuple expansions are valid.
	type Rule struct {
	// APIGroups is the API groups the resources belong to. '*' is all groups.
	// If '*' is present, the length of the slice must be one.
	// Required.
	APIGroups []string `json:"apiGroups,omitempty" protobuf:"bytes,1,rep,name=apiGroups"`
	// APIVersions is the API versions the resources belong to. '*' is all versions.
	// If '*' is present, the length of the slice must be one.
	// Required.
	APIVersions []string `json:"apiVersions,omitempty" protobuf:"bytes,2,rep,name=apiVersions"`
	// Resources is a list of resources this rule applies to.
	//
	// For example:
	// 'pods' means pods.
	// 'pods/log' means the log subresource of pods.
	// '*' means all resources, but not subresources.
	// 'pods/*' means all subresources of pods.
	// '*/scale' means all scale subresources.
	// '*/*' means all resources and their subresources.
	//
	// If wildcard is present, the validation rule will ensure resources do not
	// overlap with each other.
	//
	// Depending on the enclosing object, subresources might not be allowed.
	// Required.
	Resources []string `json:"resources,omitempty" protobuf:"bytes,3,rep,name=resources"`
	}

If this field is set, then only specific namespaces will apply the InitializerConfiguration. Otherwise, applying for all namespaces by default, just like what we are using right now.

WDYT?

[ahmetb]

This is a duplicate of #51290.

• I see that in Istio case we're looking at opt-in Namespaces for initialization. I'm curious if opt-out should also be possible (i.e. I don't want any initialization in kube-system or kube-addons namespaces. There's definitely use case for this, and in this case I probably shouldn't start namespaces field to all my initializerconfigurations). What do you think?

• I'm not sure if []string of Namespace names is a pattern currently discouraged by the API. A pattern in use for selecting namespaces is in the NetworkPolicy API. It has a NamespaceSelector field of type LabelSelector to do label-based selection. Arguably both options have merits, but I'm curious if the list of names has any disadvantage (why didn't NetworkPolicy API go with this)?

[dixudx]

I'm curious if opt-out should also be possible (i.e. I don't want any initialization in kube-system or kube-addons namespaces. There's definitely use case for this, and in this case I probably shouldn't start namespaces field to all my initializerconfigurations).

Well, to some extent, this could help solve the problem. But only these selected kube* namespaces. What if we expand the scope and do not want to apply initializers to other selected namespaces, not just kube* namespaces. When running a multi-tenant cluster, it'd be useful to apply some policies in just certain namespaces.

We should not add magic name exceptions for general purpose mechanisms and should strictly allow those system components to congregate in kube-system. I think we'd better to provider a more general way to incorporate this. WDYT?

I'm not sure if []string of Namespace names is a pattern currently discouraged by the API.

Yes, this may not be that efficiency and elegant.

A pattern in use for selecting namespaces is in the NetworkPolicy API. It has a NamespaceSelector field of type LabelSelector to do label-based selection. Arguably both options have merits, but I'm curious if the list of names has any disadvantage (why didn't NetworkPolicy API go with this)?

+1. This looks nicer and provides more powerful selecting rules, which may be the a great bonus for choosing this. Also this keeps consistent with NetworkPolicy.

Filed a PR #53879. @gyliu513 @ahmetb PTAL.

[ahmetb]

@dixudx I was just curious whether []string is a worse idea than NamespaceSelector or not, from an API design perspective.

I think almost no one labels their namespaces currently (or at least I've not seen in anywhere). That's why I don't find NetworkPolicy's design in this case particularly useful (i.e. I have to label existing namespaces like default to apply cross-NS policies with it.)

What do folks think about allowing both, but only one can be allowed at a time:

namespaces:
  namespaceSelector: (...LabelSelector)
  names: (...[]string)

[dixudx]

I was just curious whether []string is a worse idea than NamespaceSelector or not, from an API design perspective.

If using []string, we may need to list the namespaces exhaustively, which is time-consuming and error-prone. For new added namespace, a manual update to the initializer is required, which is not quite convenient and flexible.

I think almost no one labels their namespaces currently (or at least I've not seen in anywhere).

@ahmetb You're right. That's why I did not use labels when matching. Maybe we may need to add some validations to check whether the namespaceSelector only contains namespace field if my implementation is finalised.

!selector.Matches(labels.Set(map[string]string{"namespace": namespace}))

That's why I don't find NetworkPolicy's design in this case particularly useful (i.e. I have to label existing namespaces like default to apply cross-NS policies with it.)

With above implementation, we don't need to explicitly add new label namespace=xxx, which may be misconfigured to other value. Also that will bring too much restrictions and inconveniences on using initializers.

[cmluciano]

@ahmetb FWIW NetworkPolicy v1 discussion is here . IIRC the original thoughts were a special field on the actual Namespace object, but this was in clear violation of proper layering.

[cmluciano]

Also I agree that this is a duplicate of #51290 .

@dixudx @gyliu513 Can we condense thoughts of this issue in another comment on #51290 to eliminate issue sprawl?

[ahmetb]

Yeah I'm still curious if we can have a namespace selector with labels XOR well as name list (opt-out version of this) for the initializers. I think labels have merit, but in practice people are not labeling things.

@dixudx I didn't fully understand your explanation above. Can you give some concrete examples?

[gyliu513]

+1 to close this and move all the discussion there.

@dixudx can you please help post a summary of the discussion to that issue and close this one?

/assign @dixudx

[dixudx]

I think labels have merit, but in practice people are not labeling things.

@ahmetb Yes, you're right. So I did not read the namespace label from metadata.Labels when admitting initializerConfigurations. Instead I make such a selector labels.Set(map[string]string{"namespace": namespace}) when finding appropriate initializers.

And namespaceSelector in InitializerConfiguration can be set like this,

....
     namespaceSelector: namespace!=kube-system
...

[ahmetb]

@dixudx so is the NamespaceSelector field going to be of type LabelSelector? Are labels going to work as selectors in addition to your example?

[dixudx]

so is the NamespaceSelector field going to be of type LabelSelector?

Yes.

Are labels going to work as selectors in addition to your example?

metadata.Labels are not used/referenced in namespaceSelector.

[ahmetb]

@dixudx (still not sure if the answer to my question is yes or no 😄 ) My understanding is that LabelSelector helps you specify a namespace like:

apiVersion: v1
metadata:
  name: foo
  labels:
    purpose: test

using

namespaceSelector:
  purpose: test

If this is not working then the behavior of the namespaceSelector field is not compatible with NetworkPolicy API. I think it should be, if we're using the same field name, it should behave the same.

[dixudx]

@ahmetb The behavior of my namespaceSelector is not like this, since we only care about namespaces here, right? The namespace value is retrieved from attributeRecord when admitting checks for create requests to add initializers.

not compatible with NetworkPolicy API. I think it should be, if we're using the same field name, it should behave the same.

Well, we could make them compatible later if there is a consensus. But if so, does it mean we have to add a new label namespace: xxx in metadata.Labels? As you said, almost no one labels their namespaces currently.

[dixudx]

/close

move discussions to #51290.