Skip to content

Azure: Allow customer provisioned virtual networks & subnets#2441

Merged
openshift-merge-robot merged 1 commit into
openshift:masterfrom
jhixson74:master_azure_bring_your_own_vnet
Oct 14, 2019
Merged

Azure: Allow customer provisioned virtual networks & subnets#2441
openshift-merge-robot merged 1 commit into
openshift:masterfrom
jhixson74:master_azure_bring_your_own_vnet

Conversation

@jhixson74

@jhixson74 jhixson74 commented Oct 1, 2019

Copy link
Copy Markdown
Member

Add NetworkResourceGroupName, VirtualNetwork, ControlPlaneSubnet, and
ComputeSubnet to the Azure platform struct and pass them to terraform. This is
the initial implementation, therefore, there is no error checking, isolation,
destroying, or terminal prompts. This is for install-config only.

For existing virtual networks, the installer will no longer create route tables, subnets,
virtual networks or network security groups.

https://jira.coreos.com/browse/CORS-1206

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 1, 2019
Comment thread pkg/asset/cluster/tfvars.go Outdated
@jhixson74
jhixson74 force-pushed the master_azure_bring_your_own_vnet branch 2 times, most recently from 4deca35 to 8fbef2f Compare October 2, 2019 01:08
@abhinavdahiya

Copy link
Copy Markdown
Contributor

/test e2e-azure

@jhixson74
jhixson74 force-pushed the master_azure_bring_your_own_vnet branch from 8fbef2f to dc252ba Compare October 2, 2019 17:56
@jhixson74

Copy link
Copy Markdown
Member Author

/test e2e-azure

Comment thread pkg/asset/machines/azure/machines.go Outdated
Comment thread pkg/tfvars/azure/azure.go Outdated
@jhixson74
jhixson74 force-pushed the master_azure_bring_your_own_vnet branch 7 times, most recently from 435177f to faefead Compare October 2, 2019 23:07
@jhixson74 jhixson74 changed the title Azure: [WIP] Allow customer provisioned virtual networks & subnets Azure: Allow customer provisioned virtual networks & subnets Oct 2, 2019
@jhixson74

Copy link
Copy Markdown
Member Author

/test e2e-azure

@jstuever jstuever left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I had a few questions. I'd also like to see @patrickdillon review considering he did this for gcp.

Comment thread data/data/azure/bootstrap/main.tf Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this rule only dependent on preexisting_network?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In Azure, only one network security group can be attached to a vnet. Since we are depending on an existing vnet, we can't create network security groups or network security rules. So if a vnet exists already, we condition this rule (and others) to not be created.

Comment thread data/data/azure/main.tf Outdated
Comment thread data/data/azure/main.tf Outdated
Comment thread data/data/azure/main.tf Outdated
Comment on lines 153 to 163

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can this also be part of the vnet submodule?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've placed this here since this is where the vnet resource is created. If we can confirm it can be safely moved, I'll move it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment thread data/data/azure/vnet/nsg.tf Outdated
Comment on lines 2 to 3

@jstuever jstuever Oct 3, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we really need and/or want to only deploy security groups when not byo vnet?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, reasoning as described above.

Comment thread data/data/azure/vnet/common.tf Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't see where this is used other than output. Perhaps consolidate all into output?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I now see where it is 'like gcp'. It was used there because the subnet creation was handled in the vpc module. However, here that is done in the main module. This would need to stay if it gets moved down into the vnet module like recommended elsewhere. Otherwise it can be removed and output can use var.virtual_network instead.

Comment thread data/data/azure/vnet/vnet.tf Outdated
@jstuever

jstuever commented Oct 3, 2019

Copy link
Copy Markdown
Contributor

/cc @patrickdillon

Comment thread data/data/azure/main.tf Outdated
Comment thread data/data/azure/vnet/variables.tf Outdated
Comment thread data/data/azure/vnet/internal-lb.tf Outdated
Comment thread pkg/tfvars/azure/azure.go Outdated
@jhixson74
jhixson74 force-pushed the master_azure_bring_your_own_vnet branch 2 times, most recently from 5078525 to c20f9bc Compare October 3, 2019 22:47
Comment thread data/data/azure/vnet/outputs.tf Outdated
Comment thread data/data/azure/bootstrap/variables.tf Outdated
@abhinavdahiya

Copy link
Copy Markdown
Contributor

/test e2e-azure

@jhixson74
jhixson74 force-pushed the master_azure_bring_your_own_vnet branch from cc42082 to ff034bd Compare October 10, 2019 20:15
@abhinavdahiya

Copy link
Copy Markdown
Contributor

/test e2e-azure

@jhixson74
jhixson74 force-pushed the master_azure_bring_your_own_vnet branch from ff034bd to b7832d2 Compare October 10, 2019 22:41
@jhixson74

Copy link
Copy Markdown
Member Author

/test e2e-azure

@jhixson74
jhixson74 force-pushed the master_azure_bring_your_own_vnet branch from b7832d2 to ee2dc6f Compare October 10, 2019 22:54
@jhixson74

