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

The awscli tests for virtual style paths (dnsmasq fix) #278

Merged
merged 4 commits into from
Jun 28, 2018
Merged

The awscli tests for virtual style paths (dnsmasq fix) #278

merged 4 commits into from
Jun 28, 2018

Conversation

Praveenrajmani
Copy link
Contributor

@Praveenrajmani Praveenrajmani commented May 31, 2018

The dnsmasq will now accept wildcard bucket name entries.

The container is spawned by

docker run -e "SERVER_ENDPOINT=192.168.86.133:9000" \ -e "DOMAIN=minio.com" -e "ACCESS_KEY=minio" \ -e "SECRET_KEY=**REDACTED**" -e "ENABLE_HTTPS=0" \ -e "ENABLE_VIRTUAL_STYLE=1" minio/mint awscli

The DOMAIN provided in the env will be resolved to the host ip in SERVER_ENDPOINT

Fixes: #213

nitisht and others added 2 commits November 20, 2017 09:50
Mint awscli tests now run against both path style and virtual style
if virtual style flag is set. By default only path style tests are
run

Fixes #213
Dockerfile Outdated
@@ -10,6 +10,10 @@ ENV GOPATH /usr/local

ENV PATH $GOPATH/bin:$GOROOT/bin:$PATH

COPY dnsmasq.conf /etc/dnsmasq.conf
Copy link
Member

Choose a reason for hiding this comment

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

This is not required.

Dockerfile Outdated
@@ -10,6 +10,10 @@ ENV GOPATH /usr/local

ENV PATH $GOPATH/bin:$GOROOT/bin:$PATH

COPY dnsmasq.conf /etc/dnsmasq.conf

EXPOSE 53/udp
Copy link
Member

Choose a reason for hiding this comment

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

As we are running name server locally, I don't think this is required.

Dockerfile.dev Outdated
@@ -12,7 +12,9 @@ ENV PATH $GOPATH/bin:$GOROOT/bin:$PATH

WORKDIR /mint

RUN apt-get --yes update && apt-get --yes upgrade && apt-get --yes --quiet install wget jq curl
EXPOSE 53/udp
Copy link
Member

Choose a reason for hiding this comment

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

same as above

README.md Outdated
@@ -37,11 +37,34 @@ Below environment variables are required to be passed to the docker container. S

| Environment variable | Description | Example |
|:--- |:--- |:--- |
| `SERVER_ENDPOINT` | Endpoint of Minio server in the format `HOST:PORT` | `play.minio.io:9000` |
| `SERVER_ENDPOINT` | Endpoint of Minio server in the format `HOST:PORT` | `192.168.86.133:9000` |
| `DOMAIN` | Domain name of the server | `minio.com` |
Copy link
Member

Choose a reason for hiding this comment

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

This is optional. Only required when virtual style is used.

README.md Outdated
@@ -37,11 +37,34 @@ Below environment variables are required to be passed to the docker container. S

| Environment variable | Description | Example |
|:--- |:--- |:--- |
| `SERVER_ENDPOINT` | Endpoint of Minio server in the format `HOST:PORT` | `play.minio.io:9000` |
| `SERVER_ENDPOINT` | Endpoint of Minio server in the format `HOST:PORT` | `192.168.86.133:9000` |
Copy link
Member

Choose a reason for hiding this comment

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

IP address is used only if virtual style is used. Otherwise its OK to have name like play.minio.io.

README.md Outdated

- Set a domain in your Minio server using environment variable MINIO_DOMAIN. For example `export MINIO_DOMAIN=minio.com`.
- Start Minio server.
- For testing Minio server via domain `minio.com` or sub-domain name like `bucket-name.minio.com`, the domain should be resolvable from Mint container via `dnsmasq`. Mint uses the domain name provided in `DOMAIN` for testing purposes. And will be resolved to the host ip in the `SERVER_ENDPOINT`.
Copy link
Member

Choose a reason for hiding this comment

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

This step (explanation) is not required. If you want to keep it documented, move above as description.

README.md Outdated
- Set a domain in your Minio server using environment variable MINIO_DOMAIN. For example `export MINIO_DOMAIN=minio.com`.
- Start Minio server.
- For testing Minio server via domain `minio.com` or sub-domain name like `bucket-name.minio.com`, the domain should be resolvable from Mint container via `dnsmasq`. Mint uses the domain name provided in `DOMAIN` for testing purposes. And will be resolved to the host ip in the `SERVER_ENDPOINT`.
- You can execute Mint against Minio server (with `MINIO_DOMAIN` set to `minio.com`) using this command,
Copy link
Member

