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

libgit2: refactor feature managed transport usage #779

Closed
wants to merge 2 commits into from
Closed

libgit2: refactor feature managed transport usage #779

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
650a397
libgit2: refactor managed transport feature usage in reconciler and c…
aryan9600 Jun 10, 2022
52d2d99
refactor tests to use new managed transport feature usage
aryan9600 Jun 10, 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.
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
ManagedTransportRegistered bool

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 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 transport isn't registered and the related
// feature is enabled, since the controller can't recover from this.
if !r.ManagedTransportRegistered {
e := &serror.Stalling{
Err: errors.New("invalid state: GitManagedTransport is enabled but managed transport isn't registered"),
Reason: sourcev1.ControllerMisbehaviorReason,
}
conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
return nil, e
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing the complexity this new approach (removing Enabled() from libgit2/managed/ package and storing the result of initialization in ManagedTransportRegistered) adds, and considering that we'll not need all these anymore soon, maybe we don't need to change any of it.
In this implementation, what was short and simple before using Enabled() which indicated if managed transport is requested and is successfully initialized, is now divided into two separate variables and we now need to add more checks to ensure that the reconciler doesn't enter into an undefined state.
Also considering that in the current implementation, if managed transport initialization fails, Enabled() wouldn't be true, maybe we should keep Enabled() and improve it a little.

source-controller/pkg/git/libgit2/managed/init.go

Lines 78 to 79 in f6a389c

err = registerManagedSSH()
enabled = true
will set enabled to true even if ssh managed transport initialization fails. We can make sure that enabled is true only when both http and ssh managed transports are initialized successfully.


// 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 transport. 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,
ManagedTransportRegistered: 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,
ManagedTransportRegistered: 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,
ManagedTransportRegistered: 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,
ManagedTransportRegistered: 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,
ManagedTransportRegistered: 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,
ManagedTransportRegistered: 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(),
ManagedTransportRegistered: 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,
ManagedTransportRegistered: 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,
ManagedTransportRegistered: true,
features: features.FeatureGates(),
}
reconciler.notify(ctx, oldObj, newObj, tt.commit, tt.res, tt.resErr)

Expand Down
35 changes: 18 additions & 17 deletions controllers/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,35 +190,36 @@ func TestMain(m *testing.M) {
var err error
testServer, err = testserver.NewTempArtifactServer()
if err != nil {
panic(fmt.Sprintf("Failed to create a temporary storage server: %v", err))
panic(fmt.Sprintf("failed to create a temporary storage server: %v", err))
}
fmt.Println("Starting the test storage server")
testServer.Start()

testStorage, err = newTestStorage(testServer.HTTPServer)
if err != nil {
panic(fmt.Sprintf("Failed to create a test storage: %v", err))
panic(fmt.Sprintf("failed to create a test storage: %v", err))
}

testMetricsH = controller.MustMakeMetrics(testEnv)

testRegistryServer, err = setupRegistryServer(ctx)
if err != nil {
panic(fmt.Sprintf("Failed to create a test registry server: %v", err))
panic(fmt.Sprintf("failed to create a test registry server: %v", err))
}

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

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,
ManagedTransportRegistered: true,
features: features.FeatureGates(),
}).SetupWithManager(testEnv); err != nil {
panic(fmt.Sprintf("Failed to start GitRepositoryReconciler: %v", err))
panic(fmt.Sprintf("failed to start GitRepositoryReconciler: %v", err))
}

if err := (&BucketReconciler{
Expand All @@ -227,7 +228,7 @@ func TestMain(m *testing.M) {
Metrics: testMetricsH,
Storage: testStorage,
}).SetupWithManager(testEnv); err != nil {
panic(fmt.Sprintf("Failed to start BucketReconciler: %v", err))
panic(fmt.Sprintf("failed to start BucketReconciler: %v", err))
}

if err := (&HelmRepositoryReconciler{
Expand All @@ -237,7 +238,7 @@ func TestMain(m *testing.M) {
Getters: testGetters,
Storage: testStorage,
}).SetupWithManager(testEnv); err != nil {
panic(fmt.Sprintf("Failed to start HelmRepositoryReconciler: %v", err))
panic(fmt.Sprintf("failed to start HelmRepositoryReconciler: %v", err))
}

if err = (&HelmRepositoryOCIReconciler{
Expand All @@ -247,7 +248,7 @@ func TestMain(m *testing.M) {
Getters: testGetters,
RegistryClientGenerator: registry.ClientGenerator,
}).SetupWithManager(testEnv); err != nil {
panic(fmt.Sprintf("Failed to start HelmRepositoryOCIReconciler: %v", err))
panic(fmt.Sprintf("failed to start HelmRepositoryOCIReconciler: %v", err))
}

testCache = cache.New(5, 1*time.Second)
Expand All @@ -262,13 +263,13 @@ func TestMain(m *testing.M) {
TTL: 1 * time.Second,
CacheRecorder: cacheRecorder,
}).SetupWithManager(testEnv); err != nil {
panic(fmt.Sprintf("Failed to start HelmRepositoryReconciler: %v", err))
panic(fmt.Sprintf("failed to start HelmRepositoryReconciler: %v", err))
}

go func() {
fmt.Println("Starting the test environment")
if err := testEnv.Start(ctx); err != nil {
panic(fmt.Sprintf("Failed to start the test environment manager: %v", err))
panic(fmt.Sprintf("failed to start the test environment manager: %v", err))
}
}()
<-testEnv.Manager.Elected()
Expand All @@ -277,17 +278,17 @@ func TestMain(m *testing.M) {

fmt.Println("Stopping the test environment")
if err := testEnv.Stop(); err != nil {
panic(fmt.Sprintf("Failed to stop the test environment: %v", err))
panic(fmt.Sprintf("failed to stop the test environment: %v", err))
}

fmt.Println("Stopping the storage server")
testServer.Stop()
if err := os.RemoveAll(testServer.Root()); err != nil {
panic(fmt.Sprintf("Failed to remove storage server dir: %v", err))
panic(fmt.Sprintf("failed to remove storage server dir: %v", err))
}

if err := os.RemoveAll(testRegistryServer.workspaceDir); err != nil {
panic(fmt.Sprintf("Failed to remove registry workspace dir: %v", err))
panic(fmt.Sprintf("failed to remove registry workspace dir: %v", err))
}

os.Exit(code)
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
}
}
Loading