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.

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

Jump to bottom

app_nonat test #591

Merged
merged 1 commit into from
May 12, 2021
Merged

app_nonat test #591

merged 1 commit into from
May 12, 2021

Conversation

sadov
Copy link
Contributor

@sadov sadov commented Apr 21, 2021

At this time blocked by #589.

Signed-off-by: Oleg Sadov [email protected]

@sadov sadov changed the title WIP app_nonat test [WIP] app_nonat test Apr 21, 2021
@sadov sadov force-pushed the test_app_nonat branch 3 times, most recently from a152362 to f135296 Compare April 23, 2021 22:01
@sadov
Copy link
Contributor Author

sadov commented Apr 26, 2021 •
edited
Loading

The test fails in the current state. EVE configs from this test:

"apps": [
        {
"interfaces": [
...
        {
    "name": "default",
    "networkId": "512839bd-a655-46ad-988d-fde5da33c3bb",
    "acls": [
        {
            "matches": [    
                {               
                    "type": "ip",   
                    "value": "0.0.0.0/0"
                }               
            ],
            "id": 1         
        }
    ]
}
]
...
"networkInstances": [
...
{
    "uuidandversion": {
        "uuid": "512839bd-a655-46ad-988d-fde5da33c3bb",
        "version": "1"
    },
    "displayname": "direct",
    "instType": 1,
    "activate": true,
    "port": {
        "type": 1,
        "name": "eth0"
    },
    "cfg": {},
    "ipType": 1,
    "ip": {}
}
...

and ztest:

"apps": [
       {
"interfaces": [
...
{
   "name": "direct",
   "networkId": "72a14885-73c4-4fe7-a4b2-f919c05bf065",
   "addr": "",
   "hostname": "",
   "cryptoEid": "",
   "lispsignature": "",
   "pemcert": null,
   "pemprivatekey": null,
   "macAddress": "",
   "acls": [
       {
           "matches": [
               {
                   "type": "ip",
                   "value": "0.0.0.0/0"
               }
           ],
           "actions": [],
           "name": "",
           "id": 3,
           "dir": "BOTH"
       }
   ]
}
]
...
"networkInstances": [
...
{
   "uuidandversion": {
       "uuid": "72a14885-73c4-4fe7-a4b2-f919c05bf065",
       "version": ""
   },
   "displayname": "TestAppNonat-sc-supermicro-e100-8-NI-2021-04-14T02-00-25",
   "instType": "ZnetInstSwitch",
   "activate": true,
   "port": {
       "type": "PhyIoNoop",
       "name": "eth0"
   },
   "cfg": {
       "oconfig": "",
       "lispConfig": null,
       "type": "ZNetOConfigVPN"
   },
   "ipType": "IPV4", 
   "ip": null,
   "dns": []
}
...

looks more or less similar, but pings through eth1 not passed.

Log file:
eden_test-app_nonat.015.log

@sadov
Copy link
Contributor Author

sadov commented Apr 26, 2021

After adding the default gw route for eth1 to the test, the ping passes. But it still gets through after removing the acl rules from the direct network.

@giggsoff giggsoff mentioned this pull request Apr 26, 2021
Merged
@zed-sadov
Copy link
Contributor

With a such changes on EVE:
lf-edge/eve#2042
test passed:

eden test tests/eclient/ -e app_nonat -v debug

@sadov sadov force-pushed the test_app_nonat branch 2 times, most recently from e6a2015 to 3fc77ff Compare May 10, 2021 12:18
@sadov sadov changed the title [WIP] app_nonat test app_nonat test May 10, 2021
@sadov sadov force-pushed the test_app_nonat branch 4 times, most recently from bdc1b56 to 0509adb Compare May 11, 2021 22:45
giggsoff
giggsoff reviewed May 12, 2021
View reviewed changes
tests/eclient/testdata/app_nonat.txt
message 'Switching network'
eden pod modify eclient --networks indirect --networks=direct --acl='direct:'
test eden.app.test -test.v -timewait 30m RUNNING eclient

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sadov please add here

message 'Checking accessibility'
exec -t 5m bash wait_ssh.sh

eden pod modify will restart app in this case, so we must wait for ssh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reasonably

giggsoff
giggsoff requested changes May 12, 2021
View reviewed changes
Copy link
Collaborator

@giggsoff giggsoff left a comment

Choose a reason for hiding this comment

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

Please add wait for ssh after pod modify and take a look at my comments

tests/eclient/testdata/app_nonat.txt Outdated
! exec -t 1m bash ping.sh
stdout '100% packet loss'

message 'Resource cleanng'
Copy link
Collaborator

Choose a reason for hiding this comment

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

cleanng->cleaning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

tests/eclient/testdata/app_nonat.txt Outdated
EDEN={{EdenConfig "eden.root"}}/{{EdenConfig "eden.bin-dist"}}/{{EdenConfig "eden.eden-bin"}}
HOST=$($EDEN eve ip)

for p in $*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason to use this loop here?

Copy link
Contributor Author

@sadov sadov May 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This was developed empirically - in some cases, the system did not fully initialize after going into the RUNNING state. We use such loops in many tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have the inner loop here for that (for i in seq 20), right?

tests/eclient/testdata/app_nonat.txt Outdated
exec -t 1m bash ping.sh
stdout '0% packet loss'

message 'Switching network'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add comment, that we remove all ACLs for direct network for which one eth1 of eclient connected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure -- mentioning the ACL here will make sense, but the exact wording will need to be coordinated with the network documentation and the second is planned to be created.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, I think if we remove all ACLs, we can be independent from spelling, we just remove them.

tests/eclient/testdata/app_nonat.txt Outdated
#echo {{template "ssh"}}$HOST route add default gw 192.168.0.2 dev eth1
#{{template "ssh"}}$HOST route add default gw 192.168.0.2 dev eth1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we remove comments here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@sadov sadov force-pushed the test_app_nonat branch 2 times, most recently from 8f76765 to ed1a925 Compare May 12, 2021 08:50
@giggsoff
Copy link
Collaborator

GCP test failed again with changes in your PR. Seems, we cannot use switch with uplink on GCP, it allocates only one address per VM. Alias IPs only usable with manual ip setting, it is not useful in our case.
So, I suggest to move the test onto large-only branch.

@zed-sadov
app_nonat test
846fea6 
Signed-off-by: Oleg Sadov <[email protected]>
@sadov sadov force-pushed the test_app_nonat branch from ed1a925 to 846fea6 Compare May 12, 2021 14:05
@sadov
Copy link
Contributor Author

sadov commented May 12, 2021

OK - moved to "large".

@mydatascience mydatascience merged commit 5e382ce into lf-edge:master May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giggsoff giggsoff giggsoff requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sadov @zed-sadov @giggsoff @mydatascience