feat: add ability to entirely opt-out of argo cd and rollouts integrations #1382

krancour · 2024-01-09T02:01:30Z

Highlights:

Allows Argo CD integration to be explicitly disabled for the controller. In which case:
- Application reconciler does not start
- Argo CD-based promotion mechanisms will error
- Argo CD Application state cannot be factored into Stage health
Allows Argo Rollouts integration to be explicitly disabled for the controller. In which case:
- AnalysisRun reconciler does not start
- Verifications will fail with an error since they're defined using AnalysisTemplates and executed as AnalysisRuns
- VerificationInfo has been expanded with new fields for better surfacing such errors
Allows Argo Rollouts integration to be explicitly disable for the API server, in which case:
- API server cannot apply manifests for AnalysisTemplate resources

WIP because I am still addressing some of @jessesuen feedback.

netlify · 2024-01-09T02:01:35Z

✅ Deploy Preview for docs-kargo-akuity-io ready!

Name	Link
🔨 Latest commit	`12bfb28`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/659f314d61a8990008adb6d0
😎 Deploy Preview	https://deploy-preview-1382.kargo.akuity.io
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-01-09T02:04:27Z

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (8b5a91f) 45.31% compared to head (12bfb28) 45.52%.

Files	Patch %	Lines
internal/controller/stages/stages.go	69.56%	7 Missing ⚠️
api/v1alpha1/stage_types.go	0.00%	6 Missing ⚠️
api/v1alpha1/zz_generated.deepcopy.go	0.00%	6 Missing ⚠️
internal/controller/promotions/promotions.go	16.66%	5 Missing ⚠️
internal/credentials/credentials.go	50.00%	3 Missing ⚠️
internal/controller/applications/applications.go	0.00%	1 Missing ⚠️
internal/controller/promotion/argocd.go	93.75%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1382      +/-   ##
==========================================
+ Coverage   45.31%   45.52%   +0.21%     
==========================================
  Files         136      136              
  Lines       11761    11823      +62     
==========================================
+ Hits         5329     5382      +53     
- Misses       6240     6250      +10     
+ Partials      192      191       -1

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

jessesuen · 2024-01-10T20:04:26Z

Instead of (or in addition to) the ROLLOUTS_INTEGRATION_ENABLED variable to indicate if integration should explicitly be enabled, could we make this inferred by ability to list analysisruns/analysistemplates CRs (even if none are found) during startup?

The reason I think this is desired is because coordinating ROLLOUTS_INTEGRATION_ENABLED across many shards will be hard to maintain and it would be better to infer this with less required upfront configuration.

I'm okay with having the option of ROLLOUTS_INTEGRATION_ENABLED be explicit (to allow things to break when things aren't expected), but in the case where it is omitted, I feel we can rely on inference for a better UX.

jessesuen · 2024-01-10T20:10:43Z

This is an example of how we avoid a configuration knob for Istio in Argo Rollouts by trying to detect if an Istio VirtualService is even present. If not, we don't attempt to start the informer.

https://github.com/argoproj/argo-rollouts/blob/master/utils/istio/istio.go#L16-L22

func DoesIstioExist(dynamicClient dynamic.Interface, namespace string) bool {
	_, err := dynamicClient.Resource(GetIstioVirtualServiceGVR()).Namespace(namespace).List(context.TODO(), metav1.ListOptions{Limit: 1})
	if err != nil {
		return false
	}
	return true
}

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>

krancour · 2024-01-10T20:54:01Z

api/v1alpha1/stage_types.go

+	// Phase describes the current phase of the Verification process. Generally,
+	// this will be a reflection of the underlying AnalysisRun's phase, however,
+	// there are exceptions to this, such as in the case where an AnalysisRun
+	// cannot be launched successfully.
+	Phase VerificationPhase `json:"phase,omitempty"`
+	// Message may contain additional information about why the verification
+	// process is in its current phase.
+	Message string `json:"message,omitempty"`


These are the main changes to the resources types.

It's always been possible that something could go wrong with a verification other than an AnalysisRun failing or erroring, so really, this is the way things should have been done from the start. Added Phase and Message fields for reporting such problems.

The specific problem that is more likely to occur, starting with this PR, is that verification is requested, but the controller doesn't have Rollouts integration enabled. This means an AnalysisRun cannot even be launched.

krancour · 2024-01-10T21:06:38Z

charts/kargo/templates/api/cluster-role.yaml

+{{- if .Values.api.rollouts.integrationEnabled }}
+  - apiGroups:
+      - argoproj.io
+    resources:
+      - analysistemplates
+    verbs:
+      - "*"
+{{- end }}


This fixes #1379

cmd/controlplane/api.go

cmd/controlplane/controller.go

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>

krancour · 2024-01-10T21:29:00Z

internal/controller/promotion/argocd.go

