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

Odo url Ingress support #2762

Merged

Conversation

yangcao77
Copy link
Contributor

@yangcao77 yangcao77 commented Mar 26, 2020

What type of PR is this?

Uncomment only one /kind line, and delete the rest.
For example, > /kind bug would simply become: /kind bug

/kind feature

What does does this PR do / why we need it:
This PR implements odo url create/list/delete/describe support for devfile

create
odo url create <urlName> --host <clusterIP> would create an ingress which domain is <urlName>.<clusterIP>
--port flag is required if the devfile has more than one ports exposed. If only one port is exposed, the flag is optional.
--secured flag is to specify if it is a secured URL.
--tls-secret flag is to specify if the user has his own tls secret want to bind with the ingress

Here are the detailed information on the https support:

  • The certificate will use x509 with rsa key.
  • SerialNumber will use time.Now() and encoded to make sure of the uniqueness.
  • All ingresses of a single component will share the same ssl certificate & tls secret. If secure=true will check if there is a valid tls secret existing for the component, if exists, use the existing one; if not, create a new one.
  • The tls secret will not be deleted by odo url delete. It will only be deleted when the component is deleted.

This PR contains unit tests for ingress.go, secret.go and integration test for odo url

Which issue(s) this PR fixes:

Fixes #2721
Fixes #2719
Fixes #2657
Implemented #2592

How to test changes / Special notes to the reviewer:
Automation test
make test-cmd-devfile-url
Expect result:

ginkgo  -randomizeAllSpecs -slowSpecThreshold=120 -timeout 7200s -nodes=2 -focus="odo devfile url command tests" tests/integration/
Running Suite: Integration Suite
================================
Random Seed: 1585200121 - Will randomize all specs
Will run 169 specs

Running in parallel across 2 nodes

SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS••SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS•SSSSSSSSS•SSSSSSSSSSSSSSSSSSSSSSSSSSSSS
Ran 4 of 169 Specs in 100.660 seconds
SUCCESS! -- 4 Passed | 0 Failed | 0 Pending | 165 Skipped

Manual test
git clone https://github.com/rajivnathan/openLibertyDevfile

odo preference set experimental true
odo create openLiberty openlibertydevfile 
odo url create testurl --port 9090 --host 1.2.3.4.nip.io --secure
odo push

Expected:

 •  Push devfile component openlibertydevfile  ...

Applying URL changes
 ✓  URL testurl: https://testurl.1.2.3.4.nip.io created
 ✓  Checking files for pushing [4ms]
 ✓  Waiting for component to start [4s]
 ✓  Syncing files to the component [3s]
 ✓  Push devfile component openlibertydevfile [10s]
 ✓  Changes successfully pushed to component

check the ingress existence:

$ kubectl get ingress
NAME                          HOSTS                              ADDRESS   PORTS     AGE
testurl                       testurl.1.2.3.4.nip.io                       80, 443   42s

check the tle secret has been created:

$ kubectl get secrets
NAME                                                           TYPE                                  DATA   AGE
openlibertydevfile-tlssecret                                   kubernetes.io/tls                     2      1m

list the url:

$ odo url list
Found the following URLs for component openlibertydevfile
NAME        URL                                PORT
testurl     https://testurl.1.2.3.4.nip.io     9090
$ odo url list -o json
{
    "kind": "List",
    "apiVersion": "udo.udo.io/v1alpha1",
    "metadata": {},
    "items": [
        {
            "kind": "Ingress",
            "apiVersion": "extensions/v1beta1",
            "metadata": {
                "name": "testurl",
                "creationTimestamp": null
            },
            "spec": {
                "tls": [
                    {
                        "hosts": [
                            "testurl.1.2.3.4.nip.io"
                        ],
                        "secretName": "openlibertydevfile-tlssecret"
                    }
                ],
                "rules": [
                    {
                        "host": "testurl.1.2.3.4.nip.io",
                        "http": {
                            "paths": [
                                {
                                    "path": "/",
                                    "backend": {
                                        "serviceName": "openlibertydevfile",
                                        "servicePort": 9090
                                    }
                                }
                            ]
                        }
                    }
                ]
            },
            "status": {
                "loadBalancer": {}
            }
        }
    ]
}

