Skip to content
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

Istio Crypto Scheduled Workflow #79

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

asubedy
Copy link
Member

@asubedy asubedy commented Jul 6, 2023

Description

This PR fixes #

Notes for Reviewers

Signed commits

  • Yes, I signed my commits.

Signed-off-by: asubedy <frexpe@pm.me>
Signed-off-by: asubedy <frexpe@pm.me>
Copy link
Member

@gyohuangxin gyohuangxin left a comment

Choose a reason for hiding this comment

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

It's great to add Istio Crypto, I found some issues.

.github/workflows/scripts/istioCrypto.sh Outdated Show resolved Hide resolved
.github/workflows/scheduled-istioCrypto-benchmark.yml Outdated Show resolved Hide resolved
.github/workflows/scheduled-istioCrypto-benchmark.yml Outdated Show resolved Hide resolved
.github/workflows/scripts/istioCrypto.sh Outdated Show resolved Hide resolved
@gyohuangxin gyohuangxin changed the title Istio Crrypto Scheduled Workflow Istio Crypto Scheduled Workflow Jul 6, 2023
Signed-off-by: asubedy <frexpe@pm.me>
Signed-off-by: asubedy <frexpe@pm.me>
@asubedy asubedy requested a review from gyohuangxin July 6, 2023 19:17
Copy link
Member

@gyohuangxin gyohuangxin left a comment

Choose a reason for hiding this comment

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

One tiny nit but otherwise LGTM. @asubedy Thanks!

.github/workflows/scripts/istioCrypto.sh Outdated Show resolved Hide resolved
Signed-off-by: asubedy <frexpe@pm.me>
@gyohuangxin
Copy link
Member

@asubedy Can I confirm that this PR ready to be merged?

@asubedy
Copy link
Member Author

asubedy commented Jul 14, 2023

No @gyohuangxin it is not cometely ready. It still needs a dynamic component registration feature. It will not be able to create performance profile now. So let's hold on merging a little. Ill let you know when its ready

@gyohuangxin gyohuangxin added the pr/do-not-merge PRs not ready to be merged label Jul 14, 2023
Signed-off-by: asubedy <frexpe@pm.me>
Signed-off-by: asubedy <frexpe@pm.me>
export TCP_INGRESS_PORT=$(kubectl -n "$INGRESS_NS" get service "$INGRESS_NAME" -o jsonpath='{.spec.ports[?(@.name=="tcp")].port}')


export GATEWAY_URL=http://$INGRESS_HOST:$INGRESS_PORT/headers
Copy link
Member

Choose a reason for hiding this comment

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

@asubedy For crypto test case, the GATEWAY_URL should be https://$INGRESS_HOST:$SECURE_INGRESS_PORT.
And we need to generate a self-signed certificate to use in performance profile, so we can create another shell script which can be named as generate_csr.sh , I can provide the script we used before:


# generate CA cerficate
openssl genrsa -out fortio.com.key 2048
openssl req -new -x509 -days 365 -key fortio.com.key -subj "/C=CN/ST=GD/L=SZ/O=fortio.com, Inc./CN=fortio.com Root CA" -out fortio.com.crt

# generate CSR
openssl req -newkey rsa:2048 -nodes -keyout httpbin.fortio.com.key -subj "/C=CN/ST=GD/L=SZ/O=fortio.com, Inc./CN=*.fortio.com" -out httpbin.fortio.com.csr
openssl x509 -req -extfile <(printf "subjectAltName=IP:10.239.241.168,DNS:fortio.com,DNS:www.fortio.com") -days 365 -in httpbin.fortio.com.csr -CA fortio.com.crt -CAkey fortio.com.key -CAcreateserial -out httpbin.fortio.com.crt

# upload key and crt as a secret
kubectl create -n istio-system secret tls httpbin-fortio-credential --key=httpbin.fortio.com.key --cert=httpbin.fortio.com.crt

Then we can use the URL and fortio.com.crt in performance file.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gyohuangxin I created the file, so we run this script after deploying Istio Crypto and deploying httpbin application right?

Copy link
Member

Choose a reason for hiding this comment

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

@asubedy Yes, it's correct.

Signed-off-by: asubedy <frexpe@pm.me>

# generate CSR
openssl req -newkey rsa:2048 -nodes -keyout httpbin.fortio.com.key -subj "/C=CN/ST=GD/L=SZ/O=fortio.com, Inc./CN=*.fortio.com" -out httpbin.fortio.com.csr
openssl x509 -req -extfile <(printf "subjectAltName=IP:10.239.241.168,DNS:fortio.com,DNS:www.fortio.com") -days 365 -in httpbin.fortio.com.csr -CA fortio.com.crt -CAkey fortio.com.key -CAcreateserial -out httpbin.fortio.com.crt
Copy link
Member

Choose a reason for hiding this comment

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

@asubedy The IP address is hardcoded here, we need to replace it to the ingress ip.

endpoint_url: ${{env.ENDPOINT_URL}}
service_mesh: ${{env.SERVICE_MESH}}
load_generator: fortio
profile_name: 'istioCrypto-load-test.yaml'
Copy link
Member