+	if a.argocdClient == nil {
+		return promo.Status.WithPhase(kargoapi.PromotionPhaseFailed), newFreight,
+			errors.New(
+				"Argo CD integration is disabled on this controller; cannot perform " +
+					"promotion",
+			)
+	}


It isn't just the Application reconciler that we cannot start when Argo CD is disabled. We also cannot carry out Argo CD-based promotion mechanisms.

krancour · 2024-01-10T21:31:34Z

internal/controller/stages/health.go

+	if r.argocdClient == nil && len(argoCDAppUpdates) > 0 {
+		h.Status = kargoapi.HealthStateUnknown
+		h.Issues = []string{
+			"Argo CD integration is disabled on this controller; cannot assess" +
+				" the health or sync status of Argo CD Applications",
+		}
+		return &h
+	}


Cannot factor Application state into Stage health if Argo CD integration is disabled.

krancour · 2024-01-10T21:33:31Z

internal/controller/stages/stages.go

@@ -70,12 +70,12 @@ type reconciler struct {
 	startVerificationFn func(
 		context.Context,
 		*kargoapi.Stage,
-	) (*kargoapi.VerificationInfo, error)
+	) *kargoapi.VerificationInfo


Function signature changed...

If something goes wrong, we report it through the VerificationInfo's Phase and Message fields. What we don't want is for a verification problem to present as a Stage problem.

krancour · 2024-01-10T21:34:45Z

