-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CM-423: Adds e2e test for istio-csr controller with grpc CreateCertificate call #226
Conversation
@PillaiManish: This pull request references CM-423 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.19." or "openshift-4.19.", but it targets "cert-manager-1.15" instead. In response to this: 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 openshift-eng/jira-lifecycle-plugin repository. |
Skipping CI for Draft Pull Request. |
/testwith openshift/cert-manager-operator/master/e2e-operator #220 |
/reopen |
@PillaiManish: Failed to re-open PR: state cannot be changed. There is already an open pull request from PillaiManish:master to openshift:master. In response to this:
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. |
@PillaiManish: This pull request references CM-423 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.19." or "openshift-4.19.", but it targets "cert-manager-1.15" instead. In response to this: 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 openshift-eng/jira-lifecycle-plugin repository. |
/testwith openshift/cert-manager-operator/master/e2e-operator #220 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refactor the tests using a fresh Ginkgo suite that can have a label for TechPreview
and Feature:IstioCSR
or the alike.
Also, per discussions with team earlier we can strip the service mesh functionality for now and only make this test, really test the grpc server exposed by istio-csr with a plain client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall the test case(s) seem to be in a good shape.
/testwith openshift/cert-manager-operator/master/e2e-operator #220 |
Please add the description. |
/testwith openshift/cert-manager-operator/master/e2e-operator #220 |
1 similar comment
/testwith openshift/cert-manager-operator/master/e2e-operator #220 |
test/e2e/istio_csr_test.go
Outdated
By("creating an grpcurl job") | ||
job := &batchv1.Job{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "grpcurl-job", | ||
}, | ||
Spec: batchv1.JobSpec{ | ||
Completions: ptr.To(int32(1)), | ||
BackoffLimit: ptr.To(back_off_limits), | ||
Template: corev1.PodTemplateSpec{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: grpcAppName, | ||
Labels: map[string]string{ | ||
"app": grpcAppName, | ||
}, | ||
}, | ||
Spec: corev1.PodSpec{ | ||
ServiceAccountName: serviceAccountName, | ||
AutomountServiceAccountToken: ptr.To(false), | ||
RestartPolicy: corev1.RestartPolicyOnFailure, | ||
Containers: []corev1.Container{ | ||
{ | ||
Name: grpcAppName, | ||
Image: "registry.redhat.io/rhel9/go-toolset", | ||
Command: []string{ | ||
"/bin/sh", | ||
"-c", | ||
}, | ||
Env: []corev1.EnvVar{ | ||
{ | ||
Name: "GOCACHE", | ||
Value: "/tmp/go-cache", | ||
}, | ||
}, | ||
Args: []string{ | ||
"GOCACHE=/tmp/go-cache && " + | ||
"export GOPATH=/tmp/go && " + | ||
"go install github.com/fullstorydev/grpcurl/cmd/[email protected] >/dev/null 2>&1 && " + | ||
"TOKEN=$(cat /var/run/secrets/istio-ca/token) && " + | ||
"/tmp/go/bin/grpcurl " + | ||
"-import-path /proto " + | ||
"-proto /proto/ca.proto " + | ||
"-H \"Authorization: Bearer $TOKEN\" " + | ||
fmt.Sprintf("-d '{\"csr\": \"%s\", \"validity_duration\": 3600}' ", csr) + | ||
"-cacert /etc/root-secret/ca.crt " + | ||
"-key /etc/root-secret/tls.key " + | ||
"-cert /etc/root-secret/tls.crt " + | ||
fmt.Sprintf("%s istio.v1.auth.IstioCertificateService/CreateCertificate", istioCSRGRPCEndpoint), | ||
}, | ||
VolumeMounts: []corev1.VolumeMount{ | ||
{Name: "root-secret", MountPath: "/etc/root-secret"}, | ||
{Name: "proto", MountPath: "/proto"}, | ||
{Name: "service-token", MountPath: "/var/run/secrets/istio-ca"}, | ||
}, | ||
}, | ||
}, | ||
Volumes: []corev1.Volume{ | ||
{ | ||
Name: "service-token", | ||
VolumeSource: corev1.VolumeSource{ | ||
Projected: &corev1.ProjectedVolumeSource{ | ||
DefaultMode: ptr.To(int32(420)), | ||
Sources: []corev1.VolumeProjection{ | ||
{ | ||
ServiceAccountToken: &corev1.ServiceAccountTokenProjection{ | ||
Audience: "istio-ca", | ||
ExpirationSeconds: ptr.To(int64(3600)), | ||
Path: "token", | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
{ | ||
Name: "root-secret", | ||
VolumeSource: corev1.VolumeSource{ | ||
Secret: &corev1.SecretVolumeSource{ | ||
SecretName: "istiod-tls", | ||
}, | ||
}, | ||
}, | ||
{ | ||
Name: "proto", | ||
VolumeSource: corev1.VolumeSource{ | ||
ConfigMap: &corev1.ConfigMapVolumeSource{ | ||
LocalObjectReference: corev1.LocalObjectReference{ | ||
Name: "proto-cm", | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer a file for this instead, as most of the field values are fixed. replaceStrInFile
can help;
however follow-up PR is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I can do it in a follow-up, if that works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's de-clutter it, how about a test/e2e/testdata/istio_csr/grpcurl_job.yaml
?
apiVersion: batch/v1
kind: Job
metadata:
name: grpcurl-istio-csr
spec:
backoffLimit: 10
completions: 1
template:
metadata:
labels:
app: grpcurl-istio-csr
name: grpcurl-istio-csr
spec:
automountServiceAccountToken: false
containers:
- args:
- |
go install github.com/fullstorydev/grpcurl/cmd/[email protected] >/dev/null 2>&1 && \
TOKEN=$(cat /var/run/secrets/istio-ca/token) && \
/tmp/go/bin/grpcurl \
-import-path /proto \
-proto /proto/ca.proto \
-H "Authorization: Bearer $TOKEN" \
-d '{"csr": "{{.CertificateSigningRequest}}", "validity_duration": 3600}' \
-cacert /etc/root-secret/ca.crt \
-key /etc/root-secret/tls.key \
-cert /etc/root-secret/tls.crt \
{{.IstioCSRStatus.IstioCSRGRPCEndpoint}} istio.v1.auth.IstioCertificateService/CreateCertificate
command:
- /bin/sh
- -c
env:
- name: GOCACHE
value: /tmp/go-cache
- name: GOPATH
value: /tmp/go
image: registry.redhat.io/rhel9/go-toolset
name: grpcurl
volumeMounts:
- mountPath: /etc/root-secret
name: root-secret
- mountPath: /proto
name: proto
- mountPath: /var/run/secrets/istio-ca
name: sa-token
restartPolicy: OnFailure
serviceAccountName: '{{.IstioCSRStatus.ServiceAccount}}'
volumes:
- name: sa-token
projected:
defaultMode: 420
sources:
- serviceAccountToken:
audience: istio-ca
expirationSeconds: 3600
path: token
- name: root-secret
secret:
secretName: istiod-tls
- configMap:
name: proto-cm
name: proto
what I meant basically pushes ahead of #247, so that within test/e2e/config_template.go
we can add:
+ // IstioCSRConfig customizes the fields in a job spec
+ type IstioCSRGRPCurlJobConfig struct {
+ CertificateSigningRequest string
+ IstioCSRStatus v1alpha1.IstioCSRStatus
+ }
to get the above yaml value filled in during test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, we'll do this on a follow-up PR after #247 lands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few nits, otherwise LGTM.
/retitle CM-423: Adds e2e test for istio-csr controller with grpc CreateCertificate call |
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only minor nits
- applys IstioCSR resource - deploys grpcurl job - which calls the grpc endpoint of istio-csr - checks the response and validates the certificate
/label qe-approved |
@PillaiManish: This pull request references CM-423 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.19." or "openshift-4.19.", but it targets "cert-manager-1.15" instead. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bharath-b-rh, lunarwhite, PillaiManish, swghosh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cherry-pick cert-manager-1.15 |
@swghosh: once the present PR merges, I will cherry-pick it on top of In response to this:
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. |
@PillaiManish: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
@swghosh: #226 failed to apply on top of branch "cert-manager-1.15":
In response to this:
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. |
The PR adds the e2e test scenarios for the new istio-csr controller.