Copy link
Copy Markdown
Member Author

/test e2e-azure

@abhinavdahiya

Copy link
Copy Markdown
Contributor

/retest

@jhixson74
jhixson74 force-pushed the master_azure_bring_your_own_vnet branch from ee2dc6f to c9feab7 Compare October 11, 2019 16:53
@jhixson74

Copy link
Copy Markdown
Member Author

/test e2e-azure

Comment thread data/data/azure/dns/variables.tf Outdated
Comment thread data/data/azure/bootstrap/variables.tf Outdated
Comment thread data/data/azure/bootstrap/main.tf Outdated
@jhixson74
jhixson74 force-pushed the master_azure_bring_your_own_vnet branch from c9feab7 to 47374f3 Compare October 11, 2019 17:03
Add NetworkResourceGroupName, VirtualNetwork, ControlPlaneSubnet, and
ComputeSubnet to the Azure platform struct and pass them to terraform. This is
the initial implementation, therefore, there is no error checking, isolation,
destroying, or terminal prompts. This is for install-config only.

For existing virtual networks, the installer will no longer create subnets,
route tables, virtual networks and network security groups.

https://jira.coreos.com/browse/CORS-1206
@jhixson74
jhixson74 force-pushed the master_azure_bring_your_own_vnet branch from 47374f3 to f84c103 Compare October 11, 2019 18:08
@jhixson74

Copy link
Copy Markdown
Member Author

/test e2e-azure

abhinavdahiya added a commit to abhinavdahiya/api that referenced this pull request Oct 11, 2019
The newtork resources like Virtual Network and Subnets are created currently for each cluster in the cluster's resource group.

To support reusing exisitng Virtual network and Subnets for OpenShift clusters, the cluster uses pre-existing ones in a separate resource group.

`networkResourceGroupName` is the resource group that contains the Virtual network and subnets for the cluster.

The resource group for network resources is the same as cluster unless specifically specified, therefore, this field is optional.

This is based on work in PR openshift/installer#2441
@jhixson74

jhixson74 commented Oct 11, 2019

Copy link
Copy Markdown
Member Author

To test this, I have created the following resources:
resource group: jhixson_vnet_rg
virtual network: jhixson_test_vnet (10.0.0.0/16)
subnet: jhixson_test_master_subnet (10.0.0.0/19)
subnet: jhixson_test_worker_subnet (10.0.32.0/19)
network security group: jhixson_test_master_nsg (22, 6443)
network security group: jhixson_test_worker_nsg (22, 80, 443, 6443)

All resources are created using the jhixson_vnet_rg resource group. I am testing with the following config:

platform:
  azure:
    baseDomainResourceGroupName: os4-common
    region: centralus
    networkResourceGroupName: jhixson_vnet_rg
    virtualNetwork: jhixson_test_vnet
    controlPlaneSubnet: jhixson_test_master_subnet
    computeSubnet: jhixson_test_worker_subnet

Once the installer is kicked off, I run this script:

while true; do
        __appid="$(oc --config=${INSTALL_DIR}/auth/kubeconfig \
                get credentialsrequests -n \
                openshift-cloud-credential-operator \
                openshift-machine-api-azure \
                -ojsonpath='{.status.providerStatus.appID}{"\n"}')"

        if [ -n "${__appid}" ]
        then
                az role assignment create --resource-group jhixson_vnet_rg --assignee "${__appid}" --role 'Contributor'
                exit 0
        fi
        sleep 5
done

This is a current workaround so that machine-api can read the subnets in the network resource group. I believe there is a PR to fix this by @abhinavdahiya in the works.

The install is successful. The install log can be viewed here.

@abhinavdahiya

abhinavdahiya commented Oct 14, 2019

Copy link
Copy Markdown
Contributor

Following the install instructions from #2441 (comment)

So to test that service type LoadBalancer is working.

Step 1: Create an application from aks-example

$ cat > azure-lb-test.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: azure-vote-back
spec:
  replicas: 1
  selector:
    matchLabels:
      app: azure-vote-back
  template:
    metadata:
      labels:
        app: azure-vote-back
    spec:
      nodeSelector:
        "beta.kubernetes.io/os": linux
      containers:
      - name: azure-vote-back
        image: redis
        resources:
          requests:
            cpu: 100m
            memory: 128Mi
          limits:
            cpu: 250m
            memory: 256Mi
        ports:
        - containerPort: 6379
          name: redis
---
apiVersion: v1
kind: Service
metadata:
  name: azure-vote-back
spec:
  ports:
  - port: 6379
  selector:
    app: azure-vote-back
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: azure-vote-front
spec:
  replicas: 1
  selector:
    matchLabels:
      app: azure-vote-front
  template:
    metadata:
      labels:
        app: azure-vote-front
    spec:
      nodeSelector:
        "beta.kubernetes.io/os": linux
      containers:
      - name: azure-vote-front
        image: microsoft/azure-vote-front:v1
        resources:
          requests:
            cpu: 100m
            memory: 128Mi
          limits:
            cpu: 250m
            memory: 256Mi
        ports:
        - containerPort: 80
        env:
        - name: REDIS
          value: "azure-vote-back"
