Skip to content

Run upstream e2e test with net-istio #1014

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 2 commits into from
Jul 13, 2021
Merged

Run upstream e2e test with net-istio #1014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb1a72f
Run e2e test with net-istio enabled
nak3 Jun 10, 2021
03de2e6
Revert makefile's temporary change
nak3 Jul 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,12 @@ test-upstream-e2e:
UNINSTALL_STRIMZI="false" ./hack/strimzi.sh
INSTALL_KAFKA=true TEST_KNATIVE_KAFKA=true ./test/upstream-e2e-tests.sh

# Run upstream E2E tests with net-istio and sidecar.
# TODO: Enable upgrade tests once upstream fixed the issue https://github.com/knative/serving/issues/11535.
test-upstream-e2e-mesh:
UNINSTALL_STRIMZI="false" ./hack/strimzi.sh
FULL_MESH=true INSTALL_KAFKA=false TEST_KNATIVE_KAFKA=false TEST_KNATIVE_UPGRADE=false ./test/upstream-e2e-tests.sh

# Run upstream E2E tests without upgrades.
test-upstream-e2e-no-upgrade:
UNINSTALL_STRIMZI="false" ./hack/strimzi.sh
Expand Down Expand Up @@ -131,4 +137,4 @@ lint:
golangci-lint run
find . -type f -path './**/*.*sh' -not -path '*vendor*' | xargs -r shellcheck
operator-sdk bundle validate ./olm-catalog/serverless-operator
git ls-files | grep -Ev '^(vendor/|.git)' | xargs misspell -error
git ls-files | grep -Ev '^(vendor/|.git)' | xargs misspell -error
45 changes: 45 additions & 0 deletions hack/lib/serverless.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,12 +153,57 @@ function deploy_knativeserving_cr {
# This is a way to test backwards compatibility of the product with the older full-blown configuration.
oc apply -n "${SERVING_NAMESPACE}" -f "${rootdir}/test/v1alpha1/resources/operator.knative.dev_v1alpha1_knativeserving_cr.yaml"

if [[ $FULL_MESH == "true" ]]; then
enable_net_istio
fi

timeout 900 "[[ \$(oc get knativeserving.operator.knative.dev knative-serving \
-n ${SERVING_NAMESPACE} -o=jsonpath='{.status.conditions[?(@.type==\"Ready\")].status}') != True ]]"

logger.success 'Knative Serving has been installed successfully.'
}

# enable_net_istio adds patch to KnativeServing:
# - Set ingress.istio.enbled to "true"
# - Set inject and rewriteAppHTTPProbers annotations for activator and autoscaler
# - Override observability.metrics.backend-destination to "none",
# as "test/v1alpha1/resources/operator.knative.dev_v1alpha1_knativeserving_cr.yaml" has the value "prometheus".
function enable_net_istio {
patchfile="$(mktemp -t knative-serving-XXXXX.yaml)"
cat - << EOF > "${patchfile}"
spec:
ingress:
istio:
enabled: true
deployments:
- annotations:
sidecar.istio.io/inject: "true"
sidecar.istio.io/rewriteAppHTTPProbers: "true"
name: activator
- annotations:
sidecar.istio.io/inject: "true"
sidecar.istio.io/rewriteAppHTTPProbers: "true"
name: autoscaler
- name: domain-mapping
replicas: 2
Comment on lines +187 to +188
Copy link
Copy Markdown
Contributor

@markusthoemmes markusthoemmes Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this? Don't we also need that in "usual" setups?

Copy link
Copy Markdown
Contributor Author

@nak3 nak3 Jul 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we have the replicas: 2 in "usual" setups.

serverless-operator/test/v1alpha1/resources/operator.knative.dev_v1alpha1_knativeserving_cr.yaml

Lines 7 to 8 in b368a3f

- name: domain-mapping
replicas: 2

But the spec.deployments is an array. So when we patch the annotation (inject and rewriteAppHTTPProbers) to spec.deployments for activator and autoscaler, domain-mapping's replicas: 2 is dropped 😓

Copy link
Copy Markdown
Contributor

@markusthoemmes markusthoemmes Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, makes sense!

config:
observability:
metrics.backend-destination: "none"
Comment on lines +189 to +191
Copy link
Copy Markdown
Contributor

@markusthoemmes markusthoemmes Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this required? Don't we do that "automatically"?

Copy link
Copy Markdown
Contributor Author

@nak3 nak3 Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Umm... we have the default as "prometheus"

serverless-operator/test/v1alpha1/resources/operator.knative.dev_v1alpha1_knativeserving_cr.yaml

Line 39 in b368a3f

metrics.backend-destination: "prometheus"

so I think we need to override it with none. Sorry if I misunderstood the meaning of "automatically".

Copy link
Copy Markdown
Contributor

@markusthoemmes markusthoemmes Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahhhh we explicitly set it. Nevermind then, I was thinking in our Golang code, where we default to none currently, if istio is enabled.

EOF

oc patch knativeserving knative-serving \
-n "${SERVING_NAMESPACE}" \
--type merge --patch-file="${patchfile}"

timeout 900 "[[ \$(oc get knativeserving.operator.knative.dev knative-serving \
-n ${SERVING_NAMESPACE} -o=jsonpath='{.status.conditions[?(@.type==\"Ready\")].status}') != True ]]"

logger.success 'KnativeServing has been updated successfully.'

# metadata-webhook adds istio annotations for e2e test by webhook.
oc apply -f https://raw.githubusercontent.com/nak3/metadata-webhook/main/examples/release.yaml
}