internal/controller/stages/verification.go

 func (r *reconciler) startVerification(
 	ctx context.Context,
 	stage *kargoapi.Stage,
-) (*kargoapi.VerificationInfo, error) {
+) *kargoapi.VerificationInfo {


Once again, if something goes wrong, we now report it through the VerificationInfo's Phase and Message fields. What we don't want is for a verification problem to look like a Stage problem.

krancour · 2024-01-10T21:36:59Z

internal/credentials/credentials.go

-	argoClient  client.Client // nil if credential borrowing is not enabled
-	cfg         KubernetesDatabaseConfig
+	kargoClient  client.Client
+	argocdClient client.Client // nil if credential borrowing is not enabled


You see changes like this throughout. "argoClient" was too ambiguous, because Rollouts is also and Argo project. argocdClient is more specific.

krancour · 2024-01-10T21:37:50Z

api/v1alpha1/stage_types.go

@@ -485,7 +523,7 @@ type FreightReference struct {
 	Charts []Chart `json:"charts,omitempty"`
 	// VerificationInfo is information about any verification process that was
 	// associated with this Freight for this Stage.
-	VerificationInfo *VerificationInfo `json:"verificationResult,omitempty"`
+	VerificationInfo *VerificationInfo `json:"verificationInfo,omitempty"`


This was an oops to begin with.

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>

krancour · 2024-01-11T00:09:07Z

Instead of (or in addition to) the ROLLOUTS_INTEGRATION_ENABLED variable to indicate if integration should explicitly be enabled, could we make this inferred by ability to list analysisruns/analysistemplates CRs (even if none are found) during startup?

The reason I think this is desired is because coordinating ROLLOUTS_INTEGRATION_ENABLED across many shards will be hard to maintain and it would be better to infer this with less required upfront configuration.

I'm okay with having the option of ROLLOUTS_INTEGRATION_ENABLED be explicit (to allow things to break when things aren't expected), but in the case where it is omitted, I feel we can rely on inference for a better UX.

@jessesuen, I addressed this in new commit 12bfb28

I think there is a benefit to explicitly disabling Argo CD and/or Rollouts integrations if they are undesired because the controller is granted slightly fewer permissions that way.

But your point about configuration for many shards being difficult is a good one. And we've also seen instances of our own colleagues being surprised at the controller going into a crash loop when they forgot to install one of the dependencies.

So where I landed was:

If you explicitly disable an integration, it is truly disabled without further condition. If you, for instance, had Argo CD or Argo Rollouts on a cluster and really wanted not to enable those integrations, this is what you'd have to do.
If you don't explicitly opt-out, there's a sanity check in case you rolled with the default by accident. So if you start up with Rollouts integration (for example) enabled, but Rollouts CRDs not installed, then you'll see a warning, but the controller will function without error, just as if that feature had been explicitly disabled.

Does all that seem ok?

Willing to tweak this further if you think I've missed the mark.

jessesuen · 2024-01-11T00:13:32Z

but Rollouts CRDs not installed, then you'll see a warning, but the controller will function without error, just as if that feature had been explicitly disabled.

Yep! That is effectively the same as what I was asking for.

krancour added kind/enhancement priority/high area/controller area/charts area/api size/medium labels Jan 9, 2024

krancour added this to the v0.4.0 milestone Jan 9, 2024

krancour self-assigned this Jan 9, 2024

krancour requested a review from a team as a code owner January 9, 2024 02:01

krancour force-pushed the krancour/optional-integrations branch 3 times, most recently from 53834d8 to 9df1f3a Compare January 9, 2024 23:18

krancour force-pushed the krancour/optional-integrations branch from 9df1f3a to 75c7834 Compare January 10, 2024 20:11

krancour marked this pull request as draft January 10, 2024 20:29

krancour force-pushed the krancour/optional-integrations branch from 75c7834 to abc446c Compare January 10, 2024 20:34

krancour added 3 commits January 10, 2024 15:35

resource type changes

ec27c1c

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>

corresponding protobuf changes

4f7ac7e

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>

run codegen

18f373c

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>

krancour force-pushed the krancour/optional-integrations branch from abc446c to c18671d Compare January 10, 2024 20:48

krancour commented Jan 10, 2024

View reviewed changes

krancour force-pushed the krancour/optional-integrations branch from c18671d to fc5b97a Compare January 10, 2024 21:06

krancour commented Jan 10, 2024

View reviewed changes

cmd/controlplane/api.go Outdated Show resolved Hide resolved

krancour commented Jan 10, 2024

View reviewed changes

cmd/controlplane/api.go Outdated Show resolved Hide resolved

krancour commented Jan 10, 2024

View reviewed changes

cmd/controlplane/controller.go Show resolved Hide resolved

krancour commented Jan 10, 2024

View reviewed changes

cmd/controlplane/controller.go Show resolved Hide resolved

krancour force-pushed the krancour/optional-integrations branch from fc5b97a to 05afd25 Compare January 10, 2024 21:23

krancour added 5 commits January 10, 2024 16:26

corresponding type conversion changes

1e0b661

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>

corresponding api server changes

294c2ac

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>

corresponding controller changes

e71ffc5

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>

corresponding chart changes

68d27ee

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>

corresponding Tiltfile changes

50a496c

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>

krancour force-pushed the krancour/optional-integrations branch from 05afd25 to 50a496c Compare January 10, 2024 21:26

krancour commented Jan 10, 2024

View reviewed changes

krancour added 2 commits January 10, 2024 19:06

address @jessesuen feedback

4c219ab

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>

re-run codegen

12bfb28

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>

krancour force-pushed the krancour/optional-integrations branch from 36db30b to 12bfb28 Compare January 11, 2024 00:07

krancour marked this pull request as ready for review January 11, 2024 00:12

jessesuen approved these changes Jan 11, 2024

View reviewed changes

krancour added this pull request to the merge queue Jan 11, 2024

Merged via the queue into akuity:main with commit bfc1fd4 Jan 11, 2024
15 checks passed

krancour deleted the krancour/optional-integrations branch January 11, 2024 01:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add ability to entirely opt-out of argo cd and rollouts integrations #1382

feat: add ability to entirely opt-out of argo cd and rollouts integrations #1382

krancour commented Jan 9, 2024 •

edited

Loading

netlify bot commented Jan 9, 2024 •

edited

Loading

codecov bot commented Jan 9, 2024 •

edited

Loading

jessesuen commented Jan 10, 2024

jessesuen commented Jan 10, 2024 •

edited

Loading

krancour Jan 10, 2024

krancour Jan 10, 2024

krancour Jan 10, 2024

krancour Jan 10, 2024

krancour Jan 10, 2024 •

edited

Loading

krancour Jan 10, 2024 •

edited

Loading

krancour Jan 10, 2024

krancour Jan 10, 2024

krancour commented Jan 11, 2024

jessesuen commented Jan 11, 2024

feat: add ability to entirely opt-out of argo cd and rollouts integrations #1382

feat: add ability to entirely opt-out of argo cd and rollouts integrations #1382

Conversation

krancour commented Jan 9, 2024 • edited Loading

netlify bot commented Jan 9, 2024 • edited Loading

✅ Deploy Preview for docs-kargo-akuity-io ready!

codecov bot commented Jan 9, 2024 • edited Loading

Codecov Report

jessesuen commented Jan 10, 2024

jessesuen commented Jan 10, 2024 • edited Loading

krancour Jan 10, 2024

Choose a reason for hiding this comment

krancour Jan 10, 2024

Choose a reason for hiding this comment

krancour Jan 10, 2024

Choose a reason for hiding this comment

krancour Jan 10, 2024

Choose a reason for hiding this comment

krancour Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

krancour Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

krancour Jan 10, 2024

Choose a reason for hiding this comment

krancour Jan 10, 2024

Choose a reason for hiding this comment

krancour commented Jan 11, 2024

jessesuen commented Jan 11, 2024

krancour commented Jan 9, 2024 •

edited

Loading

netlify bot commented Jan 9, 2024 •

edited

Loading

codecov bot commented Jan 9, 2024 •

edited

Loading

jessesuen commented Jan 10, 2024 •

edited

Loading

krancour Jan 10, 2024 •

edited

Loading

krancour Jan 10, 2024 •

edited

Loading