---
apiVersion: v1
kind: Service
metadata:
  name: azure-vote-front
spec:
  type: LoadBalancer
  ports:
  - port: 80
  selector:
    app: azure-vote-front
---
apiVersion: v1
kind: Service
metadata:
  name: azure-vote-front-int
  annotations:
    service.beta.kubernetes.io/azure-load-balancer-internal: "true"
spec:
  type: LoadBalancer
  ports:
  - port: 80
  selector:
    app: azure-vote-front

$ oc apply -f azure-lb-test.yaml

Step 2: find the LB IPs

$ oc get svc
NAME               TYPE           CLUSTER-IP       EXTERNAL-IP                            PORT(S)        AGE
azure-vote-back    ClusterIP      172.30.214.88    <none>                                 6379/TCP       5m52s
azure-vote-front   LoadBalancer   172.30.194.10    52.189.68.98                           80:30626/TCP   5m52s
internal-app       LoadBalancer   172.30.236.168   10.0.32.10                             80:32431/TCP   2m35s
kubernetes         ClusterIP      172.30.0.1       <none>                                 443/TCP        3d22h
openshift          ExternalName   <none>           kubernetes.default.svc.cluster.local   <none>         3d21h

Step 3: Access the public IP for LB

$ curl http://52.189.68.98
<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
    <link rel="stylesheet" type="text/css" href="/static/default.css">
    <title>Azure Voting App</title>

    <script language="JavaScript">
        function send(form){
        }
    </script>

</head>
<body>
    <div id="container">
        <form id="form" name="form" action="/"" method="post"><center>
        <div id="logo">Azure Voting App</div>
        <div id="space"></div>
        <div id="form">
        <button name="vote" value="Cats" onclick="send()" class="button button1">Cats</button>
        <button name="vote" value="Dogs" onclick="send()" class="button button2">Dogs</button>
        <button name="vote" value="reset" onclick="send()" class="button button3">Reset</button>
        <div id="space"></div>
        <div id="space"></div>
        <div id="results"> Cats - 1 | Dogs - 1 </div>
        </form>
        </div>
    </div>
</body>
</html>

Step 4: Jump on the cluster to access the internal endpoint.

$ oc debug node/adahiya-4-q6jfs-worker-centralus1-shn29 --image fedora
Starting pod/adahiya-4-q6jfs-worker-centralus1-shn29-debug ...
To use host binaries, run `chroot /host`
Pod IP: 10.0.32.4
If you don't see a command prompt, try pressing enter.
sh-5.0# curl
curl: try 'curl --help' or 'curl --manual' for more information
sh-5.0# curl http://10.0.32.10
<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
    <link rel="stylesheet" type="text/css" href="/static/default.css">
    <title>Azure Voting App</title>

    <script language="JavaScript">
        function send(form){
        }
    </script>

</head>
<body>
    <div id="container">
        <form id="form" name="form" action="/"" method="post"><center>
        <div id="logo">Azure Voting App</div>
        <div id="space"></div>
        <div id="form">
        <button name="vote" value="Cats" onclick="send()" class="button button1">Cats</button>
        <button name="vote" value="Dogs" onclick="send()" class="button button2">Dogs</button>
        <button name="vote" value="reset" onclick="send()" class="button button3">Reset</button>
        <div id="space"></div>
        <div id="space"></div>
        <div id="results"> Cats - 1 | Dogs - 1 </div>
        </form>
        </div>
    </div>
</body>
sh-5.0# curl http://52.189.68.98
<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
    <link rel="stylesheet" type="text/css" href="/static/default.css">
    <title>Azure Voting App</title>

    <script language="JavaScript">
        function send(form){
        }
    </script>

</head>
<body>
    <div id="container">
        <form id="form" name="form" action="/"" method="post"><center>
        <div id="logo">Azure Voting App</div>
        <div id="space"></div>
        <div id="form">
        <button name="vote" value="Cats" onclick="send()" class="button button1">Cats</button>
        <button name="vote" value="Dogs" onclick="send()" class="button button2">Dogs</button>
        <button name="vote" value="reset" onclick="send()" class="button button3">Reset</button>
        <div id="space"></div>
        <div id="space"></div>
        <div id="results"> Cats - 1 | Dogs - 1 </div>
        </form>
        </div>
    </div>
</body>
</html>sh-5.0#

This satisfies all the requirements from #2441 (comment)

@abhinavdahiya

Copy link
Copy Markdown
Contributor

/test e2e-azure

@abhinavdahiya

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2019
@openshift-ci-robot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, jhixson74

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 14, 2019
@openshift-ci-robot

Copy link
Copy Markdown
Contributor

@jhixson74: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-aws-disruptive 57be82d6abf9aca39ecf0444d472d09d2c43c7d3 link /test e2e-aws-disruptive
ci/prow/e2e-aws-scaleup-rhel7 f84c103 link /test e2e-aws-scaleup-rhel7
ci/prow/e2e-azure f84c103 link /test e2e-azure

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

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. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants