istioctl: fix install with pilot volumes/volumeMounts/extraContainerArgs #51818
Conversation
zirain
added
cherrypick/release-1.21
istio-testing
added
the
size/L
|
/retest |
| env: | ||
| ENABLE_CA_SERVER: false | ||
| extraContainerArgs: | ||
| - --tlsCertFile=/etc/cert-manager/tls/tls.crt |
There was a problem hiding this comment.
Any reason to not use the build-in defaults ( by mounting the volume in the expected location ) ?
The '/var/run/secret' is preferred for consistency and because (some) tools know to skip it.
| - --tlsKeyFile=/etc/cert-manager/tls/tls.key | ||
| - --caCertFile=/etc/cert-manager/ca/root-cert.pem | ||
| volumeMounts: | ||
| - name: cert-manager |
There was a problem hiding this comment.
Doesn't the base chart already have the volumes for this (with optional mount) ?
I'm confused on what is the purpose for this file - nothing should need to be configured if the defaults are used.
There was a problem hiding this comment.
Fixing istioctl to allow volumes is ok - but please don't test or use the 'mounted' certs as an example.
For args - the majority of config is via env variables, which are well supported - it would be much better to fix the few remaining flags and stop having 3 ways to configure Istiod ( mesh config is the other). And really we mainly use env variables and CLI options as a way to introduce de-facto APIs but avoid the API design/UX/review ( because changes to MeshConfig and the api/ repo typically are strictly reviewed and require TOC approvals ).
|
@costinm the main purpose of this PR is make istioctl can work with cert-manager istio-csr, helm will work after #45517. without this, we need something like: apiVersion: install.istio.io/v1alpha1
kind: IstioOperator
metadata:
name: cert-manager-integration
namespace: istio-system
spec:
values:
global:
caAddress: cert-manager-istio-csr.cert-manager.svc:443
components:
pilot:
k8s:
env:
# Disable istiod CA Sever functionality
- name: ENABLE_CA_SERVER
value: "false"
overlays:
- apiVersion: apps/v1
kind: Deployment
name: istiod
patches:
# Mount istiod serving and webhook certificate from Secret mount
- path: spec.template.spec.containers.[name:discovery].args[7]
value: "--tlsCertFile=/etc/cert-manager/tls/tls.crt"
- path: spec.template.spec.containers.[name:discovery].args[8]
value: "--tlsKeyFile=/etc/cert-manager/tls/tls.key"
- path: spec.template.spec.containers.[name:discovery].args[9]
value: "--caCertFile=/etc/cert-manager/ca/root-cert.pem"
- path: spec.template.spec.containers.[name:discovery].volumeMounts[6]
value:
name: cert-manager
mountPath: "/etc/cert-manager/tls"
readOnly: true
- path: spec.template.spec.containers.[name:discovery].volumeMounts[7]
value:
name: ca-root-cert
mountPath: "/etc/cert-manager/ca"
readOnly: true
- path: spec.template.spec.volumes[6]
value:
name: cert-manager
secret:
secretName: istiod-tls
- path: spec.template.spec.volumes[7]
value:
name: ca-root-cert
configMap:
defaultMode: 420
name: istio-ca-root-cert |
|
I don't get it: the template already has
```
- name: istio-csr-dns-cert
secret:
secretName: istiod-tls
optional: true
# If this config map is present in istio-system - this will be used
for mesh cert
# verification and distributed in namespaces.
- name: istio-csr-ca-configmap
configMap:
name: istio-ca-root-cert
defaultMode: 420
optional: true
```
and the CLI parameters have default values that correspond to the mounts.
Why would you want to mount with different volume mounts and paths -
when the out-of-box install just works ?
…On Mon, Jul 1, 2024 at 5:11 PM zirain ***@***.***> wrote:
@costinm <https://github.com/costinm> the main purpose of this PR is make
istioctl can work with cert-manager istio-csr, helm will work after #45517
<#45517>.
without this, we need something like:
apiVersion: install.istio.io/v1alpha1
kind: IstioOperator
metadata:
name: cert-manager-integration
namespace: istio-system
spec:
values:
global:
caAddress: cert-manager-istio-csr.cert-manager.svc:443
components:
pilot:
k8s:
env:
# Disable istiod CA Sever functionality
- name: ENABLE_CA_SERVER
value: "false"
overlays:
- apiVersion: apps/v1
kind: Deployment
name: istiod
patches:
# Mount istiod serving and webhook certificate from Secret mount
- path: spec.template.spec.containers.[name:discovery].args[7]
value: "--tlsCertFile=/etc/cert-manager/tls/tls.crt"
- path: spec.template.spec.containers.[name:discovery].args[8]
value: "--tlsKeyFile=/etc/cert-manager/tls/tls.key"
- path: spec.template.spec.containers.[name:discovery].args[9]
value: "--caCertFile=/etc/cert-manager/ca/root-cert.pem"
- path: spec.template.spec.containers.[name:discovery].volumeMounts[6]
value:
name: cert-manager
mountPath: "/etc/cert-manager/tls"
readOnly: true
- path: spec.template.spec.containers.[name:discovery].volumeMounts[7]
value:
name: ca-root-cert
mountPath: "/etc/cert-manager/ca"
readOnly: true
- path: spec.template.spec.volumes[6]
value:
name: cert-manager
secret:
secretName: istiod-tls
- path: spec.template.spec.volumes[7]
value:
name: ca-root-cert
configMap:
defaultMode: 420
name: istio-ca-root-cert
cert-manager/istio-csr#113
<cert-manager/istio-csr#113>
—
Reply to this email directly, view it on GitHub
<#51818 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAUR2SQSZKHOZLVWXHGXJLZKHV4BAVCNFSM6AAAAABKEVQOT6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBRGUYDEOJZGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
If anything is broken with the default template - and the file paths - we
can fix the actual template, so neither Helm nor Istioctl
will need any special options.
User only has to create the Secret and ConfigMap with the names that are
currently specified in the chart - I don't
see anything completely unacceptable on how they are named or the default
locations - I understand others may
have different naming preferences, but the PR has been reviewed and
approved and the author gets to
set the names they like...
…On Mon, Jul 1, 2024 at 6:50 PM Costin Manolache ***@***.***> wrote:
I don't get it: the template already has
```
- name: istio-csr-dns-cert
secret:
secretName: istiod-tls
optional: true
# If this config map is present in istio-system - this will be used for mesh cert
# verification and distributed in namespaces.
- name: istio-csr-ca-configmap
configMap:
name: istio-ca-root-cert
defaultMode: 420
optional: true
```
and the CLI parameters have default values that correspond to the mounts.
Why would you want to mount with different volume mounts and paths - when the out-of-box install just works ?
On Mon, Jul 1, 2024 at 5:11 PM zirain ***@***.***> wrote:
> @costinm <https://github.com/costinm> the main purpose of this PR is
> make istioctl can work with cert-manager istio-csr, helm will work after
> #45517 <#45517>.
>
> without this, we need something like:
>
> apiVersion: install.istio.io/v1alpha1
> kind: IstioOperator
> metadata:
> name: cert-manager-integration
> namespace: istio-system
> spec:
> values:
> global:
> caAddress: cert-manager-istio-csr.cert-manager.svc:443
> components:
> pilot:
> k8s:
> env:
> # Disable istiod CA Sever functionality
> - name: ENABLE_CA_SERVER
> value: "false"
> overlays:
> - apiVersion: apps/v1
> kind: Deployment
> name: istiod
> patches:
> # Mount istiod serving and webhook certificate from Secret mount
> - path: spec.template.spec.containers.[name:discovery].args[7]
> value: "--tlsCertFile=/etc/cert-manager/tls/tls.crt"
> - path: spec.template.spec.containers.[name:discovery].args[8]
> value: "--tlsKeyFile=/etc/cert-manager/tls/tls.key"
> - path: spec.template.spec.containers.[name:discovery].args[9]
> value: "--caCertFile=/etc/cert-manager/ca/root-cert.pem"
> - path: spec.template.spec.containers.[name:discovery].volumeMounts[6]
> value:
> name: cert-manager
> mountPath: "/etc/cert-manager/tls"
> readOnly: true
> - path: spec.template.spec.containers.[name:discovery].volumeMounts[7]
> value:
> name: ca-root-cert
> mountPath: "/etc/cert-manager/ca"
> readOnly: true
> - path: spec.template.spec.volumes[6]
> value:
> name: cert-manager
> secret:
> secretName: istiod-tls
> - path: spec.template.spec.volumes[7]
> value:
> name: ca-root-cert
> configMap:
> defaultMode: 420
> name: istio-ca-root-cert
>
> cert-manager/istio-csr#113
> <cert-manager/istio-csr#113>
>
> —
> Reply to this email directly, view it on GitHub
> <#51818 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAAUR2SQSZKHOZLVWXHGXJLZKHV4BAVCNFSM6AAAAABKEVQOT6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBRGUYDEOJZGU>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
Thanks for point out, I didn't notice that, I should be following an out-of-date document. |
do we need to make istioctl not report the issue like following: Error: validation errors (use --force to override):
bad StructValue: json: cannot unmarshal string into Go value of type map[string]json.RawMessage
Error: validation errors (use --force to override):
unknown field "secret" in k8s.io.api.core.v1.Volumethose are supported by helm, but not working with istioctl, maybe istio-cser isn't an good example. |
istio-testing
added
the
needs-rebase
|
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-sigs/prow repository. |
Please provide a description of this PR:
Introduce in #45517, fix following errors: