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.

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
7 changes: 4 additions & 3 deletions v2/cmd/controller/app/setup.go
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"
common "github.com/Azure/azure-service-operator/v2/pkg/common/config"

"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: identity.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.AzureClientCertificate); cert != "" {
certPassword := os.Getenv(common.AzureClientCertificatePassword)
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
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/config"
)

// 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(config.OperatorMode)
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(config.AzureSubscriptionID)
result.PodNamespace = os.Getenv(config.PodNamespace)
result.TargetNamespaces = parseTargetNamespaces(os.Getenv(config.TargetNamespaces))
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(config.ResourceManagerEndpoint, DefaultEndpoint)
result.ResourceManagerAudience = envOrDefault(config.ResourceManagerAudience, DefaultAudience)
result.AzureAuthorityHost = envOrDefault(config.AzureAuthorityHost, DefaultAADAuthorityHost)
result.ClientID = os.Getenv(config.AzureClientID)
result.TenantID = os.Getenv(config.AzureTenantID)

// 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(config.UseWorkloadIdentityAuth))

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

// 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", config.PodNamespace)
}
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", config.TargetNamespaces)
}
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(config.SyncPeriod, "1h")
if syncPeriodStr == "" {
return nil, nil
}
Expand Down
32 changes: 17 additions & 15 deletions v2/internal/identity/credential_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ 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/annotations"
"github.com/Azure/azure-service-operator/v2/pkg/common/config"
"github.com/Azure/azure-service-operator/v2/pkg/genruntime"
"github.com/Azure/azure-service-operator/v2/pkg/genruntime/core"
)
Expand All @@ -30,6 +30,8 @@ const (
globalCredentialSecretName = "aso-controller-settings"
// #nosec
NamespacedSecretName = "aso-credential"
// #nosec
FederatedTokenFilePath = "/var/run/secrets/tokens/azure-identity"
)

// Credential describes a credential used to connect to Azure
Expand Down Expand Up @@ -96,7 +98,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, annotations.PerResourceSecret)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -185,36 +187,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[config.AzureSubscriptionID]
if !ok {
err := core.NewSecretNotFoundError(
nsName,
errors.Errorf(
"credential Secret %q does not contain key %q",
nsName,
config.SubscriptionIDVar))
config.AzureSubscriptionID))
errs = append(errs, err)
}

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

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

Expand All @@ -223,7 +225,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[config.AzureClientSecret]; 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 +239,9 @@ func (c *credentialProvider) newCredentialFromSecret(secret *v1.Secret) (*Creden
}, nil
}

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

Expand All @@ -260,16 +262,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: 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,
config.AzureClientSecret,
string(clientID),
config.FederatedTokenFilePath)
FederatedTokenFilePath)
return nil, err
}

Expand Down
16 changes: 8 additions & 8 deletions v2/internal/identity/credential_provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ 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/annotations"
"github.com/Azure/azure-service-operator/v2/pkg/common/config"
)

const testPodNamespace = "azureserviceoperator-system-test"
Expand Down Expand Up @@ -103,7 +103,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{annotations.PerResourceSecret: perResourceCredentialName.Name}
err = res.kubeClient.Create(ctx, rg)
g.Expect(err).ToNot(HaveOccurred())

Expand All @@ -128,7 +128,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{annotations.PerResourceSecret: credentialNamespacedName.Name}
err = res.kubeClient.Create(ctx, rg)
g.Expect(err).ToNot(HaveOccurred())

Expand Down Expand Up @@ -204,10 +204,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[config.AzureClientID] = []byte(fakeID)
secretData[config.AzureClientSecret] = []byte(fakeID)
secretData[config.AzureTenantID] = []byte(fakeID)
secretData[config.AzureSubscriptionID] = []byte(fakeID)

return &v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Expand Down
29 changes: 19 additions & 10 deletions v2/internal/reconcilers/arm/arm_client_cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,11 @@ 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/annotations"
config2 "github.com/Azure/azure-service-operator/v2/pkg/common/config"
"github.com/Azure/azure-service-operator/v2/pkg/genruntime/core"
)

Expand Down Expand Up @@ -141,7 +142,9 @@ 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{
annotations.PerResourceSecret: perResourceCredentialName.Name,
}
err = res.kubeClient.Create(ctx, rg)
g.Expect(err).ToNot(HaveOccurred())

Expand Down Expand Up @@ -173,7 +176,9 @@ 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{
annotations.PerResourceSecret: perResourceCredentialName.String(),
}
err = res.kubeClient.Create(ctx, rg)
g.Expect(err).ToNot(HaveOccurred())

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

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

Expand Down Expand Up @@ -229,7 +236,9 @@ 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{
annotations.PerResourceSecret: credentialNamespacedName.Name,
}
err = res.kubeClient.Create(ctx, rg)
g.Expect(err).ToNot(HaveOccurred())

Expand Down Expand Up @@ -305,7 +314,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[config2.AzureClientID] = []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 +348,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[config2.AzureClientID] = []byte(fakeID)
secretData[config2.AzureClientSecret] = []byte(fakeID)
secretData[config2.AzureTenantID] = []byte(fakeID)
secretData[config2.AzureSubscriptionID] = []byte(fakeID)

return &v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Expand Down
3 changes: 2 additions & 1 deletion v2/internal/reconcilers/generic/generic_reconciler.go
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/annotations"
"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)
annotations.ReconcilePolicy, reconcilePolicy)

err := gr.Reconciler.UpdateStatus(ctx, log, gr.Recorder, obj)
if err != nil {
Expand Down
Loading