Clear env var so Azure e2e generates a local keypair

[mboersma]

To implement ClusterLogCollector for CAPZ using an SSH mechanism, Azure e2e tests need access to the private key as well. By leaving AZURE_SSH_PUBLIC_KEY_FILE empty, testing will fall back to generating a local keypair just for that run.

This works well as demonstrated in kubernetes-sigs/cluster-api-provider-azure#976.

Since the key materials aren't accessible outside of the prow CI environment, I don't think this introduces a security risk. But please let me know if you disagree or have any other feedback. cc: @nader-ziada @CecileRobertMichon @devigned

[k8s-ci-robot]

Hi @mboersma. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[devigned]

/lgtm

The key is short lived. I feel good about this change.

[CecileRobertMichon]

/ok-to-test

[devigned]

/lgtm cancel

Looks like there is more work to be done.

[devigned]

/approve cancel

[mboersma]

I thought overriding the env var was the least disruptive way, but apparently that won't work.

I think either we could remove the preset-azure-cred label from the e2e task, or remove usage of AZURE_SSH_PUBLIC_KEY_FILE entirely. I don't see how either approach would break things, but let me look deeper.

[CecileRobertMichon]

Pretty sure we need preset-azure-cred for the service principal creds

[mboersma]

I made a new config label that retains everything e2e appears to need, but removes the AZURE_SSH_PUBLIC_KEY_FILE env var.

[devigned]

@mboersma what do you think about reusing preset-azure-cred-only? Just remove the ssh pub key from it. I don't think it's needed there.

[mboersma]

what do you think about reusing preset-azure-cred-only?

Sure, but it appears we need REGISTRY as well (and may be expecting KUBE_VERBOSE=0). I can add those to preset-azure-cred-only instead if that's ok.

[devigned]

I think we should be ok with the default registry via https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/2e100d19787016a78130fd5b9440dedbad754c85/scripts/ci-e2e.sh#L53-L64.

We run other ci-e2e jobs with just preset-azure-cred-only.

[devigned]

/lgtm

[CecileRobertMichon]

are there any other jobs (eg. periodic CAPI e2e) that we're expecting to get logs from that should also be updated?

[mboersma]

Yes, looks like we should do the same for pull-cluster-api-provider-azure-capi-e2e--good thinking. Those are the two places that ./scripts/ci-e2e.sh is called.

[mboersma]

Derp, nevermind--we are already there: that was why preset-azure-cred-only was added in the first place. So I think that covers it.

[CecileRobertMichon]

/lgtm
/assign @chewong @dims

[dims]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devigned, dims, mboersma

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

The pull request process is described here

Needs approval from an approver in each of these files:

• config/OWNERS [dims]

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

[k8s-ci-robot]

@mboersma: Updated the following 2 configmaps:

• job-config configmap in namespace default at cluster test-infra-trusted using the following files:
  • key cluster-api-provider-azure-presubmits.yaml using file config/jobs/kubernetes-sigs/cluster-api-provider-azure/cluster-api-provider-azure-presubmits.yaml
• config configmap in namespace default at cluster test-infra-trusted using the following files:
  • key config.yaml using file config/prow/config.yaml

In response to this:

To implement ClusterLogCollector for CAPZ using an SSH mechanism, Azure e2e tests need access to the private key as well. By leaving AZURE_SSH_PUBLIC_KEY_FILE empty, testing will fall back to generating a local keypair just for that run.

This works well as demonstrated in kubernetes-sigs/cluster-api-provider-azure#976.

Since the key materials aren't accessible outside of the prow CI environment, I don't think this introduces a security risk. But please let me know if you disagree or have any other feedback. cc: @nader-ziada @CecileRobertMichon @devigned

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.