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

ACM-2940 Setup cluster-manager webhook proxying in hosted-mode MCE #390

Closed
wants to merge 4 commits into from

Conversation

cameronmwall
Copy link
Contributor

@cameronmwall cameronmwall commented Feb 20, 2023

The PR create several new resources when enabling cluster manager so as to allow proxying for the web hooks. The most important of these is the konnectivity deployment which takes in the addresses of the two web hook services. These addresses are also added to be cluster manager itself. When this deployment is up and running in the MCE target namespace, proxying should be working. In order to test this I get a oc api-resources on the hosted cluster which was expected to complete successfully

Issue: https://issues.redhat.com/browse/ACM-2940

Signed-off-by: Cameron Wall <cwall@redhat.com>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 20, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cameronmwall

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sonarcloud
Copy link

sonarcloud bot commented Feb 20, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 16 Code Smells

6.8% 6.8% Coverage
6.1% 6.1% Duplication

Copy link
Collaborator

@JakobGray JakobGray left a comment

Choose a reason for hiding this comment

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

It would be helpful if you could add a description of the changes this PR introduces, and maybe also what to look for to know that it's doing what it should.


err = r.Client.Get(ctx, registrationWebhookServiceNN, registrationWebhookService)
err2 := r.Client.Get(ctx, workWebhookServiceNN, workWebhookService)
addresses := []string{registrationWebhookService.Spec.ClusterIP, workWebhookService.Spec.ClusterIP}
Copy link
Collaborator

Choose a reason for hiding this comment

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

If err or err2 != nil would this line panic?

return ctrl.Result{}, fmt.Errorf("error applying object Name: %s Kind: %s, %w", cmTemplate.GetName(), cmTemplate.GetKind(), err)
}
}
log.Info("made it this far")
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove

Kind: "NetworkPolicy",
},
)
hCNS, _ := utils.GetHostedClusterNamespace(mce)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should address possible error

} else {

// Apply clustermanager
cmTemplate := foundation.HostedClusterManager(mce, r.Images, registrationWebhookService.Spec.ClusterIP, workWebhookService.Spec.ClusterIP)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible MCE will declare itself available before the clusterIPs are set?

command:
- /usr/bin/proxy-agent
# MCE has an external image entry for the proxy agent image. SHould we use that?
image: quay.io/stolostron/apiserver-network-proxy:latest
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there an image that needs to be onboarded for this deployment?

@cameronmwall
Copy link
Contributor Author

It would be helpful if you could add a description of the changes this PR introduces, and maybe also what to look for to know that it's doing what it should.

The PR create several new resources when enabling cluster manager so as to allow proxying for the web hooks. The most important of these is the konnectivity deployment which takes in the addresses of the two web hook services. These addresses are also added to be cluster manager itself. When this deployment is up and running in the MCE target namespace, proxying should be working. In order to test this I get a oc api-resources on the hosted cluster which was expected to complete successfully

@JakobGray JakobGray changed the title add proxying ACM-2940 Setup cluster-manager webhook proxying in hosted-mode MCE Feb 23, 2023
Copy link
Collaborator

@JakobGray JakobGray left a comment

Choose a reason for hiding this comment

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

On initial run with these changes it didn't seem to change much, until I found out I had to poke the hosted MCE to get it to progress. Despite that the MCE was showing as Available. I believe it's because this doesn't meet the criteria:

MCE status tracks success of resources applied

So MCE is still only showing one Deployment and one ClusterManager in its status.

Can you provide a list of commands to run to verify everything is deployed as it should be? For example:

oc get deploy cluster-manager-webhook-konnectivity-agent -n <mceNS>
oc get service cluster-manager-work-webhook -n <mceNS>
KUBECONFIG=<hosted-kubeconfig> oc get service .....

I tried the oc api-resources call and nothing stood out to me before or after the changes were applied so I'm not sure if anything happened.

func GetKonnectivitySecret(mce *backplanev1.MultiClusterEngine) (types.NamespacedName, error) {
nn := types.NamespacedName{}
if mce.Annotations == nil || mce.Annotations[AnnotationHyperShiftNamespace] == "" {
return nn, fmt.Errorf("no kubeconfig secret annotation defined in %s", mce.Name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is a typo

Copy link
Contributor

@eemurphy eemurphy left a comment

Choose a reason for hiding this comment

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

I have no other comments than what Jakob has added, otherwise was able to do oc api-resources and see the konnectivity deployment as expected

Signed-off-by: Cameron Wall <cwall@redhat.com>
Signed-off-by: Cameron Wall <cwall@redhat.com>
Signed-off-by: Cameron Wall <cwall@redhat.com>
@cameronmwall
Copy link
Contributor Author

/hold

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 27, 2023

@cameronmwall: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/test-unit e02b9cc link true /test test-unit
ci/prow/sonar-pre-submit e02b9cc link true /test sonar-pre-submit

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@cameronmwall cameronmwall deleted the 2940-hosted-proxy branch March 27, 2023 20:44
@cameronmwall cameronmwall mentioned this pull request Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants