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

Always add ActiveGate ports no matter the capability #4069

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

gkrenn
Copy link
Contributor

@gkrenn gkrenn commented Nov 18, 2024

Description

This PR changes the handling of the ActiveGate ports. They will be activated no matter the enabled capability, the only requirement is that at least 1 capability has to be activated.

Jira

How can this be tested?

  • install to cluster
  • create ActiveGate DynaKube
  • service dynakube-activegate should be created as well as the ports on the ActiveGate container

@gkrenn gkrenn added the activegate Changes related to Activegate label Nov 18, 2024
@gkrenn gkrenn requested a review from a team as a code owner November 18, 2024 10:08
@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 78.26087% with 5 lines in your changes missing coverage. Please review.

Project coverage is 64.66%. Comparing base (080c62c) to head (6b753f2).

Files with missing lines Patch % Lines
...akube/activegate/internal/capability/reconciler.go 28.57% 3 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4069      +/-   ##
==========================================
- Coverage   64.68%   64.66%   -0.03%     
==========================================
  Files         397      397              
  Lines       26509    26491      -18     
==========================================
- Hits        17148    17131      -17     
  Misses       8031     8031              
+ Partials     1330     1329       -1     
Flag Coverage Δ
unittests 64.66% <78.26%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@@ -93,9 +93,7 @@ func (ag *Spec) IsMetricsIngestEnabled() bool {
}

func (ag *Spec) NeedsService() bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why keep the check at all? if there's an activegate there should be a service

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will adapt it so it is always created

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it

Copy link
Contributor

@aorcholski aorcholski left a comment

Choose a reason for hiding this comment

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

I just try to understand the logic related to AG service.

I think we need NeedsService() function because of KSPM.

going up the call stack :

func (r *Reconciler) deleteCapability(ctx context.Context, agCapability capability.Capability) error {
-> if err := r.deleteService(ctx, agCapability); err != nil {
-> if r.dk.ActiveGate().NeedsService() {
		return nil
     }
-> func (ag *Spec) NeedsService() bool {
	return len(ag.Capabilities) > 0 ||
		ag.enabledDependencies.extensions ||
		ag.enabledDependencies.kspm      <---- depends on KSPM
    }     

So KSPM needs the AG service to exist.

In that case how do we create the AG service if KSPM is enabled?

dynakube/activegate/reconciler.go:Reconcile()

for _, agCapability := range capability.GenerateActiveGateCapabilities(r.dk) {
	if agCapability.Enabled() {
			return r.createCapability(ctx, agCapability)           

going up the call stack :

for _, agCapability := range capability.GenerateActiveGateCapabilities(r.dk) {
-> func NewMultiCapability(dk *dynakube.DynaKube) Capability {
-> if dk == nil || !dk.ActiveGate().IsEnabled() {
-> return len(ag.Capabilities) > 0 || ag.enabledDependencies.Any()
-> func (d dependencies) Any() bool {
	return d.extensions   <---- doesn't depend on KSPM
    }

we get disabled MultiCapability because KSPM has no meaning from AG capability point of view.

Then :

if agCapability.Enabled() {
-> func (capability *capabilityBase) Enabled() bool {
	return capability.enabled
    }

returns false and instance of AG is not created - good. Unfortunately this also means the AG service is not created - KSPM is unhappy, right?

@gkrenn
Copy link
Contributor Author

gkrenn commented Nov 22, 2024

@aorcholski Enabling KSPM standalone should not be possible. It is blocked by the validation webhook, as it requires enabled Kubernetes-monitoring capability.

If kubemon is enabled, the service will be created.

We could add kspm to the Any() function in the ActiveGate spec just to make sure in case we accidentally delete the check in the validation webhook. What is your opinion? @chrismuellner - As long as the check in the validation webhook is there everything would be fine, but modeling the dependency in the ActiveGate spec would be cleaner in my opinion:

func (d dependencies) Any() bool {
	return d.extensions || d.kspm
}

@chrismuellner
Copy link
Collaborator

We could add kspm to the Any() function in the ActiveGate spec just to make sure in case we accidentally delete the check in the validation webhook.

I think (hope) that's an unlikely scenario. If there are benefits to adding this validation in the Activegate spec as well then let's do it, otherwise let's not overcomplicate something that should either not be possible or handled by the validation webhook.

Copy link
Contributor

@aorcholski aorcholski left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
activegate Changes related to Activegate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants