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
Bug 1906732: Add E2E to ensure Cluster Wide Proxies are honoured in Machine API components #192
Bug 1906732: Add E2E to ensure Cluster Wide Proxies are honoured in Machine API components #192
Conversation
Skipping CI for Draft Pull Request. |
2e6e886
to
a34e344
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, left a few comments which are pretty much all stylistic nit picks, please update as appropriate
e399407
to
136b47c
Compare
/test |
@rvanderp3: The
Use
In response to this:
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. |
/test golint |
/test govet,goimports |
@rvanderp3: The specified target(s) for
Use
In response to this:
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. |
/test govet |
136b47c
to
765c38d
Compare
pkg/framework/proxies.go
Outdated
corev1 "k8s.io/api/core/v1" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/apimachinery/pkg/util/wait" | ||
"k8s.io/klog" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be using klog/v2
now
"k8s.io/klog" | |
"k8s.io/klog/v2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this still needs to be adressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks. i'll take care of that today.
pkg/framework/proxies.go
Outdated
var machineSet *mapiv1beta1.MachineSet | ||
var machineSetParams MachineSetParams | ||
|
||
machineSetParams = BuildMachineSetParams(client, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need to declare these vars up here, they can be created inline
var machineSet *mapiv1beta1.MachineSet | |
var machineSetParams MachineSetParams | |
machineSetParams = BuildMachineSetParams(client, 1) | |
machineSetParams := BuildMachineSetParams(client, 1) |
pkg/framework/proxies.go
Outdated
} | ||
node := nodes[0] | ||
delete(node.Labels, "node-role.kubernetes.io/worker") | ||
client.Update(context.Background(), node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not check this error?
client.Update(context.Background(), node) | |
if err := client.Update(context.Background(), node); err != nil { | |
return nil, err | |
} |
pkg/framework/proxies.go
Outdated
// DeployClusterProxy Deploys an HTTP proxy to the proxy node | ||
func DeployClusterProxy(c runtimeclient.Client) (*appsv1.Deployment, error) { | ||
configMap := corev1.ConfigMap{ | ||
TypeMeta: metav1.TypeMeta{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can get rid of this one
TypeMeta: metav1.TypeMeta{}, |
pkg/framework/proxies.go
Outdated
|
||
// WaitForProxyInjectionSync waits for the deployment to sync with the state of the cluster-proxy | ||
func WaitForProxyInjectionSync(c runtimeclient.Client, name, namespace string, shouldBePresent bool) bool { | ||
if err := wait.PollImmediate(RetryShort, WaitLong, func() (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a WaitLong
, might want to reduce the retry, perhaps RetryMedium
would be more appropriate here?
pkg/framework/proxies.go
Outdated
hasHttpsProxy && | ||
hasNoProxy) == shouldBePresent, nil | ||
}); err != nil { | ||
klog.Errorf("Error checking isDeploymentAvailable: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error message needs an update
pkg/framework/proxies.go
Outdated
if updatedProxy.Spec.HTTPProxy != "" { | ||
proxy.Spec.HTTPProxy = updatedProxy.Spec.HTTPProxy | ||
} else { | ||
proxy.Spec.HTTPProxy = "" | ||
} | ||
if updatedProxy.Spec.HTTPSProxy != "" { | ||
proxy.Spec.HTTPSProxy = updatedProxy.Spec.HTTPSProxy | ||
} else { | ||
proxy.Spec.HTTPSProxy = "" | ||
} | ||
if updatedProxy.Spec.NoProxy != "" { | ||
proxy.Spec.NoProxy = updatedProxy.Spec.NoProxy | ||
} else { | ||
proxy.Spec.NoProxy = "" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these can be condensed to
if updatedProxy.Spec.HTTPProxy != "" { | |
proxy.Spec.HTTPProxy = updatedProxy.Spec.HTTPProxy | |
} else { | |
proxy.Spec.HTTPProxy = "" | |
} | |
if updatedProxy.Spec.HTTPSProxy != "" { | |
proxy.Spec.HTTPSProxy = updatedProxy.Spec.HTTPSProxy | |
} else { | |
proxy.Spec.HTTPSProxy = "" | |
} | |
if updatedProxy.Spec.NoProxy != "" { | |
proxy.Spec.NoProxy = updatedProxy.Spec.NoProxy | |
} else { | |
proxy.Spec.NoProxy = "" | |
} | |
proxy.Spec.HTTPProxy = updatedProxy.Spec.HTTPProxy | |
proxy.Spec.HTTPSProxy = updatedProxy.Spec.HTTPSProxy | |
proxy.Spec.NoProxy = updatedProxy.Spec.NoProxy |
But do we really need to do this? Would we not want to just c.Update(context.Background(), updatedProxy)
? Do we even need this wrapper function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very good call. when I wrote that I thought those variables inited to nil when undefined, which they don't. I'll make that change.
var machineSet *mapiv1beta1.MachineSet | ||
var proxyMachineSet *mapiv1beta1.MachineSet | ||
var machineSetParams framework.MachineSetParams | ||
It("reflect the configured proxy in machine-api-controller", func() { | ||
By("creating host for proxy") | ||
client, err := framework.LoadClient() | ||
Expect(err).NotTo(HaveOccurred()) | ||
proxyMachineSet, err = framework.CreateProxyMachineSet(client) | ||
Expect(err).NotTo(HaveOccurred()) | ||
|
||
By("deploying an HTTP proxy") | ||
deployment, err := framework.DeployClusterProxy(client) | ||
Expect(err).NotTo(HaveOccurred()) | ||
Expect(framework.IsDeploymentAvailable(client, deployment.Name, deployment.Namespace)).To(BeTrue()) | ||
|
||
By("configuring cluster-wide proxy") | ||
pods, err := framework.GetPods(client, map[string]string{"app": "squid"}) | ||
Expect(err).NotTo(HaveOccurred()) | ||
Expect(len(pods.Items) > 0).To(BeTrue()) | ||
proxy := configv1.Proxy{} | ||
proxy.Spec.HTTPProxy = "http://" + pods.Items[0].Status.HostIP + ":80" | ||
proxy.Spec.HTTPSProxy = "http://" + pods.Items[0].Status.HostIP + ":80" | ||
framework.SetClusterProxy(client, proxy) | ||
Expect(err).NotTo(HaveOccurred()) | ||
|
||
By("waiting for machine-api-controller deployment to reflect configured cluster-wide proxy") | ||
Expect(framework.WaitForProxyInjectionSync(client, maoManagedDeployment, framework.MachineAPINamespace, true)).To(BeTrue()) | ||
}) | ||
|
||
It("allow a machineset to be created and destroyed", func() { | ||
client, err := framework.LoadClient() | ||
Expect(err).NotTo(HaveOccurred()) | ||
|
||
By("creating a machineset") | ||
|
||
machineSetParams = framework.BuildMachineSetParams(client, 3) | ||
machineSet, err = framework.CreateMachineSet(client, machineSetParams) | ||
Expect(err).ToNot(HaveOccurred()) | ||
framework.WaitForMachineSet(client, machineSet.GetName()) | ||
|
||
By("destroying a machineset") | ||
err = client.Delete(context.Background(), machineSet) | ||
Expect(err).ToNot(HaveOccurred()) | ||
framework.WaitForMachineSetDelete(client, machineSet) | ||
}) | ||
|
||
It("reflect an unconfigured proxy in machine-api-controller", func() { | ||
client, err := framework.LoadClient() | ||
Expect(err).NotTo(HaveOccurred()) | ||
|
||
By("unconfiguring cluster-wide proxy") | ||
proxy := configv1.Proxy{} | ||
proxy.Spec.HTTPProxy = "" | ||
proxy.Spec.HTTPSProxy = "" | ||
framework.SetClusterProxy(client, proxy) | ||
Expect(err).NotTo(HaveOccurred()) | ||
|
||
By("waiting for machine-api-controller deployment to reflect unconfigured cluster-wide proxy") | ||
Expect(framework.WaitForProxyInjectionSync(client, maoManagedDeployment, framework.MachineAPINamespace, false)).To(BeTrue()) | ||
framework.DestroyClusterProxy(client) | ||
|
||
Expect(proxyMachineSet != nil).To(BeTrue()) | ||
framework.DeleteMachineSets(client, proxyMachineSet) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This either needs to be one It
statement as below, or it needs to be a BeforeEach
which sets up the cluster-wide proxy and an AfterEach
which cleans this up again, with the It
statement being the one that creates and destroys a MachineSet
var machineSet *mapiv1beta1.MachineSet | |
var proxyMachineSet *mapiv1beta1.MachineSet | |
var machineSetParams framework.MachineSetParams | |
It("reflect the configured proxy in machine-api-controller", func() { | |
By("creating host for proxy") | |
client, err := framework.LoadClient() | |
Expect(err).NotTo(HaveOccurred()) | |
proxyMachineSet, err = framework.CreateProxyMachineSet(client) | |
Expect(err).NotTo(HaveOccurred()) | |
By("deploying an HTTP proxy") | |
deployment, err := framework.DeployClusterProxy(client) | |
Expect(err).NotTo(HaveOccurred()) | |
Expect(framework.IsDeploymentAvailable(client, deployment.Name, deployment.Namespace)).To(BeTrue()) | |
By("configuring cluster-wide proxy") | |
pods, err := framework.GetPods(client, map[string]string{"app": "squid"}) | |
Expect(err).NotTo(HaveOccurred()) | |
Expect(len(pods.Items) > 0).To(BeTrue()) | |
proxy := configv1.Proxy{} | |
proxy.Spec.HTTPProxy = "http://" + pods.Items[0].Status.HostIP + ":80" | |
proxy.Spec.HTTPSProxy = "http://" + pods.Items[0].Status.HostIP + ":80" | |
framework.SetClusterProxy(client, proxy) | |
Expect(err).NotTo(HaveOccurred()) | |
By("waiting for machine-api-controller deployment to reflect configured cluster-wide proxy") | |
Expect(framework.WaitForProxyInjectionSync(client, maoManagedDeployment, framework.MachineAPINamespace, true)).To(BeTrue()) | |
}) | |
It("allow a machineset to be created and destroyed", func() { | |
client, err := framework.LoadClient() | |
Expect(err).NotTo(HaveOccurred()) | |
By("creating a machineset") | |
machineSetParams = framework.BuildMachineSetParams(client, 3) | |
machineSet, err = framework.CreateMachineSet(client, machineSetParams) | |
Expect(err).ToNot(HaveOccurred()) | |
framework.WaitForMachineSet(client, machineSet.GetName()) | |
By("destroying a machineset") | |
err = client.Delete(context.Background(), machineSet) | |
Expect(err).ToNot(HaveOccurred()) | |
framework.WaitForMachineSetDelete(client, machineSet) | |
}) | |
It("reflect an unconfigured proxy in machine-api-controller", func() { | |
client, err := framework.LoadClient() | |
Expect(err).NotTo(HaveOccurred()) | |
By("unconfiguring cluster-wide proxy") | |
proxy := configv1.Proxy{} | |
proxy.Spec.HTTPProxy = "" | |
proxy.Spec.HTTPSProxy = "" | |
framework.SetClusterProxy(client, proxy) | |
Expect(err).NotTo(HaveOccurred()) | |
By("waiting for machine-api-controller deployment to reflect unconfigured cluster-wide proxy") | |
Expect(framework.WaitForProxyInjectionSync(client, maoManagedDeployment, framework.MachineAPINamespace, false)).To(BeTrue()) | |
framework.DestroyClusterProxy(client) | |
Expect(proxyMachineSet != nil).To(BeTrue()) | |
framework.DeleteMachineSets(client, proxyMachineSet) | |
}) | |
It("create machines when configured behind a proxy", func() { | |
By("creating host for proxy") | |
client, err := framework.LoadClient() | |
Expect(err).NotTo(HaveOccurred()) | |
proxyMachineSet, err := framework.CreateProxyMachineSet(client) | |
Expect(err).NotTo(HaveOccurred()) | |
By("deploying an HTTP proxy") | |
deployment, err := framework.DeployClusterProxy(client) | |
Expect(err).NotTo(HaveOccurred()) | |
Expect(framework.IsDeploymentAvailable(client, deployment.Name, deployment.Namespace)).To(BeTrue()) | |
By("configuring cluster-wide proxy") | |
pods, err := framework.GetPods(client, map[string]string{"app": "squid"}) | |
Expect(err).NotTo(HaveOccurred()) | |
Expect(len(pods.Items) > 0).To(BeTrue()) | |
proxy := configv1.Proxy{} | |
proxy.Spec.HTTPProxy = "http://" + pods.Items[0].Status.HostIP + ":80" | |
proxy.Spec.HTTPSProxy = "http://" + pods.Items[0].Status.HostIP + ":80" | |
framework.SetClusterProxy(client, proxy) | |
Expect(err).NotTo(HaveOccurred()) | |
By("waiting for machine-api-controller deployment to reflect configured cluster-wide proxy") | |
Expect(framework.WaitForProxyInjectionSync(client, maoManagedDeployment, framework.MachineAPINamespace, true)).To(BeTrue()) | |
By("creating a machineset") | |
machineSetParams = framework.BuildMachineSetParams(client, 3) | |
machineSet, err = framework.CreateMachineSet(client, machineSetParams) | |
Expect(err).ToNot(HaveOccurred()) | |
framework.WaitForMachineSet(client, machineSet.GetName()) | |
By("destroying a machineset") | |
Expect(client.Delete(context.Background(), machineSet)).To(Succeed()) | |
framework.WaitForMachineSetDelete(client, machineSet) | |
By("unconfiguring cluster-wide proxy") | |
proxy := configv1.Proxy{} | |
proxy.Spec.HTTPProxy = "" | |
proxy.Spec.HTTPSProxy = "" | |
framework.SetClusterProxy(client, proxy) | |
Expect(err).NotTo(HaveOccurred()) | |
By("waiting for machine-api-controller deployment to reflect unconfigured cluster-wide proxy") | |
Expect(framework.WaitForProxyInjectionSync(client, maoManagedDeployment, framework.MachineAPINamespace, false)).To(BeTrue()) | |
framework.DestroyClusterProxy(client) | |
Expect(proxyMachineSet != nil).To(BeTrue()) | |
framework.DeleteMachineSets(client, proxyMachineSet) | |
}) |
@@ -164,3 +167,71 @@ var _ = Describe("[Feature:Operators] Machine API cluster operator status should | |||
Expect(framework.IsStatusAvailable(client, "machine-api")).To(BeTrue()) | |||
}) | |||
}) | |||
|
|||
var _ = Describe("[Feature:Operators] When cluster-wide proxy is configured, Machine API cluster operator should ", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this can run in parallel with other tests, I think we need to mark as serial as suggested below, but I think @elmiko can confirm this
var _ = Describe("[Feature:Operators] When cluster-wide proxy is configured, Machine API cluster operator should ", func() { | |
var _ = Describe("[Serial][Feature:Operators] When cluster-wide proxy is configured, Machine API cluster operator should ", func() { |
765c38d
to
8514125
Compare
55a7ed6
to
e5a647a
Compare
e5a647a
to
7281a9e
Compare
2c5481d
to
f919381
Compare
/retest Test failures look unrelated, infrastructure issues seemingly |
/retest |
b35d351
to
bbe459a
Compare
/retest |
3 similar comments
/retest |
/retest |
/retest |
pkg/framework/proxies.go
Outdated
corev1 "k8s.io/api/core/v1" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/apimachinery/pkg/util/wait" | ||
"k8s.io/klog" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this still needs to be adressed
bbe459a
to
a457aa7
Compare
/retest |
1 similar comment
/retest |
There's a few nits I could make about consistency with imports and naming, but I think at this point it's better to get this merged than nitpick. Will need to create a BZ to get this merged now though. I'll get that done shortly. /approve Thanks for all your work on this @rvanderp3! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed 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:
Approvers can indicate their approval by writing |
/retitle Bug 1906732: Add E2E to ensure Cluster Wide Proxies are honoured in Machine API components |
@rvanderp3: This pull request references Bugzilla bug 1906732, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
/lgtm
@rvanderp3: All pull requests linked via external trackers have merged: Bugzilla bug 1906732 has been moved to the MODIFIED state. In response to this:
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. |
No description provided.