function deploy_knativeeventing_cr {
logger.info 'Deploy Knative Eventing'

Expand Down
1 change: 1 addition & 0 deletions hack/lib/vars.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,4 @@ export TEST_KNATIVE_KAFKA="${TEST_KNATIVE_KAFKA:-false}"
export INSTALL_SERVING="${INSTALL_SERVING:-true}"
export INSTALL_EVENTING="${INSTALL_EVENTING:-true}"
export INSTALL_KAFKA="${INSTALL_KAFKA:-false}"
export FULL_MESH="${FULL_MESH:-false}"
18 changes: 17 additions & 1 deletion test/serving.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,27 @@ function upstream_knative_serving_e2e_and_conformance_tests {
image_template="registry.ci.openshift.org/openshift/knative-${KNATIVE_SERVING_VERSION}:knative-serving-test-{{.Name}}"
OPENSHIFT_TEST_OPTIONS="--kubeconfig $KUBECONFIG --enable-beta --enable-alpha --resolvabledomain"

if [[ $FULL_MESH == "true" ]]; then
subdomain=$(oc get ingresses.config.openshift.io cluster -o jsonpath="{.spec.domain}")
OPENSHIFT_TEST_OPTIONS+=" --https --customdomain=$subdomain"

# Use x509ignoreCN=0.
# This should not be necesssary if we could ceate certs with SAN. However, openssl command in the CI
# seems old version and so it does not have "-addext" option to add SAN.
export GODEBUG="x509ignoreCN=0"
Comment on lines +55 to +58
Copy link
Copy Markdown
Contributor

@markusthoemmes markusthoemmes Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to fix that. Can we update the openssl command in https://github.com/openshift-knative/serverless-operator/blob/main/openshift/ci-operator/build-image/Dockerfile?

Copy link
Copy Markdown
Contributor Author

@nak3 nak3 Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried but the base image of registry.ci.openshift.org/openshift/release:golang-1.15 is CentOS 7 and the latest openssl command was still 1.10.

$ docker run build-image rpm -qf `which openssl`
openssl-1.0.2k-21.el7_9.x86_64


# TODO: SRVKS-211: Can not run grpc and http2 tests.
rm ./test/e2e/grpc_test.go
rm ./test/e2e/http2_test.go
# Remove h2c test
sed -ie '46,50d' ./test/conformance/runtime/protocol_test.go
Comment on lines +60 to +64
Copy link
Copy Markdown
Contributor

@markusthoemmes markusthoemmes Jul 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that we'll have to document gRPC/HTTP2 not working for mesh for now, right?

Copy link
Copy Markdown
Contributor Author

@nak3 nak3 Jul 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will open the JIRA.

fi

local parallel=3

if [[ $(oc get infrastructure cluster -ojsonpath='{.status.platform}') = VSphere ]]; then
# Since we don't have LoadBalancers working, gRPC tests will always fail.
rm ./test/e2e/grpc_test.go
rm -f ./test/e2e/grpc_test.go
parallel=2
fi

Expand Down
5 changes: 5 additions & 0 deletions test/upstream-e2e-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ logger.success '🚀 Cluster prepared for testing.'

scale_up_workers

# Install ServiceMesh and enable mTLS.
if [[ $FULL_MESH == true ]]; then
UNINSTALL_MESH="false" install_mesh
fi

# Run upgrade tests
if [[ $TEST_KNATIVE_UPGRADE == true ]]; then
install_serverless_previous
Expand Down