test(e2e): simplify cluster parameter extraction #5742
Conversation
cameronmeissner
changed the title
…eissner/e2e-params
cameronmeissner
requested review from
juan-lee,
UtheMan,
ganeshkumarashok,
anujmaheshwari1,
AlisonB319,
Devinwong,
lilypan26,
AbelHu,
junjiezhang1997,
jason1028kr,
djsly and
phealy
as code owners
cameronmeissner
requested review from
r2k1,
timmy-wright and
zachary-bailey
as code owners
|
linux e2es failing due to unrelated issues |
| }, nil | ||
| } | ||
|
|
||
| func extractClusterParameters(ctx context.Context, t *testing.T, kube *Kubeclient, cluster *armcontainerservice.ManagedCluster) (*ClusterParams, error) { | ||
| resp, err := config.Azure.AKS.ListClusterAdminCredentials(ctx, config.ResourceGroupName, *cluster.Name, nil) |
There was a problem hiding this comment.
nit: ik we always set it, but should we include a nil check for cluster.Name
There was a problem hiding this comment.
the cluster object comes from a call to AKS.Get/CreateOrUpdate, so it shouldn't ever be the case that we make it this far in test execution and Name is nil without an error having previously occurring
There was a problem hiding this comment.
the result will be the same whether you check it for nil or not - a test will fail
There was a problem hiding this comment.
A kube client is already passed to this function.
Does it mean we already loaded credentials before? Are we doing it second time?
The change makes sense, just wonder if we can optimize it a one step further
There was a problem hiding this comment.
yeah good point, and unfortunately I saw this after I pressed the merge button :D
What type of PR is this?
What this PR does / why we need it:
right now we extract all cluster params except for the bootstrap token using a privileged pod running a node of the e2e cluster's system pool - this is an error prone process and requires at least one node to be running in the system pool at the time of test execution. it turns out we can get the cluster CA certificate data and cluster fqdn directly from the
ListClusterAdminCredentialsAPI on the AKS ARM client insteadfurther, this PR removes the API server cert and client key cluster parameters, as they aren't strictly needed with how we bootstrap nodes for e2e tests
Which issue(s) this PR fixes:
Fixes #
Requirements:
Special notes for your reviewer:
Release note: