Feat/sam pipeline OIDC #4270

xazhao · 2022-09-30T06:46:55Z

Which issue(s) does this change fix?

Adding OIDC support to SAM Pipeline.

Why is this change necessary?

How does it address the issue?

What side effects does this change have?

Mandatory Checklist

PRs will only be reviewed after checklist is complete

Add input/output type hints to new functions/methods
Write design document if needed (Do I need to write a design document?)
Write/update unit tests
Write/update integration tests
Write/update functional tests if needed
make pr passes
make update-reproducible-reqs if dependencies were changed
Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

* support for GitHub Actions OIDC * GitHub Actions oidc support * Unit tests for GitHub Actions OIDC support * Add tests for GitHub Actions OIDC support * Add comments for new methods and fix formatting * use choice for identity provider instead of flag * Create pipeline oidc provider class * Remove unused methods * Add defaults for OIDC URL and ClientID * Get subject claim from object and use Session to get Client * Mock Session during test * Create method to share common logic * Add AllowedValues to CreateNewOidcProvider * Remove duplicate calls to samconfig * Add text to prompt * fix formatting * Add links to technical methods comments

* Support GitLab OIDC for pipeline bootstrap * Fix incorrect variable mappings * use constants instead of strings

* Support Bitbucket OIDC in pipeline bootstrap * Add documentation * Make capitlization consistent * Add comments to method * Re-use permissions provider answer for init * Make help text shorter * add constant inside constructor * Fix comments * move variables into class * change saved iam parameter * change help message * use dataclass

requirements/base.txt

samcli/commands/pipeline/bootstrap/cli.py

mndeveci · 2022-10-07T20:55:46Z

samcli/lib/cookiecutter/template.py