Choose a reason for hiding this comment

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

We need to add the certificate generated by generate_csr.sh script in the profile, we can add another item cert_path to use the certificate. But it depends on this PR: meshery/meshery#8148.


# generate CA cerficate
openssl genrsa -out fortio.com.key 2048
openssl req -new -x509 -days 365 -key fortio.com.key -subj "/C=CN/ST=GD/L=SZ/O=fortio.com, Inc./CN=fortio.com Root CA" -out fortio.com.crt
Copy link
Member

Choose a reason for hiding this comment

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

The address fortio.com is come from application we used in our test environment, it may be confusing to other developers. Can you replace all fortio.com here with another word, such as the application used in this test?

Copy link
Member Author

Choose a reason for hiding this comment

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

@gyohuangxin so change all the instances that mentions fortio.com with httpbin_app there are some instances that have httpbin.fortio.com.key so we will change this into httpbin.httpbin_app.key right?

@gyohuangxin
Copy link
Member

@asubedy Left some comments, if you have any doubts feel free to ask here or on Slack channel. Thanks!

@gyohuangxin
Copy link
Member

Related to #78 @asubedy Any updates here?

Signed-off-by: asubedy <frexpe@pm.me>
@asubedy
Copy link
Member Author

asubedy commented Aug 14, 2023

@gyohuangxin could you please send the httpbin application file that you used file I am using this file but it is showing me some error

# Copyright Istio Authors
#
#   Licensed under the Apache License, Version 2.0 (the "License");
#   you may not use this file except in compliance with the License.
#   You may obtain a copy of the License at
#
#       http://www.apache.org/licenses/LICENSE-2.0
#
#   Unless required by applicable law or agreed to in writing, software
#   distributed under the License is distributed on an "AS IS" BASIS,
#   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
#   See the License for the specific language governing permissions and
#   limitations under the License.

##################################################################################################
# httpbin service
##################################################################################################
apiVersion: v1
kind: ServiceAccount
metadata:
  name: httpbin
---
apiVersion: v1
kind: Service
metadata:
  name: httpbin
  labels:
    app: httpbin
    service: httpbin
spec:
  ports:
  - name: http
    port: 8000
    targetPort: 80
  selector:
    app: httpbin
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: httpbin
spec:
  replicas: 1
  selector:
    matchLabels:
      app: httpbin
      version: v1
  template:
    metadata:
      labels:
        app: httpbin
        version: v1
    spec:
      serviceAccountName: httpbin
      containers:
      - image: docker.io/kong/httpbin
        imagePullPolicy: IfNotPresent
        name: httpbin
        ports:
        - containerPort: 80
---
apiVersion: networking.istio.io/v1alpha3
kind: Gateway
metadata:
  name: httpbin-gateway
spec:
  # The selector matches the ingress gateway pod labels.
  # If you installed Istio using Helm following the standard documentation, this would be "istio=ingress"
  selector:
    istio: ingressgateway
  servers:
  - port:
      number: 80
      name: http
      protocol: HTTP
    hosts:
    - "httpbin.example.com"
---
apiVersion: networking.istio.io/v1alpha3
kind: VirtualService
metadata:
  name: httpbin
spec:
  hosts:
  - "httpbin.example.com"
  gateways:
  - httpbin-gateway
  http:
  - match:
    - uri:
        prefix: /status
    - uri:
        prefix: /delay
    route:
    - destination:
        port:
          number: 8000
        host: httpbin

@gyohuangxin
Copy link
Member

@asubedy I think it comes from the Istio example: https://github.com/istio/istio/blob/master/samples/httpbin/httpbin.yaml, we are also using it. Can you show me the error you met?

@asubedy
Copy link
Member Author

asubedy commented Aug 16, 2023

While using GitHub action this is the error I got:
image

When I try it on my local system this is the error i get
Screenshot 2023-08-14 at 21 04 25

@asubedy
Copy link
Member Author

asubedy commented Aug 16, 2023

A quick note that I have added these configuration too on the httpbin application
image

as given in this guide

@gyohuangxin
Copy link
Member

While using GitHub action this is the error I got: image

When I try it on my local system this is the error i get Screenshot 2023-08-14 at 21 04 25

I'm not sure what happen when you use mesheryctl app onboard to apply this file, can you try kubectl apply -f directly?

@stale
Copy link

stale bot commented Oct 15, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the issue/stale Issue has not had any activity for an extended period of time label Oct 15, 2023
Copy link

stale bot commented Dec 15, 2023

This issue is being automatically closed due to inactivity. However, you may choose to reopen this issue.

@stale stale bot closed this Dec 15, 2023
@saurabh100ni saurabh100ni removed the issue/stale Issue has not had any activity for an extended period of time label Jan 1, 2024
@saurabh100ni saurabh100ni added the issue/willfix This issue will be worked on label Jan 1, 2024
@saurabh100ni saurabh100ni reopened this Jan 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue/willfix This issue will be worked on pr/do-not-merge PRs not ready to be merged
Development

Successfully merging this pull request may close these issues.

None yet

3 participants