Skip to content

Commit

Permalink
refactor managed transports feature usage in reconciler and libgit2
Browse files Browse the repository at this point in the history 
Adds a new field `Managed` to `CheckoutOpts` to let checkout impls
figure out when to use managed transport. Updates the reconciler to
return a stalling error when managed transports are not registered but
the feature is enabled.

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
  • Loading branch information
@aryan9600
aryan9600 committed Jun 3, 2022
1 parent 3b3b48d commit cd25362
Show file tree
Hide file tree
Showing 12 changed files with 373 additions and 300 deletions.
4 changes: 4 additions & 0 deletions api/v1beta2/condition_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,4 +100,8 @@ const (

// CacheOperationFailedReason signals a failure in cache operation.
CacheOperationFailedReason string = "CacheOperationFailed"

// ControllerMisbehaviorReason signals a failure caused by misbehavior/
// UB of the controller.
ControllerMisbehaviorReason string = "ControllerMisbehavior"
)
60 changes: 37 additions & 23 deletions controllers/gitrepository_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,9 @@ type GitRepositoryReconciler struct {
kuberecorder.EventRecorder
helper.Metrics

Storage *Storage
ControllerName string
Storage *Storage
ControllerName string
ManagedTransportsRegistered bool

requeueDependency time.Duration
features map[string]bool
Expand Down Expand Up @@ -718,6 +719,40 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context,
checkoutOpts.SemVer = ref.SemVer
}

// managed GIT transport only affects the libgit2 implementation
if enabled, ok := r.features[features.GitManagedTransport]; ok && enabled &&
obj.Spec.GitImplementation == sourcev1.LibGit2Implementation {
// We return a stalling error if managed transports aren't registered and the related
// feature is enabled, since the controller can't recover from this.
if !r.ManagedTransportsRegistered {
e := &serror.Stalling{
Err: errors.New("can't use managed transports because they are not registered"),
Reason: sourcev1.ControllerMisbehaviorReason,
}
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
return nil, e
}

// We set the TransportOptionsURL of this set of authentication options here by constructing
// a unique URL that won't clash in a multi tenant environment. This unique URL is used by
// libgit2 managed transports. This enables us to bypass the inbuilt credentials callback in
// libgit2, which is inflexible and unstable.
if strings.HasPrefix(obj.Spec.URL, "http") {
authOpts.TransportOptionsURL = fmt.Sprintf("http://%s/%s/%d", obj.Name, obj.UID, obj.Generation)
} else if strings.HasPrefix(obj.Spec.URL, "ssh") {
authOpts.TransportOptionsURL = fmt.Sprintf("ssh://%s/%s/%d", obj.Name, obj.UID, obj.Generation)
} else {
e := &serror.Stalling{
Err: fmt.Errorf("git repository URL '%s' has invalid transport type, supported types are: http, https, ssh", obj.Spec.URL),
Reason: sourcev1.URLInvalidReason,
}
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
return nil, e
}

checkoutOpts.Managed = true
}

// Only if the object has an existing artifact in storage, attempt to
// short-circuit clone operation. reconcileStorage has already verified
// that the artifact exists.
Expand All @@ -742,27 +777,6 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context,
return nil, e
}

// managed GIT transport only affects the libgit2 implementation
if enabled, ok := r.features[features.GitManagedTransport]; ok && enabled &&
obj.Spec.GitImplementation == sourcev1.LibGit2Implementation {
// We set the TransportOptionsURL of this set of authentication options here by constructing
// a unique URL that won't clash in a multi tenant environment. This unique URL is used by
// libgit2 managed transports. This enables us to bypass the inbuilt credentials callback in
// libgit2, which is inflexible and unstable.
if strings.HasPrefix(obj.Spec.URL, "http") {
authOpts.TransportOptionsURL = fmt.Sprintf("http://%s/%s/%d", obj.Name, obj.UID, obj.Generation)
} else if strings.HasPrefix(obj.Spec.URL, "ssh") {
authOpts.TransportOptionsURL = fmt.Sprintf("ssh://%s/%s/%d", obj.Name, obj.UID, obj.Generation)
} else {
e := &serror.Stalling{
Err: fmt.Errorf("git repository URL '%s' has invalid transport type, supported types are: http, https, ssh", obj.Spec.URL),
Reason: sourcev1.URLInvalidReason,
}
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
return nil, e
}
}