Choose a reason for hiding this comment

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

You would need to add MINIO_DOMAIN, ENABLE_VIRTUAL_STYLE and keep SERVER_ENDPOINT contains IP:PORT

# run tests
endpoint="http://$SERVER_ENDPOINT"
SERVER_IP=`echo "${SERVER_ENDPOINT%%:*}"`
SERVER_PORT=`echo "${SERVER_ENDPOINT/*:/}"`
Copy link
Member

Choose a reason for hiding this comment

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

No need to have echo. Similarly these info is required only when ENABLE_VIRTUAL_STYLE is on.

You would need to validate whether SERVER_IP is really an IPv4 address or not. If not you would need to error out and fail. So this needs to be in mint.sh

if [ "$ENABLE_HTTPS" -eq 1 ]; then
endpoint="https://$SERVER_ENDPOINT"
endpoint="https://$DOMAIN:$SERVER_PORT"
Copy link
Member

Choose a reason for hiding this comment

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

you don't need to change endpoint i.e. this is correct for path style.

# check the access style and run tests if virtual style is set
if [ "$ENABLE_VIRTUAL_STYLE" -eq 1 ]; then
dnsmasq --address=/$DOMAIN/$SERVER_IP --user=root
echo -e "nameserver 127.0.0.1\n$(cat /etc/resolv.conf)" > /etc/resolv.conf
Copy link
Member

Choose a reason for hiding this comment

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

below code is much better than echo style.

cat - /etc/resolv.conf > /etc/resolv.conf <<EOF
nameserver 127.0.0.1
EOF

@Praveenrajmani
Copy link
Contributor Author

@nitisht @balamurugana Changes updated.

README.md Outdated
@@ -37,11 +37,35 @@ Below environment variables are required to be passed to the docker container. S

| Environment variable | Description | Example |
|:--- |:--- |:--- |
| `SERVER_ENDPOINT` | Endpoint of Minio server in the format `HOST:PORT` | `play.minio.io:9000` |
| `SERVER_ENDPOINT` | Endpoint of Minio server in the format `HOST:PORT`. Should be `IP:PORT` for virtual-style tests | `play.minio.io:900` |
Copy link
Member

Choose a reason for hiding this comment

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

  1. Below text is better
    Endpoint of Minio server in the format HOST:PORT; for virtual style IP:PORT

  2. Fix the example; play.minio.io:9000

README.md Outdated
@@ -37,11 +37,35 @@ Below environment variables are required to be passed to the docker container. S

| Environment variable | Description | Example |
|:--- |:--- |:--- |
| `SERVER_ENDPOINT` | Endpoint of Minio server in the format `HOST:PORT` | `play.minio.io:9000` |
| `SERVER_ENDPOINT` | Endpoint of Minio server in the format `HOST:PORT`. Should be `IP:PORT` for virtual-style tests | `play.minio.io:900` |
| `DOMAIN` | (OPTIONAL) (Required for virtual-style tests) Domain name of the server | `minio.com` |
Copy link
Member

Choose a reason for hiding this comment

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

  1. Below text is better
    Value of MINIO_DOMAIN environment variable used in Minio server
  2. Use better example'; myminio.com

README.md Outdated
| `ACCESS_KEY` | Access key of access `SERVER_ENDPOINT` | `Q3AM3UQ867SPQQA43P2F` |
| `SECRET_KEY` | Secret Key of access `SERVER_ENDPOINT` | `zuf+tfteSlswRu7BJ86wekitnifILbZam1KYY3TG` |
| `ENABLE_HTTPS` | (Optional) Set `1` to indicate to use HTTPS to access `SERVER_ENDPOINT`. Defaults to `0` (HTTP) | `1` |
| `MINT_MODE` | (Optional) Set mode indicating what category of tests to be run by values `core` or `full`. Defaults to `core` | `full` |
| `ENABLE_VIRTUAL_STYLE` | (Optional) Set `1` to enable virtual style access to `SERVER_ENDPOINT`. Defaults to `0` (Path style) | `1` |
Copy link
Member

Choose a reason for hiding this comment

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

Set 1 to indicate to use virtual style access ...

