Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Add etcd encryption key #2756

Merged

Conversation

CecileRobertMichon
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon commented Apr 23, 2018

What this PR does / why we need it:

Re-implement #2694 because CLA was not signed.

In @croeck 's words:

Bugfix to deploy a common etcd encryption key on all master nodes.

  • Before this commit, all masters (if there should be more than one) would end up with different encryption keys, resulting in all but one masters to not be ready at all time.
    • calico did not start, complain about invalid padding in the secret
    • /etc/cni/net.d was not present on all but one master
  • This key can either be specified as an option to kubeConfig, or will be generated before deployment.

Test setup

  • Kubernetes 1.10.1
  • custom acs-engine build (this fix on top of Update Calico to 3.1 #2521, which was also rebased on the latest master)
    • Mac OS X 10.12.6
  • 3 masters
  • 2 agent nodes
{
  "apiVersion": "vlabs",
  "properties": {
    "orchestratorProfile": {
      "orchestratorType": "Kubernetes",
      "orchestratorVersion": "1.10.1",
      "kubernetesConfig": {
        "enableDataEncryptionAtRest": true,
        "useManagedIdentity": true,
        "enableRbac": true,
        "networkPolicy": "calico",
        "privateCluster": {
            "enabled": true
        },
        "dnsServiceIP": "10.11.96.96",
        "serviceCidr": "10.11.64.0/18",
        "clusterSubnet": "10.11.128.0/17",
        "etcdDiskSizeGB": "256"
      }
    },
    "aadProfile": {
      "serverAppID": "****",
      "clientAppID": "****",
      "tenantID": "****"
    },
    "masterProfile": {
      "count": 3,
      "dnsPrefix": "dev2",
      "vmSize": "Standard_A4_v2",
      "vnetSubnetId": "/subscriptions/****/resourceGroups/****/providers/Microsoft.Network/virtualNetworks/dev2-k8s-network/subnets/dev2-k8s-master-subnet",
      "firstConsecutiveStaticIP": "10.11.24.10",
      "vnetCidr": "10.11.0.0/16"
    },
    "agentPoolProfiles": [
      {
        "name": "agent",
        "count": 2,
        "vmSize": "Standard_A4_v2",
        "vnetSubnetId": "/subscriptions/****/resourceGroups/****/providers/Microsoft.Network/virtualNetworks/dev2-k8s-network/subnets/dev2-k8s-worker-subnet",
        "availabilityProfile": "AvailabilitySet"
      }
    ],
    "linuxProfile": {
      "adminUsername": "****",
      "ssh": {
        "publicKeys": [
          {
            "keyData": "ssh-rsa ****"
          }
        ]
      }
    },
    "servicePrincipalProfile": {
      "clientId": "****",
      "secret": "****"
    }
  }
}

Which issue this PR fixes

relates to #2521 and #2587
In this PR it is stated that Kubernetes 1.10 was not yet achieved with Calico 3. This was however possible for me.

relates to #2202
Most likely fixes this issue, but I did not test with 1.9 and calico 2.x

  • documentation
  • unit tests
  • tested backward compatibility (ie. deploy with previous version, upgrade with this branch)

@ghost ghost added the in progress label Apr 23, 2018
if o.KubernetesConfig.EtcdEncryptionKey != "" {
_, err = base64.StdEncoding.DecodeString(o.KubernetesConfig.EtcdEncryptionKey)
if err != nil {
return fmt.Errorf("etcdEncryptionKey must be base64 encoded. Please provide a valid base64 encoded value or leave the etcdEncryptionKey empty to auto-generate the value")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ritazh I added base64 encoding validation here as follow up to your comment in #2694, let me know if you know of better way of checking if a string is base64 encoded...

Copy link
Member

Choose a reason for hiding this comment

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

👍

@codecov
Copy link

codecov bot commented Apr 23, 2018

Codecov Report

Merging #2756 into master will decrease coverage by 0.01%.
The diff coverage is 37.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2756      +/-   ##
==========================================
- Coverage   46.93%   46.92%   -0.02%     
==========================================
  Files          86       86              
  Lines       12786    12802      +16     
==========================================
+ Hits         6001     6007       +6     
- Misses       6231     6240       +9     
- Partials      554      555       +1
Impacted Files Coverage Δ
pkg/api/types.go 28.88% <ø> (ø) ⬆️
pkg/api/vlabs/types.go 18.57% <ø> (ø) ⬆️
pkg/api/vlabs/validate.go 42.93% <0%> (-0.3%) ⬇️
pkg/api/converterfromapi.go 9.85% <0%> (-0.02%) ⬇️
pkg/api/convertertoapi.go 56.56% <100%> (+0.05%) ⬆️
pkg/acsengine/engine.go 62.78% <33.33%> (-0.05%) ⬇️
pkg/acsengine/defaults.go 62.74% <57.14%> (-0.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 244055e...a4c1297. Read the comment docs.

Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

lgtm

@CecileRobertMichon
Copy link
Contributor Author

CecileRobertMichon commented Apr 30, 2018

Copy link
Member

@ritazh ritazh left a comment

Choose a reason for hiding this comment

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

LGTM

@CecileRobertMichon CecileRobertMichon merged commit a3a804d into Azure:master Apr 30, 2018
@ghost ghost removed the in progress label Apr 30, 2018
@CecileRobertMichon CecileRobertMichon deleted the etcd-encryption-key branch June 6, 2018 19:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants