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

Refactor constants from internal/.. to pkg/common package. #3171

Merged
merged 12 commits into from
Aug 7, 2023
Merged

Refactor constants from internal/.. to pkg/common package. #3171

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3b44917
Refactor constants
super-harsh Jul 31, 2023
eb6244f
Update comment
super-harsh Jul 31, 2023
492e474
Add header
super-harsh Jul 31, 2023
16c386e
Get public facing constants for reconcile policy
super-harsh Jul 31, 2023
bf3c765
Update test const references
super-harsh Jul 31, 2023
a2e9c6d
Move common constants to respective packages
super-harsh Aug 2, 2023
f5354e6
fix ref
super-harsh Aug 2, 2023
c5b2607
Merge branch 'main' into refactor/constants
super-harsh Aug 3, 2023
0a0a467
Merge branch 'main' into refactor/constants
super-harsh Aug 4, 2023
41ee7b3
Refactor to move reconcilePolicy to common.annotations
super-harsh Aug 7, 2023
caa6da8
Merge branch 'main' into refactor/constants
super-harsh Aug 7, 2023
8b044bc
Move reconcilePolicy annotations to reconcile_policy.go
super-harsh Aug 7, 2023
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
7 changes: 4 additions & 3 deletions v2/cmd/controller/app/setup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import (
. "github.com/Azure/azure-service-operator/v2/internal/logging"
asometrics "github.com/Azure/azure-service-operator/v2/internal/metrics"
armreconciler "github.com/Azure/azure-service-operator/v2/internal/reconcilers/arm"
"github.com/Azure/azure-service-operator/v2/pkg/common"
Copy link
Member

Choose a reason for hiding this comment

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

I'll beat @matthchr to this (because he always catches me out) - move this into the import group for local packages, just below.


"github.com/Azure/azure-service-operator/v2/api"
"github.com/Azure/azure-service-operator/v2/internal/config"
Expand Down Expand Up @@ -253,7 +254,7 @@ func getDefaultAzureTokenCredential(cfg config.Values, setupLog logr.Logger) (az
credential, err := azidentity.NewWorkloadIdentityCredential(&azidentity.WorkloadIdentityCredentialOptions{
ClientID: cfg.ClientID,
TenantID: cfg.TenantID,
TokenFilePath: config.FederatedTokenFilePath,
TokenFilePath: common.FederatedTokenFilePath,
})
if err != nil {
return nil, errors.Wrapf(err, "unable to get workload identity credential")
Expand All @@ -262,8 +263,8 @@ func getDefaultAzureTokenCredential(cfg config.Values, setupLog logr.Logger) (az
return credential, nil
}

if cert := os.Getenv(config.ClientCertificateVar); cert != "" {
certPassword := os.Getenv(config.ClientCertificatePasswordVar)
if cert := os.Getenv(common.ClientCertificateVar); cert != "" {
certPassword := os.Getenv(common.ClientCertificatePasswordVar)
credential, err := identity.NewClientCertificateCredential(cfg.TenantID, cfg.ClientID, []byte(cert), []byte(certPassword))
if err != nil {
return nil, errors.Wrapf(err, "unable to get client certificate credential")
Expand Down
49 changes: 15 additions & 34 deletions v2/internal/config/vars.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,27 +12,8 @@ import (

"github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud"
"github.com/pkg/errors"
)

const (
// #nosec
ClientSecretVar = "AZURE_CLIENT_SECRET"
SubscriptionIDVar = "AZURE_SUBSCRIPTION_ID"
TenantIDVar = "AZURE_TENANT_ID"
ClientIDVar = "AZURE_CLIENT_ID"
ClientCertificateVar = "AZURE_CLIENT_CERTIFICATE"
// #nosec
ClientCertificatePasswordVar = "AZURE_CLIENT_CERTIFICATE_PASSWORD"
targetNamespacesVar = "AZURE_TARGET_NAMESPACES"
operatorModeVar = "AZURE_OPERATOR_MODE"
syncPeriodVar = "AZURE_SYNC_PERIOD"
resourceManagerEndpointVar = "AZURE_RESOURCE_MANAGER_ENDPOINT"
resourceManagerAudienceVar = "AZURE_RESOURCE_MANAGER_AUDIENCE"
azureAuthorityHostVar = "AZURE_AUTHORITY_HOST"
podNamespaceVar = "POD_NAMESPACE"
useWorkloadIdentityAuth = "USE_WORKLOAD_IDENTITY_AUTH"
// #nosec
FederatedTokenFilePath = "/var/run/secrets/tokens/azure-identity"
"github.com/Azure/azure-service-operator/v2/pkg/common"
)

// These are hardcoded because the init function that initializes them in azcore isn't in /cloud it's in /arm which
Expand Down Expand Up @@ -167,7 +148,7 @@ func (v Values) Cloud() cloud.Configuration {
// environment variables.
func ReadFromEnvironment() (Values, error) {
var result Values
modeValue := os.Getenv(operatorModeVar)
modeValue := os.Getenv(common.OperatorModeVar)
if modeValue == "" {
result.OperatorMode = OperatorModeBoth
} else {
Expand All @@ -180,21 +161,21 @@ func ReadFromEnvironment() (Values, error) {

var err error

result.SubscriptionID = os.Getenv(SubscriptionIDVar)
result.PodNamespace = os.Getenv(podNamespaceVar)
result.TargetNamespaces = parseTargetNamespaces(os.Getenv(targetNamespacesVar))
result.SubscriptionID = os.Getenv(common.SubscriptionIDVar)
result.PodNamespace = os.Getenv(common.PodNamespaceVar)
result.TargetNamespaces = parseTargetNamespaces(os.Getenv(common.TargetNamespacesVar))
result.SyncPeriod, err = parseSyncPeriod()
result.ResourceManagerEndpoint = envOrDefault(resourceManagerEndpointVar, DefaultEndpoint)
result.ResourceManagerAudience = envOrDefault(resourceManagerAudienceVar, DefaultAudience)
result.AzureAuthorityHost = envOrDefault(azureAuthorityHostVar, DefaultAADAuthorityHost)
result.ClientID = os.Getenv(ClientIDVar)
result.TenantID = os.Getenv(TenantIDVar)
result.ResourceManagerEndpoint = envOrDefault(common.ResourceManagerEndpointVar, DefaultEndpoint)
result.ResourceManagerAudience = envOrDefault(common.ResourceManagerAudienceVar, DefaultAudience)
result.AzureAuthorityHost = envOrDefault(common.AzureAuthorityHostVar, DefaultAADAuthorityHost)
result.ClientID = os.Getenv(common.ClientIDVar)
result.TenantID = os.Getenv(common.TenantIDVar)

// Ignoring error here, as any other value or empty value means we should default to false
result.UseWorkloadIdentityAuth, _ = strconv.ParseBool(os.Getenv(useWorkloadIdentityAuth))
result.UseWorkloadIdentityAuth, _ = strconv.ParseBool(os.Getenv(common.UseWorkloadIdentityAuth))

if err != nil {
return result, errors.Wrapf(err, "parsing %q", syncPeriodVar)
return result, errors.Wrapf(err, "parsing %q", common.SyncPeriodVar)
}

// Not calling validate here to support using from tests where we
Expand All @@ -219,10 +200,10 @@ func ReadAndValidate() (Values, error) {
// Validate checks whether the configuration settings are consistent.
func (v Values) Validate() error {
if v.PodNamespace == "" {
return errors.Errorf("missing value for %s", podNamespaceVar)
return errors.Errorf("missing value for %s", common.PodNamespaceVar)
}
if !v.OperatorMode.IncludesWatchers() && len(v.TargetNamespaces) > 0 {
return errors.Errorf("%s must include watchers to specify target namespaces", targetNamespacesVar)
return errors.Errorf("%s must include watchers to specify target namespaces", common.TargetNamespacesVar)
}
return nil
}
Expand All @@ -243,7 +224,7 @@ func parseTargetNamespaces(fromEnv string) []string {

// parseSyncPeriod parses the sync period from the environment
func parseSyncPeriod() (*time.Duration, error) {
syncPeriodStr := envOrDefault(syncPeriodVar, "1h")
syncPeriodStr := envOrDefault(common.SyncPeriodVar, "1h")
if syncPeriodStr == "" {
return nil, nil
}
Expand Down
29 changes: 14 additions & 15 deletions v2/internal/identity/credential_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,8 @@ import (
"k8s.io/apimachinery/pkg/types"
kerrors "k8s.io/apimachinery/pkg/util/errors"

"github.com/Azure/azure-service-operator/v2/internal/config"
"github.com/Azure/azure-service-operator/v2/internal/reconcilers"
"github.com/Azure/azure-service-operator/v2/internal/util/kubeclient"
"github.com/Azure/azure-service-operator/v2/pkg/common"
"github.com/Azure/azure-service-operator/v2/pkg/genruntime"
"github.com/Azure/azure-service-operator/v2/pkg/genruntime/core"
)
Expand Down Expand Up @@ -96,7 +95,7 @@ func NewCredentialProvider(
// If no matching credential can be found, an error is returned.
func (c *credentialProvider) GetCredential(ctx context.Context, obj genruntime.MetaObject) (*Credential, error) {
// Resource annotation
cred, err := c.getCredentialFromAnnotation(ctx, obj, reconcilers.PerResourceSecretAnnotation)
cred, err := c.getCredentialFromAnnotation(ctx, obj, common.PerResourceSecretAnnotation)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -185,36 +184,36 @@ func (c *credentialProvider) newCredentialFromSecret(secret *v1.Secret) (*Creden

nsName := types.NamespacedName{Namespace: secret.GetNamespace(), Name: secret.GetName()}

subscriptionID, ok := secret.Data[config.SubscriptionIDVar]
subscriptionID, ok := secret.Data[common.SubscriptionIDVar]
if !ok {
err := core.NewSecretNotFoundError(
nsName,
errors.Errorf(
"credential Secret %q does not contain key %q",
nsName,
config.SubscriptionIDVar))
common.SubscriptionIDVar))
errs = append(errs, err)
}

tenantID, ok := secret.Data[config.TenantIDVar]
tenantID, ok := secret.Data[common.TenantIDVar]
if !ok {
err := core.NewSecretNotFoundError(
nsName,
errors.Errorf(
"credential Secret %q does not contain key %q",
nsName,
config.TenantIDVar))
common.TenantIDVar))
errs = append(errs, err)
}

clientID, ok := secret.Data[config.ClientIDVar]
clientID, ok := secret.Data[common.ClientIDVar]
if !ok {
err := core.NewSecretNotFoundError(
nsName,
errors.Errorf(
"credential Secret %q does not contain key %q",
nsName,
config.ClientIDVar))
common.ClientIDVar))
errs = append(errs, err)
}

Expand All @@ -223,7 +222,7 @@ func (c *credentialProvider) newCredentialFromSecret(secret *v1.Secret) (*Creden
return nil, kerrors.NewAggregate(errs)
}

if clientSecret, hasClientSecret := secret.Data[config.ClientSecretVar]; hasClientSecret {
if clientSecret, hasClientSecret := secret.Data[common.ClientSecretVar]; hasClientSecret {
tokenCredential, err := azidentity.NewClientSecretCredential(string(tenantID), string(clientID), string(clientSecret), nil)
if err != nil {
return nil, errors.Wrap(err, errors.Errorf("invalid Client Secret Credential for %q encountered", nsName).Error())
Expand All @@ -237,9 +236,9 @@ func (c *credentialProvider) newCredentialFromSecret(secret *v1.Secret) (*Creden
}, nil
}

if clientCert, hasClientCert := secret.Data[config.ClientCertificateVar]; hasClientCert {
if clientCert, hasClientCert := secret.Data[common.ClientCertificateVar]; hasClientCert {
var clientCertPassword []byte
if p, hasClientCertPassword := secret.Data[config.ClientCertificatePasswordVar]; hasClientCertPassword {
if p, hasClientCertPassword := secret.Data[common.ClientCertificatePasswordVar]; hasClientCertPassword {
clientCertPassword = p
}

Expand All @@ -260,16 +259,16 @@ func (c *credentialProvider) newCredentialFromSecret(secret *v1.Secret) (*Creden
tokenCredential, err := azidentity.NewWorkloadIdentityCredential(&azidentity.WorkloadIdentityCredentialOptions{
ClientID: string(clientID),
TenantID: string(tenantID),
TokenFilePath: config.FederatedTokenFilePath,
TokenFilePath: common.FederatedTokenFilePath,
})
if err != nil {
err = errors.Wrapf(
err,
"credential secret %q does not contain key %q and failed to get workload identity credential for clientID %q from %q ",
nsName,
config.ClientSecretVar,
common.ClientSecretVar,
string(clientID),
config.FederatedTokenFilePath)
common.FederatedTokenFilePath)
return nil, err
}

Expand Down
15 changes: 7 additions & 8 deletions v2/internal/identity/credential_provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,10 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client/fake"

resources "github.com/Azure/azure-service-operator/v2/api/resources/v1api20200601"
"github.com/Azure/azure-service-operator/v2/internal/config"
"github.com/Azure/azure-service-operator/v2/internal/reconcilers"
"github.com/Azure/azure-service-operator/v2/internal/resolver"
"github.com/Azure/azure-service-operator/v2/internal/util/kubeclient"
"github.com/Azure/azure-service-operator/v2/internal/util/to"
"github.com/Azure/azure-service-operator/v2/pkg/common"
)

const testPodNamespace = "azureserviceoperator-system-test"
Expand Down Expand Up @@ -103,7 +102,7 @@ func TestCredentialProvider_ResourceScopeCredentialAndNamespaceCredential_Prefer
g.Expect(err).ToNot(HaveOccurred())

rg := newResourceGroup("test-namespace")
rg.Annotations = map[string]string{reconcilers.PerResourceSecretAnnotation: perResourceCredentialName.Name}
rg.Annotations = map[string]string{common.PerResourceSecretAnnotation: perResourceCredentialName.Name}
err = res.kubeClient.Create(ctx, rg)
g.Expect(err).ToNot(HaveOccurred())

Expand All @@ -128,7 +127,7 @@ func TestCredentialProvider_SecretDoesNotExist_ReturnsError(t *testing.T) {
}

rg := newResourceGroup("test-namespace")
rg.Annotations = map[string]string{reconcilers.PerResourceSecretAnnotation: credentialNamespacedName.Name}
rg.Annotations = map[string]string{common.PerResourceSecretAnnotation: credentialNamespacedName.Name}
err = res.kubeClient.Create(ctx, rg)
g.Expect(err).ToNot(HaveOccurred())

Expand Down Expand Up @@ -204,10 +203,10 @@ func newResourceGroup(namespace string) *resources.ResourceGroup {

func newSecret(namespacedName types.NamespacedName) *v1.Secret {
secretData := make(map[string][]byte)
secretData[config.ClientIDVar] = []byte(fakeID)
secretData[config.ClientSecretVar] = []byte(fakeID)
secretData[config.TenantIDVar] = []byte(fakeID)
secretData[config.SubscriptionIDVar] = []byte(fakeID)
secretData[common.ClientIDVar] = []byte(fakeID)
secretData[common.ClientSecretVar] = []byte(fakeID)
secretData[common.TenantIDVar] = []byte(fakeID)
secretData[common.SubscriptionIDVar] = []byte(fakeID)

return &v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Expand Down
20 changes: 10 additions & 10 deletions v2/internal/reconcilers/arm/arm_client_cache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ import (
"github.com/Azure/azure-service-operator/v2/internal/config"
"github.com/Azure/azure-service-operator/v2/internal/identity"
"github.com/Azure/azure-service-operator/v2/internal/metrics"
"github.com/Azure/azure-service-operator/v2/internal/reconcilers"
"github.com/Azure/azure-service-operator/v2/internal/resolver"
"github.com/Azure/azure-service-operator/v2/internal/util/kubeclient"
"github.com/Azure/azure-service-operator/v2/internal/util/to"
"github.com/Azure/azure-service-operator/v2/pkg/common"
"github.com/Azure/azure-service-operator/v2/pkg/genruntime/core"
)

Expand Down Expand Up @@ -141,7 +141,7 @@ func Test_ARMClientCache_ReturnsPerResourceScopedClientOverNamespacedClient(t *t
g.Expect(err).ToNot(HaveOccurred())

rg := newResourceGroup("test-namespace")
rg.Annotations = map[string]string{reconcilers.PerResourceSecretAnnotation: perResourceCredentialName.Name}
rg.Annotations = map[string]string{common.PerResourceSecretAnnotation: perResourceCredentialName.Name}
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer if we spread these literal maps across multiple lines, as we already typically do elsewhere.

Suggested change
rg.Annotations = map[string]string{common.PerResourceSecretAnnotation: perResourceCredentialName.Name}
rg.Annotations = map[string]string{
common.PerResourceSecretAnnotation: perResourceCredentialName.Name,
}

err = res.kubeClient.Create(ctx, rg)
g.Expect(err).ToNot(HaveOccurred())

Expand Down Expand Up @@ -173,7 +173,7 @@ func Test_ARMClientCache_PerResourceSecretInDifferentNamespace_ReturnsError(t *t
g.Expect(err).ToNot(HaveOccurred())

rg := newResourceGroup("test-namespace")
rg.Annotations = map[string]string{reconcilers.PerResourceSecretAnnotation: perResourceCredentialName.String()}
rg.Annotations = map[string]string{common.PerResourceSecretAnnotation: perResourceCredentialName.String()}
err = res.kubeClient.Create(ctx, rg)
g.Expect(err).ToNot(HaveOccurred())

Expand All @@ -198,7 +198,7 @@ func Test_ARMClientCache_ReturnsError_IfSecretNotFound(t *testing.T) {
}

rg := newResourceGroup("")
rg.Annotations = map[string]string{reconcilers.PerResourceSecretAnnotation: credentialNamespacedName.Name}
rg.Annotations = map[string]string{common.PerResourceSecretAnnotation: credentialNamespacedName.Name}
err = res.kubeClient.Create(ctx, rg)
g.Expect(err).ToNot(HaveOccurred())

Expand Down Expand Up @@ -229,7 +229,7 @@ func Test_ARMClientCache_ReturnsPerResourceScopedClient(t *testing.T) {
g.Expect(err).ToNot(HaveOccurred())

rg := newResourceGroup("test-namespace")
rg.Annotations = map[string]string{reconcilers.PerResourceSecretAnnotation: credentialNamespacedName.Name}
rg.Annotations = map[string]string{common.PerResourceSecretAnnotation: credentialNamespacedName.Name}
err = res.kubeClient.Create(ctx, rg)
g.Expect(err).ToNot(HaveOccurred())

Expand Down Expand Up @@ -305,7 +305,7 @@ func Test_ARMClientCache_ReturnsNamespaceScopedClient_SecretChanged(t *testing.T

// change secret and check if we get a new client
old := secret
secret.Data[config.ClientIDVar] = []byte("11111111-1111-1111-1111-111111111111")
secret.Data[common.ClientIDVar] = []byte("11111111-1111-1111-1111-111111111111")
err = res.kubeClient.Patch(ctx, secret, MergeFrom(old))
g.Expect(err).ToNot(HaveOccurred())

Expand Down Expand Up @@ -339,10 +339,10 @@ func Test_ARMClientCache_ReturnsGlobalClient(t *testing.T) {

func newSecret(namespacedName types.NamespacedName) *v1.Secret {
secretData := make(map[string][]byte)
secretData[config.ClientIDVar] = []byte(fakeID)
secretData[config.ClientSecretVar] = []byte(fakeID)
secretData[config.TenantIDVar] = []byte(fakeID)
secretData[config.SubscriptionIDVar] = []byte(fakeID)
secretData[common.ClientIDVar] = []byte(fakeID)
secretData[common.ClientSecretVar] = []byte(fakeID)
secretData[common.TenantIDVar] = []byte(fakeID)
secretData[common.SubscriptionIDVar] = []byte(fakeID)

return &v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Expand Down
3 changes: 2 additions & 1 deletion v2/internal/reconcilers/generic/generic_reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/Azure/azure-service-operator/v2/internal/reconcilers"
"github.com/Azure/azure-service-operator/v2/internal/util/interval"
"github.com/Azure/azure-service-operator/v2/internal/util/kubeclient"
"github.com/Azure/azure-service-operator/v2/pkg/common"
"github.com/Azure/azure-service-operator/v2/pkg/genruntime"
"github.com/Azure/azure-service-operator/v2/pkg/genruntime/conditions"
)
Expand Down Expand Up @@ -333,7 +334,7 @@ func (gr *GenericReconciler) handleSkipReconcile(ctx context.Context, log logr.L
reconcilePolicy := reconcilers.GetReconcilePolicy(obj, log) // TODO: Pull this whole method up here
log.V(Status).Info(
"Skipping creation/update of resource due to policy",
reconcilers.ReconcilePolicyAnnotation, reconcilePolicy)
common.ReconcilePolicyAnnotation, reconcilePolicy)

err := gr.Reconciler.UpdateStatus(ctx, log, gr.Recorder, obj)
if err != nil {
Expand Down
9 changes: 4 additions & 5 deletions v2/internal/reconcilers/predicates.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,16 @@ import (
"sigs.k8s.io/controller-runtime/pkg/predicate"

"github.com/Azure/azure-service-operator/v2/internal/util/predicates"
"github.com/Azure/azure-service-operator/v2/pkg/common"
"github.com/Azure/azure-service-operator/v2/pkg/genruntime"
)

const PerResourceSecretAnnotation = "serviceoperator.azure.com/credential-from"

// ARMReconcilerAnnotationChangedPredicate creates a predicate that emits events when annotations
// interesting to the generic ARM reconciler are changed
func ARMReconcilerAnnotationChangedPredicate() predicate.Predicate {
return predicates.MakeSelectAnnotationChangedPredicate(
map[string]predicates.HasAnnotationChanged{
ReconcilePolicyAnnotation: HasReconcilePolicyAnnotationChanged,
common.ReconcilePolicyAnnotation: HasReconcilePolicyAnnotationChanged,
})
}

Expand All @@ -31,13 +30,13 @@ func ARMReconcilerAnnotationChangedPredicate() predicate.Predicate {
func ARMPerResourceSecretAnnotationChangedPredicate() predicate.Predicate {
return predicates.MakeSelectAnnotationChangedPredicate(
map[string]predicates.HasAnnotationChanged{
PerResourceSecretAnnotation: HasAnnotationChanged,
common.PerResourceSecretAnnotation: HasAnnotationChanged,
})
}

// GetReconcilePolicy gets the reconcile-policy from the ReconcilePolicyAnnotation
func GetReconcilePolicy(obj genruntime.MetaObject, log logr.Logger) ReconcilePolicy {
policyStr := obj.GetAnnotations()[ReconcilePolicyAnnotation]
policyStr := obj.GetAnnotations()[common.ReconcilePolicyAnnotation]
policy, err := ParseReconcilePolicy(policyStr)
if err != nil {
log.Error(
Expand Down
Loading