README.md Outdated

### Test virtual style access against Minio server

For testing Minio server via domain `minio.com` or sub-domain name like `bucket-name.minio.com`, the domain should be resolvable from Mint container via `dnsmasq`. Mint uses the domain name provided in `DOMAIN` for testing purposes. And will be resolved to the host ip in the `SERVER_ENDPOINT`.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this text is helpful for mint users. We could remove

README.md Outdated

To test Minio server virtual style access with Mint, follow these steps:

- Set a domain in your Minio server using environment variable MINIO_DOMAIN. For example `export MINIO_DOMAIN=minio.com`.
Copy link
Member

Choose a reason for hiding this comment

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

Change minio.com into myminio.com

README.md Outdated


```sh
$ docker run -e "SERVER_ENDPOINT=192.168.86.133:9000" \
Copy link
Member

Choose a reason for hiding this comment

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

Format this command as same as in other places.

README.md Outdated
-e "ACCESS_KEY=minio" \
-e "SECRET_KEY=minio123" \
-e "ENABLE_HTTPS=0" \
-e "ENABLE_VIRTUAL_STYLE=1" minio/mint awscli
Copy link
Member

Choose a reason for hiding this comment

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

no need to specify awscli

README.md Outdated

- Set a domain in your Minio server using environment variable MINIO_DOMAIN. For example `export MINIO_DOMAIN=minio.com`.
- Start Minio server.
- You can execute Mint against Minio server (with `MINIO_DOMAIN` set to `minio.com`) using this command
Copy link
Member

Choose a reason for hiding this comment

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

Use one style like i.e. don't use you can execute

mint.sh Outdated
export SERVER_REGION
export ENABLE_HTTPS
Copy link
Member

Choose a reason for hiding this comment

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

No need to move this line from previous position.

if [ "$ENABLE_HTTPS" -eq 1 ]; then
endpoint="https://$SERVER_ENDPOINT"

endpoint="https://$SERVER_ENDPOINT"
Copy link
Member

Choose a reason for hiding this comment

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

You should not remove HTTPS check


# check the access style and run tests if virtual style is set
if [ "$ENABLE_VIRTUAL_STYLE" -eq 1 ]; then

Copy link
Member

Choose a reason for hiding this comment

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

remove this blank line

if [ "$ENABLE_VIRTUAL_STYLE" -eq 1 ]; then

SERVER_IP=${SERVER_ENDPOINT%%:*}
SERVER_PORT=${SERVER_ENDPOINT/*:/}
Copy link
Member

Choose a reason for hiding this comment

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

Always use double quotes

SERVER_PORT=${SERVER_ENDPOINT/*:/}

# validate the server_ip
if [[ $SERVER_IP =~ ^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+$ ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

This if is not required because you are splitting it by . anyway. My recommendation is to use array than list of strings below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@balamurugana The condition is used to check whether the ip has four dots . Else it exits with an error

Copy link
Member

Choose a reason for hiding this comment

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

As you split the string by ., you don't need to do the check. Below code is good enough

readarray -td. octets <<< "$SERVER_IP"
if [ "${#octets[@]}" -ne 4 ]; then
	echo "$SERVER_IP must be an IP address"
	exit 1
fi
for octet in "${octets[@]}"; do
	if [ "$octet" -lt 0 ] 2>/dev/null || [ "$octet" -gt 255 ] 2>/dev/null; then
		echo "$SERVER_IP must be an IP address"
		exit 1
	fi
done

As mentioned that, this check needs to be moved to mint.sh to fail earlier and later enhancement for other tests use virtual style

# validate the server_ip
if [[ $SERVER_IP =~ ^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+$ ]]; then
for OCTET in $(echo "$SERVER_IP" | tr '.' ' '); do
if [[ $OCTET -lt 0 || $OCTET -gt 255 ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

  1. do not use OCTET. all caps mean exported/global variable as a convention.
  2. this check should be done in mint.sh so that you fail early.

endpoint="https://$DOMAIN:$SERVER_PORT"
fi

dnsmasq --address=/"$DOMAIN"/"$SERVER_IP" --user=root
Copy link
Member

Choose a reason for hiding this comment

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

use "/$DOMAIN/$SERVER_IP" this is much cleaner

fi

# run path style tests
aws configure set default.s3.addressing_style path
Copy link
Member

Choose a reason for hiding this comment

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

move this if block after ./test.sh ... i.e. you are resetting back as you set to virtual style before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so you mean we have to execute path style tests first and then virtual style (only If the flag is set to 1) ?? @balamurugana

Copy link
Member

Choose a reason for hiding this comment

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

I was saying add below code into line#31

if [ "$ENABLE_VIRTUAL_STYLE" -eq 1 ]; then
	## setup local DNS server to resolve *.domain.name into IP address
	dnsmasq --address="/$DOMAIN/$SERVER_IP" --user=root
	sed -i "1inameserver 127.0.0.1" /etc/resolv.conf

	endpoint="http://$DOMAIN:$SERVER_PORT"
	if [ "$ENABLE_HTTPS" -eq 1 ]; then
		endpoint="https://$DOMAIN:$SERVER_PORT"
	fi

	aws configure set default.s3.addressing_style virtual
	./test.sh "$endpoint"  1>>"$output_log_file" 2>"$error_log_file"
	aws configure set default.s3.addressing_style path
fi

This way it is completely fused without disturbed existing code flow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@balamurugana So you mean the addressing_style is path by default ??
If not, the above change will not execute path style tests , As we are setting the styleaws configure set default.s3.addressing_style path only if the virtual flag is set .
If the flag is not set , the addressing style will not be set to path at all .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@balamurugana i think i misunderstood the change request here

@Praveenrajmani
Copy link
Contributor Author

@balamurugana @nitisht The changes are done .

Copy link
Contributor

@nitisht nitisht left a comment

Choose a reason for hiding this comment

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

LGTM overall, few doc changes

README.md Outdated
| `ACCESS_KEY` | Access key of access `SERVER_ENDPOINT` | `Q3AM3UQ867SPQQA43P2F` |
| `SECRET_KEY` | Secret Key of access `SERVER_ENDPOINT` | `zuf+tfteSlswRu7BJ86wekitnifILbZam1KYY3TG` |
| `ENABLE_HTTPS` | (Optional) Set `1` to indicate to use HTTPS to access `SERVER_ENDPOINT`. Defaults to `0` (HTTP) | `1` |
| `MINT_MODE` | (Optional) Set mode indicating what category of tests to be run by values `core` or `full`. Defaults to `core` | `full` |
| `ENABLE_VIRTUAL_STYLE` | (Optional) Set `1` to indicate virtual style access . Defaults to `0` (Path style) | `1` |
Copy link
Contributor

Choose a reason for hiding this comment

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

you can club the optional flags DOMAIN and ENABLE_VIRTUAL_STYLE together at the last. Also don't use default values for optional fields and use optional (instead of OPTIONAL or Optional)

README.md Outdated
- Start Minio server.
- Execute Mint against Minio server (with `MINIO_DOMAIN` set to `myminio.com`) using this command


Copy link
Contributor

Choose a reason for hiding this comment

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

remove extra line

mint.sh Outdated
SERVER_IP="${SERVER_ENDPOINT%%:*}"
SERVER_PORT="${SERVER_ENDPOINT/*:/}"
# validate the server_ip
octets=(${SERVER_IP//./ })
Copy link
Member

Choose a reason for hiding this comment

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

Use quotes here "${SERVER_IP//./ }"

mint.sh Outdated
echo

echo "To get logs, run 'docker cp ${CONTAINER_ID}:/mint/log /tmp/mint-logs'"

Copy link
Member

Choose a reason for hiding this comment

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

remove extra whitespaces

README.md Outdated
@@ -37,12 +37,34 @@ Below environment variables are required to be passed to the docker container. S

| Environment variable | Description | Example |
|:--- |:--- |:--- |
| `SERVER_ENDPOINT` | Endpoint of Minio server in the format `HOST:PORT` | `play.minio.io:9000` |
| `SERVER_ENDPOINT` | Endpoint of Minio server in the format `HOST:PORT`; for virtual style `IP:PORT` | `play.minio.io:900` |
| `DOMAIN` | (OPTIONAL) (Required for virtual-style tests) Value of MINIO_DOMAIN environment variable used in Minio server | `myminio.com` |
Copy link
Member

Choose a reason for hiding this comment

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

Make Optional and remove (Required ...) which is conflicting with optional

@@ -29,9 +29,25 @@ aws configure set aws_access_key_id "$ACCESS_KEY"
aws configure set aws_secret_access_key "$SECRET_KEY"
aws configure set default.region "$SERVER_REGION"

# run tests

Copy link
Member

Choose a reason for hiding this comment

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

Remove this empty line

endpoint="http://$SERVER_ENDPOINT"
if [ "$ENABLE_HTTPS" -eq 1 ]; then
endpoint="https://$SERVER_ENDPOINT"
fi

# check the access style and run tests if virtual style is set
Copy link
Member

Choose a reason for hiding this comment

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

  1. # run tests for virtual style if provided is good enough
  2. Also move this block above endpoint declaration so that its cleanly patched

@Praveenrajmani
Copy link
Contributor Author

@balamurugana @nitisht the changes are done

README.md Outdated
@@ -37,12 +37,33 @@ Below environment variables are required to be passed to the docker container. S

| Environment variable | Description | Example |
|:--- |:--- |:--- |
| `SERVER_ENDPOINT` | Endpoint of Minio server in the format `HOST:PORT` | `play.minio.io:9000` |
| `SERVER_ENDPOINT` | Endpoint of Minio server in the format `HOST:PORT`; for virtual style `IP:PORT` | `play.minio.io:900` |
Copy link
Member

Choose a reason for hiding this comment

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

port is 9000

README.md Outdated
| `ENABLE_HTTPS` | (optional) Set `1` to indicate to use HTTPS to access `SERVER_ENDPOINT`. Defaults to `0` (HTTP) | `1` |
| `MINT_MODE` | (optional) Set mode indicating what category of tests to be run by values `core` or `full`. Defaults to `core` | `full` |
| `DOMAIN` | (optional) Value of MINIO_DOMAIN environment variable used in Minio server | `myminio.com` |
| `ENABLE_VIRTUAL_STYLE` | (optional) Set `1` to indicate virtual style access . Defaults to `0` (Path style) | `1` |
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why change Optional to optional. I specifically added it for readability.

README.md Outdated
-e "ACCESS_KEY=minio" \
-e "SECRET_KEY=minio123" \
-e "ENABLE_HTTPS=0" \
-e "ENABLE_VIRTUAL_STYLE=1" minio/mint
Copy link
Member

Choose a reason for hiding this comment

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

Fix formatting

README.md Outdated
- Start Minio server.
- Execute Mint against Minio server (with `MINIO_DOMAIN` set to `myminio.com`) using this command
```sh

