From 6e5ee1c52e5327494a2c77ae9c15590189835ac2 Mon Sep 17 00:00:00 2001 From: Ruben Vargas Date: Mon, 2 Aug 2021 05:17:54 -0500 Subject: [PATCH 1/3] Update operatorhub script (#1516) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Update operatorhub script Signed-off-by: Ruben Vargas * More improvments to the operatorhub script Signed-off-by: Ruben Vargas * Reintroduced checked pr template Signed-off-by: Ruben Vargas * Remove duplicated script Signed-off-by: Ruben Vargas * Check all necessary items on the template Signed-off-by: Ruben Vargas Co-authored-by: Juraci Paixão Kröhling --- .ci/.checked-pr-template.md | 51 +++++++++++++ .ci/.operatorhub-pr-template.md | 20 +++-- .ci/operatorhub.sh | 126 +++++++++----------------------- 3 files changed, 96 insertions(+), 101 deletions(-) create mode 100644 .ci/.checked-pr-template.md diff --git a/.ci/.checked-pr-template.md b/.ci/.checked-pr-template.md new file mode 100644 index 000000000..f627e0f9a --- /dev/null +++ b/.ci/.checked-pr-template.md @@ -0,0 +1,51 @@ +Thanks submitting your Operator. Please check below list before you create your Pull Request. + +### New Submissions + +* [x] Are you familiar with our [contribution guidelines](https://github.com/operator-framework/community-operators/blob/master/docs/contributing-via-pr.md)? +* [x] Have you [packaged and deployed](https://github.com/operator-framework/community-operators/blob/master/docs/testing-operators.md) your Operator for Operator Framework? +* [x] Have you tested your Operator with all Custom Resource Definitions? +* [x] Have you tested your Operator in all supported [installation modes](https://github.com/operator-framework/operator-lifecycle-manager/blob/master/doc/design/building-your-csv.md#operator-metadata)? +* [x] Have you considered whether you want use [semantic versioning order](https://github.com/operator-framework/community-operators/blob/master/docs/operator-ci-yaml.md#semver-mode)? +* [x] Is your submission [signed](https://github.com/operator-framework/community-operators/blob/master/docs/contributing-prerequisites.md#sign-your-work)? +* [x] Is operator [icon](https://github.com/operator-framework/community-operators/blob/master/docs/packaging-operator.md#operator-icon) set? + +### Updates to existing Operators + +* [x] Did you create a `ci.yaml` file according to the [update instructions](https://github.com/operator-framework/community-operators/blob/master/docs/operator-ci-yaml.md)? +* [x] Is your new CSV pointing to the previous version with the `replaces` property if you chose `replaces-mode` via the `updateGraph` property in `ci.yaml`? +* [x] Is your new CSV referenced in the [appropriate channel](https://github.com/operator-framework/community-operators/blob/master/docs/packaging-operator.md#channels) defined in the `package.yaml` or `annotations.yaml` ? +* [ ] Have you tested an update to your Operator when deployed via OLM? +* [x] Is your submission [signed](https://github.com/operator-framework/community-operators/blob/master/docs/contributing-prerequisites.md#sign-your-work)? + +### Your submission should not + +* [x] Modify more than one operator +* [x] Modify an Operator you don't own +* [x] Rename an operator - please remove and add with a different name instead +* [x] Submit operators to both `upstream-community-operators` and `community-operators` at once +* [x] Modify any files outside the above mentioned folders +* [x] Contain more than one commit. **Please squash your commits.** + +### Operator Description must contain (in order) + +1. [x] Description about the managed Application and where to find more information +2. [x] Features and capabilities of your Operator and how to use it +3. [x] Any manual steps about potential pre-requisites for using your Operator + +### Operator Metadata should contain + +* [x] Human readable name and 1-liner description about your Operator +* [x] Valid [category name](https://github.com/operator-framework/community-operators/blob/master/docs/packaging-operator.md#categories)1 +* [x] One of the pre-defined [capability levels](https://github.com/operator-framework/operator-courier/blob/4d1a25d2c8d52f7de6297ec18d8afd6521236aa2/operatorcourier/validate.py#L556)2 +* [x] Links to the maintainer, source code and documentation +* [x] Example templates for all Custom Resource Definitions intended to be used +* [x] A quadratic logo + +Remember that you can preview your CSV [here](https://operatorhub.io/preview). + +-- + +1 If you feel your Operator does not fit any of the pre-defined categories, file an issue against this repo and explain your need + +2 For more information see [here](https://sdk.operatorframework.io/docs/overview/#operator-capability-level) \ No newline at end of file diff --git a/.ci/.operatorhub-pr-template.md b/.ci/.operatorhub-pr-template.md index a8096e74d..3070374af 100644 --- a/.ci/.operatorhub-pr-template.md +++ b/.ci/.operatorhub-pr-template.md @@ -2,23 +2,21 @@ Thanks submitting your Operator. Please check below list before you create your ### New Submissions -* [ ] Does your operator have [nested directory structure](https://github.com/operator-framework/community-operators/blob/master/docs/contributing.md#create-a-bundle)? -* [ ] Have you selected the Project *Community Operator Submissions* in your PR on the right-hand menu bar? -* [ ] Are you familiar with our [contribution guidelines](https://github.com/operator-framework/community-operators/blob/master/docs/contributing.md)? +* [ ] Are you familiar with our [contribution guidelines](https://github.com/operator-framework/community-operators/blob/master/docs/contributing-via-pr.md)? * [ ] Have you [packaged and deployed](https://github.com/operator-framework/community-operators/blob/master/docs/testing-operators.md) your Operator for Operator Framework? * [ ] Have you tested your Operator with all Custom Resource Definitions? * [ ] Have you tested your Operator in all supported [installation modes](https://github.com/operator-framework/operator-lifecycle-manager/blob/master/doc/design/building-your-csv.md#operator-metadata)? -* [ ] Have you considered whether you want use [semantic versioning order](https://github.com/operator-framework/community-operators/blob/master/docs/contributing.md#updating-your-existing-operator)? -* [ ] Is your submission [signed](https://github.com/operator-framework/community-operators/blob/master/docs/contributing.md#sign-your-work)? -* [ ] Is operator [icon](https://github.com/operator-framework/community-operators/blob/master/docs/contributing.md#operator-icon) set? +* [ ] Have you considered whether you want use [semantic versioning order](https://github.com/operator-framework/community-operators/blob/master/docs/operator-ci-yaml.md#semver-mode)? +* [ ] Is your submission [signed](https://github.com/operator-framework/community-operators/blob/master/docs/contributing-prerequisites.md#sign-your-work)? +* [ ] Is operator [icon](https://github.com/operator-framework/community-operators/blob/master/docs/packaging-operator.md#operator-icon) set? ### Updates to existing Operators -* [ ] Did you create a `ci.yaml` file according to the [update instructions](https://github.com/operator-framework/community-operators/blob/master/docs/contributing.md#updating-your-existing-operator)? +* [ ] Did you create a `ci.yaml` file according to the [update instructions](https://github.com/operator-framework/community-operators/blob/master/docs/operator-ci-yaml.md)? * [ ] Is your new CSV pointing to the previous version with the `replaces` property if you chose `replaces-mode` via the `updateGraph` property in `ci.yaml`? -* [ ] Is your new CSV referenced in the [appropriate channel](https://github.com/operator-framework/community-operators/blob/master/docs/contributing.md#bundle-format) defined in the `package.yaml` or `annotations.yaml` ? +* [ ] Is your new CSV referenced in the [appropriate channel](https://github.com/operator-framework/community-operators/blob/master/docs/packaging-operator.md#channels) defined in the `package.yaml` or `annotations.yaml` ? * [ ] Have you tested an update to your Operator when deployed via OLM? -* [ ] Is your submission [signed](https://github.com/operator-framework/community-operators/blob/master/docs/contributing.md#sign-your-work)? +* [ ] Is your submission [signed](https://github.com/operator-framework/community-operators/blob/master/docs/contributing-prerequisites.md#sign-your-work)? ### Your submission should not @@ -38,7 +36,7 @@ Thanks submitting your Operator. Please check below list before you create your ### Operator Metadata should contain * [ ] Human readable name and 1-liner description about your Operator -* [ ] Valid [category name](https://github.com/operator-framework/community-operators/blob/master/docs/required-fields.md#categories)1 +* [ ] Valid [category name](https://github.com/operator-framework/community-operators/blob/master/docs/packaging-operator.md#categories)1 * [ ] One of the pre-defined [capability levels](https://github.com/operator-framework/operator-courier/blob/4d1a25d2c8d52f7de6297ec18d8afd6521236aa2/operatorcourier/validate.py#L556)2 * [ ] Links to the maintainer, source code and documentation * [ ] Example templates for all Custom Resource Definitions intended to be used @@ -50,4 +48,4 @@ Remember that you can preview your CSV [here](https://operatorhub.io/preview). 1 If you feel your Operator does not fit any of the pre-defined categories, file an issue against this repo and explain your need -2 For more information see [here](https://github.com/operator-framework/operator-sdk/blob/master/doc/images/operator-capability-level.svg) +2 For more information see [here](https://sdk.operatorframework.io/docs/overview/#operator-capability-level) diff --git a/.ci/operatorhub.sh b/.ci/operatorhub.sh index 36e5b103d..eb1184694 100755 --- a/.ci/operatorhub.sh +++ b/.ci/operatorhub.sh @@ -1,17 +1,25 @@ #!/bin/bash -if [ -z ${COMMUNITY_OPERATORS_REPOSITORY} ]; then - COMMUNITY_OPERATORS_REPOSITORY="$(dirname $(dirname $(pwd)))/operator-framework/community-operators" - echo "COMMUNITY_OPERATORS_REPOSITORY not set, using ${COMMUNITY_OPERATORS_REPOSITORY}" + +COMMUNITY_OPERATORS_REPOSITORY="k8s-operatorhub/community-operators" +UPSTREAM_REPOSITORY="redhat-openshift-ecosystem/community-operators-prod" +LOCAL_REPOSITORIES_PATH=${LOCAL_REPOSITORIES_PATH:-"$(dirname $(dirname $(pwd)))"} + + +if [[ ! -d "${LOCAL_REPOSITORIES_PATH}/${COMMUNITY_OPERATORS_REPOSITORY}" ]]; then + echo "${LOCAL_REPOSITORIES_PATH}/${COMMUNITY_OPERATORS_REPOSITORY} doesn't exist, aborting." + exit 1 fi -if [ ! -d ${COMMUNITY_OPERATORS_REPOSITORY} ]; then - echo "${COMMUNITY_OPERATORS_REPOSITORY} doesn't exist, aborting." +if [[ ! -d "${LOCAL_REPOSITORIES_PATH}/${UPSTREAM_REPOSITORY}" ]]; then + echo "${LOCAL_REPOSITORIES_PATH}/${UPSTREAM_REPOSITORY} doesn't exist, aborting." exit 1 fi + OLD_PWD=$(pwd) VERSION=$(grep operator= versions.txt | awk -F= '{print $2}') + PKG_FILE=deploy/olm-catalog/jaeger-operator/jaeger-operator.package.yaml CSV_FILE=deploy/olm-catalog/jaeger-operator/manifests/jaeger-operator.clusterserviceversion.yaml CRD_FILE=deploy/crds/jaegertracing.io_jaegers_crd.yaml @@ -21,104 +29,42 @@ CRD_FILE=deploy/crds/jaegertracing.io_jaegers_crd.yaml DEST_PKG_FILE=jaeger.package.yaml DEST_CSV_FILE=jaeger.v${VERSION}.clusterserviceversion.yaml -cd "${COMMUNITY_OPERATORS_REPOSITORY}" - -git remote | grep upstream > /dev/null -if [ $? != 0 ]; then - echo "Cannot find a remote named 'upstream'. Adding one." - git remote add upstream git@github.com:operator-framework/community-operators.git -fi - -git fetch -q upstream -git checkout -q master -git rebase -q upstream/master +for dest in ${COMMUNITY_OPERATORS_REPOSITORY} ${UPSTREAM_REPOSITORY}; do + cd "${LOCAL_REPOSITORIES_PATH}/${dest}" + git remote | grep upstream > /dev/null + if [[ $? != 0 ]]; then + echo "Cannot find a remote named 'upstream'. Adding one." + git remote add upstream git@github.com:${dest}.git + fi -for dest in upstream-community-operators community-operators; do - mkdir -p "${COMMUNITY_OPERATORS_REPOSITORY}/${dest}/jaeger/${VERSION}" + git fetch -q upstream + git checkout -q main + git rebase -q upstream/main - cp "${OLD_PWD}/${PKG_FILE}" "${COMMUNITY_OPERATORS_REPOSITORY}/${dest}/jaeger/${DEST_PKG_FILE}" - cp "${OLD_PWD}/${CSV_FILE}" "${COMMUNITY_OPERATORS_REPOSITORY}/${dest}/jaeger/${VERSION}/${DEST_CSV_FILE}" - cp "${OLD_PWD}/${CRD_FILE}" "${COMMUNITY_OPERATORS_REPOSITORY}/${dest}/jaeger/${VERSION}" + mkdir -p "${dest}/operators/jaeger/${VERSION}" - git checkout -q master + cp "${OLD_PWD}/${PKG_FILE}" "${dest}/operators/jaeger/${DEST_PKG_FILE}" + cp "${OLD_PWD}/${CSV_FILE}" "${dest}/operators/jaeger/${VERSION}/${DEST_CSV_FILE}" + cp "${OLD_PWD}/${CRD_FILE}" "${dest}/operators/jaeger/${VERSION}" - git checkout -q -b Update-Jaeger-${dest}-to-${VERSION} - if [ $? != 0 ]; then + git checkout -q -b Update-Jaeger-to-${VERSION} + if [[ $? != 0 ]]; then echo "Cannot switch to the new branch Update-Jaeger-${dest}-to-${VERSION}. Aborting" exit 1 fi - git add "${COMMUNITY_OPERATORS_REPOSITORY}/${dest}/" - git commit -sqm "Update Jaeger ${dest} to v${VERSION}" - git push -q + git add ${dest} + git commit -sqm "Update Jaeger to v${VERSION}" + - command -v hub > /dev/null - if [ $? != 0 ]; then - echo "'hub' command not found, can't submit the PR on your behalf." + command -v gh > /dev/null + if [[ $? != 0 ]]; then + echo "'gh' command not found, can't submit the PR on your behalf." break fi - tmpfile=$(mktemp /tmp/Update-Jaeger-${dest}-to-${VERSION}.XXX) - cat > ${tmpfile} <<- EOM -Update Jaeger ${dest} to v${VERSION} - -Thanks submitting your Operator. Please check below list before you create your Pull Request. - -### New Submissions - -* [x] Does your operator have [nested directory structure](https://github.com/operator-framework/community-operators/blob/master/docs/contributing.md#create-a-bundle)? -* [x] Have you selected the Project *Community Operator Submissions* in your PR on the right-hand menu bar? -* [x] Are you familiar with our [contribution guidelines](https://github.com/operator-framework/community-operators/blob/master/docs/contributing.md)? -* [x] Have you [packaged and deployed](https://github.com/operator-framework/community-operators/blob/master/docs/testing-operators.md) your Operator for Operator Framework? -* [x] Have you tested your Operator with all Custom Resource Definitions? -* [x] Have you tested your Operator in all supported [installation modes](https://github.com/operator-framework/operator-lifecycle-manager/blob/master/doc/design/building-your-csv.md#operator-metadata)? -* [x] Have you considered whether you want use [semantic versioning order](https://github.com/operator-framework/community-operators/blob/master/docs/contributing.md#updating-your-existing-operator)? -* [x] Is your submission [signed](https://github.com/operator-framework/community-operators/blob/master/docs/contributing.md#sign-your-work)? -* [x] Is operator [icon](https://github.com/operator-framework/community-operators/blob/master/docs/contributing.md#operator-icon) set? - -### Updates to existing Operators - -* [x] Did you create a ci.yaml file according to the [update instructions](https://github.com/operator-framework/community-operators/blob/master/docs/contributing.md#updating-your-existing-operator)? -* [x] Is your new CSV pointing to the previous version with the replaces property if you chose replaces-mode via the updateGraph property in ci.yaml? -* [x] Is your new CSV referenced in the [appropriate channel](https://github.com/operator-framework/community-operators/blob/master/docs/contributing.md#bundle-format) defined in the package.yaml or annotations.yaml ? -* [ ] Have you tested an update to your Operator when deployed via OLM? -* [x] Is your submission [signed](https://github.com/operator-framework/community-operators/blob/master/docs/contributing.md#sign-your-work)? - -### Your submission should not - -* [x] Modify more than one operator -* [x] Modify an Operator you don't own -* [x] Rename an operator - please remove and add with a different name instead -* [x] Submit operators to both upstream-community-operators and community-operators at once -* [x] Modify any files outside the above mentioned folders -* [x] Contain more than one commit. **Please squash your commits.** - -### Operator Description must contain (in order) - -1. [x] Description about the managed Application and where to find more information -2. [x] Features and capabilities of your Operator and how to use it -3. [x] Any manual steps about potential pre-requisites for using your Operator - -### Operator Metadata should contain - -* [x] Human readable name and 1-liner description about your Operator -* [x] Valid [category name](https://github.com/operator-framework/community-operators/blob/master/docs/required-fields.md#categories)1 -* [x] One of the pre-defined [capability levels](https://github.com/operator-framework/operator-courier/blob/4d1a25d2c8d52f7de6297ec18d8afd6521236aa2/operatorcourier/validate.py#L556)2 -* [x] Links to the maintainer, source code and documentation -* [x] Example templates for all Custom Resource Definitions intended to be used -* [x] A quadratic logo - -Remember that you can preview your CSV [here](https://operatorhub.io/preview). - --- - -1 If you feel your Operator does not fit any of the pre-defined categories, file an issue against this repo and explain your need - -2 For more information see [here](https://github.com/operator-framework/operator-sdk/blob/master/doc/images/operator-capability-level.svg) -EOM - echo "Submitting PR on your behalf via 'hub'" - hub pull-request -F ${tmpfile} + gh pr create --title "Update Jaeger to v${VERSION}" --body-file "${OLD_PWD}/.ci/.checked-pr-template.md" rm ${tmpfile} done From 980858691a75681c29990729a7e23496af91a3f0 Mon Sep 17 00:00:00 2001 From: Ruben Vargas Date: Tue, 3 Aug 2021 03:41:59 -0500 Subject: [PATCH 2/3] Add gRPC port for jaeger-query into its service resource (#1521) Signed-off-by: Ruben Vargas --- deploy/crds/jaegertracing.io_jaegers_crd.yaml | 3 +++ pkg/apis/jaegertracing/v1/jaeger_types.go | 4 ++++ .../jaegertracing/v1/zz_generated.openapi.go | 7 +++++++ pkg/service/query.go | 12 ++++++++++++ pkg/service/query_test.go | 16 +++++++++++----- 5 files changed, 37 insertions(+), 5 deletions(-) diff --git a/deploy/crds/jaegertracing.io_jaegers_crd.yaml b/deploy/crds/jaegertracing.io_jaegers_crd.yaml index defb1e399..118f6d2c0 100644 --- a/deploy/crds/jaegertracing.io_jaegers_crd.yaml +++ b/deploy/crds/jaegertracing.io_jaegers_crd.yaml @@ -6149,6 +6149,9 @@ spec: type: string nullable: true type: object + grpcNodePort: + format: int32 + type: integer image: type: string labels: diff --git a/pkg/apis/jaegertracing/v1/jaeger_types.go b/pkg/apis/jaegertracing/v1/jaeger_types.go index 65d01f5f7..2038fd93e 100644 --- a/pkg/apis/jaegertracing/v1/jaeger_types.go +++ b/pkg/apis/jaegertracing/v1/jaeger_types.go @@ -266,6 +266,10 @@ type JaegerQuerySpec struct { // NodePort represents the port at which the NodePort service to allocate NodePort int32 `json:"nodePort,omitempty"` + // +optional + // NodePort represents the port at which the NodePort service to allocate + GRPCNodePort int32 `json:"grpcNodePort,omitempty"` + // +optional // TracingEnabled if set to false adds the JAEGER_DISABLED environment flag and removes the injected // agent container from the query component to disable tracing requests to the query service. diff --git a/pkg/apis/jaegertracing/v1/zz_generated.openapi.go b/pkg/apis/jaegertracing/v1/zz_generated.openapi.go index 0e7afaf78..1f3cd288f 100644 --- a/pkg/apis/jaegertracing/v1/zz_generated.openapi.go +++ b/pkg/apis/jaegertracing/v1/zz_generated.openapi.go @@ -1792,6 +1792,13 @@ func schema_pkg_apis_jaegertracing_v1_JaegerQuerySpec(ref common.ReferenceCallba Format: "int32", }, }, + "grpcNodePort": { + SchemaProps: spec.SchemaProps{ + Description: "NodePort represents the port at which the NodePort service to allocate", + Type: []string{"integer"}, + Format: "int32", + }, + }, "tracingEnabled": { SchemaProps: spec.SchemaProps{ Description: "TracingEnabled if set to false adds the JAEGER_DISABLED environment flag and removes the injected agent container from the query component to disable tracing requests to the query service. The default, if ommited, is true", diff --git a/pkg/service/query.go b/pkg/service/query.go index 2a06d9118..0d57ed4b9 100644 --- a/pkg/service/query.go +++ b/pkg/service/query.go @@ -24,9 +24,16 @@ func NewQueryService(jaeger *v1.Jaeger, selector map[string]string) *corev1.Serv Port: int32(GetPortForQueryService(jaeger)), TargetPort: intstr.FromInt(getTargetPortForQueryService(jaeger)), }, + { + Name: "grpc-query", + Port: int32(16685), + TargetPort: intstr.FromInt(16685), + }, } if jaeger.Spec.Query.ServiceType == corev1.ServiceTypeNodePort { ports[0].NodePort = GetNodePortForQueryService(jaeger) + ports[1].NodePort = GetGRPCNodePortForQueryService(jaeger) + } return &corev1.Service{ @@ -100,3 +107,8 @@ func getTypeForQueryService(jaeger *v1.Jaeger) corev1.ServiceType { func GetNodePortForQueryService(jaeger *v1.Jaeger) int32 { return jaeger.Spec.Query.NodePort } + +// GetGRPCNodePortForQueryService returns the query service grpc NodePort for this Jaeger instance +func GetGRPCNodePortForQueryService(jaeger *v1.Jaeger) int32 { + return jaeger.Spec.Query.GRPCNodePort +} diff --git a/pkg/service/query_test.go b/pkg/service/query_test.go index ce20c24b7..8212b1018 100644 --- a/pkg/service/query_test.go +++ b/pkg/service/query_test.go @@ -20,8 +20,9 @@ func TestQueryServiceNameAndPorts(t *testing.T) { svc := NewQueryService(jaeger, selector) assert.Equal(t, "testqueryservicenameandports-query", svc.ObjectMeta.Name) - assert.Len(t, svc.Spec.Ports, 1) + assert.Len(t, svc.Spec.Ports, 2) assert.Equal(t, int32(16686), svc.Spec.Ports[0].Port) + assert.Equal(t, int32(16685), svc.Spec.Ports[1].Port) assert.Equal(t, "http-query", svc.Spec.Ports[0].Name) assert.Equal(t, intstr.FromInt(16686), svc.Spec.Ports[0].TargetPort) assert.Len(t, svc.Spec.ClusterIP, 0) // make sure we get a cluster IP @@ -47,8 +48,9 @@ func TestQueryServiceNameAndPortsWithOAuthProxy(t *testing.T) { svc := NewQueryService(jaeger, selector) assert.Equal(t, "testqueryservicenameandportswithoauthproxy-query", svc.ObjectMeta.Name) - assert.Len(t, svc.Spec.Ports, 1) + assert.Len(t, svc.Spec.Ports, 2) assert.Equal(t, int32(443), svc.Spec.Ports[0].Port) + assert.Equal(t, int32(16685), svc.Spec.Ports[1].Port) assert.Equal(t, "https-query", svc.Spec.Ports[0].Name) assert.Equal(t, intstr.FromInt(8443), svc.Spec.Ports[0].TargetPort) } @@ -62,10 +64,12 @@ func TestQueryServiceNodePortWithIngress(t *testing.T) { svc := NewQueryService(jaeger, selector) assert.Equal(t, "testqueryservicenodeportwithingress-query", svc.ObjectMeta.Name) - assert.Len(t, svc.Spec.Ports, 1) + assert.Len(t, svc.Spec.Ports, 2) assert.Equal(t, int32(16686), svc.Spec.Ports[0].Port) + assert.Equal(t, int32(16685), svc.Spec.Ports[1].Port) assert.Equal(t, "http-query", svc.Spec.Ports[0].Name) assert.Equal(t, int32(0), svc.Spec.Ports[0].NodePort) + assert.Equal(t, int32(0), svc.Spec.Ports[1].NodePort) assert.Equal(t, intstr.FromInt(16686), svc.Spec.Ports[0].TargetPort) assert.Equal(t, svc.Spec.Type, corev1.ServiceTypeNodePort) // make sure we get a NodePort service } @@ -79,8 +83,9 @@ func TestQueryServiceLoadBalancerWithIngress(t *testing.T) { svc := NewQueryService(jaeger, selector) assert.Equal(t, "testqueryservicenodeportwithingress-query", svc.ObjectMeta.Name) - assert.Len(t, svc.Spec.Ports, 1) + assert.Len(t, svc.Spec.Ports, 2) assert.Equal(t, int32(16686), svc.Spec.Ports[0].Port) + assert.Equal(t, int32(16685), svc.Spec.Ports[1].Port) assert.Equal(t, "http-query", svc.Spec.Ports[0].Name) assert.Equal(t, intstr.FromInt(16686), svc.Spec.Ports[0].TargetPort) assert.Equal(t, svc.Spec.Type, corev1.ServiceTypeLoadBalancer) // make sure we get a LoadBalancer service @@ -96,8 +101,9 @@ func TestQueryServiceSpecifiedNodePortWithIngress(t *testing.T) { svc := NewQueryService(jaeger, selector) assert.Equal(t, "testqueryservicespecifiednodeportwithingress-query", svc.ObjectMeta.Name) - assert.Len(t, svc.Spec.Ports, 1) + assert.Len(t, svc.Spec.Ports, 2) assert.Equal(t, int32(16686), svc.Spec.Ports[0].Port) + assert.Equal(t, int32(16685), svc.Spec.Ports[1].Port) assert.Equal(t, "http-query", svc.Spec.Ports[0].Name) assert.Equal(t, int32(32767), svc.Spec.Ports[0].NodePort) // make sure we get the same NodePort as set above assert.Equal(t, intstr.FromInt(16686), svc.Spec.Ports[0].TargetPort) From 05b0c69973d9a35ff52c6e16c8449b7d8ffc15b7 Mon Sep 17 00:00:00 2001 From: Ruben Vargas Date: Wed, 4 Aug 2021 04:38:53 -0500 Subject: [PATCH 3/3] Allow TLS flags to be disabled (#1440) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Validate presence of tls flags using only the prefix of the flag Signed-off-by: Ruben Vargas * Test explicit disable tls options Signed-off-by: Ruben Vargas * Handle flags update case Signed-off-by: Ruben Vargas Co-authored-by: Juraci Paixão Kröhling --- pkg/deployment/agent.go | 2 +- pkg/deployment/agent_test.go | 28 +++++++++-- pkg/deployment/all_in_one.go | 10 +++- pkg/deployment/all_in_one_test.go | 78 ++++++++++++++++++++++++------- pkg/deployment/collector.go | 4 +- pkg/deployment/collector_test.go | 66 ++++++++++++++++++-------- pkg/inject/sidecar.go | 2 +- pkg/inject/sidecar_test.go | 63 +++++++++++++++++++------ 8 files changed, 193 insertions(+), 60 deletions(-) diff --git a/pkg/deployment/agent.go b/pkg/deployment/agent.go index 0a2adc5f1..285d0fec5 100644 --- a/pkg/deployment/agent.go +++ b/pkg/deployment/agent.go @@ -45,7 +45,7 @@ func (a *Agent) Get() *appsv1.DaemonSet { // Enable tls by default for openshift platform if viper.GetString("platform") == v1.FlagPlatformOpenShift { - if len(util.FindItem("--reporter.grpc.tls.enabled=true", args)) == 0 { + if len(util.FindItem("--reporter.grpc.tls.enabled=", args)) == 0 { args = append(args, "--reporter.grpc.tls.enabled=true") args = append(args, fmt.Sprintf("--reporter.grpc.tls.ca=%s", ca.ServiceCAPath)) args = append(args, fmt.Sprintf("--reporter.grpc.tls.server-name=%s.%s.svc.cluster.local", service.GetNameForHeadlessCollectorService(a.jaeger), a.jaeger.Namespace)) diff --git a/pkg/deployment/agent_test.go b/pkg/deployment/agent_test.go index 49ec7bfec..57edcf172 100644 --- a/pkg/deployment/agent_test.go +++ b/pkg/deployment/agent_test.go @@ -201,9 +201,10 @@ func TestAgentArgumentsOpenshiftTLS(t *testing.T) { defer viper.Reset() for _, tt := range []struct { - name string - options v1.Options - expectedArgs []string + name string + options v1.Options + expectedArgs []string + nonExpectedArgs []string }{ { name: "Openshift CA", @@ -232,6 +233,21 @@ func TestAgentArgumentsOpenshiftTLS(t *testing.T) { "--reporter.grpc.tls.ca=/my/custom/ca", }, }, + { + name: "Explicit disable TLS", + options: v1.NewOptions(map[string]interface{}{ + "a-option": "a-value", + "reporter.grpc.tls.enabled": "false", + }), + expectedArgs: []string{ + "--a-option=a-value", + "--reporter.grpc.host-port=dns:///my-instance-collector-headless.test:14250", + "--reporter.grpc.tls.enabled=false", + }, + nonExpectedArgs: []string{ + "--reporter.grpc.tls.enabled=true", + }, + }, } { t.Run(tt.name, func(t *testing.T) { jaeger := v1.NewJaeger(types.NamespacedName{ @@ -251,6 +267,12 @@ func TestAgentArgumentsOpenshiftTLS(t *testing.T) { assert.Greater(t, len(util.FindItem(arg, dep.Spec.Template.Spec.Containers[0].Args)), 0) } + if tt.nonExpectedArgs != nil { + for _, arg := range tt.nonExpectedArgs { + assert.Equal(t, len(util.FindItem(arg, dep.Spec.Template.Spec.Containers[0].Args)), 0) + } + } + assert.Len(t, dep.Spec.Template.Spec.Volumes, 2) assert.Len(t, dep.Spec.Template.Spec.Containers[0].VolumeMounts, 2) }) diff --git a/pkg/deployment/all_in_one.go b/pkg/deployment/all_in_one.go index 10a7babda..1edb6efd4 100644 --- a/pkg/deployment/all_in_one.go +++ b/pkg/deployment/all_in_one.go @@ -65,7 +65,13 @@ func (a *AllInOne) Get() *appsv1.Deployment { configmap.Update(a.jaeger, commonSpec, &options) sampling.Update(a.jaeger, commonSpec, &options) - tls.Update(a.jaeger, commonSpec, &options) + + + // If tls is not explicitly set, update jaeger CR with the tls flags according to the platform + if len(util.FindItem("--collector.grpc.tls.enabled=", options)) == 0 { + tls.Update(a.jaeger, commonSpec, &options) + } + ca.Update(a.jaeger, commonSpec) ca.AddServiceCA(a.jaeger, commonSpec) storage.UpdateGRPCPlugin(a.jaeger, commonSpec) @@ -74,7 +80,7 @@ func (a *AllInOne) Get() *appsv1.Deployment { // even though the agent is in the same process as the collector, they communicate via gRPC, and the collector has TLS enabled, // as it might receive connections from external agents if viper.GetString("platform") == v1.FlagPlatformOpenShift { - if len(util.FindItem("--reporter.grpc.tls.enabled=true", options)) == 0 { + if len(util.FindItem("--reporter.grpc.tls.enabled=", options)) == 0 { options = append(options, "--reporter.grpc.tls.enabled=true") options = append(options, fmt.Sprintf("--reporter.grpc.tls.ca=%s", ca.ServiceCAPath)) options = append(options, fmt.Sprintf("--reporter.grpc.tls.server-name=%s.%s.svc.cluster.local", service.GetNameForHeadlessCollectorService(a.jaeger), a.jaeger.Namespace)) diff --git a/pkg/deployment/all_in_one_test.go b/pkg/deployment/all_in_one_test.go index ea2731308..1f7b74c9e 100644 --- a/pkg/deployment/all_in_one_test.go +++ b/pkg/deployment/all_in_one_test.go @@ -338,27 +338,69 @@ func TestAllInOneArgumentsOpenshiftTLS(t *testing.T) { viper.Set("platform", v1.FlagPlatformOpenShift) defer viper.Reset() - jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"}) - jaeger.Spec.AllInOne.Options = v1.NewOptions(map[string]interface{}{ - "a-option": "a-value", - }) + for _, tt := range []struct { + name string + options v1.Options + expectedArgs []string + nonExpectedArgs []string + }{ + { + name: "Openshift CA", + options: v1.NewOptions(map[string]interface{}{ + "a-option": "a-value", + }), + expectedArgs: []string{ + "--a-option=a-value", + "--collector.grpc.tls.enabled=true", + "--collector.grpc.tls.cert=/etc/tls-config/tls.crt", + "--collector.grpc.tls.key=/etc/tls-config/tls.key", + "--sampling.strategies-file", + "--reporter.grpc.tls.ca", + "--reporter.grpc.tls.enabled", + "--reporter.grpc.tls.server-name", + }, + }, + { + name: "Explicit disable TLS", + options: v1.NewOptions(map[string]interface{}{ + "a-option": "a-value", + "reporter.grpc.tls.enabled": "false", + "collector.grpc.tls.enabled": "false", + }), + expectedArgs: []string{ + "--a-option=a-value", + "--reporter.grpc.tls.enabled=false", + "--collector.grpc.tls.enabled=false", + "--sampling.strategies-file", + }, + nonExpectedArgs: []string{ + "--reporter.grpc.tls.enabled=true", + "--collector.grpc.tls.enabled=true", + }, + }, + } { + jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"}) + jaeger.Spec.AllInOne.Options = tt.options - // test - a := NewAllInOne(jaeger) - dep := a.Get() + // test + a := NewAllInOne(jaeger) + dep := a.Get() - // verify - assert.Len(t, dep.Spec.Template.Spec.Containers, 1) - assert.Len(t, dep.Spec.Template.Spec.Containers[0].Args, 8) - assert.NotEmpty(t, util.FindItem("--a-option=a-value", dep.Spec.Template.Spec.Containers[0].Args)) - assert.NotEmpty(t, util.FindItem("--collector.grpc.tls.enabled=true", dep.Spec.Template.Spec.Containers[0].Args)) - assert.NotEmpty(t, util.FindItem("--collector.grpc.tls.cert=/etc/tls-config/tls.crt", dep.Spec.Template.Spec.Containers[0].Args)) - assert.NotEmpty(t, util.FindItem("--collector.grpc.tls.key=/etc/tls-config/tls.key", dep.Spec.Template.Spec.Containers[0].Args)) - assert.NotEmpty(t, util.FindItem("--sampling.strategies-file", dep.Spec.Template.Spec.Containers[0].Args)) + // verify + assert.Len(t, dep.Spec.Template.Spec.Containers, 1) + assert.Len(t, dep.Spec.Template.Spec.Containers[0].Args, len(tt.expectedArgs)) + + for _, arg := range tt.expectedArgs { + assert.NotEmpty(t, util.FindItem(arg, dep.Spec.Template.Spec.Containers[0].Args)) + } + + if tt.nonExpectedArgs != nil { + for _, arg := range tt.nonExpectedArgs { + assert.Equal(t, len(util.FindItem(arg, dep.Spec.Template.Spec.Containers[0].Args)), 0) + } + } + } - assert.NotEmpty(t, util.FindItem("--reporter.grpc.tls.ca", dep.Spec.Template.Spec.Containers[0].Args)) - assert.NotEmpty(t, util.FindItem("--reporter.grpc.tls.enabled", dep.Spec.Template.Spec.Containers[0].Args)) - assert.NotEmpty(t, util.FindItem("--reporter.grpc.tls.server-name", dep.Spec.Template.Spec.Containers[0].Args)) } func TestAllInOneServiceLinks(t *testing.T) { diff --git a/pkg/deployment/collector.go b/pkg/deployment/collector.go index ae844b924..db270b76a 100644 --- a/pkg/deployment/collector.go +++ b/pkg/deployment/collector.go @@ -76,10 +76,10 @@ func (c *Collector) Get() *appsv1.Deployment { c.jaeger.Spec.Storage.Options.Filter(storageType.OptionsPrefix())) sampling.Update(c.jaeger, commonSpec, &options) - if len(util.FindItem("--collector.grpc.tls.enabled=true", args)) == 0 { + if len(util.FindItem("--collector.grpc.tls.enabled=", args)) == 0 { tls.Update(c.jaeger, commonSpec, &options) - ca.Update(c.jaeger, commonSpec) } + ca.Update(c.jaeger, commonSpec) storage.UpdateGRPCPlugin(c.jaeger, commonSpec) // ensure we have a consistent order of the arguments diff --git a/pkg/deployment/collector_test.go b/pkg/deployment/collector_test.go index 8eaf4d68e..b980da28d 100644 --- a/pkg/deployment/collector_test.go +++ b/pkg/deployment/collector_test.go @@ -510,31 +510,55 @@ func TestCollectorAutoscalersSetMaxReplicas(t *testing.T) { func TestCollectoArgumentsOpenshiftTLS(t *testing.T) { viper.Set("platform", v1.FlagPlatformOpenShift) defer viper.Reset() - for _, tt := range []struct { - name string - options v1.Options - expectedCert string - expectedKey string + name string + options v1.Options + expectedArgs []string + nonExpectedArgs []string }{ { - name: "Openshift certificates", + name: "Openshift CA", options: v1.NewOptions(map[string]interface{}{ "a-option": "a-value", }), - expectedCert: "/etc/tls-config/tls.crt", - expectedKey: "/etc/tls-config/tls.key", + expectedArgs: []string{ + "--a-option=a-value", + "--collector.grpc.tls.enabled=true", + "--collector.grpc.tls.cert=/etc/tls-config/tls.crt", + "--collector.grpc.tls.key=/etc/tls-config/tls.key", + "--sampling.strategies-file", + }, }, { - name: "Custom certificates", + name: "Custom CA", options: v1.NewOptions(map[string]interface{}{ "a-option": "a-value", "collector.grpc.tls.enabled": "true", "collector.grpc.tls.cert": "/my/custom/cert", "collector.grpc.tls.key": "/my/custom/key", }), - expectedCert: "/my/custom/cert", - expectedKey: "/my/custom/key", + expectedArgs: []string{ + "--a-option=a-value", + "--collector.grpc.tls.enabled=true", + "--collector.grpc.tls.cert=/my/custom/cert", + "--collector.grpc.tls.key=/my/custom/key", + "--sampling.strategies-file", + }, + }, + { + name: "Explicit disable TLS", + options: v1.NewOptions(map[string]interface{}{ + "a-option": "a-value", + "collector.grpc.tls.enabled": "false", + }), + expectedArgs: []string{ + "--a-option=a-value", + "--collector.grpc.tls.enabled=false", + "--sampling.strategies-file", + }, + nonExpectedArgs: []string{ + "--collector.grpc.tls.enabled=true", + }, }, } { t.Run(tt.name, func(t *testing.T) { @@ -544,15 +568,19 @@ func TestCollectoArgumentsOpenshiftTLS(t *testing.T) { a := NewCollector(jaeger) dep := a.Get() + // verify assert.Len(t, dep.Spec.Template.Spec.Containers, 1) - assert.Len(t, dep.Spec.Template.Spec.Containers[0].Args, 5) - assert.Greater(t, len(util.FindItem("--a-option=a-value", dep.Spec.Template.Spec.Containers[0].Args)), 0) - - // the following are added automatically - assert.Greater(t, len(util.FindItem("--collector.grpc.tls.enabled=true", dep.Spec.Template.Spec.Containers[0].Args)), 0) - assert.Greater(t, len(util.FindItem("--collector.grpc.tls.cert="+tt.expectedCert, dep.Spec.Template.Spec.Containers[0].Args)), 0) - assert.Greater(t, len(util.FindItem("--collector.grpc.tls.key="+tt.expectedKey, dep.Spec.Template.Spec.Containers[0].Args)), 0) - assert.Greater(t, len(util.FindItem("--sampling.strategies-file", dep.Spec.Template.Spec.Containers[0].Args)), 0) + assert.Len(t, dep.Spec.Template.Spec.Containers[0].Args, len(tt.expectedArgs)) + + for _, arg := range tt.expectedArgs { + assert.NotEmpty(t, util.FindItem(arg, dep.Spec.Template.Spec.Containers[0].Args)) + } + + if tt.nonExpectedArgs != nil { + for _, arg := range tt.nonExpectedArgs { + assert.Equal(t, len(util.FindItem(arg, dep.Spec.Template.Spec.Containers[0].Args)), 0) + } + } }) } diff --git a/pkg/inject/sidecar.go b/pkg/inject/sidecar.go index fe32f1def..ffd3b7840 100644 --- a/pkg/inject/sidecar.go +++ b/pkg/inject/sidecar.go @@ -188,7 +188,7 @@ func container(jaeger *v1.Jaeger, dep *appsv1.Deployment, agentIdx int) corev1.C // Enable tls by default for openshift platform if viper.GetString("platform") == v1.FlagPlatformOpenShift { - if len(util.FindItem("--reporter.grpc.tls.enabled=true", args)) == 0 { + if len(util.FindItem("--reporter.grpc.tls.enabled=", args)) == 0 { args = append(args, "--reporter.grpc.tls.enabled=true") args = append(args, fmt.Sprintf("--reporter.grpc.tls.ca=%s", ca.ServiceCAPath)) } diff --git a/pkg/inject/sidecar_test.go b/pkg/inject/sidecar_test.go index e42b51d78..8a49c75d1 100644 --- a/pkg/inject/sidecar_test.go +++ b/pkg/inject/sidecar_test.go @@ -768,20 +768,28 @@ func containsOptionWithPrefix(t *testing.T, args []string, prefix string) bool { } func TestSidecarArgumentsOpenshiftTLS(t *testing.T) { + viper.Set("platform", v1.FlagPlatformOpenShift) defer viper.Reset() for _, tt := range []struct { - name string - options v1.Options - expectedCA string + name string + options v1.Options + expectedArgs []string + nonExpectedArgs []string }{ { name: "Openshift CA", options: v1.NewOptions(map[string]interface{}{ "a-option": "a-value", }), - expectedCA: ca.ServiceCAPath, + expectedArgs: []string{ + "--a-option=a-value", + "--reporter.grpc.tls.enabled=true", + "--reporter.grpc.tls.ca=" + ca.ServiceCAPath, + "--reporter.grpc.host-port=dns:///my-instance-collector-headless.test.svc:14250", + "--agent.tags=", + }, }, { name: "Custom CA", @@ -790,7 +798,29 @@ func TestSidecarArgumentsOpenshiftTLS(t *testing.T) { "reporter.grpc.tls.enabled": "true", "reporter.grpc.tls.ca": "/my/custom/ca", }), - expectedCA: "/my/custom/ca", + expectedArgs: []string{ + "--a-option=a-value", + "--reporter.grpc.host-port=dns:///my-instance-collector-headless.test.svc:14250", + "--reporter.grpc.tls.enabled=true", + "--reporter.grpc.tls.ca=/my/custom/ca", + "--agent.tags=", + }, + }, + { + name: "Explicit disable TLS", + options: v1.NewOptions(map[string]interface{}{ + "a-option": "a-value", + "reporter.grpc.tls.enabled": "false", + }), + expectedArgs: []string{ + "--a-option=a-value", + "--reporter.grpc.host-port=dns:///my-instance-collector-headless.test.svc:14250", + "--reporter.grpc.tls.enabled=false", + "--agent.tags=", + }, + nonExpectedArgs: []string{ + "--reporter.grpc.tls.enabled=true", + }, }, } { t.Run(tt.name, func(t *testing.T) { @@ -803,15 +833,20 @@ func TestSidecarArgumentsOpenshiftTLS(t *testing.T) { dep = Sidecar(jaeger, dep) assert.Len(t, dep.Spec.Template.Spec.Containers, 2) - assert.Len(t, dep.Spec.Template.Spec.Containers[1].Args, 5) - assert.Greater(t, len(util.FindItem("--a-option=a-value", dep.Spec.Template.Spec.Containers[1].Args)), 0) - assert.Greater(t, len(util.FindItem("--agent.tags", dep.Spec.Template.Spec.Containers[1].Args)), 0) - assert.Greater(t, len(util.FindItem("--reporter.grpc.host-port=dns:///my-instance-collector-headless.test.svc:14250", dep.Spec.Template.Spec.Containers[1].Args)), 0) - assert.Greater(t, len(util.FindItem("--reporter.grpc.tls.enabled=true", dep.Spec.Template.Spec.Containers[1].Args)), 0) - assert.Greater(t, len(util.FindItem("--reporter.grpc.tls.ca="+tt.expectedCA, dep.Spec.Template.Spec.Containers[1].Args)), 0) - agentTagsMap := parseAgentTags(dep.Spec.Template.Spec.Containers[1].Args) - assert.Contains(t, agentTagsMap, "container.name") - assert.Equal(t, agentTagsMap["container.name"], "only_container") + assert.Len(t, dep.Spec.Template.Spec.Containers[1].Args, len(tt.expectedArgs)) + + for _, arg := range tt.expectedArgs { + assert.Greater(t, len(util.FindItem(arg, dep.Spec.Template.Spec.Containers[1].Args)), 0) + } + + if tt.nonExpectedArgs != nil { + for _, arg := range tt.nonExpectedArgs { + assert.Equal(t, len(util.FindItem(arg, dep.Spec.Template.Spec.Containers[1].Args)), 0) + } + } + + assert.Len(t, dep.Spec.Template.Spec.Volumes, 2) + assert.Len(t, dep.Spec.Template.Spec.Containers[1].VolumeMounts, 2) }) } }