initial ClusterTrustBundle v1alpha1 support #55592
Conversation
0811538 to
7951ec4
Compare
manifests/charts/istio-control/istio-discovery/files/gateway-injection-template.yaml
Outdated
Show resolved
Hide resolved
| - apiGroups: ["certificates.k8s.io"] | ||
| resources: | ||
| - "clustertrustbundles" | ||
| verbs: ["update", "create", "delete"] |
There was a problem hiding this comment.
can we scope to a single name (istio-ca-root-cert)? I think it should work but the per-name permissions are a little wonky in k8s so not certain
There was a problem hiding this comment.
turns out just using resourceName does not work, but it works in conjunction with signerName. I am using that now
| projected: | ||
| sources: | ||
| - clusterTrustBundle: | ||
| name: istio-ca-root-cert |
There was a problem hiding this comment.
I am not sure the answer to this: are there plausible use cases where a user may deploy a cert + a trust bundle, and Istio just consumes that and doesn't create it?
Seems like 'maybe' and we can add that later without issue.
There was a problem hiding this comment.
I agree with 'maybe' being the answer here. For now, this only covers the simplest use cases where a) the internal CA is used with a self-signed cert generated by istiod or b) with a cert passed into istiod through the cacerts secret. In both cases, istiod will manage the ClusterTrustBundle. We might want to keep it that way to keep things simple - ie if you use an external CA to manage the cert/key, that only has to update the cacerts secret and not bother with ClusterTrustBundle
| @@ -183,8 +183,16 @@ spec: | |||
| expirationSeconds: 43200 | |||
| audience: istio-ca | |||
| - name: istiod-ca-cert | |||
| {{- if (.Values.env).ENABLE_CLUSTER_TRUST_BUNDLE_API }} | |||
There was a problem hiding this comment.
nit: the API here is a bit awkward since we enable it by setting an env var on the Ztunnel daemon, but ztunnel doesn't actually read the env var at all.
Given its for experimental enablement its not too bad, we are effectively making up for the fact we don't have a first class "feature flag" concept
There was a problem hiding this comment.
yeah, I didn't want to add a global helm value for this as it's really not something we want to keep around anyway. honestly I would really appreciate first class feature flags, this stuff is always a bit hacky. As soon as a Pilot experimental flag needs to be known about by e.g. the gateways things become weird. Maybe we can pick up that discussion again
| }, | ||
| } | ||
|
|
||
| existing, err := c.client.Kube().CertificatesV1alpha1().ClusterTrustBundles().Get(context.Background(), istioClusterTrustBundleName, metav1.GetOptions{}) |
There was a problem hiding this comment.
Typically we would read from the informer cache here. Given this is so low-traffic this isn't too important here, but given we do that in every other part of Istio its probably best for consistency (technically this approach is slightly less race-conditiony, but both are, so we need to be eventually consistent anyways)
There was a problem hiding this comment.
switched to informer cache
| existing, err := c.client.Kube().CertificatesV1alpha1().ClusterTrustBundles().Get(context.Background(), istioClusterTrustBundleName, metav1.GetOptions{}) | ||
| if err == nil { | ||
| // Update existing bundle | ||
| existing.Spec.TrustBundle = string(rootCert) |
There was a problem hiding this comment.
very nit: we could skip if its the same. For example, a new istiod on startup
There was a problem hiding this comment.
comparing and skipping if equal now
4cbb525 to
47bd4fb
Compare
releasenotes/notes/43986.yaml
Outdated
| - 43986 | ||
| releaseNotes: | ||
| - | | ||
| **Added** experimental support for the v1alpha1 ClusterTrustBundle API. This can be enabled by setting `values.pilot.env.ENABLE_CLUSTER_TRUST_BUNDLE_API=true`. Note that you will have to make sure to also enable the respective feature gates in your cluster, see KEP-3257 (https://github.com/kubernetes/enhancements/tree/master/keps/sig-auth/3257-cluster-trust-bundles) for details. |
There was a problem hiding this comment.
| **Added** experimental support for the v1alpha1 ClusterTrustBundle API. This can be enabled by setting `values.pilot.env.ENABLE_CLUSTER_TRUST_BUNDLE_API=true`. Note that you will have to make sure to also enable the respective feature gates in your cluster, see KEP-3257 (https://github.com/kubernetes/enhancements/tree/master/keps/sig-auth/3257-cluster-trust-bundles) for details. | |
| **Added** experimental support for the v1alpha1 `ClusterTrustBundle` API. This can be enabled by setting `values.pilot.env.ENABLE_CLUSTER_TRUST_BUNDLE_API=true`. Note that you will have to make sure to also enable the respective feature gates in your cluster, see [KEP-3257](https://github.com/kubernetes/enhancements/tree/master/keps/sig-auth/3257-cluster-trust-bundles) for details. |
3559b9d to
fd134cf
Compare
|
@howardjohn @dhawton, all comments have been addressed, PTAL |
This adds a feature flag to enable basic ClusterTrustBundle API support. If enabled, it replaces the old Namespace controller with a simpler ClusterTrustBundle controller.
fd134cf to
aee438a
Compare
|
@dgn this broke postsubmit since In the past we have done this via different configs in |
I'll look into it. For now, I can just remove the additional kind config, as there is no integration test for ClusterTrustBundles anyway. |
In istio#55592, the default prow config was changed to include support for ClusterTrustBundles by default, which breaks on k8s<v1.27. This moves the ClusterTrustBundle enablement into a separate config, as it's only used in manual tests anyway. We can use the separate config when we write an integration test.
In #55592, the default prow config was changed to include support for ClusterTrustBundles by default, which breaks on k8s<v1.27. This moves the ClusterTrustBundle enablement into a separate config, as it's only used in manual tests anyway. We can use the separate config when we write an integration test.
* Revamped Chart Structure since Istio Operator is deprecated now * Renamed cray-istio chart to cray-istio-ingress * Removed charts cray-istio-deploy and cray-istio-operator * Added charts cray-istio-base and cray-istio-pilot * Updated version tags and images * Updated LICENSES * Support setting namespaces on gateway charts (istio/istio#32675) * More consistent helm charts labels (istio/istio#52463) * By default, exclude gateways from ambient mesh enrollment (istio/istio#54825) * Upgrade docker-kubectl version * Modify rbac and change jwtPolicy * initial ClusterTrustBundle v1alpha1 support (istio/istio#55592) * Update Image and App version to 1.26.0 * Add a check before accessing the Values * Update chart values to comply with upstream
|
@dgn this required more work on rbac? |
This adds a feature flag to enable basic ClusterTrustBundle API support. If enabled, it replaces the old Namespace controller with a simpler ClusterTrustBundle controller.
Fixes #43986