describe the url:

$ odo url describe testurl
NAME        URL                                PORT
testurl     https://testurl.1.2.3.4.nip.io     9090

delete the url:

$ odo url delete testurl
? Are you sure you want to delete the url testurl Yes
 ✓  URL testurl removed from the config file

To delete URL on the OpenShift Cluster, please use `odo push`

check the url info has been deleted from .odo/env/env.yaml

$ cat .odo/env/env.yaml 
ComponentSettings:
  Url: []

run odo push

e$ odo push
 •  Push devfile component openlibertydevfile  ...

Applying URL changes
 ✓  URL testurl successfully deleted
 ✓  Checking file changes for pushing [3ms]
 ✓  No file changes detected, skipping build. Use the '-f' flag to force the build.
 ✓  Push devfile component openlibertydevfile [1s]
 ✓  Changes successfully pushed to component

check the ingress has been deleted. but the tls secret still exist

run odo url list again:

$ odo url list
 ✗  no URLs found for component openlibertydevfile

Signed-off-by: Stephanie <stephanie.cao@ibm.com>
Signed-off-by: Stephanie <stephanie.cao@ibm.com>
Signed-off-by: Stephanie <stephanie.cao@ibm.com>
Signed-off-by: Stephanie <stephanie.cao@ibm.com>
Signed-off-by: Stephanie <stephanie.cao@ibm.com>
Signed-off-by: Stephanie <stephanie.cao@ibm.com>
Signed-off-by: Stephanie <stephanie.cao@ibm.com>
Signed-off-by: Stephanie <stephanie.cao@ibm.com>
Signed-off-by: Stephanie <stephanie.cao@ibm.com>
Signed-off-by: Stephanie <stephanie.cao@ibm.com>
Signed-off-by: Stephanie <stephanie.cao@ibm.com>
Signed-off-by: Stephanie <stephanie.cao@ibm.com>
Signed-off-by: Stephanie <stephanie.cao@ibm.com>
Signed-off-by: Stephanie <stephanie.cao@ibm.com>
Signed-off-by: Stephanie <stephanie.cao@ibm.com>
Signed-off-by: Stephanie <stephanie.cao@ibm.com>
@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Mar 26, 2020
@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. Required by Prow. label Mar 26, 2020
@openshift-ci-robot
Copy link
Collaborator

Hi @yangcao77. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

@amitkrout
Copy link
Contributor

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. Required by Prow. labels Mar 26, 2020
@mik-dass
Copy link
Contributor

@yangcao77 Please add some steps/instructions on how to test this PR in the description.

@yangcao77
Copy link
Contributor Author

/test v4.3-integration-e2e-benchmark

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Apr 3, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Apr 3, 2020
@yangcao77
Copy link
Contributor Author

yangcao77 commented Apr 3, 2020

@mik-dass I finally found out what caused the failure.
It is because this issue: #2796
oco create now takes --namespace, but does not set the value. So I switched to use --project for component creation.
Another failure is because DevfilePush requires a namespace, when using the --now flag to push after url creation, the namespace is empty, and therefore would use the current namespace, which is incorrect due to the parallel runs. With my change, if the namespace is empty, it will get the namespace from env.yaml.
Also added a resolveNamespace() function when creating a context for devfile component, so if namespace is not provided, it will get it from env.yaml, and set kclient.Client.Namespace to use the correct namespace. (same behavior as resolveProject() for odo component)

These changes resolved the namespace mismatch in parallel runs, and the test finally passed. (https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/openshift_odo/2762/pull-ci-openshift-odo-master-v4.1-integration-e2e-benchmark/1540)

@elsony
Copy link

elsony commented Apr 3, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 3, 2020
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 4, 2020
@elsony
Copy link

elsony commented Apr 4, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 4, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 4, 2020
@elsony
Copy link

elsony commented Apr 4, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 4, 2020
@mik-dass
Copy link
Contributor

mik-dass commented Apr 4, 2020

@mik-dass I finally found out what caused the failure.
It is because this issue: #2796

@yangcao77 OK, let's get this in once the CI passes. Thanks for the quick response 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation lgtm Indicates that a PR is ready to be merged. Required by Prow. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
10 participants