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

Use AzureClusterIdentity when running ci e2e tests #1360

Merged
merged 7 commits into from
Jul 9, 2021

Conversation

nader-ziada
Copy link
Contributor

@nader-ziada nader-ziada commented May 4, 2021

What type of PR is this?
/kind feature

What this PR does / why we need it:

  • use AzureClusterIdentity in ci to make sure it working in all cases

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

- Update aad-pod-identity to v1.8.0 which upgrades CRDs from apiextensions/v1beta1 to apiextensions/v1
- Update calico version used by templates and tests

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 4, 2021
@k8s-ci-robot k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 4, 2021
@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e

@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e

1 similar comment
@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e

@nader-ziada
Copy link
Contributor Author

the private cluster test specifically is failing every time, looking into it

@nader-ziada
Copy link
Contributor Author

the private cluster test specifically is failing every time, looking into it

I believe the private cluster test is failing because of this issue here https://azure.github.io/aad-pod-identity/docs/troubleshooting/#token-requests-calls-fail-with-io-timeout

@nader-ziada nader-ziada force-pushed the ci-identity branch 2 times, most recently from 5f08411 to fde19c6 Compare May 21, 2021 19:48
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 21, 2021
@nader-ziada nader-ziada force-pushed the ci-identity branch 3 times, most recently from 8d97990 to 2feac5d Compare May 25, 2021 16:43
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2021
@nader-ziada
Copy link
Contributor Author

/test pull-cluster-api-provider-azure-e2e-windows

@nader-ziada
Copy link
Contributor Author

more info about issues with tests failing in this PR can be found in this slack thread https://kubernetes.slack.com/archives/CEX9HENG7/p1621362051002500

tldr;

  • the nmi pod is having dns issues and can't authenticate when the management cluster is running on the Azure (not on a kind cluster) as in the case of the private cluster test. The dns issues happen when using dnsPolicy=ClusterFirstWithHostNet as recommended for any pod using hostNetwork=true
  • The worker node has no issues connecting and the nmi pod works fine with dnsPolicy=ClusterFirst
  • but there seems to be an issue with either how we configure the nmi pod, or the azure cluster vnet

@nader-ziada nader-ziada force-pushed the ci-identity branch 2 times, most recently from 3349f33 to a9b8079 Compare July 7, 2021 13:41
@nader-ziada
Copy link
Contributor Author

@nader-ziada tried out tilt locally, found two issues:

  • if you run tilt up a second time it fails with error: failed to create secret secrets "cluster-identity-secret" already exists: we should delete the secret and recreate it every tilt up so it's idempotent (same thing we do with configmaps)
  • the secret env var is getting printed in tilt logs, any way we can avoid that?
  • added the delete statement similar to the configmaps
  • added the quiet=true option to prevent printing

@nader-ziada nader-ziada force-pushed the ci-identity branch 3 times, most recently from 8f345ef to 49735f2 Compare July 8, 2021 02:05
Tiltfile Outdated Show resolved Hide resolved
@nader-ziada nader-ziada force-pushed the ci-identity branch 3 times, most recently from f3c6bbc to 30e135a Compare July 8, 2021 22:20
Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

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

All nits.

hack/create-identity-secret.sh Outdated Show resolved Hide resolved
hack/gen-flavors.sh Outdated Show resolved Hide resolved
templates/flavors/aks/kustomization.yaml Outdated Show resolved Hide resolved
templates/flavors/windows/kustomization.yaml Outdated Show resolved Hide resolved
templates/test/ci/prow-ci-version/kustomization.yaml Outdated Show resolved Hide resolved
templates/test/ci/prow-identity-from-env/mhc.yaml Outdated Show resolved Hide resolved
@CecileRobertMichon
Copy link
Contributor

Do we want to keep all 7 commits in the final merge or are some of these squashable?

@nader-ziada
Copy link
Contributor Author

Do we want to keep all 7 commits in the final merge or are some of these squashable?

I think they are for different parts, but I can squash if you prefer

@CecileRobertMichon
Copy link
Contributor

I think they are for different parts, but I can squash if you prefer

nope, all good, just checking they are distinct

@nader-ziada
Copy link
Contributor Author

applied @devigned comments about newlines

Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2021
@CecileRobertMichon
Copy link
Contributor

/lgtm

@nader-ziada
Copy link
Contributor Author

thanks for all the help with debugging this PR @CecileRobertMichon @devigned @mboersma

@k8s-ci-robot k8s-ci-robot merged commit 625daf0 into kubernetes-sigs:master Jul 9, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.5.0 milestone Jul 9, 2021
@devigned
Copy link
Contributor

devigned commented Jul 9, 2021

@nader-ziada great work on this one! This one was a tough task. Thank you for taking this on.

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. area/provider/azure Issues or PRs related to azure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants