Skip to content

[Fleet] Deprecate is_default and is_default_fleet_server flags#6724

Merged
barkbay merged 13 commits intoelastic:mainfrom
barkbay:deprecate-is_default-and-is_default_fleet_server-flags
Apr 27, 2023
Merged

[Fleet] Deprecate is_default and is_default_fleet_server flags#6724
barkbay merged 13 commits intoelastic:mainfrom
barkbay:deprecate-is_default-and-is_default_fleet_server-flags

Conversation

@barkbay
Copy link
Copy Markdown
Contributor

@barkbay barkbay commented Apr 25, 2023

This PR:

  • Returns a warning in the webhook response if spec.PolicyID is not set:
{
	"kind": "AdmissionReview",
	"apiVersion": "admission.k8s.io/v1beta1",
	"response": {
		"uid": "705ab4f5-6393-11e8-b7cc-42010a800002",
		"allowed": true,
		"status": {
			"metadata": {},
			"code": 200
		},
		"warnings": [
			"spec.PolicyID is empty, spec.PolicyID will become mandatory in a future release"
		]
	}
}
  • Removes is_default and is_default_fleet_server from our examples and our documentation, adds spec.PolicyID instead.

TODO:

  • Create an event/log if is_default or is_default_fleet_server is set.
  • There are still some references in:
    • The Helm Charts
    • E2E tests: I think I will not update E2EFleetPolicies as long as the flags are supported. spec.PolicyID will be tested as part of the updates in config/recipes/elastic-agent, for example with TestFleetKubernetesIntegrationRecipe. TODO: create an issue to track this effort once these 2 flags are actually removed
  • For the next major release, should we already skip the default policy mechanism, and make a spec.PolicyID mandatory field for any stack version > 8.x.x ?

Fix #6678

@barkbay barkbay added >enhancement Enhancement of existing functionality v2.8.0 labels Apr 25, 2023
@barkbay
Copy link
Copy Markdown
Contributor Author

barkbay commented Apr 25, 2023

This is what the warning looks like from a CLI perspective:

image

@barkbay barkbay force-pushed the deprecate-is_default-and-is_default_fleet_server-flags branch from 411eeb4 to 7eb423f Compare April 26, 2023 11:36
@barkbay barkbay marked this pull request as ready for review April 26, 2023 13:26
Copy link
Copy Markdown
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

LGTM want to run a few tests still. I like the way you integrated warnings into the webhook tooling 👍

)

var (
MandatoryPolicyIDVersion = version.MustParse("9.0.0-SNAPSHOT")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we know it is going away in 9.0.0 or is this only a placeholder? If so let's add a comment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @juliaElastic 👋 , could you confirm that is_default and is_default_fleet_server flags should be removed in 9.0.0?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, that is the plan.

Comment thread pkg/utils/test/events.go Outdated
// We have read the min. expected number of events
return gotEvents
}
case <-c:
Copy link
Copy Markdown
Collaborator

@pebrc pebrc Apr 26, 2023

Choose a reason for hiding this comment

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

Do we need the timeout given that this is always used in a unit test context and there is no expectation of concurrent use?

	t.Helper()
	if minEventCount == 0 {
		return nil
	}
	gotEvents := make([]string, 0, minEventCount)
	gotEventCount := 0
	for i := 0; i < minEventCount; i++ {
		select {
		case e := <-recorder.Events:
			gotEvents = append(gotEvents, e)
			gotEventCount++
		default:
			t.Errorf("expected at least %d events, got %d", minEventCount, gotEventCount)
		}
	}
	return gotEvents

also the name is could be then something along the lines of ReadNEvents

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do we need the timeout given that this is always used in a unit test context and there is no expectation of concurrent use?

I think we need a timeout so we eventually exit the function if there are less events than minEventCount. Otherwise, the function may wait forever on case e := <-recorder.Events.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That's why I added the default clause in the select so that it immediately errors out or maybe I am missing your point.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry I'm missed your original point. I was assuming that we didn't want to depend on whether or not events were published synchronously in the recorder. But I agree that it is fair to say that it will be the case here.

Copy link
Copy Markdown
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

LGTM I tried the webhook very nice!

@barkbay barkbay merged commit 326eb00 into elastic:main Apr 27, 2023
@barkbay barkbay deleted the deprecate-is_default-and-is_default_fleet_server-flags branch April 27, 2023 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>enhancement Enhancement of existing functionality v2.8.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove is_default and is_default_fleet_server flag from ECK recipes

3 participants