@@ -121,6 +121,7 @@ def run_interactive_flows(self, context: Optional[Dict] = None) -> Dict:
        """
        try:
            context = context if context else {}
+            context["shared_values"] = "default"


Can you explain why this is needed? And can you also confirm that it won't break sam init flow?

No it won't break the sam init flow. This value is used in sam pipeline init templates to utilize previous answer to skip some questions.

samcli/lib/utils/managed_cloudformation_stack.py

samcli/commands/pipeline/bootstrap/cli.py

mndeveci · 2022-10-12T05:07:36Z

samcli/commands/pipeline/bootstrap/cli.py

+
+from samcli.commands.pipeline.bootstrap.pipeline_oidc_provider import PipelineOidcProvider
 from samcli.lib.config.samconfig import SamConfig
-from samcli.lib.pipeline.bootstrap.stage import Stage
+
+from samcli.commands.pipeline.bootstrap.oidc_config import (
+    BitbucketOidcConfig,
+    GitHubOidcConfig,
+    OidcConfig,
+    GitLabOidcConfig,
+)
+


Apologies for been picky, but these imports can be moved down as well.

These imports are also used in another function as input, so I think they need to be at the top. Correct me if I'm wrong.

looks like they are used in other functions only for type indication. We can wrap them by quotes instead, e.g.

def my_function(arg: "MyClass"): pass

Good suggestion! Make it work. Thanks!

sriram-mv

This is a large PR and will require another pass.

sriram-mv · 2022-10-12T19:08:50Z

samcli/commands/pipeline/bootstrap/cli.py

+    required=False,
+)
+@click.option(
+    "--gitlab-group",


Some of these options don't play well with each other I'm assuming. There needs to be appropriate error messaging if they are specified together.

Currently it takes all options then decides which option to use based on what oidc-provider is. I see your point is helpful. I'm thinking we can leave it to another PR for improvements which includes some refactoring.

I learned from @xazhao that these new options, if not applicable, will just be ignored. I think it makes sense to handle them for now, as at least these different groups (GH Action, Gitlab and Bitbucket) won't conflict each other. But IMO there's a room to improve in terms of UX, to give customers clearer messages about what options they really need to supply.

samcli/commands/pipeline/bootstrap/cli.py

sriram-mv · 2022-10-12T19:15:53Z

samcli/lib/pipeline/bootstrap/stage_resources.yaml

+                    {
+                      "Effect": "Allow",
+                      "Principal": {
+                        "Federated": "arn:aws:iam::${AWS::AccountId}:oidc-provider/${Url}"


use AWS::Partition instead of :aws: , other places need to be cross checked as well.

Good catch! Will update it.

hawflau · 2022-10-12T21:18:01Z

samcli/commands/pipeline/bootstrap/guided_context.py

+        click.echo("\t1 - IAM (default)")
+        click.echo("\t2 - OpenID Connect (OIDC)")
+        user_provider = click.prompt("Choice", type=click.Choice((["1", "2"])), show_default=False, default="1")
+        self.permissions_provider = OPEN_ID_CONNECT if user_provider == "2" else "iam"


nit: why not also use a constant for "iam"?

Good catch. Updating it to a constant.

hawflau · 2022-10-12T21:39:20Z

samcli/commands/pipeline/bootstrap/cli.py

+
+from samcli.commands.pipeline.bootstrap.pipeline_oidc_provider import PipelineOidcProvider
 from samcli.lib.config.samconfig import SamConfig
-from samcli.lib.pipeline.bootstrap.stage import Stage
+
+from samcli.commands.pipeline.bootstrap.oidc_config import (
+    BitbucketOidcConfig,
+    GitHubOidcConfig,
+    OidcConfig,
+    GitLabOidcConfig,
+)
+


looks like they are used in other functions only for type indication. We can wrap them by quotes instead, e.g.

def my_function(arg: "MyClass"): pass

sriram-mv

Conditionally approving this based on following follow-on improvements.

Improved error messaging when conflicting options are chosen.
Improved handling on changing the entities referenced within the configuration file through parameters
Showcase in the help text which options are related and go together, there are a lot of options for this command at present.
Lazy imports within cli() instead of top level imports, this causes the help text loading to slow down.

dismissing as two approvals are obtained

sidhujus added 3 commits September 29, 2022 17:54

Support GitLab OIDC for pipeline bootstrap (#4037)

5868729

* Support GitLab OIDC for pipeline bootstrap * Fix incorrect variable mappings * use constants instead of strings

github-actions bot added pr/internal area/pipeline labels Sep 30, 2022

xazhao and others added 7 commits October 3, 2022 11:28

Optimize interactive flow and fix bugs

e41e0ad

Support bootstrap stack update

8f327f0

fix duplicate bootstrap bug

6faedf5

bug fix

23ae73e

Update UX and fix unit tests

2911a19

Improve UX and add more tests

ea3e274

Merge branch 'develop' into feat/sam-pipeline-oidc

518ecc8

xazhao marked this pull request as ready for review October 7, 2022 18:43

recover the link

3dd7e5d

mndeveci suggested changes Oct 7, 2022

View reviewed changes

xazhao and others added 2 commits October 7, 2022 14:28

Address comments

b8a89ae

Merge branch 'develop' into feat/sam-pipeline-oidc

cb2ecb6

xazhao requested a review from mndeveci October 7, 2022 23:43

mndeveci reviewed Oct 8, 2022

View reviewed changes

samcli/commands/pipeline/bootstrap/cli.py Outdated Show resolved Hide resolved

samcli/commands/pipeline/bootstrap/cli.py Show resolved Hide resolved

Re-organize imports

5291b70

mndeveci previously requested changes Oct 12, 2022

View reviewed changes

sriram-mv suggested changes Oct 12, 2022

View reviewed changes

xazhao and others added 2 commits October 12, 2022 14:22

Use AWS Partition instead of hardcoding aws

bb04f19

Merge branch 'develop' into feat/sam-pipeline-oidc

7c7518f

hawflau reviewed Oct 12, 2022

View reviewed changes

hawflau suggested changes Oct 12, 2022

View reviewed changes

xazhao and others added 2 commits October 12, 2022 14:50

Address some comments

57435b1

Merge branch 'develop' into feat/sam-pipeline-oidc

0d195d8

sriram-mv self-requested a review October 12, 2022 23:33

Recover imports

a6dbdbb

xazhao requested review from hawflau and sriram-mv and removed request for sriram-mv and hawflau October 12, 2022 23:37

sriram-mv approved these changes Oct 12, 2022

View reviewed changes

hawflau approved these changes Oct 13, 2022

View reviewed changes

xazhao requested a review from mndeveci October 13, 2022 00:23

hawflau merged commit 393db0c into aws:develop Oct 13, 2022

xazhao mentioned this pull request Oct 27, 2022

Add error messages to conflict options #4350

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/sam pipeline OIDC #4270

Feat/sam pipeline OIDC #4270

xazhao commented Sep 30, 2022 •

edited

Loading

mndeveci Oct 7, 2022

xazhao Oct 7, 2022

mndeveci Oct 12, 2022

xazhao Oct 12, 2022

hawflau Oct 12, 2022

xazhao Oct 12, 2022

sriram-mv left a comment

sriram-mv Oct 12, 2022

xazhao Oct 12, 2022

hawflau Oct 12, 2022

sriram-mv Oct 12, 2022

xazhao Oct 12, 2022

hawflau Oct 12, 2022

xazhao Oct 12, 2022

hawflau Oct 12, 2022

sriram-mv left a comment •

edited

Loading

Feat/sam pipeline OIDC #4270

Feat/sam pipeline OIDC #4270

Conversation

xazhao commented Sep 30, 2022 • edited Loading

Which issue(s) does this change fix?

Why is this change necessary?

How does it address the issue?

What side effects does this change have?

Mandatory Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sriram-mv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sriram-mv left a comment • edited Loading

Choose a reason for hiding this comment

xazhao commented Sep 30, 2022 •

edited

Loading

sriram-mv left a comment •

edited

Loading