Skip to content

Commit

Permalink
apply review suggestions
Browse files Browse the repository at this point in the history 
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
  • Loading branch information
@kvaps
kvaps committed Sep 4, 2023
1 parent 5e3be24 commit 6c22622
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 121 deletions.
27 changes: 13 additions & 14 deletions pkg/cloud-init/cloud-init.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,22 +163,21 @@ func ReadCloudInitVolumeDataSource(vmi *v1.VirtualMachineInstance, secretSourceD
return nil, nil
}

func resolveSSHPublicKeys(accessCredentials []v1.AccessCredential, secretSourceDir string, methodNoCloud, methodConfigDrive bool) (map[string]string, error) {
func isNoCloudAccessCredential(accessCred v1.AccessCredential) bool {
return accessCred.SSHPublicKey != nil && accessCred.SSHPublicKey.PropagationMethod.NoCloud != nil
}

func isConfigDriveAccessCredential(accessCred v1.AccessCredential) bool {
return accessCred.SSHPublicKey != nil && accessCred.SSHPublicKey.PropagationMethod.ConfigDrive != nil
}

func resolveSSHPublicKeys(accessCredentials []v1.AccessCredential, secretSourceDir string, isAccessCredentialValidFunc func(v1.AccessCredential) bool) (map[string]string, error) {
keys := make(map[string]string)
count := 0
for _, accessCred := range accessCredentials {

switch {
case methodNoCloud:
// check to see if access credential is propagated by no cloud or not
if accessCred.SSHPublicKey == nil || accessCred.SSHPublicKey.PropagationMethod.NoCloud == nil {
continue
}
case methodConfigDrive:
// check to see if access credential is propagated by config drive or not
if accessCred.SSHPublicKey == nil || accessCred.SSHPublicKey.PropagationMethod.ConfigDrive == nil {
continue
}
if !isAccessCredentialValidFunc(accessCred) {
continue
}

secretName := ""
Expand Down Expand Up @@ -222,7 +221,7 @@ func resolveSSHPublicKeys(accessCredentials []v1.AccessCredential, secretSourceD
//
// Note: when using this function, make sure that your code can access the secret volumes.
func resolveNoCloudSecrets(vmi *v1.VirtualMachineInstance, secretSourceDir string) (map[string]string, error) {
keys, err := resolveSSHPublicKeys(vmi.Spec.AccessCredentials, secretSourceDir, true, false)
keys, err := resolveSSHPublicKeys(vmi.Spec.AccessCredentials, secretSourceDir, isNoCloudAccessCredential)
if err != nil {
return keys, err
}
Expand Down Expand Up @@ -261,7 +260,7 @@ func resolveNoCloudSecrets(vmi *v1.VirtualMachineInstance, secretSourceDir strin
//
// Note: when using this function, make sure that your code can access the secret volumes.
func resolveConfigDriveSecrets(vmi *v1.VirtualMachineInstance, secretSourceDir string) (map[string]string, error) {
keys, err := resolveSSHPublicKeys(vmi.Spec.AccessCredentials, secretSourceDir, false, true)
keys, err := resolveSSHPublicKeys(vmi.Spec.AccessCredentials, secretSourceDir, isConfigDriveAccessCredential)
if err != nil {
return keys, err
}
Expand Down
2 changes: 2 additions & 0 deletions pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2723,6 +2723,7 @@ var _ = Describe("Validating VMICreate Admitter", func() {
}
causes := ValidateVirtualMachineInstanceSpec(k8sfield.NewPath("fake"), &vmi.Spec, config)
Expect(causes).To(HaveLen(1))
Expect(causes[0].Message).To(ContainSubstring("requires a noCloud volume to exist"))
})
It("should reject a configDrive ssh access credential when no configDrive volume exists", func() {
vmi := api.NewMinimalVMI("testvmi")
Expand Down Expand Up @@ -2753,6 +2754,7 @@ var _ = Describe("Validating VMICreate Admitter", func() {
}
causes := ValidateVirtualMachineInstanceSpec(k8sfield.NewPath("fake"), &vmi.Spec, config)
Expect(causes).To(HaveLen(1))
Expect(causes[0].Message).To(ContainSubstring("requires a configDrive volume to exist"))
})
It("should reject a ssh access credential without a source", func() {
vmi := api.NewMinimalVMI("testvmi")
Expand Down
148 changes: 54 additions & 94 deletions tests/credentials_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -292,55 +292,19 @@ var _ = Describe("[sig-compute]Guest Access Credentials", decorators.SigCompute,
Eventually(matcher.ThisVMI(vmi), 240*time.Second, 2*time.Second).Should(matcher.HaveConditionTrue(v1.VirtualMachineInstanceUnsupportedAgent))
})
})
Context("with secret and noCloud propagation", func() {
It("[test_id:6224]should have ssh-key under authorized keys", func() {
secretID := "my-pub-key"
userData := fmt.Sprintf(
"#cloud-config\npassword: %s\nchpasswd: { expire: False }\n",
fedoraPassword,
)
vmi := tests.NewRandomVMIWithEphemeralDiskAndNoCloudUserdataHighMemory(cd.ContainerDiskFor(cd.ContainerDiskFedoraTestTooling), userData)
vmi.Namespace = util.NamespaceTestDefault
vmi.Spec.AccessCredentials = []v1.AccessCredential{
{
SSHPublicKey: &v1.SSHPublicKeyAccessCredential{
Source: v1.SSHPublicKeyAccessCredentialSource{
Secret: &v1.AccessCredentialSecretSource{
SecretName: secretID,
},
},
PropagationMethod: v1.SSHPublicKeyAccessCredentialPropagationMethod{
NoCloud: &v1.NoCloudSSHPublicKeyAccessCredentialPropagation{},
},
},
},
}

key1 := "ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEA6NF8iallvQVp22WDkT test-ssh-key1"
key2 := "ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEA6NF8iallvQVp22WDkT test-ssh-key2"
key3 := "ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEA6NF8iallvQVp22WDkT test-ssh-key3"

By("Creating a secret with three ssh keys")
secret := kubev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: secretID,
Namespace: vmi.Namespace,
Labels: map[string]string{
util.SecretLabel: secretID,
},
},
Type: "Opaque",
Data: map[string][]byte{
"my-key1": []byte(key1),
"my-key2": []byte(key2),
"my-key3": []byte(key3),
},
}
_, err := virtClient.CoreV1().Secrets(vmi.Namespace).Create(context.Background(), &secret, metav1.CreateOptions{})
Expect(err).ToNot(HaveOccurred())

tests.RunVMIAndExpectLaunchIgnoreWarnings(vmi, 180)

Context("with secret and cloudInit propagation", func() {
var vmi *v1.VirtualMachineInstance
secretID := "my-pub-key"
userData := fmt.Sprintf(
"#cloud-config\npassword: %s\nchpasswd: { expire: False }\n",
fedoraPassword,
)

key1 := "ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEA6NF8iallvQVp22WDkT test-ssh-key1"
key2 := "ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEA6NF8iallvQVp22WDkT test-ssh-key2"
key3 := "ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEA6NF8iallvQVp22WDkT test-ssh-key3"

verifySSHKeys := func(vmi *v1.VirtualMachineInstance) {
By("Verifying all three pub ssh keys in secret are in VMI guest")
ExecutingBatchCmd(vmi, []expect.Batcher{
&expect.BSnd{S: "\n"},
Expand All @@ -357,35 +321,11 @@ var _ = Describe("[sig-compute]Guest Access Credentials", decorators.SigCompute,
&expect.BSnd{S: "cat /home/fedora/.ssh/authorized_keys\n"},
&expect.BExp{R: "test-ssh-key3"},
}, time.Second*180)
})
})
Context("with secret and configDrive propagation", func() {
It("[test_id:6224]should have ssh-key under authorized keys", func() {
secretID := "my-pub-key"
userData := fmt.Sprintf(
"#cloud-config\npassword: %s\nchpasswd: { expire: False }\n",
fedoraPassword,
)
vmi := tests.NewRandomVMIWithEphemeralDiskAndConfigDriveUserdataHighMemory(cd.ContainerDiskFor(cd.ContainerDiskFedoraTestTooling), userData)
vmi.Namespace = util.NamespaceTestDefault
vmi.Spec.AccessCredentials = []v1.AccessCredential{
{
SSHPublicKey: &v1.SSHPublicKeyAccessCredential{
Source: v1.SSHPublicKeyAccessCredentialSource{
Secret: &v1.AccessCredentialSecretSource{
SecretName: secretID,
},
},
PropagationMethod: v1.SSHPublicKeyAccessCredentialPropagationMethod{
ConfigDrive: &v1.ConfigDriveSSHPublicKeyAccessCredentialPropagation{},
},
},
},
}
}

key1 := "ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEA6NF8iallvQVp22WDkT test-ssh-key1"
key2 := "ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEA6NF8iallvQVp22WDkT test-ssh-key2"
key3 := "ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEA6NF8iallvQVp22WDkT test-ssh-key3"
BeforeEach(func() {
vmi = tests.NewRandomVMIWithEphemeralDiskHighMemory(cd.ContainerDiskFor(cd.ContainerDiskFedoraTestTooling))
vmi.Namespace = util.NamespaceTestDefault

By("Creating a secret with three ssh keys")
secret := kubev1.Secret{
Expand All @@ -405,25 +345,45 @@ var _ = Describe("[sig-compute]Guest Access Credentials", decorators.SigCompute,
}
_, err := virtClient.CoreV1().Secrets(vmi.Namespace).Create(context.Background(), &secret, metav1.CreateOptions{})
Expect(err).ToNot(HaveOccurred())
})

It("[test_id:TODO]should have ssh-key under authorized keys added by NoCloud", func() {
tests.AddCloudInitNoCloudData(vmi, "disk1", userData, "", false)
vmi.Spec.AccessCredentials = []v1.AccessCredential{
{
SSHPublicKey: &v1.SSHPublicKeyAccessCredential{
Source: v1.SSHPublicKeyAccessCredentialSource{
Secret: &v1.AccessCredentialSecretSource{
SecretName: secretID,
},
},
PropagationMethod: v1.SSHPublicKeyAccessCredentialPropagationMethod{
NoCloud: &v1.NoCloudSSHPublicKeyAccessCredentialPropagation{},
},
},
},
}
tests.RunVMIAndExpectLaunchIgnoreWarnings(vmi, 180)

By("Verifying all three pub ssh keys in secret are in VMI guest")
ExecutingBatchCmd(vmi, []expect.Batcher{
&expect.BSnd{S: "\n"},
&expect.BSnd{S: "\n"},
&expect.BExp{R: "login:"},
&expect.BSnd{S: "fedora\n"},
&expect.BExp{R: "Password:"},
&expect.BSnd{S: fedoraPassword + "\n"},
&expect.BExp{R: "\\$"},
&expect.BSnd{S: "cat /home/fedora/.ssh/authorized_keys\n"},
&expect.BExp{R: "test-ssh-key1"},
&expect.BSnd{S: "cat /home/fedora/.ssh/authorized_keys\n"},
&expect.BExp{R: "test-ssh-key2"},
&expect.BSnd{S: "cat /home/fedora/.ssh/authorized_keys\n"},
&expect.BExp{R: "test-ssh-key3"},
}, time.Second*180)
verifySSHKeys(vmi)
})
It("[test_id:6224]should have ssh-key under authorized keys added by configDrive", func() {
tests.AddCloudInitConfigDriveData(vmi, "disk1", userData, "", false)
vmi.Spec.AccessCredentials = []v1.AccessCredential{
{
SSHPublicKey: &v1.SSHPublicKeyAccessCredential{
Source: v1.SSHPublicKeyAccessCredentialSource{
Secret: &v1.AccessCredentialSecretSource{
SecretName: secretID,
},
},
PropagationMethod: v1.SSHPublicKeyAccessCredentialPropagationMethod{
ConfigDrive: &v1.ConfigDriveSSHPublicKeyAccessCredentialPropagation{},
},
},
},
}
tests.RunVMIAndExpectLaunchIgnoreWarnings(vmi, 180)
verifySSHKeys(vmi)
})
})
})
13 changes: 0 additions & 13 deletions tests/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -685,13 +685,6 @@ func NewRandomVMIWithEphemeralDiskAndUserdataHighMemory(containerImage string, u
return vmi
}

func NewRandomVMIWithEphemeralDiskAndNoCloudUserdataHighMemory(containerImage string, userData string) *v1.VirtualMachineInstance {
vmi := NewRandomVMIWithEphemeralDiskAndNoCloudUserdata(containerImage, userData)

vmi.Spec.Domain.Resources.Requests[k8sv1.ResourceMemory] = resource.MustParse("512M")
return vmi
}

func NewRandomVMIWithEphemeralDiskAndConfigDriveUserdataHighMemory(containerImage string, userData string) *v1.VirtualMachineInstance {
vmi := NewRandomVMIWithEphemeralDiskAndConfigDriveUserdata(containerImage, userData)

Expand Down Expand Up @@ -851,12 +844,6 @@ func NewRandomVMIWithEphemeralDiskAndUserdata(containerImage string, userData st
return vmi
}

func NewRandomVMIWithEphemeralDiskAndNoCloudUserdata(containerImage string, userData string) *v1.VirtualMachineInstance {
vmi := NewRandomVMIWithEphemeralDisk(containerImage)
AddCloudInitNoCloudData(vmi, "disk1", userData, "", false)
return vmi
}

func NewRandomVMIWithEphemeralDiskAndConfigDriveUserdata(containerImage string, userData string) *v1.VirtualMachineInstance {
vmi := NewRandomVMIWithEphemeralDisk(containerImage)
AddCloudInitConfigDriveData(vmi, "disk1", userData, "", false)
Expand Down

0 comments on commit 6c22622

Please sign in to comment.