commit, err := checkoutStrategy.Checkout(gitCtx, dir, obj.Spec.URL, authOpts)
if err != nil {
e := serror.NewGeneric(
Expand Down
71 changes: 40 additions & 31 deletions controllers/gitrepository_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -499,10 +499,11 @@ func TestGitRepositoryReconciler_reconcileSource_authStrategy(t *testing.T) {
}

r := &GitRepositoryReconciler{
Client: builder.Build(),
EventRecorder: record.NewFakeRecorder(32),
Storage: testStorage,
features: features.FeatureGates(),
Client: builder.Build(),
EventRecorder: record.NewFakeRecorder(32),
Storage: testStorage,
ManagedTransportsRegistered: true,
features: features.FeatureGates(),
}

for _, i := range testGitImplementations {
Expand Down Expand Up @@ -697,10 +698,11 @@ func TestGitRepositoryReconciler_reconcileSource_checkoutStrategy(t *testing.T)
}

r := &GitRepositoryReconciler{
Client: fakeclient.NewClientBuilder().WithScheme(runtime.NewScheme()).Build(),
EventRecorder: record.NewFakeRecorder(32),
Storage: testStorage,
features: features.FeatureGates(),
Client: fakeclient.NewClientBuilder().WithScheme(runtime.NewScheme()).Build(),
EventRecorder: record.NewFakeRecorder(32),
Storage: testStorage,
ManagedTransportsRegistered: true,
features: features.FeatureGates(),
}

for _, tt := range tests {
Expand Down Expand Up @@ -922,9 +924,10 @@ func TestGitRepositoryReconciler_reconcileArtifact(t *testing.T) {
resetChmod(tt.dir, 0o755, 0o644)

r := &GitRepositoryReconciler{
EventRecorder: record.NewFakeRecorder(32),
Storage: testStorage,
features: features.FeatureGates(),
EventRecorder: record.NewFakeRecorder(32),
Storage: testStorage,
ManagedTransportsRegistered: true,
features: features.FeatureGates(),
}

obj := &sourcev1.GitRepository{
Expand Down Expand Up @@ -1065,11 +1068,12 @@ func TestGitRepositoryReconciler_reconcileInclude(t *testing.T) {
}

r := &GitRepositoryReconciler{
Client: builder.Build(),
EventRecorder: record.NewFakeRecorder(32),
Storage: storage,
requeueDependency: dependencyInterval,
features: features.FeatureGates(),
Client: builder.Build(),
EventRecorder: record.NewFakeRecorder(32),
Storage: storage,
ManagedTransportsRegistered: true,
requeueDependency: dependencyInterval,
features: features.FeatureGates(),
}

obj := &sourcev1.GitRepository{
Expand Down Expand Up @@ -1237,9 +1241,10 @@ func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) {
}()

r := &GitRepositoryReconciler{
EventRecorder: record.NewFakeRecorder(32),
Storage: testStorage,
features: features.FeatureGates(),
EventRecorder: record.NewFakeRecorder(32),
Storage: testStorage,
ManagedTransportsRegistered: true,
features: features.FeatureGates(),
}

obj := &sourcev1.GitRepository{
Expand Down Expand Up @@ -1279,9 +1284,10 @@ func TestGitRepositoryReconciler_reconcileDelete(t *testing.T) {
g := NewWithT(t)

r := &GitRepositoryReconciler{
EventRecorder: record.NewFakeRecorder(32),
Storage: testStorage,
features: features.FeatureGates(),
EventRecorder: record.NewFakeRecorder(32),
Storage: testStorage,
ManagedTransportsRegistered: true,
features: features.FeatureGates(),
}

obj := &sourcev1.GitRepository{
Expand Down Expand Up @@ -1417,9 +1423,10 @@ func TestGitRepositoryReconciler_verifyCommitSignature(t *testing.T) {
}

r := &GitRepositoryReconciler{
EventRecorder: record.NewFakeRecorder(32),
Client: builder.Build(),
features: features.FeatureGates(),
EventRecorder: record.NewFakeRecorder(32),
Client: builder.Build(),
ManagedTransportsRegistered: true,
features: features.FeatureGates(),
}

obj := &sourcev1.GitRepository{
Expand Down Expand Up @@ -1558,10 +1565,11 @@ func TestGitRepositoryReconciler_ConditionsUpdate(t *testing.T) {
builder := fakeclient.NewClientBuilder().WithScheme(testEnv.GetScheme()).WithObjects(obj)

r := &GitRepositoryReconciler{
Client: builder.Build(),
EventRecorder: record.NewFakeRecorder(32),
Storage: testStorage,
features: features.FeatureGates(),
Client: builder.Build(),
EventRecorder: record.NewFakeRecorder(32),
Storage: testStorage,
ManagedTransportsRegistered: true,
features: features.FeatureGates(),
}

key := client.ObjectKeyFromObject(obj)
Expand Down Expand Up @@ -1925,8 +1933,9 @@ func TestGitRepositoryReconciler_notify(t *testing.T) {
}

reconciler := &GitRepositoryReconciler{
EventRecorder: recorder,
features: features.FeatureGates(),
EventRecorder: recorder,
ManagedTransportsRegistered: true,
features: features.FeatureGates(),
}
reconciler.notify(ctx, oldObj, newObj, tt.commit, tt.res, tt.resErr)

Expand Down
16 changes: 10 additions & 6 deletions controllers/suite_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,14 +211,18 @@ func TestMain(m *testing.M) {
features.GitManagedTransport: true,
})

managed.InitManagedTransport(logr.Discard())
err = managed.InitManagedTransport(logr.Discard())
if err != nil {
panic(fmt.Sprintf("Failed to register managed transports; %v", err))
}

if err := (&GitRepositoryReconciler{
Client: testEnv,
EventRecorder: record.NewFakeRecorder(32),
Metrics: testMetricsH,
Storage: testStorage,
features: features.FeatureGates(),
Client: testEnv,
EventRecorder: record.NewFakeRecorder(32),
Metrics: testMetricsH,
Storage: testStorage,
ManagedTransportsRegistered: true,
features: features.FeatureGates(),
}).SetupWithManager(testEnv); err != nil {
panic(fmt.Sprintf("Failed to start GitRepositoryReconciler: %v", err))
}
Expand Down
8 changes: 0 additions & 8 deletions internal/features/features.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,3 @@ func FeatureGates() map[string]bool {
func Enabled(feature string) (bool, error) {
return feathelper.Enabled(feature)
}

// Disable disables the specified feature. If the feature is not
// present, it's a no-op.
func Disable(feature string) {
if _, ok := features[feature]; ok {
features[feature] = false
}
}
24 changes: 15 additions & 9 deletions main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,12 +204,22 @@ func main() {
}
storage := mustInitStorage(storagePath, storageAdvAddr, artifactRetentionTTL, artifactRetentionRecords, setupLog)

var registered bool
if enabled, _ := features.Enabled(features.GitManagedTransport); enabled {
if err = managed.InitManagedTransport(ctrl.Log.WithName("managed-transport")); err != nil {
setupLog.Error(err, "could not register managed transports")
} else {
registered = true
}
}

if err = (&controllers.GitRepositoryReconciler{
Client: mgr.GetClient(),
EventRecorder: eventRecorder,
Metrics: metricsH,
Storage: storage,
ControllerName: controllerName,
Client: mgr.GetClient(),
EventRecorder: eventRecorder,
Metrics: metricsH,
Storage: storage,
ControllerName: controllerName,
ManagedTransportsRegistered: registered,
}).SetupWithManagerAndOptions(mgr, controllers.GitRepositoryReconcilerOptions{
MaxConcurrentReconciles: concurrent,
DependencyRequeueInterval: requeueDependency,
Expand Down Expand Up @@ -310,10 +320,6 @@ func main() {
startFileServer(storage.BasePath, storageAddr, setupLog)
}()

if enabled, _ := features.Enabled(features.GitManagedTransport); enabled {
managed.InitManagedTransport(ctrl.Log.WithName("managed-transport"))
}

setupLog.Info("starting manager")
if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil {
setupLog.Error(err, "problem running manager")
Expand Down
Loading

0 comments on commit cd25362

Please sign in to comment.