Copy link
Member

Choose a reason for hiding this comment

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

Remove this empty line

README.md Outdated
-e "ENABLE_VIRTUAL_STYLE=1" minio/mint
```

We set the `ENABLE_VIRTUAL_STYLE` flag to `1` to indicate that virtual style tests be run.
Copy link
Member

Choose a reason for hiding this comment

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

As the section/information is provided already, this text is not required.

mint.sh Outdated
if [ "$ENABLE_VIRTUAL_STYLE" -eq 1 ]; then
SERVER_IP="${SERVER_ENDPOINT%%:*}"
SERVER_PORT="${SERVER_ENDPOINT/*:/}"
# validate the server_ip
Copy link
Member

Choose a reason for hiding this comment

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

Check if SERVER_IP is actually IPv4 address

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@balamurugana we are already checking that right ?

Copy link
Member

Choose a reason for hiding this comment

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

I meant the comment is wrong # validate the server_ip

This will now facilitate the randomized wildcard bucket names
in awscli tests (virtual style paths).

Fixes #213
@Praveenrajmani
Copy link
Contributor Author

@balamurugana The changes are done. Please check

@balamurugana
Copy link
Member

LGTM. Please resolve conflicts.

Copy link
Contributor

@nitisht nitisht left a comment

Choose a reason for hiding this comment

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

LGTM

@harshavardhana harshavardhana merged commit 37a6cd8 into minio:master Jun 28, 2018
@Amirreza1396
Copy link

The dnsmasq will now accept wildcard bucket name entries.

The container is spawned by

docker run -e "SERVER_ENDPOINT=192.168.86.133:9000" \ -e "DOMAIN=minio.com" -e "ACCESS_KEY=minio" \ -e "SECRET_KEY=**REDACTED**" -e "ENABLE_HTTPS=0" \ -e "ENABLE_VIRTUAL_STYLE=1" minio/mint awscli

The DOMAIN provided in the env will be resolved to the host ip in SERVER_ENDPOINT

Fixes: #213

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test for virtual host style requests
5 participants