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.

Sign up for GitHub

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

gitrepo: Refactor feature usage and fix tests #746

Closed
wants to merge 3 commits into from
Closed

gitrepo: Refactor feature usage and fix tests #746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
95baea0
Refactor feature usage and fix GitRepository tests
aryan9600 May 29, 2022
0c4b0fb
shift managed tests to a new file to force later execution
aryan9600 May 30, 2022
02a89ad
refactor managed transports feature usage in reconciler and libgit2
aryan9600 May 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions api/v1beta2/condition_types.go
View file
Open in desktop
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.
aryan9600 marked this conversation as resolved.
Show resolved Hide resolved
ControllerMisbehaviorReason string = "ControllerMisbehavior"
)
67 changes: 43 additions & 24 deletions controllers/gitrepository_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ import (
"github.com/fluxcd/source-controller/internal/reconcile/summarize"
"github.com/fluxcd/source-controller/internal/util"
"github.com/fluxcd/source-controller/pkg/git"
"github.com/fluxcd/source-controller/pkg/git/libgit2/managed"
"github.com/fluxcd/source-controller/pkg/git/strategy"
"github.com/fluxcd/source-controller/pkg/sourceignore"
)
Expand Down Expand Up @@ -113,8 +112,9 @@ type GitRepositoryReconciler struct {
kuberecorder.EventRecorder
helper.Metrics

Storage *Storage
ControllerName string
Storage *Storage
ControllerName string
ManagedTransportsRegistered bool
Copy link
Member

Choose a reason for hiding this comment

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

The name ManagedTransportsRegistered may mislead folks that don't fully understand how managed transport works.

Given that transport registration happens at git2go, and any registered transport can be overwritten, we can never confirm at this abstraction level that our Managed Transport is registered. What we can do is to define the intention that Managed Transport is to be enabled/used.

Maybe?

Suggested change
ManagedTransportsRegistered bool
UseManagedTransport bool

Copy link
Member Author

Choose a reason for hiding this comment

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

Even though transport registration at git2go, I think we can be confident about it since, if the transports fail to register, a non-nil error would be returned. I think it's safe to assume that if the error is nil, our transports have been registered. UseManagedTransport implies the same thing as GitManagedTransport=true, which could cause confusion, in terms of code readability.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this field reflects the state of the world, not a configuration but what happened when we tried to register. If it succeeded, it's true, else it's false. It's a way for main.go (almost a controller manager) to communicate with the reconciler that the transport registration was successful or failed. What we want is expressed in the feature gate, but the effective state of the system is reflected in this field.
It'd be good to have test coverage for the scenario this introduces. The checkout related tests that will behave differently should have a case for when the managed transport registration failed.

I'm observing discrepancy in the usage of the terms. The feature gate has it in singular form GitManagedTransport, but this field makes it plural and it's also reflected in the associated error message ("they"). Based on my observations, we present this as one thing and don't provide granular options to register multiple managed transports. Another observation is that a lot of the code and comments @aryan9600 introduced has it in plural and the ones by @pjbgf use it in singular.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, based on this discussion, it may be better to have a description of this field, what it means. Unlike other public fields, the intention of this field may not be clear.


requeueDependency time.Duration
features map[string]bool
Expand Down Expand Up @@ -142,7 +142,12 @@ func (r *GitRepositoryReconciler) SetupWithManagerAndOptions(mgr ctrl.Manager, o
}

// Check and enable gated features.
if oc, _ := features.Enabled(features.OptimizedGitClones); oc {
mt, _ := features.Enabled(features.GitManagedTransport)
if mt {
r.features[features.GitManagedTransport] = true
}
// OptimizedGitClones is only enabled when GitManagedTransport is enabled.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// OptimizedGitClones is only enabled when GitManagedTransport is enabled.
// OptimizedGitClones is only supported when GitManagedTransport is enabled.

if oc, _ := features.Enabled(features.OptimizedGitClones); oc && mt {
r.features[features.OptimizedGitClones] = true
}

Expand Down Expand Up @@ -714,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.
Comment on lines +725 to +726
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// 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.
// We return a stalling error if managed transports is not being used when the related
// feature is enabled, as this is an invalid state.

if !r.ManagedTransportsRegistered {
e := &serror.Stalling{
Err: errors.New("can't use managed transports because they are not registered"),
Copy link
Member

Choose a reason for hiding this comment

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

Similar to the other point above, ideally we would keep terminology based on the level of abstraction we are operating at. In this case, if the feature gate is enabled, the controller should use managed transport or have it enabled or something around those lines.

Suggested change
Err: errors.New("can't use managed transports because they are not registered"),
Err: errors.New("invalid state: GitManagedTransport must be used when feature is enabled"),

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 @@ -738,26 +777,6 @@ func (r *GitRepositoryReconciler) gitCheckout(ctx context.Context,
return nil, e
}

// managed GIT transport only affects the libgit2 implementation
if managed.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
Open in desktop
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
17 changes: 11 additions & 6 deletions controllers/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,14 +209,19 @@ func TestMain(m *testing.M) {

fg := feathelper.FeatureGates{}
fg.SupportedFeatures(features.FeatureGates())
managed.InitManagedTransport(logr.Discard())

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

Choose a reason for hiding this comment

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

Suggested change
panic(fmt.Sprintf("Failed to register managed transports; %v", err))
panic(fmt.Sprintf("failed to register managed transports: %v", err))

Copy link
Contributor

@darkowlzz darkowlzz Jun 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is in alignment with the rest of the panic errors in this file.
I'd prefer to move all such code into a separate function so that we don't have to write so many panic calls, similar to what's done for setting up test OCI registry. But maybe separately.

}

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
Open in desktop
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
}
}
31 changes: 15 additions & 16 deletions main.go
View file
Open in desktop
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")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
setupLog.Error(err, "could not register managed transports")
setupLog.Error(err, "could not initialize managed transport")

} 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,17 +320,6 @@ func main() {
startFileServer(storage.BasePath, storageAddr, setupLog)
}()

if enabled, _ := features.Enabled(features.GitManagedTransport); enabled {
managed.InitManagedTransport(ctrl.Log.WithName("managed-transport"))
} else {
if optimize, _ := feathelper.Enabled(features.OptimizedGitClones); optimize {
features.Disable(features.OptimizedGitClones)
setupLog.Info(
"disabling optimized git clones; git clones can only be optimized when using managed transport",
)
}
}

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